| Message ID | 20210811080637.2596434-1-u.kleine-koenig@pengutronix.de |
|---|---|
| Headers | show |
| Series | PCI: Drop duplicated tracking of a pci_dev's bound driver | expand |
On Wed, Aug 11, 2021 at 10:06:33AM +0200, Uwe Kleine-K??nig wrote: > static inline const char *eeh_driver_name(struct pci_dev *pdev) > { > - return (pdev && pdev->driver) ? pdev->driver->name : "<null>"; > + const char *drvstr = pdev ? dev_driver_string(&pdev->dev) : ""; > + > + if (*drvstr == '\0') > + return "<null>"; > + > + return drvstr; This looks rather obsfucated due to the fact that dev_driver_string never returns '\0', and due to the strange mix of a tenary operation and the if on a related condition. > } > > #endif /* CONFIG_EEH */ > diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c > index 69c10a7b7c61..dc2ffa686964 100644 > --- a/drivers/bcma/host_pci.c > +++ b/drivers/bcma/host_pci.c > @@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev, > if (err) > goto err_kfree_bus; > > - name = dev_name(&dev->dev); > - if (dev->driver && dev->driver->name) > - name = dev->driver->name; > + name = dev_driver_string(&dev->dev); > + if (*name == '\0') > + name = dev_name(&dev->dev); Where does this '\0' check come from? > + > + name = dev_driver_string(&dev->dev); > + if (*name == '\0') > + name = dev_name(&dev->dev); > + More of this weirdness.
On Thu, Aug 12, 2021 at 08:07:20AM +0100, Christoph Hellwig wrote: > On Wed, Aug 11, 2021 at 10:06:33AM +0200, Uwe Kleine-K??nig wrote: > > static inline const char *eeh_driver_name(struct pci_dev *pdev) > > { > > - return (pdev && pdev->driver) ? pdev->driver->name : "<null>"; > > + const char *drvstr = pdev ? dev_driver_string(&pdev->dev) : ""; > > + > > + if (*drvstr == '\0') > > + return "<null>"; > > + > > + return drvstr; > > This looks rather obsfucated due to the fact that dev_driver_string > never returns '\0', and due to the strange mix of a tenary operation > and the if on a related condition. dev_driver_string() might return "" (via dev_bus_name()). If that happens *drvstr == '\0' becomes true. Would the following be better?: const char *drvstr; if (pdev) return "<null>"; drvstr = dev_driver_string(&pdev->dev); if (!strcmp(drvstr, "")) return "<null>"; return drvstr; When I thought about this hunk I considered it ugly to have "<null>" in it twice. > > } > > > > #endif /* CONFIG_EEH */ > > diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c > > index 69c10a7b7c61..dc2ffa686964 100644 > > --- a/drivers/bcma/host_pci.c > > +++ b/drivers/bcma/host_pci.c > > @@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev, > > if (err) > > goto err_kfree_bus; > > > > - name = dev_name(&dev->dev); > > - if (dev->driver && dev->driver->name) > > - name = dev->driver->name; > > + name = dev_driver_string(&dev->dev); > > + if (*name == '\0') > > + name = dev_name(&dev->dev); > > Where does this '\0' check come from? The original code is equivalent to if (dev->driver && dev->driver->name) name = dev->driver->name; else: name = dev_name(...); As dev_driver_string() implements something like: if (dev->driver && dev->driver->name) return dev->driver->name; else return ""; the change looks fine to me. (One could wonder if it's sensible to fall back to the device name if the driver has no nice name, but this isn't new with my change.) > > + name = dev_driver_string(&dev->dev); > > + if (*name == '\0') > > + name = dev_name(&dev->dev); > > + > > More of this weirdness. I admit it's not pretty. Would it help to use !strcmp(name, "") instead of *name == '\0'? Any other constructive suggestion? Best regards Uwe
On Thu, Aug 12, 2021 at 10:14:25AM +0200, Uwe Kleine-K??nig wrote: > dev_driver_string() might return "" (via dev_bus_name()). If that happens > *drvstr == '\0' becomes true. > > Would the following be better?: > > const char *drvstr; > > if (pdev) > return "<null>"; > > drvstr = dev_driver_string(&pdev->dev); > > if (!strcmp(drvstr, "")) > return "<null>"; > > return drvstr; > > When I thought about this hunk I considered it ugly to have "<null>" in > it twice. Well, if you want to avoid that you can do: if (pdev) { const char *name = dev_driver_string(&pdev->dev); if (strcmp(drvstr, "")) return name; } return "<null>"; Which would be a lot more readable.
From: Uwe Kleine-König <uwe@kleine-koenig.org> Hello, Today the following is always true for a struct pci_dev *pdev: pdev->driver == pdev->dev.driver ? to_pci_driver(pdev->dev.driver) : NULL This series is about getting rid of struct pci_dev::driver. The first three patches are unmodified compared to v2 (apart from an added Reviewed-by tag) and are just minor cleanups. Patch #4 replaces all usages of pci_dev::driver->name by dev_driver_string(). Patch #5 simplifies struct mpt_pci_driver by dropping an unused parameter from a function callback. The calculation of this parameter made use of struct pci_dev::driver. Patch #6 simplifies adf_enable_aer() and moves one assignment done in that function to the initializer of the respective static data. Patch #7 then modifies all remaining users of struct pci_dev::driver to use to_pci_driver(pdev->dev.driver) instead and finally patch #8 gets rid of the driver member. Note this series is only build tested. Theoretically patches #4 and #7 could be split by subsystem, there are no dependencies, but I'd prefer that all patches go in together via the pci tree to simplify the procedure. If you don't agree please speak up. Best regards Uwe Uwe Kleine-König (8): PCI: Simplify pci_device_remove() PCI: Drop useless check from pci_device_probe() xen/pci: Drop some checks that are always true PCI: replace pci_dev::driver usage that gets the driver name scsi: message: fusion: Remove unused parameter of mpt_pci driver's probe() crypto: qat - simplify adf_enable_aer() PCI: Replace pci_dev::driver usage by pci_dev::dev.driver PCI: Drop duplicated tracking of a pci_dev's bound driver arch/powerpc/include/asm/ppc-pci.h | 7 ++- arch/powerpc/kernel/eeh_driver.c | 10 +-- arch/x86/events/intel/uncore.c | 2 +- arch/x86/kernel/probe_roms.c | 2 +- drivers/bcma/host_pci.c | 7 ++- drivers/crypto/hisilicon/qm.c | 2 +- drivers/crypto/qat/qat_4xxx/adf_drv.c | 7 +-- drivers/crypto/qat/qat_c3xxx/adf_drv.c | 7 +-- drivers/crypto/qat/qat_c62x/adf_drv.c | 7 +-- drivers/crypto/qat/qat_common/adf_aer.c | 10 +-- .../crypto/qat/qat_common/adf_common_drv.h | 2 +- drivers/crypto/qat/qat_dh895xcc/adf_drv.c | 7 +-- drivers/message/fusion/mptbase.c | 7 +-- drivers/message/fusion/mptbase.h | 2 +- drivers/message/fusion/mptctl.c | 4 +- drivers/message/fusion/mptlan.c | 2 +- drivers/misc/cxl/guest.c | 24 ++++--- drivers/misc/cxl/pci.c | 30 +++++---- .../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 | 25 +++++--- drivers/pci/pci-driver.c | 45 ++++++------- drivers/pci/pci.c | 4 +- drivers/pci/pcie/err.c | 36 ++++++----- drivers/pci/xen-pcifront.c | 63 +++++++++---------- drivers/ssb/pcihost_wrapper.c | 8 ++- drivers/usb/host/xhci-pci.c | 2 +- include/linux/pci.h | 1 - 30 files changed, 164 insertions(+), 167 deletions(-) base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c