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@xxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> >
> > ...
> >
> > > 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@xxxxxxxxxxxxxx/
#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.
