[PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Venkat Duvvuru]

By default pci utilities/subsystem tries to read 32k bytes of vpd data no matter
what the device supports. This can lead to unexpected behavior depending
on how each of the devices handle this condition. This patch fixes the
problem for Emulex adapter family.

v1:
Addressed Bjorn's comments
1. Removed Vendor id and Device id macros from pci_ids.h and
   using the Vendor and Device id values directly in DECLARE_PCI_FIXUP_FINAL() lines.

Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
---
 drivers/pci/quirks.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 80c2d01..95d88f3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3832,3 +3832,36 @@ void pci_dev_specific_enable_acs(struct pci_dev *dev)
 		}
 	}
 }
+
+#define SLI_INTF_REG_OFFSET     0x58
+#define SLI_INTF_FT_MASK        0x00000001
+static void quirk_elx_limit_vpd(struct pci_dev *dev)
+{
+	int virtfn;
+	u32 sli_intf;
+
+	pci_read_config_dword(dev, SLI_INTF_REG_OFFSET, &sli_intf);
+	virtfn = (sli_intf & SLI_INTF_FT_MASK) ? 1 : 0;
+
+	if (dev->vpd) {
+		if (virtfn)
+			dev->vpd->len = 0x200;
+		else
+			dev->vpd->len = 0x400;
+	}
+}
+
+DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x211, quirk_elx_limit_vpd);
+DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x221, quirk_elx_limit_vpd);
+/* BE2 */
+DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x700, quirk_elx_limit_vpd);
+/* BE3 */
+DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x710, quirk_elx_limit_vpd);
+/* LANCER */
+DECLARE_PCI_FIXUP_FINAL(0x10df, 0xe220, quirk_elx_limit_vpd);
+/* LANCER VF */
+DECLARE_PCI_FIXUP_FINAL(0x10df, 0xe228, quirk_elx_limit_vpd);
+/* SKYHAWK */
+DECLARE_PCI_FIXUP_FINAL(0x10df, 0x720, quirk_elx_limit_vpd);
+/* SKYHAWK VF */
+DECLARE_PCI_FIXUP_FINAL(0x10df, 0x728, quirk_elx_limit_vpd);
-- 
1.7.1

Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Bjorn Helgaas]

On Thu, Oct 16, 2014 at 02:16:42PM +0530, Venkat Duvvuru wrote:
> By default pci utilities/subsystem tries to read 32k bytes of vpd data no matter
> what the device supports. This can lead to unexpected behavior depending
> on how each of the devices handle this condition. This patch fixes the
> problem for Emulex adapter family.
> 
> v1:
> Addressed Bjorn's comments
> 1. Removed Vendor id and Device id macros from pci_ids.h and
>    using the Vendor and Device id values directly in DECLARE_PCI_FIXUP_FINAL() lines.
> 
> Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>

Hi Venkat,

I'll merge this (in some form), but I'd like the changelog to include more
details about what unexpected behavior occurs when reading too much data.
This is to help people who trip over this problem find this patch as the
solution.

In my opinion, this is a hardware defect, and I'd like to know what your
hardware folks think, because I don't want to have to merge similar quirks
for future devices.  Here's my reasoning:

If a device doesn't implement the entire 32K of possible VPD space, I would
expect the device to just return zeros or 0xff, or maybe alias the space by
dropping the high-order unused address bits.

The only thing I see in the spec related to this is (PCI r3.0, Appendix I,
"VPD Data" description): "Reading or writing data outside of the VPD space
in the storage component is not allowed."  The only way I see for software
to determine the size of the storage is to parse the data looking for an
End Tag.

I don't think it's reasonable to make correct hardware operation depend on
the contents of the storage, so if something bad happens when software
reads past the end, that looks like a hardware defect to me.

Bjorn

> ---
>  drivers/pci/quirks.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 80c2d01..95d88f3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3832,3 +3832,36 @@ void pci_dev_specific_enable_acs(struct pci_dev *dev)
>  		}
>  	}
>  }
> +
> +#define SLI_INTF_REG_OFFSET     0x58
> +#define SLI_INTF_FT_MASK        0x00000001
> +static void quirk_elx_limit_vpd(struct pci_dev *dev)
> +{
> +	int virtfn;
> +	u32 sli_intf;
> +
> +	pci_read_config_dword(dev, SLI_INTF_REG_OFFSET, &sli_intf);
> +	virtfn = (sli_intf & SLI_INTF_FT_MASK) ? 1 : 0;
> +
> +	if (dev->vpd) {
> +		if (virtfn)
> +			dev->vpd->len = 0x200;
> +		else
> +			dev->vpd->len = 0x400;
> +	}
> +}
> +
> +DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x211, quirk_elx_limit_vpd);
> +DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x221, quirk_elx_limit_vpd);
> +/* BE2 */
> +DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x700, quirk_elx_limit_vpd);
> +/* BE3 */
> +DECLARE_PCI_FIXUP_FINAL(0x19a2, 0x710, quirk_elx_limit_vpd);
> +/* LANCER */
> +DECLARE_PCI_FIXUP_FINAL(0x10df, 0xe220, quirk_elx_limit_vpd);
> +/* LANCER VF */
> +DECLARE_PCI_FIXUP_FINAL(0x10df, 0xe228, quirk_elx_limit_vpd);
> +/* SKYHAWK */
> +DECLARE_PCI_FIXUP_FINAL(0x10df, 0x720, quirk_elx_limit_vpd);
> +/* SKYHAWK VF */
> +DECLARE_PCI_FIXUP_FINAL(0x10df, 0x728, quirk_elx_limit_vpd);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Venkat Duvvuru]

Hi Bjorn,
Please find my comments inline.

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Thursday, October 23, 2014 9:11 PM
> To: Venkat Duvvuru
> Cc: linux-pci@vger.kernel.org
> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual
> length supported.
> 
> On Thu, Oct 16, 2014 at 02:16:42PM +0530, Venkat Duvvuru wrote:
> > By default pci utilities/subsystem tries to read 32k bytes of vpd data no
> matter
> > what the device supports. This can lead to unexpected behavior depending
> > on how each of the devices handle this condition. This patch fixes the
> > problem for Emulex adapter family.
> >
> > v1:
> > Addressed Bjorn's comments
> > 1. Removed Vendor id and Device id macros from pci_ids.h and
> >    using the Vendor and Device id values directly in
> DECLARE_PCI_FIXUP_FINAL() lines.
> >
> > Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
> 
> Hi Venkat,
> 
> I'll merge this (in some form), but I'd like the changelog to include more
> details about what unexpected behavior occurs when reading too much data.
> This is to help people who trip over this problem find this patch as the
> solution.
[Venkat] "Timeout" happens on excessive VPD reads and  Kernel keeps logging the following message 
"vpd r/w failed.  This is likely a firmware bug on this device.  Contact the card vendor for a firmware update"
> 
> In my opinion, this is a hardware defect, and I'd like to know what your
> hardware folks think, because I don't want to have to merge similar quirks
> for future devices.  Here's my reasoning:
> 
> If a device doesn't implement the entire 32K of possible VPD space, I would
> expect the device to just return zeros or 0xff, or maybe alias the space by
> dropping the high-order unused address bits.
[Venkat] We do return 0xffs beyond the supported size but excessive VPD reads are causing timeouts when the adapter is handling some high priority work.

> 
> The only thing I see in the spec related to this is (PCI r3.0, Appendix I,
> "VPD Data" description): "Reading or writing data outside of the VPD space
> in the storage component is not allowed."  The only way I see for software
> to determine the size of the storage is to parse the data looking for an
> End Tag.
> 
> I don't think it's reasonable to make correct hardware operation depend on
> the contents of the storage, so if something bad happens when software
> reads past the end, that looks like a hardware defect to me.

Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Bjorn Helgaas]

