| Message ID | 20211004125935.2300113-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: Drop duplicated tracking of a pci_dev's bound driver | expand |
On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: > Hello, > > this is v6 of the quest to drop the "driver" member from struct pci_dev > which tracks the same data (apart from a constant offset) as dev.driver. I like this a lot and applied it to pci/driver for v5.16, thanks! 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. Full interdiff from this v6 series: diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c index deaaef6efe34..36e84d904260 100644 --- a/arch/x86/kernel/probe_roms.c +++ b/arch/x86/kernel/probe_roms.c @@ -80,17 +80,15 @@ static struct resource video_rom_resource = { */ 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; - if (pdev->dev.driver) { - struct pci_driver *drv = to_pci_driver(pdev->dev.driver); - for (id = drv->id_table; id && id->vendor; id++) - if (id->vendor == vendor && id->device == device) - break; - } + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++) + if (id->vendor == vendor && id->device == device) + break; return id && id->vendor; } diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c index d997c9c3ebb5..7eb3706cf42d 100644 --- a/drivers/misc/cxl/guest.c +++ b/drivers/misc/cxl/guest.c @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu, pci_channel_state_t state) { struct pci_dev *afu_dev; + struct pci_driver *afu_drv; + struct pci_error_handlers *err_handler; if (afu->phb == NULL) return; list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { - struct pci_driver *afu_drv; - - if (!afu_dev->dev.driver) - continue; - afu_drv = to_pci_driver(afu_dev->dev.driver); + if (!afu_drv) + continue; + err_handler = afu_drv->err_handler; switch (bus_error_event) { case CXL_ERROR_DETECTED_EVENT: afu_dev->error_state = state; - if (afu_drv->err_handler && - afu_drv->err_handler->error_detected) - afu_drv->err_handler->error_detected(afu_dev, state); - break; + if (err_handler && + err_handler->error_detected) + err_handler->error_detected(afu_dev, state); + break; case CXL_SLOT_RESET_EVENT: afu_dev->error_state = state; - if (afu_drv->err_handler && - afu_drv->err_handler->slot_reset) - afu_drv->err_handler->slot_reset(afu_dev); - break; + if (err_handler && + err_handler->slot_reset) + err_handler->slot_reset(afu_dev); + break; case CXL_RESUME_EVENT: - if (afu_drv->err_handler && - afu_drv->err_handler->resume) - afu_drv->err_handler->resume(afu_dev); - break; + if (err_handler && + err_handler->resume) + err_handler->resume(afu_dev); + break; } } } diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c index 7e7545d01e27..08bd81854101 100644 --- a/drivers/misc/cxl/pci.c +++ b/drivers/misc/cxl/pci.c @@ -1795,6 +1795,8 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu, pci_channel_state_t state) { struct pci_dev *afu_dev; + struct pci_driver *afu_drv; + struct pci_error_handlers *err_handler; pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET; pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET; @@ -1805,16 +1807,16 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu, return result; list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { - struct pci_driver *afu_drv; - if (!afu_dev->dev.driver) - continue; - afu_drv = to_pci_driver(afu_dev->dev.driver); + if (!afu_drv) + continue; afu_dev->error_state = state; - if (afu_drv->err_handler) - afu_result = afu_drv->err_handler->error_detected(afu_dev, state); + err_handler = afu_drv->err_handler; + if (err_handler) + afu_result = err_handler->error_detected(afu_dev, + state); /* Disconnect trumps all, NONE trumps NEED_RESET */ if (afu_result == PCI_ERS_RESULT_DISCONNECT) result = PCI_ERS_RESULT_DISCONNECT; @@ -1974,6 +1976,8 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev) struct cxl_afu *afu; struct cxl_context *ctx; struct pci_dev *afu_dev; + struct pci_driver *afu_drv; + struct pci_error_handlers *err_handler; pci_ers_result_t afu_result = PCI_ERS_RESULT_RECOVERED; pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED; int i; @@ -2005,8 +2009,6 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev) continue; list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { - struct pci_driver *afu_drv; - /* Reset the device context. * TODO: make this less disruptive */ @@ -2032,14 +2034,13 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev) * shouldn't start new work until we call * their resume function. */ - if (!afu_dev->dev.driver) - continue; - afu_drv = to_pci_driver(afu_dev->dev.driver); + if (!afu_drv) + continue; - if (afu_drv->err_handler && - afu_drv->err_handler->slot_reset) - afu_result = afu_drv->err_handler->slot_reset(afu_dev); + err_handler = afu_drv->err_handler; + if (err_handler && err_handler->slot_reset) + afu_result = err_handler->slot_reset(afu_dev); if (afu_result == PCI_ERS_RESULT_DISCONNECT) result = PCI_ERS_RESULT_DISCONNECT; @@ -2066,6 +2067,8 @@ static void cxl_pci_resume(struct pci_dev *pdev) struct cxl *adapter = pci_get_drvdata(pdev); struct cxl_afu *afu; struct pci_dev *afu_dev; + struct pci_driver *afu_drv; + struct pci_error_handlers *err_handler; int i; /* Everything is back now. Drivers should restart work now. @@ -2080,11 +2083,13 @@ static void cxl_pci_resume(struct pci_dev *pdev) continue; list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { - struct pci_driver *afu_drv; - if (afu_dev->dev.driver && - (afu_drv = to_pci_driver(afu_dev->dev.driver))->err_handler && - afu_drv->err_handler->resume) - afu_drv->err_handler->resume(afu_dev); + afu_drv = to_pci_driver(afu_dev->dev.driver); + if (!afu_drv) + continue; + + err_handler = afu_drv->err_handler; + if (err_handler && err_handler->resume) + err_handler->resume(afu_dev); } } spin_unlock(&adapter->afu_list_lock); diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 0d0a34347868..fa4b52bb1e05 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -168,11 +168,8 @@ static ssize_t sriov_vf_total_msix_show(struct device *dev, u32 vf_total_msix = 0; device_lock(dev); - if (!dev->driver) - goto unlock; - pdrv = to_pci_driver(dev->driver); - if (!pdrv->sriov_get_vf_total_msix) + if (!pdrv || !pdrv->sriov_get_vf_total_msix) goto unlock; vf_total_msix = pdrv->sriov_get_vf_total_msix(pdev); @@ -199,19 +196,14 @@ static ssize_t sriov_vf_msix_count_store(struct device *dev, return -EINVAL; device_lock(&pdev->dev); - if (!pdev->dev.driver) { - ret = -EOPNOTSUPP; - goto err_pdev; - } - - pdrv = to_pci_driver(pdev->dev.driver); - if (!pdrv->sriov_set_msix_vec_count) { + pdrv = to_pci_driver(dev->driver); + if (!pdrv || !pdrv->sriov_set_msix_vec_count) { ret = -EOPNOTSUPP; goto err_pdev; } device_lock(&vf_dev->dev); - if (vf_dev->dev.driver) { + if (to_pci_driver(vf_dev->dev.driver)) { /* * A driver is already attached to this VF and has configured * itself based on the current MSI-X vector count. Changing @@ -405,14 +397,13 @@ static ssize_t sriov_numvfs_store(struct device *dev, goto exit; /* is PF driver loaded */ - if (!pdev->dev.driver) { + pdrv = to_pci_driver(dev->driver); + if (!pdrv) { pci_info(pdev, "no driver bound to device; cannot configure SR-IOV\n"); ret = -ENOENT; goto exit; } - pdrv = to_pci_driver(pdev->dev.driver); - /* is PF driver loaded w/callback */ if (!pdrv->sriov_configure) { pci_info(pdev, "driver does not support SR-IOV configuration via sysfs\n"); diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index e94aa338bab4..3884a1542e86 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -454,7 +454,7 @@ static int pci_device_probe(struct device *dev) static void pci_device_remove(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + struct pci_driver *drv = to_pci_driver(dev->driver); if (drv->remove) { pm_runtime_get_sync(dev); @@ -489,15 +489,12 @@ static void pci_device_remove(struct device *dev) static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); + struct pci_driver *drv = to_pci_driver(dev->driver); pm_runtime_resume(dev); - if (pci_dev->dev.driver) { - struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); - - if (drv->shutdown) - drv->shutdown(pci_dev); - } + if (drv && drv->shutdown) + drv->shutdown(pci_dev); /* * If this is a kexec reboot, turn off Bus Master bit on the @@ -588,25 +585,22 @@ static int pci_pm_reenable_device(struct pci_dev *pci_dev) static int pci_legacy_suspend(struct device *dev, pm_message_t state) { struct pci_dev *pci_dev = to_pci_dev(dev); + struct pci_driver *drv = to_pci_driver(dev->driver); - if (dev->driver) { - struct pci_driver *drv = to_pci_driver(dev->driver); + if (drv && drv->suspend) { + pci_power_t prev = pci_dev->current_state; + int error; - if (drv->suspend) { - pci_power_t prev = pci_dev->current_state; - int error; + error = drv->suspend(pci_dev, state); + suspend_report_result(drv->suspend, error); + if (error) + return error; - error = drv->suspend(pci_dev, state); - suspend_report_result(drv->suspend, error); - if (error) - return error; - - if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 - && pci_dev->current_state != PCI_UNKNOWN) { - pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev, - "PCI PM: Device state not saved by %pS\n", - drv->suspend); - } + if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 + && pci_dev->current_state != PCI_UNKNOWN) { + pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev, + "PCI PM: Device state not saved by %pS\n", + drv->suspend); } } @@ -632,17 +626,12 @@ static int pci_legacy_suspend_late(struct device *dev, pm_message_t state) static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); + struct pci_driver *drv = to_pci_driver(dev->driver); pci_fixup_device(pci_fixup_resume, pci_dev); - if (pci_dev->dev.driver) { - struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); - - if (drv->resume) - return drv->resume(pci_dev); - } - - return pci_pm_reenable_device(pci_dev); + return drv && drv->resume ? + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); } /* Auxiliary functions used by the new power management framework */ @@ -656,14 +645,8 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev) static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev) { - struct pci_driver *drv; - bool ret; - - if (!pci_dev->dev.driver) - return false; - - drv = to_pci_driver(pci_dev->dev.driver); - ret = drv && (drv->suspend || drv->resume); + struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + bool ret = drv && (drv->suspend || drv->resume); /* * Legacy PM support is used by default, so warn if the new framework is @@ -1255,11 +1238,11 @@ static int pci_pm_runtime_suspend(struct device *dev) int error; /* - * If pci_dev->dev.driver is not set (unbound), we leave the device in D0, - * but it may go to D3cold when the bridge above it runtime suspends. - * Save its config space in case that happens. + * If the device has no driver, we leave it in D0, but it may go to + * D3cold when the bridge above it runtime suspends. Save its + * config space in case that happens. */ - if (!pci_dev->dev.driver) { + if (!to_pci_driver(dev->driver)) { pci_save_state(pci_dev); return 0; } @@ -1316,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev) */ pci_restore_standard_config(pci_dev); - if (!dev->driver) + if (!to_pci_driver(dev->driver)) return 0; pci_fixup_device(pci_fixup_resume_early, pci_dev); @@ -1335,13 +1318,14 @@ static int pci_pm_runtime_resume(struct device *dev) static int pci_pm_runtime_idle(struct device *dev) { + struct pci_dev *pci_dev = to_pci_dev(dev); const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; /* - * If dev->driver is not set (unbound), the device should - * always remain in D0 regardless of the runtime PM status + * If the device has no driver, it should always remain in D0 + * regardless of the runtime PM status */ - if (!dev->driver) + if (!to_pci_driver(dev->driver)) return 0; if (!pm) @@ -1448,8 +1432,10 @@ static struct pci_driver pci_compat_driver = { */ struct pci_driver *pci_dev_driver(const struct pci_dev *dev) { - if (dev->dev.driver) - return to_pci_driver(dev->dev.driver); + struct pci_driver *drv = to_pci_driver(dev->dev.driver); + + if (drv) + return drv; else { int i; for (i = 0; i <= PCI_ROM_RESOURCE; i++) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ccecf740de59..5298ce131f8c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5088,13 +5088,14 @@ EXPORT_SYMBOL_GPL(pci_dev_unlock); static void pci_dev_save_and_disable(struct pci_dev *dev) { + 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; /* - * dev->driver->err_handler->reset_prepare() is protected against - * races with ->remove() by the device lock, which must be held by - * the caller. + * drv->err_handler->reset_prepare() is protected against races + * with ->remove() by the device lock, which must be held by the + * caller. */ if (err_handler && err_handler->reset_prepare) err_handler->reset_prepare(dev); @@ -5119,15 +5120,15 @@ static void pci_dev_save_and_disable(struct pci_dev *dev) static void pci_dev_restore(struct pci_dev *dev) { + 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; pci_restore_state(dev); /* - * dev->driver->err_handler->reset_done() is protected against - * races with ->remove() by the device lock, which must be held by - * the caller. + * drv->err_handler->reset_done() is protected against races with + * ->remove() by the device lock, which must be held by the caller. */ if (err_handler && err_handler->reset_done) err_handler->reset_done(dev); diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index b314b54f7821..42385fe6b7fa 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -54,9 +54,10 @@ static int report_error_detected(struct pci_dev *dev, const struct pci_error_handlers *err_handler; 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 || !pdrv->err_handler->error_detected) { /* * If any device in the subtree does not have an error_detected @@ -92,13 +93,14 @@ static int report_normal_detected(struct pci_dev *dev, void *data) static int report_mmio_enabled(struct pci_dev *dev, void *data) { - pci_ers_result_t vote, *result = data; struct pci_driver *pdrv; + pci_ers_result_t vote, *result = data; const struct pci_error_handlers *err_handler; device_lock(&dev->dev); - if (!dev->dev.driver || - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler || + pdrv = to_pci_driver(dev->dev.driver); + if (!pdrv || + !pdrv->err_handler || !pdrv->err_handler->mmio_enabled) goto out; @@ -112,13 +114,14 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data) static int report_slot_reset(struct pci_dev *dev, void *data) { + struct pci_driver *pdrv; pci_ers_result_t vote, *result = data; const struct pci_error_handlers *err_handler; - struct pci_driver *pdrv; device_lock(&dev->dev); - if (!dev->dev.driver || - !(pdrv = to_pci_driver(dev->dev.driver))->err_handler || + pdrv = to_pci_driver(dev->dev.driver); + if (!pdrv || + !pdrv->err_handler || !pdrv->err_handler->slot_reset) goto out; @@ -132,13 +135,14 @@ static int report_slot_reset(struct pci_dev *dev, void *data) static int report_resume(struct pci_dev *dev, void *data) { - const struct pci_error_handlers *err_handler; struct pci_driver *pdrv; + const struct pci_error_handlers *err_handler; device_lock(&dev->dev); + pdrv = dev->driver; 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; diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index 73831fb87a1e..0ec76b4af16f 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -588,7 +588,6 @@ static pci_ers_result_t pcifront_common_process(int cmd, struct pcifront_device *pdev, pci_channel_state_t state) { - pci_ers_result_t result; struct pci_driver *pdrv; int bus = pdev->sh_info->aer_op.bus; int devfn = pdev->sh_info->aer_op.devfn; @@ -598,13 +597,12 @@ static pci_ers_result_t pcifront_common_process(int cmd, dev_dbg(&pdev->xdev->dev, "pcifront AER process: cmd %x (bus:%x, devfn%x)", cmd, bus, devfn); - 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); @@ -612,27 +610,18 @@ static pci_ers_result_t pcifront_common_process(int cmd, pci_dbg(pcidev, "trying to call AER service\n"); switch (cmd) { case XEN_PCI_OP_aer_detected: - result = pdrv->err_handler-> - error_detected(pcidev, state); - break; + return pdrv->err_handler->error_detected(pcidev, state); case XEN_PCI_OP_aer_mmio: - result = pdrv->err_handler-> - mmio_enabled(pcidev); - break; + return pdrv->err_handler->mmio_enabled(pcidev); case XEN_PCI_OP_aer_slotreset: - result = pdrv->err_handler-> - slot_reset(pcidev); - break; + return pdrv->err_handler->slot_reset(pcidev); case XEN_PCI_OP_aer_resume: pdrv->err_handler->resume(pcidev); - break; + return PCI_ERS_RESULT_NONE; default: dev_err(&pdev->xdev->dev, - "bad request in aer recovery " - "operation!\n"); + "bad request in AER recovery operation!\n"); } - - return result; } return PCI_ERS_RESULT_NONE; diff --git a/include/linux/pci.h b/include/linux/pci.h index 7c1ceb39035c..03bfdb25a55c 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -899,7 +899,10 @@ struct pci_driver { struct pci_dynids dynids; }; -#define to_pci_driver(drv) container_of(drv, struct pci_driver, driver) +static inline struct pci_driver *to_pci_driver(struct device_driver *drv) +{ + return drv ? container_of(drv, struct pci_driver, driver) : NULL; +} /** * PCI_DEVICE - macro used to describe a specific PCI device
On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote: > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: > > Hello, > > > > this is v6 of the quest to drop the "driver" member from struct pci_dev > > which tracks the same data (apart from a constant offset) as dev.driver. > > I like this a lot and applied it to pci/driver for v5.16, thanks! > > 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. OK. > Full interdiff from this v6 series: > > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c > index deaaef6efe34..36e84d904260 100644 > --- a/arch/x86/kernel/probe_roms.c > +++ b/arch/x86/kernel/probe_roms.c > @@ -80,17 +80,15 @@ static struct resource video_rom_resource = { > */ > 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; > > - if (pdev->dev.driver) { > - struct pci_driver *drv = to_pci_driver(pdev->dev.driver); > - for (id = drv->id_table; id && id->vendor; id++) > - if (id->vendor == vendor && id->device == device) > - break; > - } > + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++) > + if (id->vendor == vendor && id->device == device) > + break; > > return id && id->vendor; > } > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c > index d997c9c3ebb5..7eb3706cf42d 100644 > --- a/drivers/misc/cxl/guest.c > +++ b/drivers/misc/cxl/guest.c > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu, > pci_channel_state_t state) > { > struct pci_dev *afu_dev; > + struct pci_driver *afu_drv; > + struct pci_error_handlers *err_handler; These two could be moved into the for loop (where afu_drv was with my patch already). This is also possible in a few other drivers. Best regards Uwe
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?
On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote: > On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote: > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: > > > Hello, > > > > > > this is v6 of the quest to drop the "driver" member from struct pci_dev > > > which tracks the same data (apart from a constant offset) as dev.driver. > > > > I like this a lot and applied it to pci/driver for v5.16, thanks! > > > > 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. > > OK. > > > Full interdiff from this v6 series: > > > > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c > > index deaaef6efe34..36e84d904260 100644 > > --- a/arch/x86/kernel/probe_roms.c > > +++ b/arch/x86/kernel/probe_roms.c > > @@ -80,17 +80,15 @@ static struct resource video_rom_resource = { > > */ > > 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; > > > > - if (pdev->dev.driver) { > > - struct pci_driver *drv = to_pci_driver(pdev->dev.driver); > > - for (id = drv->id_table; id && id->vendor; id++) > > - if (id->vendor == vendor && id->device == device) > > - break; > > - } > > + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++) > > + if (id->vendor == vendor && id->device == device) > > + break; > > > > return id && id->vendor; > > } > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c > > index d997c9c3ebb5..7eb3706cf42d 100644 > > --- a/drivers/misc/cxl/guest.c > > +++ b/drivers/misc/cxl/guest.c > > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu, > > pci_channel_state_t state) > > { > > struct pci_dev *afu_dev; > > + struct pci_driver *afu_drv; > > + struct pci_error_handlers *err_handler; > > These two could be moved into the for loop (where afu_drv was with my > patch already). This is also possible in a few other drivers. That's true, they could. I tried to follow the prevailing style in the file. At least in cxl, I didn't see any other cases of declarations being in the minimal scope like that. Bjorn
Hello, On Wed, Oct 13, 2021 at 05:54:28AM -0500, Bjorn Helgaas wrote: > On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote: > > On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote: > > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c > > > index d997c9c3ebb5..7eb3706cf42d 100644 > > > --- a/drivers/misc/cxl/guest.c > > > +++ b/drivers/misc/cxl/guest.c > > > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu, > > > pci_channel_state_t state) > > > { > > > struct pci_dev *afu_dev; > > > + struct pci_driver *afu_drv; > > > + struct pci_error_handlers *err_handler; > > > > These two could be moved into the for loop (where afu_drv was with my > > patch already). This is also possible in a few other drivers. > > That's true, they could. I tried to follow the prevailing style in > the file. At least in cxl, I didn't see any other cases of > declarations being in the minimal scope like that. I don't care much, do whatever you consider nice. I'm happy you liked the cleanup and that you took it. Best regards Uwe
On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > 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. It is a little unusual. I only found three of 77 that are NULL-aware: to_moxtet_driver() to_siox_driver() to_spi_driver() It seems worthwhile to me because it makes the patch and the resulting code significantly cleaner. Here's one example without the NULL check: @@ -493,12 +493,15 @@ static void pci_device_remove(struct device *dev) static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; pm_runtime_resume(dev); - if (drv && drv->shutdown) - drv->shutdown(pci_dev); + if (pci_dev->dev.driver) { + struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + + if (drv->shutdown) + drv->shutdown(pci_dev); + } static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); pm_runtime_resume(dev); if (pci_dev->dev.driver) { struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); if (drv->shutdown) drv->shutdown(pci_dev); } and here's the same thing with the NULL check: @@ -493,7 +493,7 @@ static void pci_device_remove(struct device *dev) static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; + struct pci_driver *drv = to_pci_driver(dev->driver); static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); struct pci_driver *drv = to_pci_driver(dev->driver); pm_runtime_resume(dev); if (drv && drv->shutdown) drv->shutdown(pci_dev); > > 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; Good cleanup for a follow-up patch, but doesn't seem directly related to the objective here. The current patch is: @@ -80,7 +80,7 @@ static struct resource video_rom_resource = { */ static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device) { - struct pci_driver *drv = pdev->driver; + struct pci_driver *drv = to_pci_driver(pdev->dev.driver); const struct pci_device_id *id; if (pdev->vendor == vendor && pdev->device == device) > > device_lock(&vf_dev->dev); > > - if (vf_dev->dev.driver) { > > + if (to_pci_driver(vf_dev->dev.driver)) { > > Hmm... Yeah, it could be either of: if (to_pci_driver(vf_dev->dev.driver)) if (vf_dev->dev.driver) I went back and forth on that and went with to_pci_driver() on the theory that we were testing the pci_driver * before and the patch is more of a mechanical change and easier to review if we test the pci_driver * after. > > + if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 > > > + && pci_dev->current_state != PCI_UNKNOWN) { > > Can we keep && on the previous line? I think this is in pci_legacy_suspend(), and I didn't touch that line. It shows up in the interdiff because without the NULL check in to_pci_driver(), we had to indent this code another level. With the NULL check, we don't need that extra indentation. > > + 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? I don't think I touched that 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)? Yes, I think so, but not sure what you're getting at here, can you elaborate? > > 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? Could, but the "dev->driver" to "to_pci_driver(dev->dev.driver)" changes are the heart of this patch, and I don't like to clutter it with unrelated changes. > > - 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? Could possibly be cleaned up. Felt like feature creep so I didn't. > > 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? Could be skipped. The string now fits on one line so I combined it to make it more greppable. Bjorn
On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote: > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > > 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: ... > > It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC. > > It is a little unusual. I only found three of 77 that are NULL-aware: > > to_moxtet_driver() > to_siox_driver() > to_spi_driver() > > It seems worthwhile to me because it makes the patch and the resulting > code significantly cleaner. I'm not objecting the change, just a remark. ... > > > + 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; > > Good cleanup for a follow-up patch, but doesn't seem directly related > to the objective here. True. Maybe you can bake one while not forgotten? ... > > > + return drv && drv->resume ? > > > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); > > > > One line? > > I don't think I touched that line. Then why they are both in + section? ... > > > + 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)? > > Yes, I think so, but not sure what you're getting at here, can you > elaborate? Getting pointer from another pointer seems waste of resources, why we can't simply struct pci_driver *drv = dev->driver; ? ... > > Stray change? Or is it in a separate patch in your tree? > > Could be skipped. The string now fits on one line so I combined it to > make it more greppable. This is inconsistency in your changes, in one case you are objecting of doing something close to the changed lines, in the other you are doing unrelated change.
On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote: > On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote: > > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > > > 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: > > > > + return drv && drv->resume ? > > > > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); > > > > > > One line? > > > > I don't think I touched that line. > > Then why they are both in + section? They're both in the + section of the interdiff because Uwe's v6 patch looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - return drv && drv->resume ? - drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); + if (pci_dev->dev.driver) { + struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + + if (drv->resume) + return drv->resume(pci_dev); + } + + return pci_pm_reenable_device(pci_dev); and my revision looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; + struct pci_driver *drv = to_pci_driver(dev->driver); so compared to Uwe's v6, I restored that section to the original code. My goal here was to make the patch as simple and easy to review as possible. > > > > + 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)? > > > > Yes, I think so, but not sure what you're getting at here, can you > > elaborate? > > Getting pointer from another pointer seems waste of resources, why we > can't simply > > struct pci_driver *drv = dev->driver; I think this is in pci_dev_save_and_disable(), and "dev" here is a struct pci_dev *. We're removing the dev->driver member. Let me know if I'm still missing something. > > > > - "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? > > > > Could be skipped. The string now fits on one line so I combined it to > > make it more greppable. > > This is inconsistency in your changes, in one case you are objecting of > doing something close to the changed lines, in the other you are doing > unrelated change. You're right, this didn't make much sense in that patch. I moved the line join to the previous patch, which unindented this section and made space for this to fit on one line. Here's the revised commit: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287
On Fri, Oct 15, 2021 at 7:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote: ... > so compared to Uwe's v6, I restored that section to the original code. > My goal here was to make the patch as simple and easy to review as > possible. Thanks for elaboration. I have got it. ... > You're right, this didn't make much sense in that patch. I moved the > line join to the previous patch, which unindented this section and > made space for this to fit on one line. Here's the revised commit: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287 Side remark: default without break or return is error prone (okay, to some extent). Perhaps adding the return statement there will make things robust and clean.
Hello, this is v6 of the quest to drop the "driver" member from struct pci_dev which tracks the same data (apart from a constant offset) as dev.driver. Changes since v5: - Some Acks added - Some fixes in "PCI: Replace pci_dev::driver usage by pci_dev::dev.driver" to properly handle that to_pci_driver(X) is wrong if X is NULL. This should fix the problem reported by Ido Schimmel. Full range diff below. This patch stack survived an allmodconfig build on arm64, m68k, powerpc, riscv, s390, sparc64 and x86_64 on top of v5.15-rc3. Best regards Uwe Uwe Kleine-König (11): PCI: Simplify pci_device_remove() PCI: Drop useless check from pci_device_probe() xen/pci: Drop some checks that are always true bcma: simplify reference to the driver's name powerpc/eeh: Don't use driver member of struct pci_dev and further cleanups ssb: Simplify determination of driver name PCI: Replace pci_dev::driver usage that gets the driver name scsi: message: fusion: Remove unused parameter of mpt_pci driver's probe() crypto: qat - simplify adf_enable_aer() PCI: Replace pci_dev::driver usage by pci_dev::dev.driver PCI: Drop duplicated tracking of a pci_dev's bound driver arch/powerpc/include/asm/ppc-pci.h | 5 - arch/powerpc/kernel/eeh.c | 8 ++ arch/powerpc/kernel/eeh_driver.c | 10 +- arch/x86/events/intel/uncore.c | 2 +- arch/x86/kernel/probe_roms.c | 10 +- drivers/bcma/host_pci.c | 6 +- drivers/crypto/hisilicon/qm.c | 2 +- drivers/crypto/qat/qat_4xxx/adf_drv.c | 7 +- drivers/crypto/qat/qat_c3xxx/adf_drv.c | 7 +- drivers/crypto/qat/qat_c62x/adf_drv.c | 7 +- drivers/crypto/qat/qat_common/adf_aer.c | 10 +- .../crypto/qat/qat_common/adf_common_drv.h | 3 +- drivers/crypto/qat/qat_dh895xcc/adf_drv.c | 7 +- drivers/message/fusion/mptbase.c | 7 +- drivers/message/fusion/mptbase.h | 2 +- drivers/message/fusion/mptctl.c | 4 +- drivers/message/fusion/mptlan.c | 2 +- drivers/misc/cxl/guest.c | 24 +++-- drivers/misc/cxl/pci.c | 30 +++--- .../ethernet/hisilicon/hns3/hns3_ethtool.c | 2 +- .../ethernet/marvell/prestera/prestera_pci.c | 2 +- drivers/net/ethernet/mellanox/mlxsw/pci.c | 2 +- .../ethernet/netronome/nfp/nfp_net_ethtool.c | 3 +- drivers/pci/iov.c | 33 +++++-- drivers/pci/pci-driver.c | 96 ++++++++++--------- drivers/pci/pci.c | 4 +- drivers/pci/pcie/err.c | 36 +++---- drivers/pci/xen-pcifront.c | 63 ++++++------ drivers/ssb/pcihost_wrapper.c | 6 +- drivers/usb/host/xhci-pci.c | 2 +- include/linux/pci.h | 1 - 31 files changed, 208 insertions(+), 195 deletions(-) Range-diff against v5: -: ------------ > 1: c2b53ab26a6b PCI: Simplify pci_device_remove() -: ------------ > 2: 2c733e1d5186 PCI: Drop useless check from pci_device_probe() -: ------------ > 3: 547ca5a7aa16 xen/pci: Drop some checks that are always true -: ------------ > 4: 40eb07353844 bcma: simplify reference to the driver's name -: ------------ > 5: bab59c1dff6d powerpc/eeh: Don't use driver member of struct pci_dev and further cleanups 1: abd70de9782d ! 6: 92f4d61bbac3 ssb: Simplify determination of driver name @@ Commit message This has the upside of not requiring the driver member of struct pci_dev which is about to be removed and being simpler. + Acked-by: Michael Büsch <m@bues.ch> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> ## drivers/ssb/pcihost_wrapper.c ## 2: 735845bd26b9 ! 7: 6303f03ab2aa PCI: Replace pci_dev::driver usage that gets the driver name @@ Commit message driver name by dev_driver_string() which implicitly makes use of struct pci_dev::dev->driver. + Acked-by: Simon Horman <simon.horman@corigine.com> (for NFP) Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> ## drivers/crypto/hisilicon/qm.c ## 3: 1e58019165b9 = 8: 658a6c00ec96 scsi: message: fusion: Remove unused parameter of mpt_pci driver's probe() 4: dea72a470141 = 9: aceaf5321603 crypto: qat - simplify adf_enable_aer() 5: b4165dda38ea ! 10: 80648d999985 PCI: Replace pci_dev::driver usage by pci_dev::dev.driver @@ arch/x86/kernel/probe_roms.c: static struct resource video_rom_resource = { static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device) { - struct pci_driver *drv = pdev->driver; -+ 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; ++ if (pdev->dev.driver) { ++ struct pci_driver *drv = to_pci_driver(pdev->dev.driver); ++ for (id = drv->id_table; id && id->vendor; id++) ++ if (id->vendor == vendor && id->device == device) ++ break; ++ } + + return id && id->vendor; + } ## drivers/misc/cxl/guest.c ## @@ drivers/misc/cxl/guest.c: static void pci_error_handlers(struct cxl_afu *afu, @@ drivers/pci/iov.c: static ssize_t sriov_vf_total_msix_show(struct device *dev, device_lock(dev); - if (!pdev->driver || !pdev->driver->sriov_get_vf_total_msix) -+ pdrv = to_pci_driver(dev->driver); -+ if (!pdrv || !pdrv->sriov_get_vf_total_msix) ++ if (!dev->driver) goto unlock; - vf_total_msix = pdev->driver->sriov_get_vf_total_msix(pdev); ++ pdrv = to_pci_driver(dev->driver); ++ if (!pdrv->sriov_get_vf_total_msix) ++ goto unlock; ++ + vf_total_msix = pdrv->sriov_get_vf_total_msix(pdev); unlock: device_unlock(dev); @@ drivers/pci/iov.c: static ssize_t sriov_vf_msix_count_store(struct device *dev, device_lock(&pdev->dev); - if (!pdev->driver || !pdev->driver->sriov_set_msix_vec_count) { ++ if (!pdev->dev.driver) { ++ ret = -EOPNOTSUPP; ++ goto err_pdev; ++ } ++ + pdrv = to_pci_driver(pdev->dev.driver); -+ if (!pdrv || !pdrv->sriov_set_msix_vec_count) { ++ if (!pdrv->sriov_set_msix_vec_count) { ret = -EOPNOTSUPP; goto err_pdev; } @@ drivers/pci/pci-driver.c: static void pci_device_remove(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; -+ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); pm_runtime_resume(dev); +- if (drv && drv->shutdown) +- drv->shutdown(pci_dev); ++ if (pci_dev->dev.driver) { ++ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); ++ ++ if (drv->shutdown) ++ drv->shutdown(pci_dev); ++ } + + /* + * If this is a kexec reboot, turn off Bus Master bit on the @@ drivers/pci/pci-driver.c: static int pci_pm_reenable_device(struct pci_dev *pci_dev) static int pci_legacy_suspend(struct device *dev, pm_message_t state) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; -+ struct pci_driver *drv = to_pci_driver(dev->driver); - if (drv && drv->suspend) { - pci_power_t prev = pci_dev->current_state; +- if (drv && drv->suspend) { +- pci_power_t prev = pci_dev->current_state; +- int error; ++ if (dev->driver) { ++ struct pci_driver *drv = to_pci_driver(dev->driver); + +- error = drv->suspend(pci_dev, state); +- suspend_report_result(drv->suspend, error); +- if (error) +- return error; ++ if (drv->suspend) { ++ pci_power_t prev = pci_dev->current_state; ++ int error; + +- if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 +- && pci_dev->current_state != PCI_UNKNOWN) { +- pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev, +- "PCI PM: Device state not saved by %pS\n", +- drv->suspend); ++ error = drv->suspend(pci_dev, state); ++ suspend_report_result(drv->suspend, error); ++ if (error) ++ return error; ++ ++ if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0 ++ && pci_dev->current_state != PCI_UNKNOWN) { ++ pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev, ++ "PCI PM: Device state not saved by %pS\n", ++ drv->suspend); ++ } + } + } + @@ drivers/pci/pci-driver.c: static int pci_legacy_suspend_late(struct device *dev, pm_message_t state) static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; -+ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); pci_fixup_device(pci_fixup_resume, pci_dev); +- return drv && drv->resume ? +- drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); ++ if (pci_dev->dev.driver) { ++ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); ++ ++ if (drv->resume) ++ return drv->resume(pci_dev); ++ } ++ ++ return pci_pm_reenable_device(pci_dev); + } + + /* Auxiliary functions used by the new power management framework */ @@ drivers/pci/pci-driver.c: static void pci_pm_default_suspend(struct pci_dev *pci_dev) static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev) { - struct pci_driver *drv = pci_dev->driver; -+ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); - bool ret = drv && (drv->suspend || drv->resume); +- bool ret = drv && (drv->suspend || drv->resume); ++ struct pci_driver *drv; ++ bool ret; ++ ++ if (!pci_dev->dev.driver) ++ return false; ++ ++ drv = to_pci_driver(pci_dev->dev.driver); ++ ret = drv && (drv->suspend || drv->resume); /* + * Legacy PM support is used by default, so warn if the new framework is @@ drivers/pci/pci-driver.c: static int pci_pm_runtime_suspend(struct device *dev) int error; 6: d93a138bd7ab = 11: 2686d69bca17 PCI: Drop duplicated tracking of a pci_dev's bound driver base-commit: 5816b3e6577eaa676ceb00a848f0fd65fe2adc29