| Message ID | 1450339169-52542-3-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Tested with a HP AE311-60001 PCIe card. It used to repeat the same VPD every 4k
for 32k now only the 154 bytes are returned and lspci -vvvv reports that the data up
to and including the end and that the check sum is good:
...
Capabilities: [74] Vital Product Data
Product Name: PCI-Express 4Gb Fibre Channel HBA
Read-only fields:
[PN] Part number: AE311-60001
[EC] Engineering changes: C-4830
[SN] Serial number: CN80847W3U
[V0] Vendor specific: PW=15W
[V2] Vendor specific: 4847
[MN] Manufacture ID: 50 58 32 35 31 30 34 30 31 2d 37 30 20 20 42
[V1] Vendor specific: 02.22
[V3] Vendor specific: 05.03.15
[V4] Vendor specific: 03.13
[V5] Vendor specific: 02.03
[YA] Asset tag: NA
[RV] Reserved: checksum good, 0 byte(s) reserved
End
...
---
Tested-by: Shane Seymour <shane.seymour@hpe.com>
--
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 Hannes, [auto build test WARNING on pci/next] [also build test WARNING on v4.4-rc5 next-20151217] url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/pci-Update-VPD-definitions/20151217-160050 base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next reproduce: make htmldocs All warnings (new ones prefixed by >>): lib/crc32.c:148: warning: No description found for parameter 'tab)[256]' lib/crc32.c:148: warning: Excess function parameter 'tab' description in 'crc32_le_generic' lib/crc32.c:293: warning: No description found for parameter 'tab)[256]' lib/crc32.c:293: warning: Excess function parameter 'tab' description in 'crc32_be_generic' lib/crc32.c:1: warning: no structured comments found >> drivers/pci/access.c:486: warning: Excess function parameter 'old_size' description in 'pci_vpd_pci22_size' vim +486 drivers/pci/access.c 470 } 471 472 static const struct pci_vpd_ops pci_vpd_f0_ops = { 473 .read = pci_vpd_f0_read, 474 .write = pci_vpd_f0_write, 475 .release = pci_vpd_pci22_release, 476 }; 477 478 /** 479 * pci_vpd_size - determine actual size of Vital Product Data 480 * @dev: pci device struct 481 * @old_size: current assumed size, also maximum allowed size 482 * 483 */ 484 static size_t 485 pci_vpd_pci22_size(struct pci_dev *dev) > 486 { 487 size_t off = 0; 488 unsigned char header[1+2]; /* 1 byte tag, 2 bytes length */ 489 490 while (off < PCI_VPD_PCI22_SIZE && 491 pci_read_vpd(dev, off, 1, header) == 1) { 492 unsigned char tag; 493 494 if (header[0] & PCI_VPD_LRDT) { --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Dec 16, 2015 at 11:59 PM, Hannes Reinecke <hare@suse.de> wrote: > PCI-2.2 VPD entries have a maximum size of 32k, but might actually > be smaller than that. To figure out the actual size one has to read > the VPD area until the 'end marker' is reached. > Trying to read VPD data beyond that marker results in 'interesting' > effects, from simple read errors to crashing the card. And to make > matters worse not every PCI card implements this properly, leaving > us with no 'end' marker or even completely invalid data. > This path modifies the size of the VPD attribute to the available > size, and disables the VPD attribute altogether if no valid data > could be read. > > Cc: Alexander Duyck <alexander.duyck@gmail.com> > Cc: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index 59ac36f..0a647b1 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = { > .release = pci_vpd_pci22_release, > }; > > +/** > + * pci_vpd_size - determine actual size of Vital Product Data > + * @dev: pci device struct > + * @old_size: current assumed size, also maximum allowed size > + * "old_siz"e was dropped so you can remove this line. > + */ > +static size_t > +pci_vpd_pci22_size(struct pci_dev *dev) > +{ > + size_t off = 0; > + unsigned char header[1+2]; /* 1 byte tag, 2 bytes length */ > + > + while (off < PCI_VPD_PCI22_SIZE && > + pci_read_vpd(dev, off, 1, header) == 1) { > + unsigned char tag; > + The offset comparison is probably redundant. There is already a check in pci_vpd_pci22_read that will check the offset and return -EINVAL if we have exceeded vpd->base.len. As such you can probably just do the pci_read_vpd comparison and drop the offset length entirely. > + if (header[0] & PCI_VPD_LRDT) { > + /* Large Resource Data Type Tag */ > + tag = pci_vpd_lrdt_tag(header); > + /* Only read length from known tag items */ > + if ((tag == PCI_VPD_LTIN_ID_STRING) || > + (tag == PCI_VPD_LTIN_RO_DATA) || > + (tag == PCI_VPD_LTIN_RW_DATA)) { > + if (pci_read_vpd(dev, off+1, 2, > + &header[1]) != 2) > + return off + 1; > + off += PCI_VPD_LRDT_TAG_SIZE + > + pci_vpd_lrdt_size(header); > + } > + } else { > + /* Short Resource Data Type Tag */ > + off += PCI_VPD_SRDT_TAG_SIZE + > + pci_vpd_srdt_size(header); > + tag = pci_vpd_srdt_tag(header); > + } > + if (tag == PCI_VPD_STIN_END) /* End tag descriptor */ > + return off; > + if ((tag != PCI_VPD_LTIN_ID_STRING) && > + (tag != PCI_VPD_LTIN_RO_DATA) && > + (tag != PCI_VPD_LTIN_RW_DATA)) { > + dev_dbg(&dev->dev, > + "invalid %s vpd tag %02x at offset %zu.", > + (header[0] & PCI_VPD_LRDT) ? "large" : "short", > + tag, off); > + break; > + } > + } > + return 0; > +} > + > int pci_vpd_pci22_init(struct pci_dev *dev) > { > struct pci_vpd_pci22 *vpd; > @@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev) > vpd->cap = cap; > vpd->busy = false; > dev->vpd = &vpd->base; > + vpd->base.len = pci_vpd_pci22_size(dev); > + if (vpd->base.len == 0) { > + dev_dbg(&dev->dev, "Disabling VPD access."); > + dev->vpd = NULL; > + kfree(vpd); > + return -ENXIO; > + } > return 0; > } It looks like this still doesn't address the VPD_REF_F0 issue I mentioned earlier. We don't need to compute the length for each function we only need to do it once. I would recommend modifying things so that you set vpd->base.len to 0 if the VPD_REF_F0 flag is set. Also I wouldn't delete the vpd configuration if the length is not correct as that will likely break several quirks that already exist that are setting the length. Also there is no need to return an error, the fact is the part has VPD but we cannot determine the length as such the correct solution is to leave it at 0. We can leave that for a quirk to sort out later if needed. You could probably move the dev_dbg message to just before the return 0 in the pci_vpd_pci22_size call and drop the entire if statement in the init function. - Alex -- 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 12/17/2015 06:13 PM, Alexander Duyck wrote: > On Wed, Dec 16, 2015 at 11:59 PM, Hannes Reinecke <hare@suse.de> wrote: >> PCI-2.2 VPD entries have a maximum size of 32k, but might actually >> be smaller than that. To figure out the actual size one has to read >> the VPD area until the 'end marker' is reached. >> Trying to read VPD data beyond that marker results in 'interesting' >> effects, from simple read errors to crashing the card. And to make >> matters worse not every PCI card implements this properly, leaving >> us with no 'end' marker or even completely invalid data. >> This path modifies the size of the VPD attribute to the available >> size, and disables the VPD attribute altogether if no valid data >> could be read. >> >> Cc: Alexander Duyck <alexander.duyck@gmail.com> >> Cc: Bjorn Helgaas <helgaas@kernel.org> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/drivers/pci/access.c b/drivers/pci/access.c >> index 59ac36f..0a647b1 100644 >> --- a/drivers/pci/access.c >> +++ b/drivers/pci/access.c >> @@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = { >> .release = pci_vpd_pci22_release, >> }; >> >> +/** >> + * pci_vpd_size - determine actual size of Vital Product Data >> + * @dev: pci device struct >> + * @old_size: current assumed size, also maximum allowed size >> + * > > "old_siz"e was dropped so you can remove this line. > >> + */ >> +static size_t >> +pci_vpd_pci22_size(struct pci_dev *dev) >> +{ >> + size_t off = 0; >> + unsigned char header[1+2]; /* 1 byte tag, 2 bytes length */ >> + >> + while (off < PCI_VPD_PCI22_SIZE && >> + pci_read_vpd(dev, off, 1, header) == 1) { >> + unsigned char tag; >> + > > The offset comparison is probably redundant. There is already a check > in pci_vpd_pci22_read that will check the offset and return -EINVAL if > we have exceeded vpd->base.len. As such you can probably just do the > pci_read_vpd comparison and drop the offset length entirely. > Indeed it does. Will be doing so. >> + if (header[0] & PCI_VPD_LRDT) { >> + /* Large Resource Data Type Tag */ >> + tag = pci_vpd_lrdt_tag(header); >> + /* Only read length from known tag items */ >> + if ((tag == PCI_VPD_LTIN_ID_STRING) || >> + (tag == PCI_VPD_LTIN_RO_DATA) || >> + (tag == PCI_VPD_LTIN_RW_DATA)) { >> + if (pci_read_vpd(dev, off+1, 2, >> + &header[1]) != 2) >> + return off + 1; >> + off += PCI_VPD_LRDT_TAG_SIZE + >> + pci_vpd_lrdt_size(header); >> + } >> + } else { >> + /* Short Resource Data Type Tag */ >> + off += PCI_VPD_SRDT_TAG_SIZE + >> + pci_vpd_srdt_size(header); >> + tag = pci_vpd_srdt_tag(header); >> + } >> + if (tag == PCI_VPD_STIN_END) /* End tag descriptor */ >> + return off; >> + if ((tag != PCI_VPD_LTIN_ID_STRING) && >> + (tag != PCI_VPD_LTIN_RO_DATA) && >> + (tag != PCI_VPD_LTIN_RW_DATA)) { >> + dev_dbg(&dev->dev, >> + "invalid %s vpd tag %02x at offset %zu.", >> + (header[0] & PCI_VPD_LRDT) ? "large" : "short", >> + tag, off); >> + break; >> + } >> + } >> + return 0; >> +} >> + >> int pci_vpd_pci22_init(struct pci_dev *dev) >> { >> struct pci_vpd_pci22 *vpd; >> @@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev) >> vpd->cap = cap; >> vpd->busy = false; >> dev->vpd = &vpd->base; >> + vpd->base.len = pci_vpd_pci22_size(dev); >> + if (vpd->base.len == 0) { >> + dev_dbg(&dev->dev, "Disabling VPD access."); >> + dev->vpd = NULL; >> + kfree(vpd); >> + return -ENXIO; >> + } >> return 0; >> } > > It looks like this still doesn't address the VPD_REF_F0 issue I > mentioned earlier. We don't need to compute the length for each > function we only need to do it once. I would recommend modifying > things so that you set vpd->base.len to 0 if the VPD_REF_F0 flag is > set. > But that would effectively inhibit access to the VPD on those devices, rendering the entire 'f0_ops' thingie quite pointless, right? I think it's better to directly retrieve the VPD length from the base pci device, that would give us the correct length _and_ save duplicate calculations. > Also I wouldn't delete the vpd configuration if the length is not > correct as that will likely break several quirks that already exist > that are setting the length. Also there is no need to return an > error, the fact is the part has VPD but we cannot determine the length > as such the correct solution is to leave it at 0. We can leave that > for a quirk to sort out later if needed. You could probably move the > dev_dbg message to just before the return 0 in the pci_vpd_pci22_size > call and drop the entire if statement in the init function. > Okay. Will be sending a new patch. Cheers, Hannes
diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 59ac36f..0a647b1 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = { .release = pci_vpd_pci22_release, }; +/** + * pci_vpd_size - determine actual size of Vital Product Data + * @dev: pci device struct + * @old_size: current assumed size, also maximum allowed size + * + */ +static size_t +pci_vpd_pci22_size(struct pci_dev *dev) +{ + size_t off = 0; + unsigned char header[1+2]; /* 1 byte tag, 2 bytes length */ + + while (off < PCI_VPD_PCI22_SIZE && + pci_read_vpd(dev, off, 1, header) == 1) { + unsigned char tag; + + if (header[0] & PCI_VPD_LRDT) { + /* Large Resource Data Type Tag */ + tag = pci_vpd_lrdt_tag(header); + /* Only read length from known tag items */ + if ((tag == PCI_VPD_LTIN_ID_STRING) || + (tag == PCI_VPD_LTIN_RO_DATA) || + (tag == PCI_VPD_LTIN_RW_DATA)) { + if (pci_read_vpd(dev, off+1, 2, + &header[1]) != 2) + return off + 1; + off += PCI_VPD_LRDT_TAG_SIZE + + pci_vpd_lrdt_size(header); + } + } else { + /* Short Resource Data Type Tag */ + off += PCI_VPD_SRDT_TAG_SIZE + + pci_vpd_srdt_size(header); + tag = pci_vpd_srdt_tag(header); + } + if (tag == PCI_VPD_STIN_END) /* End tag descriptor */ + return off; + if ((tag != PCI_VPD_LTIN_ID_STRING) && + (tag != PCI_VPD_LTIN_RO_DATA) && + (tag != PCI_VPD_LTIN_RW_DATA)) { + dev_dbg(&dev->dev, + "invalid %s vpd tag %02x at offset %zu.", + (header[0] & PCI_VPD_LRDT) ? "large" : "short", + tag, off); + break; + } + } + return 0; +} + int pci_vpd_pci22_init(struct pci_dev *dev) { struct pci_vpd_pci22 *vpd; @@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev) vpd->cap = cap; vpd->busy = false; dev->vpd = &vpd->base; + vpd->base.len = pci_vpd_pci22_size(dev); + if (vpd->base.len == 0) { + dev_dbg(&dev->dev, "Disabling VPD access."); + dev->vpd = NULL; + kfree(vpd); + return -ENXIO; + } return 0; }
PCI-2.2 VPD entries have a maximum size of 32k, but might actually be smaller than that. To figure out the actual size one has to read the VPD area until the 'end marker' is reached. Trying to read VPD data beyond that marker results in 'interesting' effects, from simple read errors to crashing the card. And to make matters worse not every PCI card implements this properly, leaving us with no 'end' marker or even completely invalid data. This path modifies the size of the VPD attribute to the available size, and disables the VPD attribute altogether if no valid data could be read. Cc: Alexander Duyck <alexander.duyck@gmail.com> Cc: Bjorn Helgaas <helgaas@kernel.org> Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)