Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 13 Oct 2021 12:26:42 +0300 | Subject | Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver |
| |
On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> I split some of the bigger patches apart so they only touched one > driver or subsystem at a time. I also updated to_pci_driver() so it > returns NULL when given NULL, which makes some of the validations > quite a bit simpler, especially in the PM code in pci-driver.c.
It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
Below are some comments as well.
...
> static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device) > { > + struct pci_driver *drv = to_pci_driver(pdev->dev.driver); > const struct pci_device_id *id; > > if (pdev->vendor == vendor && pdev->device == device) > return true;
> + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++) > + if (id->vendor == vendor && id->device == device)
> + break;
return true;
> return id && id->vendor;
return false;
> }
...
> + afu_result = err_handler->error_detected(afu_dev, > + state);
One line?
...
> device_lock(&vf_dev->dev); > - if (vf_dev->dev.driver) { > + if (to_pci_driver(vf_dev->dev.driver)) {
Hmm...
...
> + if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> + && pci_dev->current_state != PCI_UNKNOWN) {
Can we keep && on the previous line?
> + pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev, > + "PCI PM: Device state not saved by %pS\n", > + drv->suspend); > }
...
> + return drv && drv->resume ? > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
One line?
...
> + struct pci_driver *drv = to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler = > - dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL; > + drv ? drv->err_handler : NULL;
Isn't dev->driver == to_pci_driver(dev->dev.driver)?
...
> + struct pci_driver *drv = to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler = > - dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL; > + drv ? drv->err_handler : NULL;
Ditto.
...
> device_lock(&dev->dev); > + pdrv = to_pci_driver(dev->dev.driver); > if (!pci_dev_set_io_state(dev, state) || > - !dev->dev.driver || > - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> + !pdrv || > + !pdrv->err_handler ||
One line now?
> !pdrv->err_handler->error_detected) {
Or this and the previous line?
...
> + pdrv = to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->mmio_enabled) > goto out;
Ditto.
...
> + pdrv = to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->slot_reset) > goto out;
Ditto.
...
> if (!pci_dev_set_io_state(dev, pci_channel_io_normal) || > - !dev->dev.driver || > - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler || > + !pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->resume) > goto out;
Ditto.
> - result = PCI_ERS_RESULT_NONE; > > pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn); > if (!pcidev || !pcidev->dev.driver) { > dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n"); > pci_dev_put(pcidev); > - return result; > + return PCI_ERS_RESULT_NONE; > } > pdrv = to_pci_driver(pcidev->dev.driver);
What about splitting the conditional to two with clear error message in each and use pci_err() in the second one?
...
> default: > dev_err(&pdev->xdev->dev, > - "bad request in aer recovery " > - "operation!\n"); > + "bad request in AER recovery operation!\n");
Stray change? Or is it in a separate patch in your tree?
-- With Best Regards, Andy Shevchenko
| |