[PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk

[PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk

[Heiner Kallweit]

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.
- */
-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;
-	}
-}
-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

Re: [PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk

[Bjorn Helgaas]

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?

> - */
> -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?

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?

> -	}
> -}
> -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
> 

Re: [PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk

[Heiner Kallweit]

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

Re: [PATCH v2] PCI/VPD: Remove not any longer needed Broadcom NIC quirk

[Bjorn Helgaas]

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>

Applied to pci/vpd for v5.13 with subject "PCI/VPD: Remove obsolete
Broadcom NIC quirk", thanks!

> ---
> 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.
> - */
> -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;
> -	}
> -}
> -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
>