CC'ing Casey, who would be able to answer this much better than me. > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Monday, November 17, 2014 3:32 PM > To: Venkat Duvvuru > Cc: linux-pci@xxxxxxxxxxxxxxx; 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@xxxxxxxxxx> wrote: > > Sorry for a delayed response. > > > >> -----Original Message----- > >> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > >> Sent: Tuesday, November 04, 2014 11:05 PM > >> To: Venkat Duvvuru > >> Cc: linux-pci@xxxxxxxxxxxxxxx > >> 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 ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