On Thu, Oct 30, 2014 at 7:38 AM, Venkat Duvvuru
<VenkatKumar.Duvvuru@emulex.com> wrote:
> Hi Bjorn,
> Please find my comments inline.
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> Sent: Thursday, October 23, 2014 9:11 PM
>> To: Venkat Duvvuru
>> Cc: linux-pci@vger.kernel.org
>> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual
>> length supported.
>>
>> On Thu, Oct 16, 2014 at 02:16:42PM +0530, Venkat Duvvuru wrote:
>> > By default pci utilities/subsystem tries to read 32k bytes of vpd data no
>> matter
>> > what the device supports. This can lead to unexpected behavior depending
>> > on how each of the devices handle this condition. This patch fixes the
>> > problem for Emulex adapter family.
>> >
>> > v1:
>> > Addressed Bjorn's comments
>> > 1. Removed Vendor id and Device id macros from pci_ids.h and
>> >    using the Vendor and Device id values directly in
>> DECLARE_PCI_FIXUP_FINAL() lines.
>> >
>> > Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
>>
>> Hi Venkat,
>>
>> I'll merge this (in some form), but I'd like the changelog to include more
>> details about what unexpected behavior occurs when reading too much data.
>> This is to help people who trip over this problem find this patch as the
>> solution.
> [Venkat] "Timeout" happens on excessive VPD reads and  Kernel keeps logging the following message
> "vpd r/w failed.  This is likely a firmware bug on this device.  Contact the card vendor for a firmware update"
>>
>> In my opinion, this is a hardware defect, and I'd like to know what your
>> hardware folks think, because I don't want to have to merge similar quirks
>> for future devices.  Here's my reasoning:
>>
>> If a device doesn't implement the entire 32K of possible VPD space, I would
>> expect the device to just return zeros or 0xff, or maybe alias the space by
>> dropping the high-order unused address bits.
> [Venkat] We do return 0xffs beyond the supported size but excessive VPD reads are causing timeouts when the adapter is handling some high priority work.

That makes it sounds like this is really an issue with how the adapter
firmware manages the workload, not something strictly related to the
size of implemented VPD space.  In other words, it sounds like it's
possible for the timeout to occur even when reading the space that
*is* implemented.

You say the kernel "keeps logging" the message.  From the code, it
looks like it should only log it once per attempt to read or write the
VPD.  Is that what you observe, or is there a problem where we don't
abort the read/write after the first timeout, and we emit many
messages?

The message is already KERN_DEBUG, so it's pretty minimal.  If the
message is the only problem, maybe we could make it a one-time thing,
so it would only be emitted once per device.

But it looks like if we time out, pci_read_vpd() will return an error
instead of returning the data it has read so far.  So I suspect *that*
is the real problem.  If so, maybe we should look into returning a
short read with the data.

VPD is defined by the spec and supported by the generic PCI core.  So,
as you can tell, I have a problem with something that requires
device-specific knowledge in that generic code because that's not a
scalable model.

