| Message ID | 20210927204326.612555-5-uwe@kleine-koenig.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | PCI: Drop duplicated tracking of a pci_dev's bound driver | expand |
Uwe Kleine-König <uwe@kleine-koenig.org> writes: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > 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 | 9 ++++++++- > drivers/bcma/host_pci.c | 7 ++++--- For bcma: Acked-by: Kalle Valo <kvalo@codeaurora.org>
On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > 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> ... > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > index 0685ece1f155..23dfb599c828 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)); I'd slightly prefer to maintain lines under 80 columns wide. But not nearly strongly enough to engage in a long debate about it. In any case, for the NFP portion of this patch. Acked-by: Simon Horman <simon.horman@corigine.com> > nfp_net_get_nspinfo(app, nsp_version); > snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version), > "%s %s %s %s", vnic_version, nsp_version, ...
On Tue, Sep 28, 2021 at 12:01:28PM +0200, Simon Horman wrote: > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > 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> > > ... > > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > > index 0685ece1f155..23dfb599c828 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)); > > I'd slightly prefer to maintain lines under 80 columns wide. > But not nearly strongly enough to engage in a long debate about it. :-) Looking at the output of git grep strlcpy.\*sizeof I wonder if it would be sensible to introduce something like #define strlcpy_array(arr, src) (strlcpy(arr, src, sizeof(arr)) + __must_be_array(arr)) but not sure this is possible without a long debate either (and this line is over 80 chars wide, too :-). > In any case, for the NFP portion of this patch. > > Acked-by: Simon Horman <simon.horman@corigine.com> Thanks Uwe
[+to Oliver, Russell for eeh_driver_name() question below] On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > 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. When you repost to fix the build issue, can you capitalize the subject line to match the other? Also, would you mind using "pci_dev.driver" instead of "pci_dev::driver"? AFAIK, the "::" operator is not actually part of C, so I think it's more confusing than useful. > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > arch/powerpc/include/asm/ppc-pci.h | 9 ++++++++- > 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, 22 insertions(+), 12 deletions(-) > > diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h > index 2b9edbf6e929..e8f1795a2acf 100644 > --- a/arch/powerpc/include/asm/ppc-pci.h > +++ b/arch/powerpc/include/asm/ppc-pci.h > @@ -57,7 +57,14 @@ 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>"; > + if (pdev) { > + const char *drvstr = dev_driver_string(&pdev->dev); > + > + if (strcmp(drvstr, "")) > + return drvstr; > + } > + > + return "<null>"; Can we just do this? if (pdev) return dev_driver_string(&pdev->dev); return "<null>"; I think it's more complicated than it's worth to include a strcmp(). It's possible this will change those error messages about "Might be infinite loop in %s driver", but that doesn't seem like a huge deal. I moved Oliver to "to:" and added Russell in case they object. > } > > #endif /* CONFIG_EEH */ > diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c > index 69c10a7b7c61..0973022d4b13 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 (!strcmp(name, "")) > + name = dev_name(&dev->dev); > err = pci_request_regions(dev, name); Again seems more complicated than it's worth to me. This is in the driver's .probe() method, so really_probe() has already set "dev->driver = drv", which means dev->driver is always set to &bcma_pci_bridge_driver here, and bcma_pci_bridge_driver.name is always "bcma-pci-bridge". Almost all callers of pci_request_regions() just hardcode the driver name or use a DRV_NAME #define So I think we should just do: err = pci_request_regions(dev, "bcma-pci-bridge"); > if (err) > goto err_pci_disable; > diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c > index 369562d34d66..8f361e54e524 100644 > --- a/drivers/crypto/hisilicon/qm.c > +++ b/drivers/crypto/hisilicon/qm.c > @@ -3085,7 +3085,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 7ea511d59e91..f279edfce3f1 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c > @@ -606,7 +606,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 0685ece1f155..23dfb599c828 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); Also seems like more trouble than it's worth. This one is a little strange but is always called for either b43_pci_bridge_driver or b44_pci_driver, both of which have .name set, so I think we should simply do: err = pci_request_regions(dev, dev_driver_string(&dev->dev)); > if (err) > goto err_pci_disable; > -- > 2.30.2 >
Hello, On Tue, Sep 28, 2021 at 12:17:59PM -0500, Bjorn Helgaas wrote: > [+to Oliver, Russell for eeh_driver_name() question below] > > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > 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. > > When you repost to fix the build issue, can you capitalize the subject > line to match the other? Yes, sure. > Also, would you mind using "pci_dev.driver" instead of > "pci_dev::driver"? AFAIK, the "::" operator is not actually part of > C, so I think it's more confusing than useful. pci_dev.driver doesn't work either in C because pci_dev is a type and not a variable. This is probably subjective, but for me pci_dev.driver looks definitively stranger than pci_dev::driver. And :: is at least not unseen in the kernel commit logs. (git log --grep=::) But if you insist I can change to . > > diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h > > index 2b9edbf6e929..e8f1795a2acf 100644 > > --- a/arch/powerpc/include/asm/ppc-pci.h > > +++ b/arch/powerpc/include/asm/ppc-pci.h > > @@ -57,7 +57,14 @@ 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>"; > > + if (pdev) { > > + const char *drvstr = dev_driver_string(&pdev->dev); > > + > > + if (strcmp(drvstr, "")) > > + return drvstr; > > + } > > + > > + return "<null>"; > > Can we just do this? > > if (pdev) > return dev_driver_string(&pdev->dev); > > return "<null>"; Works for me, too. It behaves a bit differerently than my suggestion (which nearly behaves identical to the status quo), but only in some degenerated cases. > I think it's more complicated than it's worth to include a strcmp(). > It's possible this will change those error messages about "Might be > infinite loop in %s driver", but that doesn't seem like a huge deal. > > I moved Oliver to "to:" and added Russell in case they object. > > > } > > > > #endif /* CONFIG_EEH */ > > diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c > > index 69c10a7b7c61..0973022d4b13 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 (!strcmp(name, "")) > > + name = dev_name(&dev->dev); > > err = pci_request_regions(dev, name); > > Again seems more complicated than it's worth to me. This is in the > driver's .probe() method, so really_probe() has already set > "dev->driver = drv", which means dev->driver is always set to > &bcma_pci_bridge_driver here, and bcma_pci_bridge_driver.name is > always "bcma-pci-bridge". > > Almost all callers of pci_request_regions() just hardcode the driver > name or use a DRV_NAME #define > > So I think we should just do: > > err = pci_request_regions(dev, "bcma-pci-bridge"); Yes, looks right. I'd put this in a separate patch. > > if (err) > > goto err_pci_disable; > > [...] > > 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); > > Also seems like more trouble than it's worth. This one is a little > strange but is always called for either b43_pci_bridge_driver or > b44_pci_driver, both of which have .name set, so I think we should > simply do: > > err = pci_request_regions(dev, dev_driver_string(&dev->dev)); yes, agreed, too. Best regards Uwe
On Tue, Sep 28, 2021 at 09:29:36PM +0200, Uwe Kleine-König wrote: > On Tue, Sep 28, 2021 at 12:17:59PM -0500, Bjorn Helgaas wrote: > > [+to Oliver, Russell for eeh_driver_name() question below] > > > > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > 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. > > Also, would you mind using "pci_dev.driver" instead of > > "pci_dev::driver"? AFAIK, the "::" operator is not actually part of > > C, so I think it's more confusing than useful. > > pci_dev.driver doesn't work either in C because pci_dev is a type and > not a variable. Sure, "pci_dev.driver" is not strictly acceptable C unless you have a "struct pci_dev pci_dev", but it's pretty common.
On Tue, Sep 28, 2021 at 12:31:29PM +0200, Uwe Kleine-König wrote: > On Tue, Sep 28, 2021 at 12:01:28PM +0200, Simon Horman wrote: > > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > 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> > > > > ... > > > > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > > > index 0685ece1f155..23dfb599c828 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)); > > > > I'd slightly prefer to maintain lines under 80 columns wide. > > But not nearly strongly enough to engage in a long debate about it. > > :-) > > Looking at the output of > > git grep strlcpy.\*sizeof > > I wonder if it would be sensible to introduce something like > > #define strlcpy_array(arr, src) (strlcpy(arr, src, sizeof(arr)) + __must_be_array(arr)) > > but not sure this is possible without a long debate either (and this > line is over 80 chars wide, too :-). My main motivation for the 80 char limit in nfp_net_ethtool.c is not that I think 80 char is universally a good limit (although that is true), but rather that I expect that is the prevailing style in nfp_net_ethtool.c. So a macro more than 80 car wide somewhere else is fine by me. However, when running checkpatch --strict over the patch it told me: WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/ #276: FILE: drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c:205: + strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev), sizeof(drvinfo->driver)); total: 0 errors, 1 warnings, 0 checks, 80 lines checked (Amusingly, more text wider than 80 column, perhaps suggesting the folly of my original comment, but lets move on from that.) As your patch doesn't introduce the usage of strlcpy() I was considering a follow-up patch to change it to strscpy(). And in general the email at the link above suggests all usages of strlcpy() should do so. So perhaps creating strscpy_array is a better idea? I have not thought about this much, and probably this just leads us to a deeper part of the rabbit hole.
Hello Simon, On Wed, Sep 29, 2021 at 10:05:42AM +0200, Simon Horman wrote: > On Tue, Sep 28, 2021 at 12:31:29PM +0200, Uwe Kleine-König wrote: > > On Tue, Sep 28, 2021 at 12:01:28PM +0200, Simon Horman wrote: > > > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote: > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > > > 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> > > > > > > ... > > > > > > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c > > > > index 0685ece1f155..23dfb599c828 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)); > > > > > > I'd slightly prefer to maintain lines under 80 columns wide. > > > But not nearly strongly enough to engage in a long debate about it. > > > > :-) > > > > Looking at the output of > > > > git grep strlcpy.\*sizeof > > > > I wonder if it would be sensible to introduce something like > > > > #define strlcpy_array(arr, src) (strlcpy(arr, src, sizeof(arr)) + __must_be_array(arr)) > > > > but not sure this is possible without a long debate either (and this > > line is over 80 chars wide, too :-). > > My main motivation for the 80 char limit in nfp_net_ethtool.c is > not that I think 80 char is universally a good limit (although that is true), > but rather that I expect that is the prevailing style in nfp_net_ethtool.c. I sent out v5 with an additional line break now. > So a macro more than 80 car wide somewhere else is fine by me. > > However, when running checkpatch --strict over the patch it told me: > > WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/ > #276: FILE: drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c:205: > + strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev), sizeof(drvinfo->driver)); > > total: 0 errors, 1 warnings, 0 checks, 80 lines checked > > (Amusingly, more text wider than 80 column, perhaps suggesting the folly of > my original comment, but lets move on from that.) > > As your patch doesn't introduce the usage of strlcpy() I was considering a > follow-up patch to change it to strscpy(). And in general the email at the > link above suggests all usages of strlcpy() should do so. So perhaps > creating strscpy_array is a better idea? What I read about strscpy() is that conversions for the sake of the conversion are not welcome. When such a conversion comes from someone involved with the driver that is also tested this is probably fine. > I have not thought about this much, and probably this just leads us to a > deeper part of the rabbit hole. I assume so, too. Best regards Uwe
diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h index 2b9edbf6e929..e8f1795a2acf 100644 --- a/arch/powerpc/include/asm/ppc-pci.h +++ b/arch/powerpc/include/asm/ppc-pci.h @@ -57,7 +57,14 @@ 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>"; + if (pdev) { + const char *drvstr = dev_driver_string(&pdev->dev); + + if (strcmp(drvstr, "")) + return drvstr; + } + + return "<null>"; } #endif /* CONFIG_EEH */ diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c index 69c10a7b7c61..0973022d4b13 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 (!strcmp(name, "")) + 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 369562d34d66..8f361e54e524 100644 --- a/drivers/crypto/hisilicon/qm.c +++ b/drivers/crypto/hisilicon/qm.c @@ -3085,7 +3085,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 7ea511d59e91..f279edfce3f1 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c @@ -606,7 +606,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 0685ece1f155..23dfb599c828 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;