On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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