Bjorn

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Venkat Duvvuru]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmpvcm4gSGVsZ2FhcyBb
bWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+IFNlbnQ6IFRodXJzZGF5LCBPY3RvYmVyIDMw
LCAyMDE0IDk6MDMgUE0NCj4gVG86IFZlbmthdCBEdXZ2dXJ1DQo+IENjOiBsaW51eC1wY2lAdmdl
ci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjFdIHBjaTogTGltaXQgVlBEIGxl
bmd0aCBvZiBFbXVsZXggYWRhcHRlcnMgdG8gdGhlIGFjdHVhbA0KPiBsZW5ndGggc3VwcG9ydGVk
Lg0KPiANCj4gT24gVGh1LCBPY3QgMzAsIDIwMTQgYXQgNzozOCBBTSwgVmVua2F0IER1dnZ1cnUN
Cj4gPFZlbmthdEt1bWFyLkR1dnZ1cnVAZW11bGV4LmNvbT4gd3JvdGU6DQo+ID4gSGkgQmpvcm4s
DQo+ID4gUGxlYXNlIGZpbmQgbXkgY29tbWVudHMgaW5saW5lLg0KPiA+DQo+ID4+IC0tLS0tT3Jp
Z2luYWwgTWVzc2FnZS0tLS0tDQo+ID4+IEZyb206IEJqb3JuIEhlbGdhYXMgW21haWx0bzpiaGVs
Z2Fhc0Bnb29nbGUuY29tXQ0KPiA+PiBTZW50OiBUaHVyc2RheSwgT2N0b2JlciAyMywgMjAxNCA5
OjExIFBNDQo+ID4+IFRvOiBWZW5rYXQgRHV2dnVydQ0KPiA+PiBDYzogbGludXgtcGNpQHZnZXIu
a2VybmVsLm9yZw0KPiA+PiBTdWJqZWN0OiBSZTogW1BBVENIIHYxXSBwY2k6IExpbWl0IFZQRCBs
ZW5ndGggb2YgRW11bGV4IGFkYXB0ZXJzIHRvIHRoZQ0KPiBhY3R1YWwNCj4gPj4gbGVuZ3RoIHN1
cHBvcnRlZC4NCj4gPj4NCj4gPj4gT24gVGh1LCBPY3QgMTYsIDIwMTQgYXQgMDI6MTY6NDJQTSAr
MDUzMCwgVmVua2F0IER1dnZ1cnUgd3JvdGU6DQo+ID4+ID4gQnkgZGVmYXVsdCBwY2kgdXRpbGl0
aWVzL3N1YnN5c3RlbSB0cmllcyB0byByZWFkIDMyayBieXRlcyBvZiB2cGQgZGF0YSBubw0KPiA+
PiBtYXR0ZXINCj4gPj4gPiB3aGF0IHRoZSBkZXZpY2Ugc3VwcG9ydHMuIFRoaXMgY2FuIGxlYWQg
dG8gdW5leHBlY3RlZCBiZWhhdmlvciBkZXBlbmRpbmcNCj4gPj4gPiBvbiBob3cgZWFjaCBvZiB0
aGUgZGV2aWNlcyBoYW5kbGUgdGhpcyBjb25kaXRpb24uIFRoaXMgcGF0Y2ggZml4ZXMgdGhlDQo+
ID4+ID4gcHJvYmxlbSBmb3IgRW11bGV4IGFkYXB0ZXIgZmFtaWx5Lg0KPiA+PiA+DQo+ID4+ID4g
djE6DQo+ID4+ID4gQWRkcmVzc2VkIEJqb3JuJ3MgY29tbWVudHMNCj4gPj4gPiAxLiBSZW1vdmVk
IFZlbmRvciBpZCBhbmQgRGV2aWNlIGlkIG1hY3JvcyBmcm9tIHBjaV9pZHMuaCBhbmQNCj4gPj4g
PiAgICB1c2luZyB0aGUgVmVuZG9yIGFuZCBEZXZpY2UgaWQgdmFsdWVzIGRpcmVjdGx5IGluDQo+
ID4+IERFQ0xBUkVfUENJX0ZJWFVQX0ZJTkFMKCkgbGluZXMuDQo+ID4+ID4NCj4gPj4gPiBTaWdu
ZWQtb2ZmLWJ5OiBWZW5rYXQgRHV2dnVydSA8VmVua2F0S3VtYXIuRHV2dnVydUBFbXVsZXguY29t
Pg0KPiA+Pg0KPiA+PiBIaSBWZW5rYXQsDQo+ID4+DQo+ID4+IEknbGwgbWVyZ2UgdGhpcyAoaW4g
c29tZSBmb3JtKSwgYnV0IEknZCBsaWtlIHRoZSBjaGFuZ2Vsb2cgdG8gaW5jbHVkZSBtb3JlDQo+
ID4+IGRldGFpbHMgYWJvdXQgd2hhdCB1bmV4cGVjdGVkIGJlaGF2aW9yIG9jY3VycyB3aGVuIHJl
YWRpbmcgdG9vIG11Y2gNCj4gZGF0YS4NCj4gPj4gVGhpcyBpcyB0byBoZWxwIHBlb3BsZSB3aG8g
dHJpcCBvdmVyIHRoaXMgcHJvYmxlbSBmaW5kIHRoaXMgcGF0Y2ggYXMgdGhlDQo+ID4+IHNvbHV0
aW9uLg0KPiA+IFtWZW5rYXRdICJUaW1lb3V0IiBoYXBwZW5zIG9uIGV4Y2Vzc2l2ZSBWUEQgcmVh
ZHMgYW5kICBLZXJuZWwga2VlcHMNCj4gbG9nZ2luZyB0aGUgZm9sbG93aW5nIG1lc3NhZ2UNCj4g
PiAidnBkIHIvdyBmYWlsZWQuICBUaGlzIGlzIGxpa2VseSBhIGZpcm13YXJlIGJ1ZyBvbiB0aGlz
IGRldmljZS4gIENvbnRhY3QgdGhlIGNhcmQNCj4gdmVuZG9yIGZvciBhIGZpcm13YXJlIHVwZGF0
ZSINCj4gPj4NCj4gPj4gSW4gbXkgb3BpbmlvbiwgdGhpcyBpcyBhIGhhcmR3YXJlIGRlZmVjdCwg
YW5kIEknZCBsaWtlIHRvIGtub3cgd2hhdCB5b3VyDQo+ID4+IGhhcmR3YXJlIGZvbGtzIHRoaW5r
LCBiZWNhdXNlIEkgZG9uJ3Qgd2FudCB0byBoYXZlIHRvIG1lcmdlIHNpbWlsYXIgcXVpcmtzDQo+
ID4+IGZvciBmdXR1cmUgZGV2aWNlcy4gIEhlcmUncyBteSByZWFzb25pbmc6DQo+ID4+DQo+ID4+
IElmIGEgZGV2aWNlIGRvZXNuJ3QgaW1wbGVtZW50IHRoZSBlbnRpcmUgMzJLIG9mIHBvc3NpYmxl
IFZQRCBzcGFjZSwgSSB3b3VsZA0KPiA+PiBleHBlY3QgdGhlIGRldmljZSB0byBqdXN0IHJldHVy
biB6ZXJvcyBvciAweGZmLCBvciBtYXliZSBhbGlhcyB0aGUgc3BhY2UgYnkNCj4gPj4gZHJvcHBp
bmcgdGhlIGhpZ2gtb3JkZXIgdW51c2VkIGFkZHJlc3MgYml0cy4NCj4gPiBbVmVua2F0XSBXZSBk
byByZXR1cm4gMHhmZnMgYmV5b25kIHRoZSBzdXBwb3J0ZWQgc2l6ZSBidXQgZXhjZXNzaXZlIFZQ
RA0KPiByZWFkcyBhcmUgY2F1c2luZyB0aW1lb3V0cyB3aGVuIHRoZSBhZGFwdGVyIGlzIGhhbmRs
aW5nIHNvbWUgaGlnaCBwcmlvcml0eQ0KPiB3b3JrLg0KPiANCj4gVGhhdCBtYWtlcyBpdCBzb3Vu
ZHMgbGlrZSB0aGlzIGlzIHJlYWxseSBhbiBpc3N1ZSB3aXRoIGhvdyB0aGUgYWRhcHRlcg0KPiBm
aXJtd2FyZSBtYW5hZ2VzIHRoZSB3b3JrbG9hZCwgbm90IHNvbWV0aGluZyBzdHJpY3RseSByZWxh
dGVkIHRvIHRoZQ0KPiBzaXplIG9mIGltcGxlbWVudGVkIFZQRCBzcGFjZS4gSW4gb3RoZXIgd29y
ZHMsIGl0IHNvdW5kcyBsaWtlIGl0J3MNCj4gcG9zc2libGUgZm9yIHRoZSB0aW1lb3V0IHRvIG9j
Y3VyIGV2ZW4gd2hlbiByZWFkaW5nIHRoZSBzcGFjZSB0aGF0DQo+ICppcyogaW1wbGVtZW50ZWQu
DQpJbiB0aGlzIGNhc2Ugd2hlbiB0aGUgaG9zdCByZWFkcyAzMmsgc3BhY2UsIHRoZSBhZGFwdGVy
IGdldHMgYXJvdW5kIDhLIGludGVycnVwdHMgYW5kIHNvbWV0aW1lcyBnZXRzIG92ZXJ3aGVsbWVk
IHdpdGggdGhlIGludGVycnVwdCBzdG9ybS4gVGhpcyBjb3VsZCBjYXVzZSB0aGUgYWRhcHRlciB0
byBzdG9wIGZ1bmN0aW9uaW5nIHByb3Blcmx5Lg0KTGltaXRpbmcgdGhlIFZQRCByZWFkIHRvIDFL
IGNhdXNlcyBvbmx5IDI1NiBpbnRlcnJ1cHRzIChvbiB0aGUgYWRhcHRlcikgYW5kIHRoZSBwcm9i
bGVtIG5ldmVyIHNlZW1zIHRvIG9jY3VyLg0KVGhpcyBoYXMgYmVlbiB0aGUgbWFpbiBtb3RpdmF0
aW9uIGJlaGluZCBteSBwYXRjaC4NCkkgZG8gYWdyZWUgdGhhdCB0aGUgdGltZW91dCBjb3VsZCBz
dGlsbCBvY2N1ciBldmVuIHdoZW4gcmVhZGluZyB0aGUgMUsgaW1wbGVtZW50ZWQgc3BhY2UsIGJ1
dCBJIGZlZWwgaXQncyBoaWdobHkgaW1wcm9iYWJsZS4NCg0KQXMgYW4gYWx0ZXJuYXRpdmUgc29s
dXRpb24sIHdvdWxkIHlvdSBiZSBvcGVuIHRvIGEgZml4IGluIFBDSSAtY29yZSB0byBzdG9wIHJl
YWRpbmcgYWZ0ZXIgdGhlIEVuZC10YWcgaXMgZGV0ZWN0ZWQ/ICAoVGhpcyBsb2dpYyBpcyB1c2Vk
IGJ5IHBjaS11dGlsaXR5IChscy12cGQuYykgd2hpbGUgcmVhZGluZyBWUEQgZGF0YS4pDQpJIG5v
dyBmZWVsIHRoYXQgdGhpcyBpcyB0aGUgKnJpZ2h0KiBzb2x1dGlvbiB0aGFuIG15IHBjaS1xdWly
a3MgcGF0Y2guDQoNCj4gWW91IHNheSB0aGUga2VybmVsICJrZWVwcyBsb2dnaW5nIiB0aGUgbWVz
c2FnZS4gIEZyb20gdGhlIGNvZGUsIGl0DQo+IGxvb2tzIGxpa2UgaXQgc2hvdWxkIG9ubHkgbG9n
IGl0IG9uY2UgcGVyIGF0dGVtcHQgdG8gcmVhZCBvciB3cml0ZSB0aGUNCj4gVlBELiAgSXMgdGhh
dCB3aGF0IHlvdSBvYnNlcnZlLCBvciBpcyB0aGVyZSBhIHByb2JsZW0gd2hlcmUgd2UgZG9uJ3QN
Cj4gYWJvcnQgdGhlIHJlYWQvd3JpdGUgYWZ0ZXIgdGhlIGZpcnN0IHRpbWVvdXQsIGFuZCB3ZSBl
bWl0IG1hbnkNCj4gbWVzc2FnZXM/DQpZZXMgdGhlIGtlcm5lbCBsb2dzIG9ubHkgZm9yIG9uZSB0
aW1lIHBlciBhdHRlbXB0IGJ1dCB0aGVyZSBhcmUgY29uZmlndXJhdGlvbnMgd2hlcmUgd2UgaGF2
ZSBtYW55IFZGcyBwZXIgUEYgYW5kIHdlIHNlZSB0aGlzIG1lc3NhZ2UgZm9yIGV2ZXJ5IFZGIGFu
ZCBQRi4NCg0KVGhhbmtzLA0KVmVua2F0Lg0K

Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Bjorn Helgaas]

