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@xxxxxxxxx>
>> ---
>> 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
>>
>>
>>
