Re: [PATCH v2 0/6] PCI: Drop duplicated tracking of a pci_dev's bound driver

From: Bjorn Helgaas
Date: Thu Aug 05 2021 - 19:42:45 EST

On Tue, Aug 03, 2021 at 12:01:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> changes since v1 (
> - 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:


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.