On Mon, Nov 3, 2014 at 5:18 AM, Venkat Duvvuru
<VenkatKumar.Duvvuru@emulex.com> wrote:
>
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> Sent: Thursday, October 30, 2014 9:03 PM
>> To: Venkat Duvvuru
>> Cc: linux-pci@vger.kernel.org
>> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual
>> length supported.
>>
>> On Thu, Oct 30, 2014 at 7:38 AM, Venkat Duvvuru
>> <VenkatKumar.Duvvuru@emulex.com> wrote:
>> > Hi Bjorn,
>> > Please find my comments inline.
>> >
>> >> -----Original Message-----
>> >> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> >> Sent: Thursday, October 23, 2014 9:11 PM
>> >> To: Venkat Duvvuru
>> >> Cc: linux-pci@vger.kernel.org
>> >> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the
>> actual
>> >> length supported.
>> >>
>> >> On Thu, Oct 16, 2014 at 02:16:42PM +0530, Venkat Duvvuru wrote:
>> >> > By default pci utilities/subsystem tries to read 32k bytes of vpd data no
>> >> matter
>> >> > what the device supports. This can lead to unexpected behavior depending
>> >> > on how each of the devices handle this condition. This patch fixes the
>> >> > problem for Emulex adapter family.
>> >> >
>> >> > v1:
>> >> > Addressed Bjorn's comments
>> >> > 1. Removed Vendor id and Device id macros from pci_ids.h and
>> >> >    using the Vendor and Device id values directly in
>> >> DECLARE_PCI_FIXUP_FINAL() lines.
>> >> >
>> >> > Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
>> >>
>> >> Hi Venkat,
>> >>
>> >> I'll merge this (in some form), but I'd like the changelog to include more
>> >> details about what unexpected behavior occurs when reading too much
>> data.
>> >> This is to help people who trip over this problem find this patch as the
>> >> solution.
>> > [Venkat] "Timeout" happens on excessive VPD reads and  Kernel keeps
>> logging the following message
>> > "vpd r/w failed.  This is likely a firmware bug on this device.  Contact the card
>> vendor for a firmware update"
>> >>
>> >> In my opinion, this is a hardware defect, and I'd like to know what your
>> >> hardware folks think, because I don't want to have to merge similar quirks
>> >> for future devices.  Here's my reasoning:
>> >>
>> >> If a device doesn't implement the entire 32K of possible VPD space, I would
>> >> expect the device to just return zeros or 0xff, or maybe alias the space by
>> >> dropping the high-order unused address bits.
>> > [Venkat] We do return 0xffs beyond the supported size but excessive VPD
>> reads are causing timeouts when the adapter is handling some high priority
>> work.
>>
>> That makes it sounds like this is really an issue with how the adapter
>> firmware manages the workload, not something strictly related to the
>> size of implemented VPD space. In other words, it sounds like it's
>> possible for the timeout to occur even when reading the space that
>> *is* implemented.
> In this case when the host reads 32k space, the adapter gets around 8K interrupts and sometimes gets overwhelmed with the interrupt storm. This could cause the adapter to stop functioning properly.
> Limiting the VPD read to 1K causes only 256 interrupts (on the adapter) and the problem never seems to occur.
> This has been the main motivation behind my patch.
> I do agree that the timeout could still occur even when reading the 1K implemented space, but I feel it's highly improbable.

OK.  I would guess that something like

  while /bin/true; do
    cat /sys/devices/pci0000:00/0000:00:00.0/vpd
  done

could still overwhelm the adapter, even with the 1K limit in place.

> As an alternative solution, would you be open to a fix in PCI -core to stop reading after the End-tag is detected?  (This logic is used by pci-utility (ls-vpd.c) while reading VPD data.)
> I now feel that this is the *right* solution than my pci-quirks patch.

That's a possibility, and if we were implementing VPD support from
scratch, I'd probably do it that way.

My only concern with changing now is that it could break existing
users for devices where the VPD content doesn't have the structure
specified by the spec.  There aren't *too* many users of
pci_read_vpd() in the tree, so it might be feasible to just ask the
bnx2x, tg3, cxgb4, sky2, and efx folks whether they think it's safe.

I took a quick look at those drivers, and it actually looks like most
of them look for the tag structure, e.g., by using pci_vpd_find_tag()
or doing something similar.  So maybe it actually would be safe to do
this.  Maybe you could have a more thorough look at them and see if
you agree?

>> You say the kernel "keeps logging" the message.  From the code, it
>> looks like it should only log it once per attempt to read or write the
>> VPD.  Is that what you observe, or is there a problem where we don't
>> abort the read/write after the first timeout, and we emit many
>> messages?
> Yes the kernel logs only for one time per attempt but there are configurations where we have many VFs per PF and we see this message for every VF and PF.

Makes sense.  It's really just one message per device, but there can
be many devices.

Bjorn

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Venkat Duvvuru]

U29ycnkgZm9yIGEgZGVsYXllZCByZXNwb25zZS4NCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2Ut
LS0tLQ0KPiBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86YmhlbGdhYXNAZ29vZ2xlLmNvbV0N
Cj4gU2VudDogVHVlc2RheSwgTm92ZW1iZXIgMDQsIDIwMTQgMTE6MDUgUE0NCj4gVG86IFZlbmth
dCBEdXZ2dXJ1DQo+IENjOiBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJl
OiBbUEFUQ0ggdjFdIHBjaTogTGltaXQgVlBEIGxlbmd0aCBvZiBFbXVsZXggYWRhcHRlcnMgdG8g
dGhlDQo+IGFjdHVhbCBsZW5ndGggc3VwcG9ydGVkLg0KPiANCj4gPiBJbiB0aGlzIGNhc2Ugd2hl
biB0aGUgaG9zdCByZWFkcyAzMmsgc3BhY2UsIHRoZSBhZGFwdGVyIGdldHMgYXJvdW5kIDhLDQo+
IGludGVycnVwdHMgYW5kIHNvbWV0aW1lcyBnZXRzIG92ZXJ3aGVsbWVkIHdpdGggdGhlIGludGVy
cnVwdCBzdG9ybS4gVGhpcw0KPiBjb3VsZCBjYXVzZSB0aGUgYWRhcHRlciB0byBzdG9wIGZ1bmN0
aW9uaW5nIHByb3Blcmx5Lg0KPiA+IExpbWl0aW5nIHRoZSBWUEQgcmVhZCB0byAxSyBjYXVzZXMg
b25seSAyNTYgaW50ZXJydXB0cyAob24gdGhlIGFkYXB0ZXIpIGFuZA0KPiB0aGUgcHJvYmxlbSBu
ZXZlciBzZWVtcyB0byBvY2N1ci4NCj4gPiBUaGlzIGhhcyBiZWVuIHRoZSBtYWluIG1vdGl2YXRp
b24gYmVoaW5kIG15IHBhdGNoLg0KPiA+IEkgZG8gYWdyZWUgdGhhdCB0aGUgdGltZW91dCBjb3Vs
ZCBzdGlsbCBvY2N1ciBldmVuIHdoZW4gcmVhZGluZyB0aGUgMUsNCj4gaW1wbGVtZW50ZWQgc3Bh
Y2UsIGJ1dCBJIGZlZWwgaXQncyBoaWdobHkgaW1wcm9iYWJsZS4NCj4gDQo+IE9LLiAgSSB3b3Vs
ZCBndWVzcyB0aGF0IHNvbWV0aGluZyBsaWtlDQo+IA0KPiAgIHdoaWxlIC9iaW4vdHJ1ZTsgZG8N
Cj4gICAgIGNhdCAvc3lzL2RldmljZXMvcGNpMDAwMDowMC8wMDAwOjAwOjAwLjAvdnBkDQo+ICAg
ZG9uZQ0KPiANCj4gY291bGQgc3RpbGwgb3ZlcndoZWxtIHRoZSBhZGFwdGVyLCBldmVuIHdpdGgg
dGhlIDFLIGxpbWl0IGluIHBsYWNlLg0KQ29ycmVjdC4NCg0KPiANCj4gPiBBcyBhbiBhbHRlcm5h
dGl2ZSBzb2x1dGlvbiwgd291bGQgeW91IGJlIG9wZW4gdG8gYSBmaXggaW4gUENJIC1jb3JlIHRv
IHN0b3ANCj4gcmVhZGluZyBhZnRlciB0aGUgRW5kLXRhZyBpcyBkZXRlY3RlZD8gIChUaGlzIGxv
Z2ljIGlzIHVzZWQgYnkgcGNpLXV0aWxpdHkgKGxzLQ0KPiB2cGQuYykgd2hpbGUgcmVhZGluZyBW
UEQgZGF0YS4pDQo+ID4gSSBub3cgZmVlbCB0aGF0IHRoaXMgaXMgdGhlICpyaWdodCogc29sdXRp
b24gdGhhbiBteSBwY2ktcXVpcmtzIHBhdGNoLg0KPiANCj4gVGhhdCdzIGEgcG9zc2liaWxpdHks
IGFuZCBpZiB3ZSB3ZXJlIGltcGxlbWVudGluZyBWUEQgc3VwcG9ydCBmcm9tDQo+IHNjcmF0Y2gs
IEknZCBwcm9iYWJseSBkbyBpdCB0aGF0IHdheS4NCj4gDQo+IE15IG9ubHkgY29uY2VybiB3aXRo
IGNoYW5naW5nIG5vdyBpcyB0aGF0IGl0IGNvdWxkIGJyZWFrIGV4aXN0aW5nDQo+IHVzZXJzIGZv
ciBkZXZpY2VzIHdoZXJlIHRoZSBWUEQgY29udGVudCBkb2Vzbid0IGhhdmUgdGhlIHN0cnVjdHVy
ZQ0KPiBzcGVjaWZpZWQgYnkgdGhlIHNwZWMuICBUaGVyZSBhcmVuJ3QgKnRvbyogbWFueSB1c2Vy
cyBvZg0KPiBwY2lfcmVhZF92cGQoKSBpbiB0aGUgdHJlZSwgc28gaXQgbWlnaHQgYmUgZmVhc2li
bGUgdG8ganVzdCBhc2sgdGhlDQo+IGJueDJ4LCB0ZzMsIGN4Z2I0LCBza3kyLCBhbmQgZWZ4IGZv
bGtzIHdoZXRoZXIgdGhleSB0aGluayBpdCdzIHNhZmUuDQo+IA0KPiBJIHRvb2sgYSBxdWljayBs
b29rIGF0IHRob3NlIGRyaXZlcnMsIGFuZCBpdCBhY3R1YWxseSBsb29rcyBsaWtlIG1vc3QNCj4g
b2YgdGhlbSBsb29rIGZvciB0aGUgdGFnIHN0cnVjdHVyZSwgZS5nLiwgYnkgdXNpbmcgcGNpX3Zw
ZF9maW5kX3RhZygpDQo+IG9yIGRvaW5nIHNvbWV0aGluZyBzaW1pbGFyLiAgU28gbWF5YmUgaXQg
YWN0dWFsbHkgd291bGQgYmUgc2FmZSB0byBkbw0KPiB0aGlzLiAgTWF5YmUgeW91IGNvdWxkIGhh
dmUgYSBtb3JlIHRob3JvdWdoIGxvb2sgYXQgdGhlbSBhbmQgc2VlIGlmDQo+IHlvdSBhZ3JlZT8N
CklmIHRoZSBkZXZpY2VzIGRvZXNuJ3QgZm9sbG93IHRoZSBzcGVjIGZvciB0aGUgVlBEIGNvbnRl
bnRzLCBwY2ktY29yZSBtYXkgZW5kdXAgcmVxdWVzdGluZyAzMmsgZGF0YSB3aGljaCBwcm9iYWJs
eSB3aWxsIG5vdCBicmVhayBleGlzdGluZyB1c2Vycy4NCkkgbG9va2VkIGludG8gYWxsIHRoZSBw
Y2lfcmVhZF92cGQgdXNlcnMgKGRyaXZlcnMpIGFuZCB0aGV5IHNlZW0gdG8gYmUgZG9pbmcgcGNp
X2ZpbmRfdnBkX3RhZyBvciBwY2lfdnBkX2ZpbmRfaW5mb19rZXl3b3JkIHRvIGZpbmQgYSBzcGVj
aWZpYyB0YWcva2V5d29yZCBhbmQgbm90IGFsbCB0aGUgdGFncy9rZXl3b3Jkcy4NCkEgc2FmZXIg
YXBwcm9hY2ggcHJvYmFibHkgaXMgdG8gbG9vayBmb3IgdGhlIGVuZCB0YWcgaW4gcGNpX3JlYWRf
dnBkLCBpZiBvZmZzZXQgaXMgInplcm8iIGJlY2F1c2Ugc29tZSBkcml2ZXJzIGFyZSBkb2luZyAi
cGNpX3JlYWRfdnBkIiB3aXRoIGEgbm9uLXplcm8gb2Zmc2V0Lg0KDQoNCi9WZW5rYXQuDQo=

Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Bjorn Helgaas]

