| Message ID | 1f6d7b6c-7189-4fe3-926b-42609724cab9@CMEXHTCAS2.ad.emulex.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- 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
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. -- 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
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 -- 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
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 -- 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
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 -- 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
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. 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. 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. /Venkat.
[+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 -- 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
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
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
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 -- 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
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);
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(-)