[PATCH resend net-next v2 0/3] cxgb4: improve PCI VPD handling

[PATCH resend net-next v2 0/3] cxgb4: improve PCI VPD handling

[Heiner Kallweit]

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

Resending the series because it seems netdev patchwork swallowed it.

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

[PATCH resend net-next v2 1/3] cxgb4: remove unused vpd_cap_addr

[Heiner Kallweit]

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>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      | 1 -
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 8e681ce72..314f8d806 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -414,7 +414,6 @@ struct pf_resources {
 };
 
 struct pci_params {
-	unsigned int vpd_cap_addr;
 	unsigned char speed;
 	unsigned char width;
 };
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 9f1965c80..6264bc66a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -3201,8 +3201,6 @@ static void cxgb4_mgmt_fill_vf_station_mac_addr(struct adapter *adap)
 	int err;
 	u8 *na;
 
-	adap->params.pci.vpd_cap_addr = pci_find_capability(adap->pdev,
-							    PCI_CAP_ID_VPD);
 	err = t4_get_raw_vpd_params(adap, &adap->params.vpd);
 	if (err)
 		return;
-- 
2.30.0

[PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space

[Heiner Kallweit]

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(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 7915d10f9..06a7954d0 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -633,9 +633,8 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 	/*
 	 * If this is a T3-based adapter, there's a 1KB VPD area at offset
 	 * 0xc00 which contains the preferred VPD values.  If this is a T4 or
-	 * later based adapter, the special VPD is at offset 0x400 for the
-	 * Physical Functions (the SR-IOV Virtual Functions have no VPD
-	 * Capabilities).  The PCI VPD Access core routines will normally
+	 * later based adapter, provide access to the full virtual EEPROM
+	 * address space. The PCI VPD Access core routines will normally
 	 * compute the size of the VPD by parsing the VPD Data Structure at
 	 * offset 0x000.  This will result in silent failures when attempting
 	 * to accesses these other VPD areas which are beyond those computed
@@ -644,7 +643,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 	if (chip == 0x0 && prod >= 0x20)
 		pci_set_vpd_size(dev, 8192);
 	else if (chip >= 0x4 && func < 0x8)
-		pci_set_vpd_size(dev, 2048);
+		pci_set_vpd_size(dev, PCI_VPD_MAX_SIZE);
 }
 
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
-- 
2.30.0

[PATCH resend net-next v2 3/3] cxgb4: remove changing VPD len

[Heiner Kallweit]

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(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_entity.h b/drivers/net/ethernet/chelsio/cxgb4/cudbg_entity.h
index 876f90e57..02ccb610a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_entity.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_entity.h
@@ -220,7 +220,6 @@ struct cudbg_mps_tcam {
 	u8 reserved[2];
 };
 
-#define CUDBG_VPD_PF_SIZE 0x800
 #define CUDBG_SCFG_VER_ADDR 0x06
 #define CUDBG_SCFG_VER_LEN 4
 #define CUDBG_VPD_VER_ADDR 0x18c7
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
index 75474f810..addac5518 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
@@ -2689,7 +2689,7 @@ int cudbg_collect_vpd_data(struct cudbg_init *pdbg_init,
 	u32 scfg_vers, vpd_vers, fw_vers;
 	struct cudbg_vpd_data *vpd_data;
 	struct vpd_params vpd = { 0 };
-	int rc, ret;
+	int rc;
 
 	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)
 		return rc;
 
-	/* Serial Configuration Version is located beyond the PF's vpd size.
-	 * Temporarily give access to entire EEPROM to get it.
-	 */
-	rc = pci_set_vpd_size(padap->pdev, EEPROMVSIZE);
-	if (rc < 0)
-		return rc;
-
-	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)
 		return rc;
 
-	if (ret)
-		return ret;
-
 	rc = cudbg_read_vpd_reg(padap, CUDBG_VPD_VER_ADDR, CUDBG_VPD_VER_LEN,
 				vpd_str);
 	if (rc)
-- 
2.30.0

Re: [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space

[Bjorn Helgaas]

[+cc Casey, Rahul]

On Fri, Feb 05, 2021 at 08:29:45PM +0100, Heiner Kallweit wrote:
> 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.

s/inofficial/unofficial/

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 7915d10f9..06a7954d0 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -633,9 +633,8 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>  	/*
>  	 * If this is a T3-based adapter, there's a 1KB VPD area at offset
>  	 * 0xc00 which contains the preferred VPD values.  If this is a T4 or
> -	 * later based adapter, the special VPD is at offset 0x400 for the
> -	 * Physical Functions (the SR-IOV Virtual Functions have no VPD
> -	 * Capabilities).  The PCI VPD Access core routines will normally
> +	 * later based adapter, provide access to the full virtual EEPROM
> +	 * address space. The PCI VPD Access core routines will normally
>  	 * compute the size of the VPD by parsing the VPD Data Structure at
>  	 * offset 0x000.  This will result in silent failures when attempting
>  	 * to accesses these other VPD areas which are beyond those computed
> @@ -644,7 +643,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>  	if (chip == 0x0 && prod >= 0x20)
>  		pci_set_vpd_size(dev, 8192);
>  	else if (chip >= 0x4 && func < 0x8)
> -		pci_set_vpd_size(dev, 2048);
> +		pci_set_vpd_size(dev, PCI_VPD_MAX_SIZE);

This code was added by 7dcf688d4c78 ("PCI/cxgb4: Extend T3 PCI quirk
to T4+ devices") [1].  Unfortunately that commit doesn't really have
the details about what it fixes, other than the silent failures it
mentions in the comment.

Some devices hang if we try to read at the wrong VPD address, and this
can be done via the sysfs "vpd" file.  Can you expand the commit log
with an argument for why it is always safe to set the size to
PCI_VPD_MAX_SIZE for these devices?

The fact that cudbg_collect_vpd_data() fiddles around with
pci_set_vpd_size() suggests to me that there is *some* problem with
reading parts of the VPD.  Otherwise, why would they bother?

940c9c458866 ("cxgb4: collect vpd info directly from hardware") [2]
added the pci_set_vpd_size() usage, but doesn't say why it's needed.
Maybe Rahul will remember?

Bjorn

[1] https://git.kernel.org/linus/7dcf688d4c78
[2] https://git.kernel.org/linus/940c9c458866

>  }
>  
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> -- 
> 2.30.0
> 
> 
> 

Re: [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space

[Heiner Kallweit]

On 05.02.2021 22:46, Bjorn Helgaas wrote:
> [+cc Casey, Rahul]
> 
> On Fri, Feb 05, 2021 at 08:29:45PM +0100, Heiner Kallweit wrote:
>> 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.
> 
> s/inofficial/unofficial/
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/pci/vpd.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>> index 7915d10f9..06a7954d0 100644
>> --- a/drivers/pci/vpd.c
>> +++ b/drivers/pci/vpd.c
>> @@ -633,9 +633,8 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>  	/*
>>  	 * If this is a T3-based adapter, there's a 1KB VPD area at offset
>>  	 * 0xc00 which contains the preferred VPD values.  If this is a T4 or
>> -	 * later based adapter, the special VPD is at offset 0x400 for the
>> -	 * Physical Functions (the SR-IOV Virtual Functions have no VPD
>> -	 * Capabilities).  The PCI VPD Access core routines will normally
>> +	 * later based adapter, provide access to the full virtual EEPROM
>> +	 * address space. The PCI VPD Access core routines will normally
>>  	 * compute the size of the VPD by parsing the VPD Data Structure at
>>  	 * offset 0x000.  This will result in silent failures when attempting
>>  	 * to accesses these other VPD areas which are beyond those computed
>> @@ -644,7 +643,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>  	if (chip == 0x0 && prod >= 0x20)
>>  		pci_set_vpd_size(dev, 8192);
>>  	else if (chip >= 0x4 && func < 0x8)
>> -		pci_set_vpd_size(dev, 2048);
>> +		pci_set_vpd_size(dev, PCI_VPD_MAX_SIZE);
> 
> This code was added by 7dcf688d4c78 ("PCI/cxgb4: Extend T3 PCI quirk
> to T4+ devices") [1].  Unfortunately that commit doesn't really have
> the details about what it fixes, other than the silent failures it
> mentions in the comment.
> 
> Some devices hang if we try to read at the wrong VPD address, and this
> can be done via the sysfs "vpd" file.  Can you expand the commit log
> with an argument for why it is always safe to set the size to
> PCI_VPD_MAX_SIZE for these devices?
> 

Seeing t4_eeprom_ptov() there is data at the end of the VPD address
space, but there may be gaps in between. I don't have test hw,
therefore it would be good if Chelsio could confirm that accessing
any address in the VPD address space (32K) is ok. If a VPD address
isn't backed by EEPROM, it should return 0x00 or 0xff, and not hang
the device.

> The fact that cudbg_collect_vpd_data() fiddles around with
> pci_set_vpd_size() suggests to me that there is *some* problem with
> reading parts of the VPD.  Otherwise, why would they bother?
> 
> 940c9c458866 ("cxgb4: collect vpd info directly from hardware") [2]
> added the pci_set_vpd_size() usage, but doesn't say why it's needed.
> Maybe Rahul will remember?
> 

In addition we have cb92148b58a4 ("PCI: Add pci_set_vpd_size() to set
VPD size"). To me it seems the VPD size quirks and this commit
try to achieve the same: allow to override the autodetected VPD len

The quirk mechanism is well established, and if possible I'd like
to get rid of pci_set_vpd_size(). I don't like the idea that the
PCI core exposes API calls for accessing a proprietary VPD data
format of one specific vendor (cxgb4 is the only user of
pci_set_vpd_size()).

> Bjorn
> 
> [1] https://git.kernel.org/linus/7dcf688d4c78
> [2] https://git.kernel.org/linus/940c9c458866
> 
>>  }
>>  
>>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>> -- 
>> 2.30.0
>>
>>
>>

Re: [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space

[Rahul Lakkireddy]

On Friday, February 02/05/21, 2021 at 23:31:24 +0100, Heiner Kallweit wrote:
> On 05.02.2021 22:46, Bjorn Helgaas wrote:
> > [+cc Casey, Rahul]
> > 
> > On Fri, Feb 05, 2021 at 08:29:45PM +0100, Heiner Kallweit wrote:
> >> 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.
> > 
> > s/inofficial/unofficial/
> > 
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/pci/vpd.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> >> index 7915d10f9..06a7954d0 100644
> >> --- a/drivers/pci/vpd.c
> >> +++ b/drivers/pci/vpd.c
> >> @@ -633,9 +633,8 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
> >>  	/*
> >>  	 * If this is a T3-based adapter, there's a 1KB VPD area at offset
> >>  	 * 0xc00 which contains the preferred VPD values.  If this is a T4 or
> >> -	 * later based adapter, the special VPD is at offset 0x400 for the
> >> -	 * Physical Functions (the SR-IOV Virtual Functions have no VPD
> >> -	 * Capabilities).  The PCI VPD Access core routines will normally
> >> +	 * later based adapter, provide access to the full virtual EEPROM
> >> +	 * address space. The PCI VPD Access core routines will normally
> >>  	 * compute the size of the VPD by parsing the VPD Data Structure at
> >>  	 * offset 0x000.  This will result in silent failures when attempting
> >>  	 * to accesses these other VPD areas which are beyond those computed
> >> @@ -644,7 +643,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
> >>  	if (chip == 0x0 && prod >= 0x20)
> >>  		pci_set_vpd_size(dev, 8192);
> >>  	else if (chip >= 0x4 && func < 0x8)
> >> -		pci_set_vpd_size(dev, 2048);
> >> +		pci_set_vpd_size(dev, PCI_VPD_MAX_SIZE);
> > 
> > This code was added by 7dcf688d4c78 ("PCI/cxgb4: Extend T3 PCI quirk
> > to T4+ devices") [1].  Unfortunately that commit doesn't really have
> > the details about what it fixes, other than the silent failures it
> > mentions in the comment.
> > 
> > Some devices hang if we try to read at the wrong VPD address, and this
> > can be done via the sysfs "vpd" file.  Can you expand the commit log
> > with an argument for why it is always safe to set the size to
> > PCI_VPD_MAX_SIZE for these devices?
> > 
> 
> Seeing t4_eeprom_ptov() there is data at the end of the VPD address
> space, but there may be gaps in between. I don't have test hw,
> therefore it would be good if Chelsio could confirm that accessing
> any address in the VPD address space (32K) is ok. If a VPD address
> isn't backed by EEPROM, it should return 0x00 or 0xff, and not hang
> the device.
> 

We've tested the patches on T5 adapter. Although there are no crashes
seen, the 32K VPD read from sysfs at certain chunks are getting wrapped
around and overwritten. We're still analyzing this.

> > The fact that cudbg_collect_vpd_data() fiddles around with
> > pci_set_vpd_size() suggests to me that there is *some* problem with
> > reading parts of the VPD.  Otherwise, why would they bother?
> > 
> > 940c9c458866 ("cxgb4: collect vpd info directly from hardware") [2]
> > added the pci_set_vpd_size() usage, but doesn't say why it's needed.
> > Maybe Rahul will remember?
> > 

If firmware has crashed, then it's not possible to collect the VPD info
from firmware. So, the VPD info is fetched from EEPROM instead, which
is unfortunately outside the VPD size of the PF.

> 
> In addition we have cb92148b58a4 ("PCI: Add pci_set_vpd_size() to set
> VPD size"). To me it seems the VPD size quirks and this commit
> try to achieve the same: allow to override the autodetected VPD len
> 
> The quirk mechanism is well established, and if possible I'd like
> to get rid of pci_set_vpd_size(). I don't like the idea that the
> PCI core exposes API calls for accessing a proprietary VPD data
> format of one specific vendor (cxgb4 is the only user of
> pci_set_vpd_size()).
> 

There seems to be a way to get the Serial Configuration Version from
some internal registers. I will send the patch soon. It should remove
the call to pci_set_vpd_size() from cudbg_lib.c.

Thanks,
Rahul

> > Bjorn
> > 
> > [1] https://git.kernel.org/linus/7dcf688d4c78
> > [2] https://git.kernel.org/linus/940c9c458866
> > 
> >>  }
> >>  
> >>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> >> -- 
> >> 2.30.0
> >>
> >>
> >>
> 

Re: [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space

[Heiner Kallweit]

On 08.02.2021 20:47, Rahul Lakkireddy wrote:
> On Friday, February 02/05/21, 2021 at 23:31:24 +0100, Heiner Kallweit wrote:
>> On 05.02.2021 22:46, Bjorn Helgaas wrote:
>>> [+cc Casey, Rahul]
>>>
>>> On Fri, Feb 05, 2021 at 08:29:45PM +0100, Heiner Kallweit wrote:
>>>> 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.
>>>
>>> s/inofficial/unofficial/
>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/pci/vpd.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>>>> index 7915d10f9..06a7954d0 100644
>>>> --- a/drivers/pci/vpd.c
>>>> +++ b/drivers/pci/vpd.c
>>>> @@ -633,9 +633,8 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>>>  	/*
>>>>  	 * If this is a T3-based adapter, there's a 1KB VPD area at offset
>>>>  	 * 0xc00 which contains the preferred VPD values.  If this is a T4 or
>>>> -	 * later based adapter, the special VPD is at offset 0x400 for the
>>>> -	 * Physical Functions (the SR-IOV Virtual Functions have no VPD
>>>> -	 * Capabilities).  The PCI VPD Access core routines will normally
>>>> +	 * later based adapter, provide access to the full virtual EEPROM
>>>> +	 * address space. The PCI VPD Access core routines will normally
>>>>  	 * compute the size of the VPD by parsing the VPD Data Structure at
>>>>  	 * offset 0x000.  This will result in silent failures when attempting
>>>>  	 * to accesses these other VPD areas which are beyond those computed
>>>> @@ -644,7 +643,7 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>>>  	if (chip == 0x0 && prod >= 0x20)
>>>>  		pci_set_vpd_size(dev, 8192);
>>>>  	else if (chip >= 0x4 && func < 0x8)
>>>> -		pci_set_vpd_size(dev, 2048);
>>>> +		pci_set_vpd_size(dev, PCI_VPD_MAX_SIZE);
>>>
>>> This code was added by 7dcf688d4c78 ("PCI/cxgb4: Extend T3 PCI quirk
>>> to T4+ devices") [1].  Unfortunately that commit doesn't really have
>>> the details about what it fixes, other than the silent failures it
>>> mentions in the comment.
>>>
>>> Some devices hang if we try to read at the wrong VPD address, and this
>>> can be done via the sysfs "vpd" file.  Can you expand the commit log
>>> with an argument for why it is always safe to set the size to
>>> PCI_VPD_MAX_SIZE for these devices?
>>>
>>
>> Seeing t4_eeprom_ptov() there is data at the end of the VPD address
>> space, but there may be gaps in between. I don't have test hw,
>> therefore it would be good if Chelsio could confirm that accessing
>> any address in the VPD address space (32K) is ok. If a VPD address
>> isn't backed by EEPROM, it should return 0x00 or 0xff, and not hang
>> the device.
>>
> 
> We've tested the patches on T5 adapter. Although there are no crashes
> seen, the 32K VPD read from sysfs at certain chunks are getting wrapped
> around and overwritten. We're still analyzing this.
> 
>>> The fact that cudbg_collect_vpd_data() fiddles around with
>>> pci_set_vpd_size() suggests to me that there is *some* problem with
>>> reading parts of the VPD.  Otherwise, why would they bother?
>>>
>>> 940c9c458866 ("cxgb4: collect vpd info directly from hardware") [2]
>>> added the pci_set_vpd_size() usage, but doesn't say why it's needed.
>>> Maybe Rahul will remember?
>>>
> 
> If firmware has crashed, then it's not possible to collect the VPD info
> from firmware. So, the VPD info is fetched from EEPROM instead, which
> is unfortunately outside the VPD size of the PF.
> 
>>
>> In addition we have cb92148b58a4 ("PCI: Add pci_set_vpd_size() to set
>> VPD size"). To me it seems the VPD size quirks and this commit
>> try to achieve the same: allow to override the autodetected VPD len
>>
>> The quirk mechanism is well established, and if possible I'd like
>> to get rid of pci_set_vpd_size(). I don't like the idea that the
>> PCI core exposes API calls for accessing a proprietary VPD data
>> format of one specific vendor (cxgb4 is the only user of
>> pci_set_vpd_size()).
>>
> 
> There seems to be a way to get the Serial Configuration Version from
> some internal registers. I will send the patch soon. It should remove
> the call to pci_set_vpd_size() from cudbg_lib.c.
> 
Great, this would be the best solution. Then we can get rid of
pci_set_vpd_size() w/o impact on what is exposed via sysfs.

So for now I'll drop patches 2 and 3 and will resend patch 1
as a single patch. 

> Thanks,
> Rahul
> 
Heiner

>>> Bjorn
>>>
>>> [1] https://git.kernel.org/linus/7dcf688d4c78
>>> [2] https://git.kernel.org/linus/940c9c458866
>>>
>>>>  }
>>>>  
>>>>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>>>> -- 
>>>> 2.30.0
>>>>
>>>>
>>>>
>>