[+cc Anish, Hariprasad (cxgb4 maintainers/contributors)]

Anish, Hariprasad, here's the problem:

  - pci_read_vpd() currently tries to read as much data as the caller
asks for (up to the 32K limit imposed by the PCI_VPD_PCI22_SIZE in
pci_vpd_pci22_init()) .  It does not look at the data, so it doesn't
stop if it sees an End Tag.

  - Some devices have buggy firmware that can't handle 32K worth of
VPD reads.  But their VPD data is in the format laid out by the spec
(PCI r3.0, sec I.1), and it does have an End Tag.

The proposal is to make pci_read_vpd() interpret the data into tagged
items (only when it starts reading at offset 0), and make it stop if
it encounters an End Tag.

On Mon, Nov 17, 2014 at 8:12 AM, Venkat Duvvuru
<VenkatKumar.Duvvuru@emulex.com> wrote:
> Sorry for a delayed response.
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> Sent: Tuesday, November 04, 2014 11:05 PM
>> To: Venkat Duvvuru
>> Cc: linux-pci@vger.kernel.org
>> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the
>> actual length supported.
>>
>> > In this case when the host reads 32k space, the adapter gets around 8K
>> interrupts and sometimes gets overwhelmed with the interrupt storm. This
>> could cause the adapter to stop functioning properly.
>> > Limiting the VPD read to 1K causes only 256 interrupts (on the adapter) and
>> the problem never seems to occur.
>> > This has been the main motivation behind my patch.
>> > I do agree that the timeout could still occur even when reading the 1K
>> implemented space, but I feel it's highly improbable.
>>
>> OK.  I would guess that something like
>>
>>   while /bin/true; do
>>     cat /sys/devices/pci0000:00/0000:00:00.0/vpd
>>   done
>>
>> could still overwhelm the adapter, even with the 1K limit in place.
> Correct.
>
>>
>> > As an alternative solution, would you be open to a fix in PCI -core to stop
>> reading after the End-tag is detected?  (This logic is used by pci-utility (ls-
>> vpd.c) while reading VPD data.)
>> > I now feel that this is the *right* solution than my pci-quirks patch.
>>
>> That's a possibility, and if we were implementing VPD support from
>> scratch, I'd probably do it that way.
>>
>> My only concern with changing now is that it could break existing
>> users for devices where the VPD content doesn't have the structure
>> specified by the spec.  There aren't *too* many users of
>> pci_read_vpd() in the tree, so it might be feasible to just ask the
>> bnx2x, tg3, cxgb4, sky2, and efx folks whether they think it's safe.
>>
>> I took a quick look at those drivers, and it actually looks like most
>> of them look for the tag structure, e.g., by using pci_vpd_find_tag()
>> or doing something similar.  So maybe it actually would be safe to do
>> this.  Maybe you could have a more thorough look at them and see if
>> you agree?
> If the devices doesn't follow the spec for the VPD contents, pci-core may endup requesting 32k data which probably will not break existing users.

The case I'm worried about is a device that doesn't follow the VPD
format spec, but its VPD contents include data that matches an End
Tag.  If we make pci_read_vpd() stop when it sees an End Tag, we may
stop reading data prematurely.

> I looked into all the pci_read_vpd users (drivers) and they seem to be doing pci_find_vpd_tag or pci_vpd_find_info_keyword to find a specific tag/keyword and not all the tags/keywords.

I agree, with one exception:  I am concerned about eeprom_rd_phys() in
cxgb4.  In that case, we use the VPD data to implement the
ethtool_ops.get_eeprom() method, and that path doesn't look at the
actual data at all, so I have no idea what the format might be.

Maybe Hariprasad or Anish can comment on this?

> A safer approach probably is to look for the end tag in pci_read_vpd, if offset is "zero" because some drivers are doing "pci_read_vpd" with a non-zero offset.

Yes, I think you can only look for the End Tag if you start reading at
offset zero.  If we start reading somewhere in the middle, we'll be
out of sync and may interpret data as an End Tag.

Bjorn

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Anish Bhatt]

Q0MnaW5nIENhc2V5LCB3aG8gd291bGQgYmUgYWJsZSB0byBhbnN3ZXIgdGhpcyBtdWNoIGJldHRl
ciB0aGFuIG1lLg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IEJqb3Ju
IEhlbGdhYXMgW21haWx0bzpiaGVsZ2Fhc0Bnb29nbGUuY29tXQ0KPiBTZW50OiBNb25kYXksIE5v
dmVtYmVyIDE3LCAyMDE0IDM6MzIgUE0NCj4gVG86IFZlbmthdCBEdXZ2dXJ1DQo+IENjOiBsaW51
eC1wY2lAdmdlci5rZXJuZWwub3JnOyBBbmlzaCBCaGF0dDsgSGFyaXByYXNhZCBTDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggdjFdIHBjaTogTGltaXQgVlBEIGxlbmd0aCBvZiBFbXVsZXggYWRhcHRl
cnMgdG8gdGhlDQo+IGFjdHVhbCBsZW5ndGggc3VwcG9ydGVkLg0KPiANCj4gWytjYyBBbmlzaCwg
SGFyaXByYXNhZCAoY3hnYjQgbWFpbnRhaW5lcnMvY29udHJpYnV0b3JzKV0NCj4gDQo+IEFuaXNo
LCBIYXJpcHJhc2FkLCBoZXJlJ3MgdGhlIHByb2JsZW06DQo+IA0KPiAgIC0gcGNpX3JlYWRfdnBk
KCkgY3VycmVudGx5IHRyaWVzIHRvIHJlYWQgYXMgbXVjaCBkYXRhIGFzIHRoZSBjYWxsZXIgYXNr
cyBmb3INCj4gKHVwIHRvIHRoZSAzMksgbGltaXQgaW1wb3NlZCBieSB0aGUgUENJX1ZQRF9QQ0ky
Ml9TSVpFIGluDQo+IHBjaV92cGRfcGNpMjJfaW5pdCgpKSAuICBJdCBkb2VzIG5vdCBsb29rIGF0
IHRoZSBkYXRhLCBzbyBpdCBkb2Vzbid0IHN0b3AgaWYgaXQgc2Vlcw0KPiBhbiBFbmQgVGFnLg0K
PiANCj4gICAtIFNvbWUgZGV2aWNlcyBoYXZlIGJ1Z2d5IGZpcm13YXJlIHRoYXQgY2FuJ3QgaGFu
ZGxlIDMySyB3b3J0aCBvZiBWUEQNCj4gcmVhZHMuICBCdXQgdGhlaXIgVlBEIGRhdGEgaXMgaW4g
dGhlIGZvcm1hdCBsYWlkIG91dCBieSB0aGUgc3BlYyAoUENJIHIzLjAsIHNlYw0KPiBJLjEpLCBh
bmQgaXQgZG9lcyBoYXZlIGFuIEVuZCBUYWcuDQo+IA0KPiBUaGUgcHJvcG9zYWwgaXMgdG8gbWFr
ZSBwY2lfcmVhZF92cGQoKSBpbnRlcnByZXQgdGhlIGRhdGEgaW50byB0YWdnZWQgaXRlbXMNCj4g
KG9ubHkgd2hlbiBpdCBzdGFydHMgcmVhZGluZyBhdCBvZmZzZXQgMCksIGFuZCBtYWtlIGl0IHN0
b3AgaWYgaXQgZW5jb3VudGVycyBhbg0KPiBFbmQgVGFnLg0KPiANCj4gT24gTW9uLCBOb3YgMTcs
IDIwMTQgYXQgODoxMiBBTSwgVmVua2F0IER1dnZ1cnUNCj4gPFZlbmthdEt1bWFyLkR1dnZ1cnVA
ZW11bGV4LmNvbT4gd3JvdGU6DQo+ID4gU29ycnkgZm9yIGEgZGVsYXllZCByZXNwb25zZS4NCj4g
Pg0KPiA+PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+PiBGcm9tOiBCam9ybiBIZWxn
YWFzIFttYWlsdG86YmhlbGdhYXNAZ29vZ2xlLmNvbV0NCj4gPj4gU2VudDogVHVlc2RheSwgTm92
ZW1iZXIgMDQsIDIwMTQgMTE6MDUgUE0NCj4gPj4gVG86IFZlbmthdCBEdXZ2dXJ1DQo+ID4+IENj
OiBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnDQo+ID4+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjFd
IHBjaTogTGltaXQgVlBEIGxlbmd0aCBvZiBFbXVsZXggYWRhcHRlcnMgdG8NCj4gPj4gdGhlIGFj
dHVhbCBsZW5ndGggc3VwcG9ydGVkLg0KPiA+Pg0KPiA+PiA+IEluIHRoaXMgY2FzZSB3aGVuIHRo
ZSBob3N0IHJlYWRzIDMyayBzcGFjZSwgdGhlIGFkYXB0ZXIgZ2V0cyBhcm91bmQNCj4gPj4gPiA4
Sw0KPiA+PiBpbnRlcnJ1cHRzIGFuZCBzb21ldGltZXMgZ2V0cyBvdmVyd2hlbG1lZCB3aXRoIHRo
ZSBpbnRlcnJ1cHQgc3Rvcm0uDQo+ID4+IFRoaXMgY291bGQgY2F1c2UgdGhlIGFkYXB0ZXIgdG8g
c3RvcCBmdW5jdGlvbmluZyBwcm9wZXJseS4NCj4gPj4gPiBMaW1pdGluZyB0aGUgVlBEIHJlYWQg
dG8gMUsgY2F1c2VzIG9ubHkgMjU2IGludGVycnVwdHMgKG9uIHRoZQ0KPiA+PiA+IGFkYXB0ZXIp
IGFuZA0KPiA+PiB0aGUgcHJvYmxlbSBuZXZlciBzZWVtcyB0byBvY2N1ci4NCj4gPj4gPiBUaGlz
IGhhcyBiZWVuIHRoZSBtYWluIG1vdGl2YXRpb24gYmVoaW5kIG15IHBhdGNoLg0KPiA+PiA+IEkg
ZG8gYWdyZWUgdGhhdCB0aGUgdGltZW91dCBjb3VsZCBzdGlsbCBvY2N1ciBldmVuIHdoZW4gcmVh
ZGluZyB0aGUNCj4gPj4gPiAxSw0KPiA+PiBpbXBsZW1lbnRlZCBzcGFjZSwgYnV0IEkgZmVlbCBp
dCdzIGhpZ2hseSBpbXByb2JhYmxlLg0KPiA+Pg0KPiA+PiBPSy4gIEkgd291bGQgZ3Vlc3MgdGhh
dCBzb21ldGhpbmcgbGlrZQ0KPiA+Pg0KPiA+PiAgIHdoaWxlIC9iaW4vdHJ1ZTsgZG8NCj4gPj4g
ICAgIGNhdCAvc3lzL2RldmljZXMvcGNpMDAwMDowMC8wMDAwOjAwOjAwLjAvdnBkDQo+ID4+ICAg
ZG9uZQ0KPiA+Pg0KPiA+PiBjb3VsZCBzdGlsbCBvdmVyd2hlbG0gdGhlIGFkYXB0ZXIsIGV2ZW4g
d2l0aCB0aGUgMUsgbGltaXQgaW4gcGxhY2UuDQo+ID4gQ29ycmVjdC4NCj4gPg0KPiA+Pg0KPiA+
PiA+IEFzIGFuIGFsdGVybmF0aXZlIHNvbHV0aW9uLCB3b3VsZCB5b3UgYmUgb3BlbiB0byBhIGZp
eCBpbiBQQ0kgLWNvcmUNCj4gPj4gPiB0byBzdG9wDQo+ID4+IHJlYWRpbmcgYWZ0ZXIgdGhlIEVu
ZC10YWcgaXMgZGV0ZWN0ZWQ/ICAoVGhpcyBsb2dpYyBpcyB1c2VkIGJ5DQo+ID4+IHBjaS11dGls
aXR5IChscy0NCj4gPj4gdnBkLmMpIHdoaWxlIHJlYWRpbmcgVlBEIGRhdGEuKQ0KPiA+PiA+IEkg
bm93IGZlZWwgdGhhdCB0aGlzIGlzIHRoZSAqcmlnaHQqIHNvbHV0aW9uIHRoYW4gbXkgcGNpLXF1
aXJrcyBwYXRjaC4NCj4gPj4NCj4gPj4gVGhhdCdzIGEgcG9zc2liaWxpdHksIGFuZCBpZiB3ZSB3
ZXJlIGltcGxlbWVudGluZyBWUEQgc3VwcG9ydCBmcm9tDQo+ID4+IHNjcmF0Y2gsIEknZCBwcm9i
YWJseSBkbyBpdCB0aGF0IHdheS4NCj4gPj4NCj4gPj4gTXkgb25seSBjb25jZXJuIHdpdGggY2hh
bmdpbmcgbm93IGlzIHRoYXQgaXQgY291bGQgYnJlYWsgZXhpc3RpbmcNCj4gPj4gdXNlcnMgZm9y
IGRldmljZXMgd2hlcmUgdGhlIFZQRCBjb250ZW50IGRvZXNuJ3QgaGF2ZSB0aGUgc3RydWN0dXJl
DQo+ID4+IHNwZWNpZmllZCBieSB0aGUgc3BlYy4gIFRoZXJlIGFyZW4ndCAqdG9vKiBtYW55IHVz
ZXJzIG9mDQo+ID4+IHBjaV9yZWFkX3ZwZCgpIGluIHRoZSB0cmVlLCBzbyBpdCBtaWdodCBiZSBm
ZWFzaWJsZSB0byBqdXN0IGFzayB0aGUNCj4gPj4gYm54MngsIHRnMywgY3hnYjQsIHNreTIsIGFu
ZCBlZnggZm9sa3Mgd2hldGhlciB0aGV5IHRoaW5rIGl0J3Mgc2FmZS4NCj4gPj4NCj4gPj4gSSB0
b29rIGEgcXVpY2sgbG9vayBhdCB0aG9zZSBkcml2ZXJzLCBhbmQgaXQgYWN0dWFsbHkgbG9va3Mg
bGlrZSBtb3N0DQo+ID4+IG9mIHRoZW0gbG9vayBmb3IgdGhlIHRhZyBzdHJ1Y3R1cmUsIGUuZy4s
IGJ5IHVzaW5nIHBjaV92cGRfZmluZF90YWcoKQ0KPiA+PiBvciBkb2luZyBzb21ldGhpbmcgc2lt
aWxhci4gIFNvIG1heWJlIGl0IGFjdHVhbGx5IHdvdWxkIGJlIHNhZmUgdG8gZG8NCj4gPj4gdGhp
cy4gIE1heWJlIHlvdSBjb3VsZCBoYXZlIGEgbW9yZSB0aG9yb3VnaCBsb29rIGF0IHRoZW0gYW5k
IHNlZSBpZg0KPiA+PiB5b3UgYWdyZWU/DQo+ID4gSWYgdGhlIGRldmljZXMgZG9lc24ndCBmb2xs
b3cgdGhlIHNwZWMgZm9yIHRoZSBWUEQgY29udGVudHMsIHBjaS1jb3JlIG1heQ0KPiBlbmR1cCBy
ZXF1ZXN0aW5nIDMyayBkYXRhIHdoaWNoIHByb2JhYmx5IHdpbGwgbm90IGJyZWFrIGV4aXN0aW5n
IHVzZXJzLg0KPiANCj4gVGhlIGNhc2UgSSdtIHdvcnJpZWQgYWJvdXQgaXMgYSBkZXZpY2UgdGhh
dCBkb2Vzbid0IGZvbGxvdyB0aGUgVlBEIGZvcm1hdA0KPiBzcGVjLCBidXQgaXRzIFZQRCBjb250
ZW50cyBpbmNsdWRlIGRhdGEgdGhhdCBtYXRjaGVzIGFuIEVuZCBUYWcuICBJZiB3ZSBtYWtlDQo+
IHBjaV9yZWFkX3ZwZCgpIHN0b3Agd2hlbiBpdCBzZWVzIGFuIEVuZCBUYWcsIHdlIG1heSBzdG9w
IHJlYWRpbmcgZGF0YQ0KPiBwcmVtYXR1cmVseS4NCj4gDQo+ID4gSSBsb29rZWQgaW50byBhbGwg
dGhlIHBjaV9yZWFkX3ZwZCB1c2VycyAoZHJpdmVycykgYW5kIHRoZXkgc2VlbSB0byBiZQ0KPiBk
b2luZyBwY2lfZmluZF92cGRfdGFnIG9yIHBjaV92cGRfZmluZF9pbmZvX2tleXdvcmQgdG8gZmlu
ZCBhIHNwZWNpZmljDQo+IHRhZy9rZXl3b3JkIGFuZCBub3QgYWxsIHRoZSB0YWdzL2tleXdvcmRz
Lg0KPiANCj4gSSBhZ3JlZSwgd2l0aCBvbmUgZXhjZXB0aW9uOiAgSSBhbSBjb25jZXJuZWQgYWJv
dXQgZWVwcm9tX3JkX3BoeXMoKSBpbg0KPiBjeGdiNC4gIEluIHRoYXQgY2FzZSwgd2UgdXNlIHRo
ZSBWUEQgZGF0YSB0byBpbXBsZW1lbnQgdGhlDQo+IGV0aHRvb2xfb3BzLmdldF9lZXByb20oKSBt
ZXRob2QsIGFuZCB0aGF0IHBhdGggZG9lc24ndCBsb29rIGF0IHRoZSBhY3R1YWwNCj4gZGF0YSBh
dCBhbGwsIHNvIEkgaGF2ZSBubyBpZGVhIHdoYXQgdGhlIGZvcm1hdCBtaWdodCBiZS4NCj4gDQo+
IE1heWJlIEhhcmlwcmFzYWQgb3IgQW5pc2ggY2FuIGNvbW1lbnQgb24gdGhpcz8NCj4gDQo+ID4g
QSBzYWZlciBhcHByb2FjaCBwcm9iYWJseSBpcyB0byBsb29rIGZvciB0aGUgZW5kIHRhZyBpbiBw
Y2lfcmVhZF92cGQsIGlmDQo+IG9mZnNldCBpcyAiemVybyIgYmVjYXVzZSBzb21lIGRyaXZlcnMg
YXJlIGRvaW5nICJwY2lfcmVhZF92cGQiIHdpdGggYSBub24tDQo+IHplcm8gb2Zmc2V0Lg0KPiAN
Cj4gWWVzLCBJIHRoaW5rIHlvdSBjYW4gb25seSBsb29rIGZvciB0aGUgRW5kIFRhZyBpZiB5b3Ug
c3RhcnQgcmVhZGluZyBhdCBvZmZzZXQNCj4gemVyby4gIElmIHdlIHN0YXJ0IHJlYWRpbmcgc29t
ZXdoZXJlIGluIHRoZSBtaWRkbGUsIHdlJ2xsIGJlIG91dCBvZiBzeW5jIGFuZA0KPiBtYXkgaW50
ZXJwcmV0IGRhdGEgYXMgYW4gRW5kIFRhZy4NCj4gDQo+IEJqb3JuDQoNCg==

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Casey Leedom]

  First, we never read more than 1KB.  So this issue doesn't affect us.  Second, each of the Phycical Functions on out adapters (T3, T4 and T5) actually has 2 1KB VPD regions.  The first region at Offset 0x0 contains a copy of our VPD values but is reserved for OEM partners if they want to put special VPD values there.  The second region at Offset 0x400 contains Chelsio's VPD and it's the one which the Host Driver and on-chip firmware read.

  Our VPDs, both at Offset 0x0 and Offset 0x400, do conform to the PCI VPD format specifications.

  So for our needs, as long as any new API allows us to continue reading our Chelsio VPD starting at Offset 0x400 instead of fixing things at Offset 0x0, we're okay.

Casey


________________________________________
From: Anish Bhatt
Sent: Monday, November 17, 2014 4:06 PM
To: Bjorn Helgaas; Venkat Duvvuru
Cc: linux-pci@vger.kernel.org; Hariprasad S; Casey Leedom
Subject: RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

CC'ing Casey, who would be able to answer this much better than me.

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Monday, November 17, 2014 3:32 PM
> To: Venkat Duvvuru
> Cc: linux-pci@vger.kernel.org; Anish Bhatt; Hariprasad S
> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to the
> actual length supported.
>
> [+cc Anish, Hariprasad (cxgb4 maintainers/contributors)]
>
> Anish, Hariprasad, here's the problem:
>
>   - pci_read_vpd() currently tries to read as much data as the caller asks for
> (up to the 32K limit imposed by the PCI_VPD_PCI22_SIZE in
> pci_vpd_pci22_init()) .  It does not look at the data, so it doesn't stop if it sees
> an End Tag.
>
>   - Some devices have buggy firmware that can't handle 32K worth of VPD
> reads.  But their VPD data is in the format laid out by the spec (PCI r3.0, sec
> I.1), and it does have an End Tag.
>
> The proposal is to make pci_read_vpd() interpret the data into tagged items
> (only when it starts reading at offset 0), and make it stop if it encounters an
> End Tag.
>
> On Mon, Nov 17, 2014 at 8:12 AM, Venkat Duvvuru
> <VenkatKumar.Duvvuru@emulex.com> wrote:
> > Sorry for a delayed response.
> >
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> >> Sent: Tuesday, November 04, 2014 11:05 PM
> >> To: Venkat Duvvuru
> >> Cc: linux-pci@vger.kernel.org
> >> Subject: Re: [PATCH v1] pci: Limit VPD length of Emulex adapters to
> >> the actual length supported.
> >>
> >> > In this case when the host reads 32k space, the adapter gets around
> >> > 8K
> >> interrupts and sometimes gets overwhelmed with the interrupt storm.
> >> This could cause the adapter to stop functioning properly.
> >> > Limiting the VPD read to 1K causes only 256 interrupts (on the
> >> > adapter) and
> >> the problem never seems to occur.
> >> > This has been the main motivation behind my patch.
> >> > I do agree that the timeout could still occur even when reading the
> >> > 1K
> >> implemented space, but I feel it's highly improbable.
> >>
> >> OK.  I would guess that something like
> >>
> >>   while /bin/true; do
> >>     cat /sys/devices/pci0000:00/0000:00:00.0/vpd
> >>   done
> >>
> >> could still overwhelm the adapter, even with the 1K limit in place.
> > Correct.
> >
> >>
> >> > As an alternative solution, would you be open to a fix in PCI -core
> >> > to stop
> >> reading after the End-tag is detected?  (This logic is used by
> >> pci-utility (ls-
> >> vpd.c) while reading VPD data.)
> >> > I now feel that this is the *right* solution than my pci-quirks patch.
> >>
> >> That's a possibility, and if we were implementing VPD support from
> >> scratch, I'd probably do it that way.
> >>
> >> My only concern with changing now is that it could break existing
> >> users for devices where the VPD content doesn't have the structure
> >> specified by the spec.  There aren't *too* many users of
> >> pci_read_vpd() in the tree, so it might be feasible to just ask the
> >> bnx2x, tg3, cxgb4, sky2, and efx folks whether they think it's safe.
> >>
> >> I took a quick look at those drivers, and it actually looks like most
> >> of them look for the tag structure, e.g., by using pci_vpd_find_tag()
> >> or doing something similar.  So maybe it actually would be safe to do
> >> this.  Maybe you could have a more thorough look at them and see if
> >> you agree?
> > If the devices doesn't follow the spec for the VPD contents, pci-core may
> endup requesting 32k data which probably will not break existing users.
>
> The case I'm worried about is a device that doesn't follow the VPD format
> spec, but its VPD contents include data that matches an End Tag.  If we make
> pci_read_vpd() stop when it sees an End Tag, we may stop reading data
> prematurely.
>
> > I looked into all the pci_read_vpd users (drivers) and they seem to be
> doing pci_find_vpd_tag or pci_vpd_find_info_keyword to find a specific
> tag/keyword and not all the tags/keywords.
>
> I agree, with one exception:  I am concerned about eeprom_rd_phys() in
> cxgb4.  In that case, we use the VPD data to implement the
> ethtool_ops.get_eeprom() method, and that path doesn't look at the actual
> data at all, so I have no idea what the format might be.
>
> Maybe Hariprasad or Anish can comment on this?
>
> > A safer approach probably is to look for the end tag in pci_read_vpd, if
> offset is "zero" because some drivers are doing "pci_read_vpd" with a non-
> zero offset.
>
> Yes, I think you can only look for the End Tag if you start reading at offset
> zero.  If we start reading somewhere in the middle, we'll be out of sync and
> may interpret data as an End Tag.
>
> Bjorn

