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@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! \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@xxxxxxxxxxxxxx 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/ |
Attachment:
signature.asc
Description: PGP signature
