From: Uwe Kleine-König <redacted>
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 <redacted>
---
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(-)
Hello,
On Mon, Sep 27, 2021 at 10:43:18PM +0200, Uwe Kleine-König wrote:
From: Uwe Kleine-König <redacted>
I sent the series from the wrong email address :-\ I should have used
the above address as sender. Also I failed to add Christoph Hellwig to
Cc: (fixed for this mail). I guess I'll have to send a v5, but I will
wait a bit until the build bots are done with this series.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
From: Simon Horman <hidden> Date: 2021-09-28 10:01:50
On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote:
From: Uwe Kleine-König <redacted>
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 <redacted>
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 <redacted>
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:
quoted
From: Uwe Kleine-König <redacted>
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 <redacted>
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 <redacted>
[+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 <redacted>
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.
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.
@@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev,if(err)gotoerr_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");
@@ -78,9 +78,11 @@ static int ssb_pcihost_probe(struct pci_dev *dev,err=pci_enable_device(dev);if(err)gotoerr_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));
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:
quoted
From: Uwe Kleine-König <redacted>
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 .
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.
@@ -175,9 +175,10 @@ static int bcma_host_pci_probe(struct pci_dev *dev,if(err)gotoerr_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.
@@ -78,9 +78,11 @@ static int ssb_pcihost_probe(struct pci_dev *dev,err=pci_enable_device(dev);if(err)gotoerr_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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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:
quoted
[+to Oliver, Russell for eeh_driver_name() question below]
On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote:
quoted
From: Uwe Kleine-König <redacted>
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.
quoted
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.
From: Simon Horman <hidden> Date: 2021-09-29 08:06:02
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:
quoted
On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote:
quoted
From: Uwe Kleine-König <redacted>
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 <redacted>
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:
quoted
On Tue, Sep 28, 2021 at 12:01:28PM +0200, Simon Horman wrote:
quoted
On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote:
quoted
From: Uwe Kleine-König <redacted>
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 <redacted>
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |