| Message ID | 20210811080637.2596434-5-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| 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.
diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h index 2b9edbf6e929..3f89dda91e89 100644 --- a/arch/powerpc/include/asm/ppc-pci.h +++ b/arch/powerpc/include/asm/ppc-pci.h @@ -57,7 +57,12 @@ void eeh_sysfs_remove_device(struct pci_dev *pdev); 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; } #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); + err = pci_request_regions(dev, name); if (err) goto err_pci_disable; diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c index 1d67f94a1d56..ddf8d0ea6a75 100644 --- a/drivers/crypto/hisilicon/qm.c +++ b/drivers/crypto/hisilicon/qm.c @@ -3003,7 +3003,7 @@ static int qm_alloc_uacce(struct hisi_qm *qm) }; int ret; - ret = strscpy(interface.name, pdev->driver->name, + ret = strscpy(interface.name, dev_driver_string(&pdev->dev), sizeof(interface.name)); if (ret < 0) return -ENAMETOOLONG; diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c index 82061ab6930f..e7a2c8d4558f 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c @@ -593,7 +593,7 @@ static void hns3_get_drvinfo(struct net_device *netdev, return; } - strncpy(drvinfo->driver, h->pdev->driver->name, + strncpy(drvinfo->driver, dev_driver_string(&h->pdev->dev), sizeof(drvinfo->driver)); drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0'; diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c index a250d394da38..a8f007f6dad2 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c @@ -720,7 +720,7 @@ static int prestera_fw_load(struct prestera_fw *fw) static int prestera_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { - const char *driver_name = pdev->driver->name; + const char *driver_name = dev_driver_string(&pdev->dev); struct prestera_fw *fw; int err; diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c index 13b0259f7ea6..8f306364f7bf 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/pci.c +++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c @@ -1876,7 +1876,7 @@ static void mlxsw_pci_cmd_fini(struct mlxsw_pci *mlxsw_pci) static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { - const char *driver_name = pdev->driver->name; + const char *driver_name = dev_driver_string(&pdev->dev); struct mlxsw_pci *mlxsw_pci; int err; diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c index 1b482446536d..1c0a9edcd005 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c @@ -202,7 +202,7 @@ nfp_get_drvinfo(struct nfp_app *app, struct pci_dev *pdev, { char nsp_version[ETHTOOL_FWVERS_LEN] = {}; - strlcpy(drvinfo->driver, pdev->driver->name, sizeof(drvinfo->driver)); + strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev), sizeof(drvinfo->driver)); nfp_net_get_nspinfo(app, nsp_version); snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version), "%s %s %s %s", vnic_version, nsp_version, diff --git a/drivers/ssb/pcihost_wrapper.c b/drivers/ssb/pcihost_wrapper.c index 410215c16920..4938ed5cfae5 100644 --- a/drivers/ssb/pcihost_wrapper.c +++ b/drivers/ssb/pcihost_wrapper.c @@ -78,9 +78,11 @@ static int ssb_pcihost_probe(struct pci_dev *dev, err = pci_enable_device(dev); if (err) goto err_kfree_ssb; - 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); + err = pci_request_regions(dev, name); if (err) goto err_pci_disable;
struct pci_dev::driver holds (apart from a constant offset) the same data as struct pci_dev::dev->driver. With the goal to remove struct pci_dev::driver to get rid of data duplication replace getting the driver name by dev_driver_string() which implicitly makes use of struct pci_dev::dev->driver. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- arch/powerpc/include/asm/ppc-pci.h | 7 ++++++- drivers/bcma/host_pci.c | 7 ++++--- drivers/crypto/hisilicon/qm.c | 2 +- drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 2 +- drivers/net/ethernet/marvell/prestera/prestera_pci.c | 2 +- drivers/net/ethernet/mellanox/mlxsw/pci.c | 2 +- drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 2 +- drivers/ssb/pcihost_wrapper.c | 8 +++++--- 8 files changed, 20 insertions(+), 12 deletions(-)