From: Heiner Kallweit <email@example.com>
To: Rahul Lakkireddy <firstname.lastname@example.org>,
Bjorn Helgaas <email@example.com>,
Raju Rangoju <firstname.lastname@example.org>,
Jakub Kicinski <email@example.com>,
David Miller <firstname.lastname@example.org>,
Bjorn Helgaas <email@example.com>,
Casey Leedom <firstname.lastname@example.org>
Cc: "email@example.com" <firstname.lastname@example.org>,
Subject: Re: [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space
Date: Mon, 8 Feb 2021 21:21:38 +0100 [thread overview]
Message-ID: <email@example.com> (raw)
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.
>>>> Signed-off-by: Heiner Kallweit <firstname.lastname@example.org>
>>>> 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") . 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") 
>>> 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
> 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.
>>>  https://git.kernel.org/linus/7dcf688d4c78
>>>  https://git.kernel.org/linus/940c9c458866
>>>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
next prev parent reply other threads:[~2021-02-08 20:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-05 19:26 [PATCH resend net-next v2 0/3] cxgb4: improve PCI VPD handling Heiner Kallweit
2021-02-05 19:29 ` [PATCH resend net-next v2 1/3] cxgb4: remove unused vpd_cap_addr Heiner Kallweit
2021-02-05 19:29 ` [PATCH resend net-next v2 2/3] PCI/VPD: Change Chelsio T4 quirk to provide access to full virtual address space Heiner Kallweit
2021-02-05 21:46 ` Bjorn Helgaas
2021-02-05 22:31 ` Heiner Kallweit
2021-02-08 19:47 ` Rahul Lakkireddy
2021-02-08 20:21 ` Heiner Kallweit [this message]
2021-02-05 19:30 ` [PATCH resend net-next v2 3/3] cxgb4: remove changing VPD len Heiner Kallweit
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).