linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Heiner Kallweit <hkallweit1@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk
Date: Wed, 3 Feb 2021 08:09:28 +0100	[thread overview]
Message-ID: <3f5b1d49-1f7f-e32b-7060-8d11afedd964@gmail.com> (raw)
In-Reply-To: <20210202231210.GA148198@bjorn-Precision-5520>

On 03.02.2021 00:12, Bjorn Helgaas wrote:
> On Thu, Dec 17, 2020 at 09:59:03PM +0100, Heiner Kallweit wrote:
>> This quirk was added in 2008 [0] when we didn't have the logic yet to
>> determine VPD size based on checking for the VPD end tag. Now that we
>> have this logic [1] and don't read beyond the end tag this quirk can
>> be removed.
>>
>> [0] 99cb233d60cb ("PCI: Limit VPD read/write lengths for Broadcom 5706, 5708, 5709 rev.")
>> [1] 104daa71b396 ("PCI: Determine actual VPD size on first access")
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> This is basically the same as what you're currently discussing
>> for the Marvell / QLogic 1077 quirk.
>> ---
>>  drivers/pci/vpd.c | 46 ----------------------------------------------
>>  1 file changed, 46 deletions(-)
>>
>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>> index 7915d10f9..ef5165eb3 100644
>> --- a/drivers/pci/vpd.c
>> +++ b/drivers/pci/vpd.c
>> @@ -578,52 +578,6 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_QLOGIC, 0x2261, quirk_blacklist_vpd);
>>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
>>  			      PCI_CLASS_BRIDGE_PCI, 8, quirk_blacklist_vpd);
>>  
>> -/*
>> - * For Broadcom 5706, 5708, 5709 rev. A nics, any read beyond the
>> - * VPD end tag will hang the device.  This problem was initially
>> - * observed when a vpd entry was created in sysfs
>> - * ('/sys/bus/pci/devices/<id>/vpd').   A read to this sysfs entry
>> - * will dump 32k of data.  Reading a full 32k will cause an access
>> - * beyond the VPD end tag causing the device to hang.  Once the device
>> - * is hung, the bnx2 driver will not be able to reset the device.
> 
>> - * We believe that it is legal to read beyond the end tag and
>> - * therefore the solution is to limit the read/write length.
> 
> So was this comment supposed to say it's *illegal* to read beyond the
> end tag?
> 

I think it's supposed to say that the PCI spec doesn't forbid accessing
VPD addresses not backed by actual VPD. But after 104daa71b396 this
isn't really relevant any longer, now we prevent such access beyond the
end tag.

>> - */
>> -static void quirk_brcm_570x_limit_vpd(struct pci_dev *dev)
>> -{
>> -	/*
>> -	 * Only disable the VPD capability for 5706, 5706S, 5708,
>> -	 * 5708S and 5709 rev. A
>> -	 */
>> -	if ((dev->device == PCI_DEVICE_ID_NX2_5706) ||
>> -	    (dev->device == PCI_DEVICE_ID_NX2_5706S) ||
>> -	    (dev->device == PCI_DEVICE_ID_NX2_5708) ||
>> -	    (dev->device == PCI_DEVICE_ID_NX2_5708S) ||
>> -	    ((dev->device == PCI_DEVICE_ID_NX2_5709) &&
>> -	     (dev->revision & 0xf0) == 0x0)) {
>> -		if (dev->vpd)
>> -			dev->vpd->len = 0x80;
> 
> And for these devices we just assume the valid VPD area is only 0x80
> bytes long?
> 

99cb233d60cb was submitted by someone from Broadcom, therefore I'd
assume that he had access to the device and data sheet, and 0x80
is the actual VPD size.

> And we believe that whatever 104daa71b396 ("PCI: Determine actual VPD
> size on first access") figures out is safe?  I'm guessing the actual
> VPD size on these NICs is determined by something on the device and it
> may not be 0x80.
> 
> So I guess the risk is that 104daa71b396 figures out the actual VPD
> size is, e.g., 0x100, and we now read the [0x80-0xff] region that we
> didn't read before, right?
> 
> If it works, great, but it is a potential change.  I'd love to remove
> these quirks.  I'm just slightly hesitant because the 99cb233d60cb
> commit log and comment seem to be exactly backwards.  Am I reading
> something wrong?
> 

After 104daa71b396 the code traverses the VPD structure until it finds
the end tag. Having said that we don't read beyond the end tag and
don't risk to hang the device, no matter whether the actual VPD size
is exactly 0x80, or less or more.

>> -	}
>> -}
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5706,
>> -			quirk_brcm_570x_limit_vpd);
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5706S,
>> -			quirk_brcm_570x_limit_vpd);
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5708,
>> -			quirk_brcm_570x_limit_vpd);
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5708S,
>> -			quirk_brcm_570x_limit_vpd);
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5709,
>> -			quirk_brcm_570x_limit_vpd);
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
>> -			PCI_DEVICE_ID_NX2_5709S,
>> -			quirk_brcm_570x_limit_vpd);
>> -
>>  static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>  {
>>  	int chip = (dev->device & 0xf000) >> 12;
>> -- 
>> 2.29.2
>>


  reply	other threads:[~2021-02-03  8:04 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-17 20:59 [PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk Heiner Kallweit
2021-02-02 23:12 ` Bjorn Helgaas
2021-02-03  7:09   ` Heiner Kallweit [this message]
2021-03-30 21:52 ` Bjorn Helgaas

Reply instructions:

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3f5b1d49-1f7f-e32b-7060-8d11afedd964@gmail.com \
    --to=hkallweit1@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=linux-pci@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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).