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:

quoted

Hello,

changes since v1 (https://lore.kernel.org/linux-pci/20210729203740.1377045-1-u.kleine-koenig@pengutronix.de (local)):

- 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 (local)
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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |