Working on PCI VPD core code I came across the Chelsio drivers.
Let's improve the way how cxgb4 handles PCI VPD.
One major goal is to eventually remove pci_set_vpd_size(),
cxgb4 is the only user. The amount of data exposed via the VPD
interface is fixed, therefore I see no benefit in providing
an interface for manipulating the VPD size.
This series touches only device-specific quirks in the core code,
therefore I think it should go via the netdev tree.
v2:
- remove patch 1 from the series
Heiner Kallweit (3):
cxgb4: remove unused vpd_cap_addr
PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual
address space
cxgb4: remove changing VPD len
.../net/ethernet/chelsio/cxgb4/cudbg_entity.h | 1 -
.../net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 ++++---------------
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 1 -
.../net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 --
drivers/pci/vpd.c | 7 +++----
5 files changed, 7 insertions(+), 25 deletions(-)
--
2.30.0
Now that the PCI VPD for Chelsio devices from T4 has been changed and VPD
len is set to PCI_VPD_MAX_SIZE (32K), we don't have to change the VPD len
any longer.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
.../net/ethernet/chelsio/cxgb4/cudbg_entity.h | 1 -
.../net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 ++++---------------
2 files changed, 4 insertions(+), 18 deletions(-)
@@ -2689,7 +2689,7 @@ int cudbg_collect_vpd_data(struct cudbg_init *pdbg_init,u32scfg_vers,vpd_vers,fw_vers;structcudbg_vpd_data*vpd_data;structvpd_paramsvpd={0};-intrc,ret;+intrc;rc=t4_get_raw_vpd_params(padap,&vpd);if(rc)
@@ -2699,24 +2699,11 @@ int cudbg_collect_vpd_data(struct cudbg_init *pdbg_init,if(rc)returnrc;-/* Serial Configuration Version is located beyond the PF's vpd size.-*TemporarilygiveaccesstoentireEEPROMtogetit.-*/-rc=pci_set_vpd_size(padap->pdev,EEPROMVSIZE);-if(rc<0)-returnrc;--ret=cudbg_read_vpd_reg(padap,CUDBG_SCFG_VER_ADDR,CUDBG_SCFG_VER_LEN,-&scfg_vers);--/* Restore back to original PF's vpd size */-rc=pci_set_vpd_size(padap->pdev,CUDBG_VPD_PF_SIZE);-if(rc<0)+rc=cudbg_read_vpd_reg(padap,CUDBG_SCFG_VER_ADDR,CUDBG_SCFG_VER_LEN,+&scfg_vers);+if(rc)returnrc;-if(ret)-returnret;-rc=cudbg_read_vpd_reg(padap,CUDBG_VPD_VER_ADDR,CUDBG_VPD_VER_LEN,vpd_str);if(rc)
cxgb4 uses the full VPD address space for accessing its EEPROM (with some
mapping, see t4_eeprom_ptov()). In cudbg_collect_vpd_data() it sets the
VPD len to 32K (PCI_VPD_MAX_SIZE), and then back to 2K (CUDBG_VPD_PF_SIZE).
Having official (structured) and inofficial (unstructured) VPD data
violates the PCI spec, let's set VPD len according to all data that can be
accessed via PCI VPD access, no matter of its structure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/pci/vpd.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Alexander Duyck <hidden> Date: 2021-02-08 17:00:37
On Fri, Feb 5, 2021 at 2:15 PM Heiner Kallweit [off-list ref] wrote:
quoted hunk
cxgb4 uses the full VPD address space for accessing its EEPROM (with some
mapping, see t4_eeprom_ptov()). In cudbg_collect_vpd_data() it sets the
VPD len to 32K (PCI_VPD_MAX_SIZE), and then back to 2K (CUDBG_VPD_PF_SIZE).
Having official (structured) and inofficial (unstructured) VPD data
violates the PCI spec, let's set VPD len according to all data that can be
accessed via PCI VPD access, no matter of its structure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/pci/vpd.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
So as I recall the size value was added when some hardware was hanging
when an out-of-bounds read occured from various tools accessing the
VPD. I'm assuming if you are enabling full access the T4 hardware can
handle cases where an out-of-bounds read is requested?
Otherwise the code itself looks fine to me.
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
From: Alexander Duyck <hidden> Date: 2021-02-08 17:07:29
On Fri, Feb 5, 2021 at 2:29 PM Heiner Kallweit [off-list ref] wrote:
Supposedly this is a leftover from T3 driver heritage. cxgb4 uses the
PCI core VPD access code that handles detection of VPD capabilities.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Instead of starting with the "Supposedly this is" it might be better
to word it along the lines of "This is likely". The "Supposedly" makes
it sound like you heard this as a rumor from somebody else.
Other than that nit about the description the change looks good to me.
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
From: Alexander Duyck <hidden> Date: 2021-02-08 17:09:34
On Fri, Feb 5, 2021 at 2:18 PM Heiner Kallweit [off-list ref] wrote:
quoted hunk
Now that the PCI VPD for Chelsio devices from T4 has been changed and VPD
len is set to PCI_VPD_MAX_SIZE (32K), we don't have to change the VPD len
any longer.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
.../net/ethernet/chelsio/cxgb4/cudbg_entity.h | 1 -
.../net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 ++++---------------
2 files changed, 4 insertions(+), 18 deletions(-)
@@ -2689,7 +2689,7 @@ int cudbg_collect_vpd_data(struct cudbg_init *pdbg_init,u32scfg_vers,vpd_vers,fw_vers;structcudbg_vpd_data*vpd_data;structvpd_paramsvpd={0};-intrc,ret;+intrc;rc=t4_get_raw_vpd_params(padap,&vpd);if(rc)
@@ -2699,24 +2699,11 @@ int cudbg_collect_vpd_data(struct cudbg_init *pdbg_init,if(rc)returnrc;-/* Serial Configuration Version is located beyond the PF's vpd size.-*TemporarilygiveaccesstoentireEEPROMtogetit.-*/-rc=pci_set_vpd_size(padap->pdev,EEPROMVSIZE);-if(rc<0)-returnrc;--ret=cudbg_read_vpd_reg(padap,CUDBG_SCFG_VER_ADDR,CUDBG_SCFG_VER_LEN,-&scfg_vers);--/* Restore back to original PF's vpd size */-rc=pci_set_vpd_size(padap->pdev,CUDBG_VPD_PF_SIZE);-if(rc<0)+rc=cudbg_read_vpd_reg(padap,CUDBG_SCFG_VER_ADDR,CUDBG_SCFG_VER_LEN,+&scfg_vers);+if(rc)returnrc;-if(ret)-returnret;-rc=cudbg_read_vpd_reg(padap,CUDBG_VPD_VER_ADDR,CUDBG_VPD_VER_LEN,vpd_str);if(rc)
Assuming that patch 2 is okay then this patch should be fine since it
is just toggling back and forth between the same value anyway.
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>