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

[Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>]

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.