[PATCH net] nfp: do not use driver_data to index device info

[PATCH net] nfp: do not use driver_data to index device info

[Simon Horman]

From: Niklas Söderlund <niklas.soderlund@corigine.com>

When adding support for multiple chips the struct pci_device_id
driver_data field was used to hold a index to lookup chip device
specific information from a table. This works but creates a regressions
for users who uses /sys/bus/pci/drivers/nfp_netvf/new_id.

For example, before the change writing "19ee 6003" to new_id was
sufficient but after one needs to write enough fields to be able to also
match on the driver_data field, "19ee 6003 19ee ffffffff ffffffff 0 1".

The usage of driver_data field was only a convenience and in the belief
the driver_data field was private to the driver and not exposed in
anyway to users. Changing the device info lookup to a function that
translates from struct pci_device_id device field instead works just as
well and removes the user facing regression.

As a bonus the enum and table with lookup information can be moved out
from a shared header file to the only file where it's used.

Reported-by: Danie du Toit <danie.dutoit@corigine.com>
Fixes: e900db704c8512bc ("nfp: parametrize QCP offset/size using dev_info")
Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 12 ++++----
 .../ethernet/netronome/nfp/nfp_netvf_main.c   |  8 ++++--
 .../ethernet/netronome/nfp/nfpcore/nfp_dev.c  | 28 ++++++++++++++++++-
 .../ethernet/netronome/nfp/nfpcore/nfp_dev.h  | 11 ++------
 4 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index eeda39e34f84..b60f2c8b6f4c 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -35,19 +35,19 @@ static const char nfp_driver_name[] = "nfp";
 static const struct pci_device_id nfp_pci_device_ids[] = {
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP3800,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP3800,
+	  PCI_ANY_ID, 0,
 	},
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP4000,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP6000,
+	  PCI_ANY_ID, 0,
 	},
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP5000,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP6000,
+	  PCI_ANY_ID, 0,
 	},
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP6000,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP6000,
+	  PCI_ANY_ID, 0,
 	},
 	{ 0, } /* Required last entry. */
 };
@@ -685,7 +685,9 @@ static int nfp_pci_probe(struct pci_dev *pdev,
 	    pdev->device == PCI_DEVICE_ID_NETRONOME_NFP6000_VF)
 		dev_warn(&pdev->dev, "Binding NFP VF device to the NFP PF driver, the VF driver is called 'nfp_netvf'\n");
 
-	dev_info = &nfp_dev_info[pci_id->driver_data];
+	dev_info = nfp_get_dev_info(pci_id);
+	if (!dev_info)
+		return -ENODEV;
 
 	err = pci_enable_device(pdev);
 	if (err < 0)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_netvf_main.c b/drivers/net/ethernet/netronome/nfp/nfp_netvf_main.c
index a51eb26dd977..c14a76b6f5a0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_netvf_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_netvf_main.c
@@ -40,11 +40,11 @@ static const char nfp_net_driver_name[] = "nfp_netvf";
 static const struct pci_device_id nfp_netvf_pci_device_ids[] = {
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP3800_VF,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP3800_VF,
+	  PCI_ANY_ID, 0,
 	},
 	{ PCI_VENDOR_ID_NETRONOME, PCI_DEVICE_ID_NETRONOME_NFP6000_VF,
 	  PCI_VENDOR_ID_NETRONOME, PCI_ANY_ID,
-	  PCI_ANY_ID, 0, NFP_DEV_NFP6000_VF,
+	  PCI_ANY_ID, 0,
 	},
 	{ 0, } /* Required last entry. */
 };
