| Message ID | 20210803100150.1543597-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 Tue, Aug 03, 2021 at 12:01:44PM +0200, Uwe Kleine-König wrote: > Hello, > > changes since v1 (https://lore.kernel.org/linux-pci/20210729203740.1377045-1-u.kleine-koenig@pengutronix.de): > > - New patch to simplify drivers/pci/xen-pcifront.c, spotted and > suggested by Boris Ostrovsky > - Fix a possible NULL pointer dereference I introduced in xen-pcifront.c > - A few whitespace improvements > - Add a commit log to patch #6 (formerly #5) > > I also expanded the audience for patches #4 and #6 to allow affected > people to actually see the changes to their drivers. > > Interdiff can be found below. > > The idea is still the same: After a few cleanups (#1 - #3) a new macro > is introduced abstracting access to struct pci_dev->driver. All users > are then converted to use this and in the last patch the macro is > changed to make use of struct pci_dev::dev->driver to get rid of the > duplicated tracking. I love the idea of this series! I looked at all the bus_type.probe() methods, it looks like pci_dev is not the only offender here. At least the following also have a driver pointer in the device struct: parisc_device.driver acpi_device.driver dio_dev.driver hid_device.driver pci_dev.driver pnp_dev.driver rio_dev.driver zorro_dev.driver Do you plan to do the same for all of them, or is there some reason why they need the pointer and PCI doesn't? In almost all cases, other buses define a "to_<bus>_driver()" interface. In fact, PCI already has a to_pci_driver(). This series adds pci_driver_of_dev(), which basically just means we can do this: pdrv = pci_driver_of_dev(pdev); instead of this: pdrv = to_pci_driver(pdev->dev.driver); I don't see any other "<bus>_driver_of_dev()" interfaces, so I assume other buses just live with the latter style? I'd rather not be different and have two ways to get the "struct pci_driver *" unless there's a good reason. Looking through the places that care about pci_dev.driver (the ones updated by patch 5/6), many of them are ... a little dubious to begin with. A few need the "struct pci_error_handlers *err_handler" pointer, so that's probably legitimate. But many just need a name, and should probably be using dev_driver_string() instead. Bjorn
Hello Bjorn, On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > On Tue, Aug 03, 2021 at 12:01:44PM +0200, Uwe Kleine-König wrote: > > Hello, > > > > changes since v1 (https://lore.kernel.org/linux-pci/20210729203740.1377045-1-u.kleine-koenig@pengutronix.de): > > > > - New patch to simplify drivers/pci/xen-pcifront.c, spotted and > > suggested by Boris Ostrovsky > > - Fix a possible NULL pointer dereference I introduced in xen-pcifront.c > > - A few whitespace improvements > > - Add a commit log to patch #6 (formerly #5) > > > > I also expanded the audience for patches #4 and #6 to allow affected > > people to actually see the changes to their drivers. > > > > Interdiff can be found below. > > > > The idea is still the same: After a few cleanups (#1 - #3) a new macro > > is introduced abstracting access to struct pci_dev->driver. All users > > are then converted to use this and in the last patch the macro is > > changed to make use of struct pci_dev::dev->driver to get rid of the > > duplicated tracking. > > I love the idea of this series! \o/ > I looked at all the bus_type.probe() methods, it looks like pci_dev is > not the only offender here. At least the following also have a driver > pointer in the device struct: > > parisc_device.driver > acpi_device.driver > dio_dev.driver > hid_device.driver > pci_dev.driver > pnp_dev.driver > rio_dev.driver > zorro_dev.driver Right, when I converted zorro_dev it was pointed out that the code was copied from pci and the latter has the same construct. :-) See https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de for the patch, I don't find where pci was pointed out, maybe it was on irc only. > Do you plan to do the same for all of them, or is there some reason > why they need the pointer and PCI doesn't? There is a list of cleanup stuff I intend to work on. Considering how working on that list only made it longer in the recent past, maybe it makes more sense to not work on it :-) > In almost all cases, other buses define a "to_<bus>_driver()" > interface. In fact, PCI already has a to_pci_driver(). > > This series adds pci_driver_of_dev(), which basically just means we > can do this: > > pdrv = pci_driver_of_dev(pdev); > > instead of this: > > pdrv = to_pci_driver(pdev->dev.driver); > > I don't see any other "<bus>_driver_of_dev()" interfaces, so I assume > other buses just live with the latter style? I'd rather not be > different and have two ways to get the "struct pci_driver *" unless > there's a good reason. Among few the busses I already fixed in this regard pci was the first that has a considerable amount of usage. So I considered it worth giving it a name. > Looking through the places that care about pci_dev.driver (the ones > updated by patch 5/6), many of them are ... a little dubious to begin > with. A few need the "struct pci_error_handlers *err_handler" > pointer, so that's probably legitimate. But many just need a name, > and should probably be using dev_driver_string() instead. Yeah, I considered adding a function to get the driver name from a pci_dev and a function to get the error handlers. Maybe it's an idea to introduce these two and then use to_pci_driver(pdev->dev.driver) for the few remaining users? Maybe doing that on top of my current series makes sense to have a clean switch from pdev->driver to pdev->dev.driver?! Best regards Uwe
On Fri, Aug 06, 2021 at 08:46:23AM +0200, Uwe Kleine-König wrote: > On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > > I looked at all the bus_type.probe() methods, it looks like pci_dev is > > not the only offender here. At least the following also have a driver > > pointer in the device struct: > > > > parisc_device.driver > > acpi_device.driver > > dio_dev.driver > > hid_device.driver > > pci_dev.driver > > pnp_dev.driver > > rio_dev.driver > > zorro_dev.driver > > Right, when I converted zorro_dev it was pointed out that the code was > copied from pci and the latter has the same construct. :-) > See > https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de > for the patch, I don't find where pci was pointed out, maybe it was on > irc only. Oh, thanks! I looked to see if you'd done something similar elsewhere, but I missed this one. > > Looking through the places that care about pci_dev.driver (the ones > > updated by patch 5/6), many of them are ... a little dubious to begin > > with. A few need the "struct pci_error_handlers *err_handler" > > pointer, so that's probably legitimate. But many just need a name, > > and should probably be using dev_driver_string() instead. > > Yeah, I considered adding a function to get the driver name from a > pci_dev and a function to get the error handlers. Maybe it's an idea to > introduce these two and then use to_pci_driver(pdev->dev.driver) for the > few remaining users? Maybe doing that on top of my current series makes > sense to have a clean switch from pdev->driver to pdev->dev.driver?! I'd propose using dev_driver_string() for these places: eeh_driver_name() (could change callers to use dev_driver_string()) bcma_host_pci_probe() qm_alloc_uacce() hns3_get_drvinfo() prestera_pci_probe() mlxsw_pci_probe() nfp_get_drvinfo() ssb_pcihost_probe() The use in mpt_device_driver_register() looks unnecessary: it's only to get a struct pci_device_id *, which is passed to ->probe() functions that don't need it. The use in adf_enable_aer() looks wrong: it sets the err_handler pointer in one of the adf_driver structs. I think those structs should be basically immutable, and the drivers that call adf_enable_aer() from their .probe() methods should set ".err_handler = &adf_err_handler" in their static adf_driver definitions instead. I think that basically leaves these: uncore_pci_probe() # .id_table, custom driver "registration" match_id() # .id_table, arch/x86/kernel/probe_roms.c xhci_pci_quirks() # .id_table pci_error_handlers() # roll-your-own AER handling, drivers/misc/cxl/guest.c I think it would be fine to use to_pci_driver(pdev->dev.driver) for these few. Bjorn
On Fri, Aug 06, 2021 at 04:24:52PM -0500, Bjorn Helgaas wrote: > On Fri, Aug 06, 2021 at 08:46:23AM +0200, Uwe Kleine-König wrote: > > On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > > > > I looked at all the bus_type.probe() methods, it looks like pci_dev is > > > not the only offender here. At least the following also have a driver > > > pointer in the device struct: > > > > > > parisc_device.driver > > > acpi_device.driver > > > dio_dev.driver > > > hid_device.driver > > > pci_dev.driver > > > pnp_dev.driver > > > rio_dev.driver > > > zorro_dev.driver > > > > Right, when I converted zorro_dev it was pointed out that the code was > > copied from pci and the latter has the same construct. :-) > > See > > https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de > > for the patch, I don't find where pci was pointed out, maybe it was on > > irc only. > > Oh, thanks! I looked to see if you'd done something similar > elsewhere, but I missed this one. > > > > Looking through the places that care about pci_dev.driver (the ones > > > updated by patch 5/6), many of them are ... a little dubious to begin > > > with. A few need the "struct pci_error_handlers *err_handler" > > > pointer, so that's probably legitimate. But many just need a name, > > > and should probably be using dev_driver_string() instead. > > > > Yeah, I considered adding a function to get the driver name from a > > pci_dev and a function to get the error handlers. Maybe it's an idea to > > introduce these two and then use to_pci_driver(pdev->dev.driver) for the > > few remaining users? Maybe doing that on top of my current series makes > > sense to have a clean switch from pdev->driver to pdev->dev.driver?! > > I'd propose using dev_driver_string() for these places: > > eeh_driver_name() (could change callers to use dev_driver_string()) > bcma_host_pci_probe() > qm_alloc_uacce() > hns3_get_drvinfo() > prestera_pci_probe() > mlxsw_pci_probe() > nfp_get_drvinfo() > ssb_pcihost_probe() So the idea is: PCI: Simplify pci_device_remove() PCI: Drop useless check from pci_device_probe() xen/pci: Drop some checks that are always true are kept as is as preparation. (Do you want to take them from this v2, or should I include them again in v3?) Then convert the list of functions above to use dev_driver_string() in a 4th patch. > The use in mpt_device_driver_register() looks unnecessary: it's only > to get a struct pci_device_id *, which is passed to ->probe() > functions that don't need it. This is patch #5. > The use in adf_enable_aer() looks wrong: it sets the err_handler > pointer in one of the adf_driver structs. I think those structs > should be basically immutable, and the drivers that call > adf_enable_aer() from their .probe() methods should set > ".err_handler = &adf_err_handler" in their static adf_driver > definitions instead. I don't understand that one without some research, probably this yields at least one patch. > I think that basically leaves these: > > uncore_pci_probe() # .id_table, custom driver "registration" > match_id() # .id_table, arch/x86/kernel/probe_roms.c > xhci_pci_quirks() # .id_table > pci_error_handlers() # roll-your-own AER handling, drivers/misc/cxl/guest.c > > I think it would be fine to use to_pci_driver(pdev->dev.driver) for > these few. Converting these will be patch 7 then and patch 8 can then drop the duplicated handling. Sounds reasonable? Best regards Uwe
On Sat, Aug 07, 2021 at 11:26:45AM +0200, Uwe Kleine-König wrote: > On Fri, Aug 06, 2021 at 04:24:52PM -0500, Bjorn Helgaas wrote: > > On Fri, Aug 06, 2021 at 08:46:23AM +0200, Uwe Kleine-König wrote: > > > On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > > > > > > I looked at all the bus_type.probe() methods, it looks like pci_dev is > > > > not the only offender here. At least the following also have a driver > > > > pointer in the device struct: > > > > > > > > parisc_device.driver > > > > acpi_device.driver > > > > dio_dev.driver > > > > hid_device.driver > > > > pci_dev.driver > > > > pnp_dev.driver > > > > rio_dev.driver > > > > zorro_dev.driver > > > > > > Right, when I converted zorro_dev it was pointed out that the code was > > > copied from pci and the latter has the same construct. :-) > > > See > > > https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de > > > for the patch, I don't find where pci was pointed out, maybe it was on > > > irc only. > > > > Oh, thanks! I looked to see if you'd done something similar > > elsewhere, but I missed this one. > > > > > > Looking through the places that care about pci_dev.driver (the ones > > > > updated by patch 5/6), many of them are ... a little dubious to begin > > > > with. A few need the "struct pci_error_handlers *err_handler" > > > > pointer, so that's probably legitimate. But many just need a name, > > > > and should probably be using dev_driver_string() instead. > > > > > > Yeah, I considered adding a function to get the driver name from a > > > pci_dev and a function to get the error handlers. Maybe it's an idea to > > > introduce these two and then use to_pci_driver(pdev->dev.driver) for the > > > few remaining users? Maybe doing that on top of my current series makes > > > sense to have a clean switch from pdev->driver to pdev->dev.driver?! > > > > I'd propose using dev_driver_string() for these places: > > > > eeh_driver_name() (could change callers to use dev_driver_string()) > > bcma_host_pci_probe() > > qm_alloc_uacce() > > hns3_get_drvinfo() > > prestera_pci_probe() > > mlxsw_pci_probe() > > nfp_get_drvinfo() > > ssb_pcihost_probe() > > So the idea is: > > PCI: Simplify pci_device_remove() > PCI: Drop useless check from pci_device_probe() > xen/pci: Drop some checks that are always true > > are kept as is as preparation. (Do you want to take them from this v2, > or should I include them again in v3?) Easiest if you include them until we merge the series. > Then convert the list of functions above to use dev_driver_string() in a > 4th patch. > > > The use in mpt_device_driver_register() looks unnecessary: it's only > > to get a struct pci_device_id *, which is passed to ->probe() > > functions that don't need it. > > This is patch #5. > > > The use in adf_enable_aer() looks wrong: it sets the err_handler > > pointer in one of the adf_driver structs. I think those structs > > should be basically immutable, and the drivers that call > > adf_enable_aer() from their .probe() methods should set > > ".err_handler = &adf_err_handler" in their static adf_driver > > definitions instead. > > I don't understand that one without some research, probably this yields > at least one patch. Yeah, it's a little messy because you'd have to make adf_err_handler non-static and add an extern for it. Sample below. > > I think that basically leaves these: > > > > uncore_pci_probe() # .id_table, custom driver "registration" > > match_id() # .id_table, arch/x86/kernel/probe_roms.c > > xhci_pci_quirks() # .id_table > > pci_error_handlers() # roll-your-own AER handling, drivers/misc/cxl/guest.c > > > > I think it would be fine to use to_pci_driver(pdev->dev.driver) for > > these few. > > Converting these will be patch 7 then and patch 8 can then drop the > duplicated handling. > > Sounds reasonable? Sounds good to me. Thanks for working on this! Bjorn diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c index a8805c815d16..75e6c5540523 100644 --- a/drivers/crypto/qat/qat_4xxx/adf_drv.c +++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c @@ -310,6 +310,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = adf_err_handler, }; module_pci_driver(adf_driver); diff --git a/drivers/crypto/qat/qat_common/adf_aer.c b/drivers/crypto/qat/qat_common/adf_aer.c index d2ae293d0df6..701c3c5f8b9b 100644 --- a/drivers/crypto/qat/qat_common/adf_aer.c +++ b/drivers/crypto/qat/qat_common/adf_aer.c @@ -166,7 +166,7 @@ static void adf_resume(struct pci_dev *pdev) dev_info(&pdev->dev, "Device is up and running\n"); } -static const struct pci_error_handlers adf_err_handler = { +const struct pci_error_handlers adf_err_handler = { .error_detected = adf_error_detected, .slot_reset = adf_slot_reset, .resume = adf_resume, @@ -187,7 +187,6 @@ int adf_enable_aer(struct adf_accel_dev *accel_dev) struct pci_dev *pdev = accel_to_pci_dev(accel_dev); struct pci_driver *pdrv = pdev->driver; - pdrv->err_handler = &adf_err_handler; pci_enable_pcie_error_reporting(pdev); return 0; } diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h index c61476553728..98a29e0b8769 100644 --- a/drivers/crypto/qat/qat_common/adf_common_drv.h +++ b/drivers/crypto/qat/qat_common/adf_common_drv.h @@ -95,6 +95,7 @@ void adf_ae_fw_release(struct adf_accel_dev *accel_dev); int adf_ae_start(struct adf_accel_dev *accel_dev); int adf_ae_stop(struct adf_accel_dev *accel_dev); +extern const struct pci_error_handlers adf_err_handler; int adf_enable_aer(struct adf_accel_dev *accel_dev); void adf_disable_aer(struct adf_accel_dev *accel_dev); void adf_reset_sbr(struct adf_accel_dev *accel_dev);
Hello, changes since v1 (https://lore.kernel.org/linux-pci/20210729203740.1377045-1-u.kleine-koenig@pengutronix.de): - New patch to simplify drivers/pci/xen-pcifront.c, spotted and suggested by Boris Ostrovsky - Fix a possible NULL pointer dereference I introduced in xen-pcifront.c - A few whitespace improvements - Add a commit log to patch #6 (formerly #5) I also expanded the audience for patches #4 and #6 to allow affected people to actually see the changes to their drivers. Interdiff can be found below. The idea is still the same: After a few cleanups (#1 - #3) a new macro is introduced abstracting access to struct pci_dev->driver. All users are then converted to use this and in the last patch the macro is changed to make use of struct pci_dev::dev->driver to get rid of the duplicated tracking. Best regards Uwe Uwe Kleine-König (6): PCI: Simplify pci_device_remove() PCI: Drop useless check from pci_device_probe() xen/pci: Drop some checks that are always true PCI: Provide wrapper to access a pci_dev's bound driver PCI: Adapt all code locations to not use struct pci_dev::driver directly PCI: Drop duplicated tracking of a pci_dev's bound driver arch/powerpc/include/asm/ppc-pci.h | 3 +- arch/powerpc/kernel/eeh_driver.c | 12 ++-- arch/x86/events/intel/uncore.c | 2 +- arch/x86/kernel/probe_roms.c | 2 +- drivers/bcma/host_pci.c | 6 +- drivers/crypto/hisilicon/qm.c | 2 +- drivers/crypto/qat/qat_common/adf_aer.c | 2 +- drivers/message/fusion/mptbase.c | 4 +- drivers/misc/cxl/guest.c | 21 +++---- drivers/misc/cxl/pci.c | 25 ++++---- .../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 | 2 +- drivers/pci/iov.c | 23 ++++--- drivers/pci/pci-driver.c | 48 +++++++-------- drivers/pci/pci.c | 10 ++-- drivers/pci/pcie/err.c | 35 ++++++----- drivers/pci/xen-pcifront.c | 60 ++++++++----------- drivers/ssb/pcihost_wrapper.c | 7 ++- drivers/usb/host/xhci-pci.c | 3 +- include/linux/pci.h | 2 +- 22 files changed, 145 insertions(+), 130 deletions(-) Range-diff against v1: 1: 7d97605df363 = 1: 8ba6e9faa18c PCI: Simplify pci_device_remove() 2: aec84c688d0f = 2: d8a7dc52091f PCI: Drop useless check from pci_device_probe() -: ------------ > 3: f4b78aa41776 xen/pci: Drop some checks that are always true 3: e6f933f532c9 = 4: 50f3daa64170 PCI: Provide wrapper to access a pci_dev's bound driver 4: d678a2924143 ! 5: 21cbd3f180a1 PCI: Adapt all code locations to not use struct pci_dev::driver directly @@ drivers/message/fusion/mptbase.c: mpt_device_driver_register(struct mpt_pci_driv - id = ioc->pcidev->driver ? - ioc->pcidev->driver->id_table : NULL; + struct pci_driver *pdrv = pci_driver_of_dev(ioc->pcidev); -+ id = pdrv ? pdrv->id_table : NULL; ++ id = pdrv ? pdrv->id_table : NULL; if (dd_cbfunc->probe) dd_cbfunc->probe(ioc->pcidev, id); } @@ drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c: static void hns3_get_drvinfo } - strncpy(drvinfo->driver, h->pdev->driver->name, -- sizeof(drvinfo->driver)); -+ strncpy(drvinfo->driver, pci_driver_of_dev(h->pdev)->name, sizeof(drvinfo->driver)); ++ strncpy(drvinfo->driver, pci_driver_of_dev(h->pdev)->name, + sizeof(drvinfo->driver)); drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0'; - strncpy(drvinfo->bus_info, pci_name(h->pdev), ## drivers/net/ethernet/marvell/prestera/prestera_pci.c ## @@ drivers/net/ethernet/marvell/prestera/prestera_pci.c: static int prestera_fw_load(struct prestera_fw *fw) @@ drivers/pci/xen-pcifront.c: static pci_ers_result_t pcifront_common_process(int pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn); - if (!pcidev || !pcidev->driver) { -+ pdrv = pci_driver_of_dev(pcidev); -+ if (!pcidev || !pdrv) { ++ if (!pcidev || !(pdrv = pci_driver_of_dev(pcidev))) { dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n"); pci_dev_put(pcidev); return result; } - pdrv = pcidev->driver; - if (pdrv) { - if (pdrv->err_handler && pdrv->err_handler->error_detected) { + if (pdrv->err_handler && pdrv->err_handler->error_detected) { + pci_dbg(pcidev, "trying to call AER service\n"); ## drivers/ssb/pcihost_wrapper.c ## @@ drivers/ssb/pcihost_wrapper.c: static int ssb_pcihost_probe(struct pci_dev *dev, @@ drivers/ssb/pcihost_wrapper.c: static int ssb_pcihost_probe(struct pci_dev *dev, name = dev_name(&dev->dev); - if (dev->driver && dev->driver->name) - name = dev->driver->name; -+ ++ + pdrv = pci_driver_of_dev(dev); + if (pdrv && pdrv->name) + name = pdrv->name; 5: 8c70ffd24380 ! 6: 02e6da6e5919 PCI: Drop duplicated tracking of a pci_dev's bound driver @@ Metadata ## Commit message ## PCI: Drop duplicated tracking of a pci_dev's bound driver + Currently it's tracked twice which driver is bound to a given pci + device. Now that all users of the pci specific one (struct + pci_dev::driver) are updated to use an access macro + (pci_driver_of_dev()), change the macro to use the information from the + driver core and remove the driver member from struct pci_dev. + Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> ## drivers/pci/pci-driver.c ## base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c