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@xxxxxxxxxxxxxx): > > - 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
