| Message ID | 20220401111936.92777-1-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] nfp: do not use driver_data to index device info | expand |
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
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>
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>
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