RE: [PATCH v1] pci: Limit VPD length of Emulex adapters to the actual length supported.

[Venkat Duvvuru]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmpvcm4gSGVsZ2FhcyBb
bWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+IFNlbnQ6IFR1ZXNkYXksIE5vdmVtYmVyIDE4
LCAyMDE0IDU6MDIgQU0NCj4gVG86IFZlbmthdCBEdXZ2dXJ1DQo+IENjOiBsaW51eC1wY2lAdmdl
ci5rZXJuZWwub3JnOyBBbmlzaCBCaGF0dDsgSGFyaXByYXNhZCBTaGVuYWkNCj4gU3ViamVjdDog
UmU6IFtQQVRDSCB2MV0gcGNpOiBMaW1pdCBWUEQgbGVuZ3RoIG9mIEVtdWxleCBhZGFwdGVycyB0
byB0aGUNCj4gYWN0dWFsIGxlbmd0aCBzdXBwb3J0ZWQuDQo+IA0KPiBbK2NjIEFuaXNoLCBIYXJp
cHJhc2FkIChjeGdiNCBtYWludGFpbmVycy9jb250cmlidXRvcnMpXQ0KPiANCj4gQW5pc2gsIEhh
cmlwcmFzYWQsIGhlcmUncyB0aGUgcHJvYmxlbToNCj4gPj4gSSB0b29rIGEgcXVpY2sgbG9vayBh
dCB0aG9zZSBkcml2ZXJzLCBhbmQgaXQgYWN0dWFsbHkgbG9va3MgbGlrZSBtb3N0DQo+ID4+IG9m
IHRoZW0gbG9vayBmb3IgdGhlIHRhZyBzdHJ1Y3R1cmUsIGUuZy4sIGJ5IHVzaW5nIHBjaV92cGRf
ZmluZF90YWcoKQ0KPiA+PiBvciBkb2luZyBzb21ldGhpbmcgc2ltaWxhci4gIFNvIG1heWJlIGl0
IGFjdHVhbGx5IHdvdWxkIGJlIHNhZmUgdG8gZG8NCj4gPj4gdGhpcy4gIE1heWJlIHlvdSBjb3Vs
ZCBoYXZlIGEgbW9yZSB0aG9yb3VnaCBsb29rIGF0IHRoZW0gYW5kIHNlZSBpZg0KPiA+PiB5b3Ug
YWdyZWU/DQo+ID4gSWYgdGhlIGRldmljZXMgZG9lc24ndCBmb2xsb3cgdGhlIHNwZWMgZm9yIHRo
ZSBWUEQgY29udGVudHMsIHBjaS1jb3JlIG1heQ0KPiBlbmR1cCByZXF1ZXN0aW5nIDMyayBkYXRh
IHdoaWNoIHByb2JhYmx5IHdpbGwgbm90IGJyZWFrIGV4aXN0aW5nIHVzZXJzLg0KPiANCj4gVGhl
IGNhc2UgSSdtIHdvcnJpZWQgYWJvdXQgaXMgYSBkZXZpY2UgdGhhdCBkb2Vzbid0IGZvbGxvdyB0
aGUgVlBEDQo+IGZvcm1hdCBzcGVjLCBidXQgaXRzIFZQRCBjb250ZW50cyBpbmNsdWRlIGRhdGEg
dGhhdCBtYXRjaGVzIGFuIEVuZA0KPiBUYWcuICBJZiB3ZSBtYWtlIHBjaV9yZWFkX3ZwZCgpIHN0
b3Agd2hlbiBpdCBzZWVzIGFuIEVuZCBUYWcsIHdlIG1heQ0KPiBzdG9wIHJlYWRpbmcgZGF0YSBw
cmVtYXR1cmVseS4NCltWZW5rYXRdIEkgdGhpbmsgaXQncyBmYWlyIHRvIGFzc3VtZSB0aGF0IGRl
dmljZXMgZWl0aGVyIGZvbGxvdyB0aGUgc3BlYyBvciB0aGV5IGRvbid0Lg0KRXZlbiBpZiB0aGUg
ZGV2aWNlcyBwYXJ0aWFsbHkgZm9sbG93IHRoZSBzcGVjLCBJZiB0aGUgZmlyc3QgYnl0ZSAoYW5k
IHRoZSBzdWJzZXF1ZW50IHJlbGV2YW50IGJ5dGVzIHdoaWNoIGFyZSBjYWxjdWxhdGVkIGJhc2Vk
IG9uIHRoZSBsZW5ndGggYW5kIGRhdGEgb2YgdGhhdCByZXNvdXJjZSkgb2YgdGhlIFZQRCBkYXRh
IGhhcyBhIHZhbGlkIHRhZywgd2UgY2FuIHN0b3AgYWZ0ZXIgdGhlIEVuZCB0YWcsIG90aGVyd2lz
ZSByZWFkIDMyayB3b3J0aCBkYXRhLg0KTWlzdW5kZXJzdGFuZGluZyBvdGhlciBWUEQgY29udGVu
dHMgYXMgRW5kIHRhZyBzZWVtcyB2ZXJ5IHVubGlrZWx5IGFzIHRoZSBwYXJzZXIgY29kZSBjYW4g
ZGlzdGluZ3Vpc2ggdGFnLCBsZW5ndGggYW5kIGRhdGEgYmFzZWQgb24gdGhlIHNwZWMuDQpGb3Ig
ZXhhbXBsZSwgcGNpIHV0aWxpdGllcyBhbHdheXMgbG9vayBmb3IgdGhlIEVuZCB0YWcgd2hpbGUg
cmVhZGluZyBWUEQgZGF0YS4gKGNhcF92cGQgcm91dGluZSBpbiBscy12cGQuYykNCg0K