@@ -83,7 +83,9 @@ static int nfp_netvf_pci_probe(struct pci_dev *pdev,
 	int stride;
 	int err;
 
-	dev_info = &nfp_dev_info[pci_id->driver_data];
+	dev_info = nfp_get_dev_info(pci_id);
+	if (!dev_info)
+		return -ENODEV;
 
 	vf = kzalloc(sizeof(*vf), GFP_KERNEL);
 	if (!vf)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.c
index 28384d6d1c6f..add14704c6e2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.c
@@ -3,11 +3,19 @@
 
 #include <linux/dma-mapping.h>
 #include <linux/kernel.h>
+#include <linux/pci_ids.h>
 #include <linux/sizes.h>
 
 #include "nfp_dev.h"
 
-const struct nfp_dev_info nfp_dev_info[NFP_DEV_CNT] = {
+enum nfp_dev_id {
+	NFP_DEV_NFP3800,
+	NFP_DEV_NFP3800_VF,
+	NFP_DEV_NFP6000,
+	NFP_DEV_NFP6000_VF,
+};
+
+static const struct nfp_dev_info nfp_dev_infos[] = {
 	[NFP_DEV_NFP3800] = {
 		.dma_mask		= DMA_BIT_MASK(40),
 		.qc_idx_mask		= GENMASK(8, 0),
@@ -47,3 +55,21 @@ const struct nfp_dev_info nfp_dev_info[NFP_DEV_CNT] = {
 		.max_qc_size		= SZ_256K,
 	},
 };
+
+const struct nfp_dev_info *nfp_get_dev_info(const struct pci_device_id *id)
+{
+	switch (id->device) {
+	case PCI_DEVICE_ID_NETRONOME_NFP3800:
+		return &nfp_dev_infos[NFP_DEV_NFP3800];
+	case PCI_DEVICE_ID_NETRONOME_NFP3800_VF:
+		return &nfp_dev_infos[NFP_DEV_NFP3800_VF];
+	case PCI_DEVICE_ID_NETRONOME_NFP4000:
+	case PCI_DEVICE_ID_NETRONOME_NFP5000:
+	case PCI_DEVICE_ID_NETRONOME_NFP6000:
+		return &nfp_dev_infos[NFP_DEV_NFP6000];
+	case PCI_DEVICE_ID_NETRONOME_NFP6000_VF:
+		return &nfp_dev_infos[NFP_DEV_NFP6000_VF];
+	default:
+		return NULL;
+	}
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.h
index d4189869cf7b..b0ad581d5f38 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.h
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_dev.h
@@ -4,16 +4,9 @@
 #ifndef _NFP_DEV_H_
 #define _NFP_DEV_H_
 
+#include <linux/mod_devicetable.h>
 #include <linux/types.h>
 
-enum nfp_dev_id {
-	NFP_DEV_NFP3800,
-	NFP_DEV_NFP3800_VF,
-	NFP_DEV_NFP6000,
-	NFP_DEV_NFP6000_VF,
-	NFP_DEV_CNT,
-};
-
 struct nfp_dev_info {
 	/* Required fields */
 	u64 dma_mask;
@@ -29,6 +22,6 @@ struct nfp_dev_info {
 	u32 qc_area_sz;
 };
 
-extern const struct nfp_dev_info nfp_dev_info[NFP_DEV_CNT];
+const struct nfp_dev_info *nfp_get_dev_info(const struct pci_device_id *id);
 
 #endif
-- 
2.30.2

Re: [PATCH net] nfp: do not use driver_data to index device info

[Andrew Lunn]

On Fri, Apr 01, 2022 at 01:19:36PM +0200, Simon Horman wrote:
> From: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> When adding support for multiple chips the struct pci_device_id
> driver_data field was used to hold a index to lookup chip device
> specific information from a table. This works but creates a regressions
> for users who uses /sys/bus/pci/drivers/nfp_netvf/new_id.
> 
> For example, before the change writing "19ee 6003" to new_id was
> sufficient but after one needs to write enough fields to be able to also
> match on the driver_data field, "19ee 6003 19ee ffffffff ffffffff 0 1".
> 
> The usage of driver_data field was only a convenience and in the belief
> the driver_data field was private to the driver and not exposed in
> anyway to users. Changing the device info lookup to a function that
> translates from struct pci_device_id device field instead works just as
> well and removes the user facing regression.
> 
> As a bonus the enum and table with lookup information can be moved out
> from a shared header file to the only file where it's used.
> 
> Reported-by: Danie du Toit <danie.dutoit@corigine.com>
> Fixes: e900db704c8512bc ("nfp: parametrize QCP offset/size using dev_info")
> Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>

Hi Simon

This is missing your own Signed-off-by:

     Andrew

Re: [PATCH net] nfp: do not use driver_data to index device info

[Simon Horman]

On Fri, Apr 01, 2022 at 03:18:43PM +0200, Andrew Lunn wrote:
> On Fri, Apr 01, 2022 at 01:19:36PM +0200, Simon Horman wrote:
> > From: Niklas Söderlund <niklas.soderlund@corigine.com>
> > 
> > When adding support for multiple chips the struct pci_device_id
> > driver_data field was used to hold a index to lookup chip device
> > specific information from a table. This works but creates a regressions
> > for users who uses /sys/bus/pci/drivers/nfp_netvf/new_id.
> > 
> > For example, before the change writing "19ee 6003" to new_id was
> > sufficient but after one needs to write enough fields to be able to also
> > match on the driver_data field, "19ee 6003 19ee ffffffff ffffffff 0 1".
> > 
> > The usage of driver_data field was only a convenience and in the belief
> > the driver_data field was private to the driver and not exposed in
> > anyway to users. Changing the device info lookup to a function that
> > translates from struct pci_device_id device field instead works just as
> > well and removes the user facing regression.
> > 
> > As a bonus the enum and table with lookup information can be moved out
> > from a shared header file to the only file where it's used.
> > 
> > Reported-by: Danie du Toit <danie.dutoit@corigine.com>
> > Fixes: e900db704c8512bc ("nfp: parametrize QCP offset/size using dev_info")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> Hi Simon
> 
> This is missing your own Signed-off-by:

Thanks Andrew,

I'm very sorry about that.

Signed-off-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net] nfp: do not use driver_data to index device info

[Jakub Kicinski]

On Fri,  1 Apr 2022 13:19:36 +0200 Simon Horman wrote:
> From: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> When adding support for multiple chips the struct pci_device_id
> driver_data field was used to hold a index to lookup chip device
> specific information from a table. This works but creates a regressions
> for users who uses /sys/bus/pci/drivers/nfp_netvf/new_id.
> 
> For example, before the change writing "19ee 6003" to new_id was
> sufficient 

Can you explain the use case? I think this worked somewhat
coincidentally. If we had entries that matched subvendor = 0
subdevice = 0 it'd fail with EEXIST.

> but after one needs to write enough fields to be able to also
> match on the driver_data field, "19ee 6003 19ee ffffffff ffffffff 0 1".
> 
> The usage of driver_data field was only a convenience and in the belief
> the driver_data field was private to the driver and not exposed in
> anyway to users. Changing the device info lookup to a function that
> translates from struct pci_device_id device field instead works just as
> well and removes the user facing regression.

I think you're trading a coincidental "feature" while breaking what
new_id is actually supposed to be used for. Which is adding IDs.
nfp_get_dev_info() you add only recognizes existing IDs.

> As a bonus the enum and table with lookup information can be moved out
> from a shared header file to the only file where it's used.
> 
> Reported-by: Danie du Toit <danie.dutoit@corigine.com>
> Fixes: e900db704c8512bc ("nfp: parametrize QCP offset/size using dev_info")
> Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>