* [PATCHv4 0/2] PCI: Safe VPD access @ 2015-12-18 8:35 Hannes Reinecke 2015-12-18 8:35 ` [PATCH 1/2] pci: Update VPD definitions Hannes Reinecke 2015-12-18 8:35 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 0 siblings, 2 replies; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 8:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke PCI VPD suffers from two problems: it has a very rudimentary interface and it relies on correctly formatted data. And essentially it provides a direct channel into the card hardware. In other words, plenty of chances to mess things up. With the original implementation we would just read the VPD space, blissfully ignorant of the data formatting of the VPD data. This would work if the VPD space happens to be properly implemented. However, I've had several reports where a simple 'cat' on the vpd attribute would return garbage (if you're lucky), trigger a timeout with a kernel warning (which actually triggered this patchset), or hang your machine (if you're really unlucky). So this patchset validates the VPD data, setting the VPD attribute to the correct size or disabling VPD access altogether if no valid data is found. Changes to v3: - Do not calculate size for pci function devices - Do not disable the attribute on error - move size calculation into the pci_vpd_ops structure Hannes Reinecke (2): pci: Update VPD definitions pci: Update VPD size with correct length drivers/pci/access.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + include/linux/pci.h | 27 ++++++++++++++++-- 3 files changed, 107 insertions(+), 2 deletions(-) -- 1.8.5.6 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] pci: Update VPD definitions 2015-12-18 8:35 [PATCHv4 0/2] PCI: Safe VPD access Hannes Reinecke @ 2015-12-18 8:35 ` Hannes Reinecke 2015-12-18 8:35 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 1 sibling, 0 replies; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 8:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke, Hannes Reinecke The 'end' tag is actually 0x0f, it's the representation as a small resource data type tag that's 0x78 (ie shifted by 3). This patch also adds helper functions to extract the resource data type tags for both large and small resource data types. Cc: Alexander Duyck <alexander.duyck@gmail.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Hannes Reinecke <hare@suse.com> --- include/linux/pci.h | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/include/linux/pci.h b/include/linux/pci.h index e828e7b..1e6856d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1827,12 +1827,13 @@ bool pci_acs_path_enabled(struct pci_dev *start, #define PCI_VPD_LRDT_RW_DATA PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RW_DATA) /* Small Resource Data Type Tag Item Names */ -#define PCI_VPD_STIN_END 0x78 /* End */ +#define PCI_VPD_STIN_END 0x0f /* End */ -#define PCI_VPD_SRDT_END PCI_VPD_STIN_END +#define PCI_VPD_SRDT_END (PCI_VPD_STIN_END << 3) #define PCI_VPD_SRDT_TIN_MASK 0x78 #define PCI_VPD_SRDT_LEN_MASK 0x07 +#define PCI_VPD_LRDT_TIN_MASK 0x7f #define PCI_VPD_LRDT_TAG_SIZE 3 #define PCI_VPD_SRDT_TAG_SIZE 1 @@ -1856,6 +1857,17 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt) } /** + * pci_vpd_lrdt_tag - Extracts the Large Resource Data Type Tag Item + * @lrdt: Pointer to the beginning of the Large Resource Data Type tag + * + * Returns the extracted Large Resource Data Type Tag item. + */ +static inline u16 pci_vpd_lrdt_tag(const u8 *lrdt) +{ + return (u16)(lrdt[0] & PCI_VPD_LRDT_TIN_MASK); +} + +/** * pci_vpd_srdt_size - Extracts the Small Resource Data Type length * @lrdt: Pointer to the beginning of the Small Resource Data Type tag * @@ -1867,6 +1879,17 @@ static inline u8 pci_vpd_srdt_size(const u8 *srdt) } /** + * pci_vpd_srdt_tag - Extracts the Small Resource Data Type Tag Item + * @lrdt: Pointer to the beginning of the Small Resource Data Type tag + * + * Returns the extracted Small Resource Data Type Tag Item. + */ +static inline u8 pci_vpd_srdt_tag(const u8 *srdt) +{ + return ((*srdt) & PCI_VPD_SRDT_TIN_MASK) >> 3; +} + +/** * pci_vpd_info_field_size - Extracts the information field length * @lrdt: Pointer to the beginning of an information field header * -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 8:35 [PATCHv4 0/2] PCI: Safe VPD access Hannes Reinecke 2015-12-18 8:35 ` [PATCH 1/2] pci: Update VPD definitions Hannes Reinecke @ 2015-12-18 8:35 ` Hannes Reinecke 2015-12-18 13:49 ` Alexander Duyck 1 sibling, 1 reply; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 8:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke, Bjorn Helgaas 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, or set it to '0' 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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + 2 files changed, 82 insertions(+) diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 59ac36f..2e23fc7 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -206,6 +206,18 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void } EXPORT_SYMBOL(pci_write_vpd); +/** + * pci_vpd_size - Determine the size of the Vital Product Data + * @dev: pci device struct + * + */ +size_t pci_vpd_size(struct pci_dev *dev) +{ + if (!dev->vpd || !dev->vpd->ops) + return 0; + return dev->vpd->ops->size(dev); +} + /* * The following routines are to prevent the user from accessing PCI config * space when it's unsafe to do so. Some devices require this during BIST and @@ -428,6 +440,53 @@ out: return ret ? ret : count; } +/** + * pci_vpd_size - determine actual size of Vital Product Data + * @dev: pci device struct + * + */ +static size_t pci_vpd_pci22_size(struct pci_dev *dev) +{ + ssize_t off = 0; + unsigned char header[1+2]; /* 1 byte tag, 2 bytes length */ + + while (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; +} + static void pci_vpd_pci22_release(struct pci_dev *dev) { kfree(container_of(dev->vpd, struct pci_vpd_pci22, base)); @@ -436,6 +495,7 @@ static void pci_vpd_pci22_release(struct pci_dev *dev) static const struct pci_vpd_ops pci_vpd_pci22_ops = { .read = pci_vpd_pci22_read, .write = pci_vpd_pci22_write, + .size = pci_vpd_pci22_size, .release = pci_vpd_pci22_release, }; @@ -469,9 +529,29 @@ static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count, return ret; } +static size_t pci_vpd_f0_size(struct pci_dev *dev) +{ + struct pci_dev *tdev = pci_get_slot(dev->bus, + PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); + ssize_t len = 0; + + if (!tdev) + return 0; + + if (tdev->vpd) { + struct pci_vpd_pci22 *vpd = + container_of(dev->vpd, struct pci_vpd_pci22, base); + + len = vpd->base.len; + } + pci_dev_put(tdev); + return len; +} + static const struct pci_vpd_ops pci_vpd_f0_ops = { .read = pci_vpd_f0_read, .write = pci_vpd_f0_write, + .size = pci_vpd_f0_size, .release = pci_vpd_pci22_release, }; @@ -497,6 +577,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev) vpd->cap = cap; vpd->busy = false; dev->vpd = &vpd->base; + vpd->base.len = pci_vpd_size(dev); return 0; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fd2f03f..33dfc7c 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -97,6 +97,7 @@ static inline bool pci_has_subordinate(struct pci_dev *pci_dev) struct pci_vpd_ops { ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf); ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); + size_t (*size)(struct pci_dev *dev); void (*release)(struct pci_dev *dev); }; -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 8:35 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke @ 2015-12-18 13:49 ` Alexander Duyck 2015-12-18 13:57 ` Hannes Reinecke 0 siblings, 1 reply; 16+ messages in thread From: Alexander Duyck @ 2015-12-18 13:49 UTC (permalink / raw) To: Hannes Reinecke Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. This isn't what I had in mind. There is no need to add an f0 version of the size function. The size for all functions other than function 0 when the F0 flag is set is 0. We aren't going to be reading their VPD, we only read the VPD region of function 0. If you just moved the call to pci_vpd_size into the conditional statement in the init function you could then set the length to 0 in the branch taken for if the F0 flag is set. You might have to reorder the init function a bit but there is no need to add additional function pointers. - Alex ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 13:49 ` Alexander Duyck @ 2015-12-18 13:57 ` Hannes Reinecke 2015-12-18 14:02 ` Alexander Duyck 0 siblings, 1 reply; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 13:57 UTC (permalink / raw) To: Alexander Duyck Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On 12/18/2015 02:49 PM, Alexander Duyck wrote: > On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. > > This isn't what I had in mind. There is no need to add an f0 version > of the size function. The size for all functions other than function > 0 when the F0 flag is set is 0. We aren't going to be reading their > VPD, we only read the VPD region of function 0. > Ah. (I'm a bit confused about the proposed action for VPD other than function 0). So the idea here is to _disallow_ access to VPDs from functions other than '0' unless these functions have different PCI IDs? Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 13:57 ` Hannes Reinecke @ 2015-12-18 14:02 ` Alexander Duyck 2015-12-18 14:14 ` Hannes Reinecke 0 siblings, 1 reply; 16+ messages in thread From: Alexander Duyck @ 2015-12-18 14:02 UTC (permalink / raw) To: Hannes Reinecke Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On Fri, Dec 18, 2015 at 5:57 AM, Hannes Reinecke <hare@suse.de> wrote: > On 12/18/2015 02:49 PM, Alexander Duyck wrote: >> >> On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. >> >> >> This isn't what I had in mind. There is no need to add an f0 version >> of the size function. The size for all functions other than function >> 0 when the F0 flag is set is 0. We aren't going to be reading their >> VPD, we only read the VPD region of function 0. >> > Ah. (I'm a bit confused about the proposed action for VPD other than > function 0). > So the idea here is to _disallow_ access to VPDs from functions other than > '0' unless these functions have different PCI IDs? If you take a look at the F0 functions what they do is bypass the VPD of the functions other than function 0. As such setting the size to 0 should really have no effect since the VPD of the function isn't actually read if the F0 flag is set. - Alex ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 14:02 ` Alexander Duyck @ 2015-12-18 14:14 ` Hannes Reinecke 2015-12-29 5:29 ` Jordan_Hargrave 0 siblings, 1 reply; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 14:14 UTC (permalink / raw) To: Alexander Duyck Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On 12/18/2015 03:02 PM, Alexander Duyck wrote: > On Fri, Dec 18, 2015 at 5:57 AM, Hannes Reinecke <hare@suse.de> wrote: >> On 12/18/2015 02:49 PM, Alexander Duyck wrote: >>> >>> On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. >>> >>> >>> This isn't what I had in mind. There is no need to add an f0 version >>> of the size function. The size for all functions other than function >>> 0 when the F0 flag is set is 0. We aren't going to be reading their >>> VPD, we only read the VPD region of function 0. >>> >> Ah. (I'm a bit confused about the proposed action for VPD other than >> function 0). >> So the idea here is to _disallow_ access to VPDs from functions other than >> '0' unless these functions have different PCI IDs? > > If you take a look at the F0 functions what they do is bypass the VPD > of the functions other than function 0. As such setting the size to 0 > should really have no effect since the VPD of the function isn't > actually read if the F0 flag is set. > Setting the size to '0' effectively inhibits you to read the VPD data. So if we were to return '0' for PCI devices with the F0 bit set we will never ever to be able to read (or write) _any_ VPD data for that PCI device/function. Which would be rendering all these F0 accessors pointless, and we might as well remove them. Hence my confusion. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-18 14:14 ` Hannes Reinecke @ 2015-12-29 5:29 ` Jordan_Hargrave 2015-12-29 17:48 ` Alexander Duyck 0 siblings, 1 reply; 16+ messages in thread From: Jordan_Hargrave @ 2015-12-29 5:29 UTC (permalink / raw) To: hare, alexander.duyck Cc: bhelgaas, mkubecek, shane.seymour, linux-pci, linux-kernel, helgaas >On 12/18/2015 03:02 PM, Alexander Duyck wrote: >> On Fri, Dec 18, 2015 at 5:57 AM, Hannes Reinecke <hare@suse.de> wrote: >>> On 12/18/2015 02:49 PM, Alexander Duyck wrote: >>>> >>>> On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. >>>> >>>> >>>> This isn't what I had in mind. There is no need to add an f0 version >>>> of the size function. The size for all functions other than function >>>> 0 when the F0 flag is set is 0. We aren't going to be reading their >>>> VPD, we only read the VPD region of function 0. >>>> >>> Ah. (I'm a bit confused about the proposed action for VPD other than >>> function 0). >>> So the idea here is to _disallow_ access to VPDs from functions other than >>> '0' unless these functions have different PCI IDs? >> >> If you take a look at the F0 functions what they do is bypass the VPD >> of the functions other than function 0. As such setting the size to 0 >> should really have no effect since the VPD of the function isn't >> actually read if the F0 flag is set. >> >Setting the size to '0' effectively inhibits you to read the VPD >data. So if we were to return '0' for PCI devices with the F0 bit >set we will never ever to be able to read (or write) _any_ VPD data >for that PCI device/function. >Which would be rendering all these F0 accessors pointless, and we >might as well remove them. > I had posted a patch recently to enable exposing the VPD-R valyes to sysfs. I need access to these to parse into systemd for network naming (biosdevname style names). The VPD-R is a readonly area contained within the PCI Vital Product data. There are some standard and vendor-specific keys stored in this region. PN = Part Number SN = Serial Number MN = Manufacturer ID Vx = Vendor-specific (x=0..9 A..Z) Biosdevname/Systemd will use these VPD keys for determining Network partitioning and port numbers for NIC cards Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com> --- drivers/pci/pci-sysfs.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 + 2 files changed, 218 insertions(+), 0 deletions(-) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index eead54c..4966ece 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -1295,6 +1295,210 @@ static struct bin_attribute pcie_config_attr = { .write = pci_write_config, }; +static umode_t vpd_attr_exist(struct kobject *kobj, + struct attribute *attr, int n) +{ + struct device *dev; + struct pci_dev *pdev; + const char *name; + int i; + + dev = container_of(kobj, struct device, kobj); + pdev = to_pci_dev(dev); + + name = attr->name; + if (pdev->vpdr_data == NULL) + return 0; + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, + pdev->vpdr_len, name); + return (i >= 0 ? S_IRUGO : 0); +} + +static ssize_t vpd_attr_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct pci_dev *pdev; + const char *name; + char kv_data[257] = { 0 }; + int i, len; + + pdev = to_pci_dev(dev); + name = attr->attr.name; + if (pdev->vpdr_data == NULL) + return 0; + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, + pdev->vpdr_len, name); + if (i >= 0) { + len = pci_vpd_info_field_size(&pdev->vpdr_data[i]); + memcpy(kv_data, pdev->vpdr_data + i + + PCI_VPD_INFO_FLD_HDR_SIZE, len); + return scnprintf(buf, PAGE_SIZE, "%s\n", kv_data); + } + return 0; +} + +#define VPD_ATTR_RO(x) \ +struct device_attribute vpd ## x = { \ + .attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \ + .show = vpd_attr_show \ +} + +VPD_ATTR_RO(PN); +VPD_ATTR_RO(EC); +VPD_ATTR_RO(MN); +VPD_ATTR_RO(SN); +VPD_ATTR_RO(V0); +VPD_ATTR_RO(V1); +VPD_ATTR_RO(V2); +VPD_ATTR_RO(V3); +VPD_ATTR_RO(V4); +VPD_ATTR_RO(V5); +VPD_ATTR_RO(V6); +VPD_ATTR_RO(V7); +VPD_ATTR_RO(V8); +VPD_ATTR_RO(V9); +VPD_ATTR_RO(VA); +VPD_ATTR_RO(VB); +VPD_ATTR_RO(VC); +VPD_ATTR_RO(VD); +VPD_ATTR_RO(VE); +VPD_ATTR_RO(VF); +VPD_ATTR_RO(VG); +VPD_ATTR_RO(VH); +VPD_ATTR_RO(VI); +VPD_ATTR_RO(VJ); +VPD_ATTR_RO(VK); +VPD_ATTR_RO(VL); +VPD_ATTR_RO(VM); +VPD_ATTR_RO(VN); +VPD_ATTR_RO(VO); +VPD_ATTR_RO(VP); +VPD_ATTR_RO(VQ); +VPD_ATTR_RO(VR); +VPD_ATTR_RO(VS); +VPD_ATTR_RO(VT); +VPD_ATTR_RO(VU); +VPD_ATTR_RO(VV); +VPD_ATTR_RO(VW); +VPD_ATTR_RO(VX); +VPD_ATTR_RO(VY); +VPD_ATTR_RO(VZ); + +static struct attribute *vpd_attributes[] = { + &vpdPN.attr, + &vpdEC.attr, + &vpdMN.attr, + &vpdSN.attr, + &vpdV0.attr, + &vpdV1.attr, + &vpdV2.attr, + &vpdV3.attr, + &vpdV4.attr, + &vpdV5.attr, + &vpdV6.attr, + &vpdV7.attr, + &vpdV8.attr, + &vpdV9.attr, + &vpdVA.attr, + &vpdVB.attr, + &vpdVC.attr, + &vpdVD.attr, + &vpdVE.attr, + &vpdVF.attr, + &vpdVG.attr, + &vpdVH.attr, + &vpdVI.attr, + &vpdVJ.attr, + &vpdVK.attr, + &vpdVL.attr, + &vpdVM.attr, + &vpdVN.attr, + &vpdVO.attr, + &vpdVP.attr, + &vpdVQ.attr, + &vpdVR.attr, + &vpdVS.attr, + &vpdVT.attr, + &vpdVU.attr, + &vpdVV.attr, + &vpdVW.attr, + &vpdVX.attr, + &vpdVY.attr, + &vpdVZ.attr, + NULL, +}; + +static struct attribute_group vpd_attr_group = { + .name = "vpdr", + .attrs = vpd_attributes, + .is_visible = vpd_attr_exist, +}; + + +static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len) +{ + u8 tag[3]; + int rc, tlen; + + *len = 0; + /* Quirk Atheros cards, reading VPD hangs system for 20s */ + if (dev->vendor == PCI_VENDOR_ID_ATHEROS || + dev->vendor == PCI_VENDOR_ID_ATTANSIC) + return -ENOENT; + rc = pci_read_vpd(dev, off, 1, tag); + if (rc != 1) + return -ENOENT; + if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F) + return -ENOENT; + if (tag[0] & PCI_VPD_LRDT) { + rc = pci_read_vpd(dev, off+1, 2, tag+1); + if (rc != 2) + return -ENOENT; + tlen = pci_vpd_lrdt_size(tag) + + PCI_VPD_LRDT_TAG_SIZE; + } else { + tlen = pci_vpd_srdt_size(tag) + + PCI_VPD_SRDT_TAG_SIZE; + tag[0] &= ~PCI_VPD_SRDT_LEN_MASK; + } + /* Verify VPD tag fits in area */ + if (tlen + off > dev->vpd->len) + return -ENOENT; + *len = tlen; + return tag[0]; +} + +static int pci_load_vpdr(struct pci_dev *dev) +{ + int rlen, ilen, tag, rc; + + /* Check for VPD-I and VPD-R tag */ + tag = pci_get_vpd_tag(dev, 0, &ilen); + if (tag != PCI_VPD_LRDT_ID_STRING) + return -ENOENT; + tag = pci_get_vpd_tag(dev, ilen, &rlen); + if (tag != PCI_VPD_LRDT_RO_DATA) + return -ENOENT; + + rlen -= PCI_VPD_LRDT_TAG_SIZE; + dev->vpdr_len = rlen; + dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC); + if (dev->vpdr_data == NULL) + return -ENOMEM; + + rc = pci_read_vpd(dev, ilen + PCI_VPD_LRDT_TAG_SIZE, + rlen, dev->vpdr_data); + if (rc != rlen) + goto error; + if (sysfs_create_group(&dev->dev.kobj, &vpd_attr_group)) + goto error; + return 0; + error: + kfree(dev->vpdr_data); + dev->vpdr_len = 0; + return -ENOENT; +} + static ssize_t reset_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { @@ -1340,6 +1544,8 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev) return retval; } dev->vpd->attr = attr; + + pci_load_vpdr(dev); } /* Active State Power Management */ @@ -1356,6 +1562,11 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev) error: pcie_aspm_remove_sysfs_dev_files(dev); if (dev->vpd && dev->vpd->attr) { + if (dev->vpdr_data) { + sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group); + kfree(dev->vpdr_data); + dev->vpdr_data = NULL; + } sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); kfree(dev->vpd->attr); } @@ -1438,6 +1649,11 @@ err: static void pci_remove_capabilities_sysfs(struct pci_dev *dev) { if (dev->vpd && dev->vpd->attr) { + if (dev->vpdr_data) { + sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group); + kfree(dev->vpdr_data); + dev->vpdr_data = NULL; + } sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); kfree(dev->vpd->attr); } diff --git a/include/linux/pci.h b/include/linux/pci.h index 6ae25aa..699cf11 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -372,6 +372,8 @@ struct pci_dev { const struct attribute_group **msi_irq_groups; #endif struct pci_vpd *vpd; + int vpdr_len; + u8 *vpdr_data; #ifdef CONFIG_PCI_ATS union { struct pci_sriov *sriov; /* SR-IOV capability related */ -- 1.7.1 >Hence my confusion. > >Cheers, > >Hannes >-- >Dr. Hannes Reinecke zSeries & Storage >hare@suse.de +49 911 74053 688 >SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg >GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton >HRB 21284 (AG Nürnberg) >-- >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 > ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-29 5:29 ` Jordan_Hargrave @ 2015-12-29 17:48 ` Alexander Duyck 2015-12-29 19:01 ` Jordan_Hargrave 0 siblings, 1 reply; 16+ messages in thread From: Alexander Duyck @ 2015-12-29 17:48 UTC (permalink / raw) To: Jordan_Hargrave Cc: Hannes Reinecke, Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On Mon, Dec 28, 2015 at 9:29 PM, <Jordan_Hargrave@dell.com> wrote: >>On 12/18/2015 03:02 PM, Alexander Duyck wrote: >>> On Fri, Dec 18, 2015 at 5:57 AM, Hannes Reinecke <hare@suse.de> wrote: >>>> On 12/18/2015 02:49 PM, Alexander Duyck wrote: >>>>> >>>>> On Fri, Dec 18, 2015 at 12:35 AM, 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, or set it to '0' if no valid data could be read. >>>>> >>>>> >>>>> This isn't what I had in mind. There is no need to add an f0 version >>>>> of the size function. The size for all functions other than function >>>>> 0 when the F0 flag is set is 0. We aren't going to be reading their >>>>> VPD, we only read the VPD region of function 0. >>>>> >>>> Ah. (I'm a bit confused about the proposed action for VPD other than >>>> function 0). >>>> So the idea here is to _disallow_ access to VPDs from functions other than >>>> '0' unless these functions have different PCI IDs? >>> >>> If you take a look at the F0 functions what they do is bypass the VPD >>> of the functions other than function 0. As such setting the size to 0 >>> should really have no effect since the VPD of the function isn't >>> actually read if the F0 flag is set. >>> >>Setting the size to '0' effectively inhibits you to read the VPD >>data. So if we were to return '0' for PCI devices with the F0 bit >>set we will never ever to be able to read (or write) _any_ VPD data >>for that PCI device/function. >>Which would be rendering all these F0 accessors pointless, and we >>might as well remove them. >> > > I had posted a patch recently to enable exposing the VPD-R valyes to sysfs. I need access > to these to parse into systemd for network naming (biosdevname style names). > > > The VPD-R is a readonly area contained within the PCI Vital Product > data. There are some standard and vendor-specific keys stored in > this region. > > PN = Part Number > SN = Serial Number > MN = Manufacturer ID > Vx = Vendor-specific (x=0..9 A..Z) > > Biosdevname/Systemd will use these VPD keys for determining Network > partitioning and port numbers for NIC cards Can you please repost this as a patch instead of as a reply to our thread about VPD size. The fact is the subject is misleading as your patch isn't actually related to VPD sizing. > Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com> > --- > drivers/pci/pci-sysfs.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 2 + > 2 files changed, 218 insertions(+), 0 deletions(-) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index eead54c..4966ece 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -1295,6 +1295,210 @@ static struct bin_attribute pcie_config_attr = { > .write = pci_write_config, > }; > > +static umode_t vpd_attr_exist(struct kobject *kobj, > + struct attribute *attr, int n) > +{ > + struct device *dev; > + struct pci_dev *pdev; > + const char *name; > + int i; > + > + dev = container_of(kobj, struct device, kobj); > + pdev = to_pci_dev(dev); > + > + name = attr->name; > + if (pdev->vpdr_data == NULL) > + return 0; > + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, > + pdev->vpdr_len, name); > + return (i >= 0 ? S_IRUGO : 0); > +} > + So I assume there is another patch that implements pci_vpd_find_info_keyword so that it can go through the vpdr_data and parse it? > +static ssize_t vpd_attr_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct pci_dev *pdev; > + const char *name; > + char kv_data[257] = { 0 }; > + int i, len; > + > + pdev = to_pci_dev(dev); > + name = attr->attr.name; > + if (pdev->vpdr_data == NULL) > + return 0; > + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, > + pdev->vpdr_len, name); > + if (i >= 0) { > + len = pci_vpd_info_field_size(&pdev->vpdr_data[i]); > + memcpy(kv_data, pdev->vpdr_data + i + > + PCI_VPD_INFO_FLD_HDR_SIZE, len); > + return scnprintf(buf, PAGE_SIZE, "%s\n", kv_data); > + } > + return 0; > +} > + > +#define VPD_ATTR_RO(x) \ > +struct device_attribute vpd ## x = { \ > + .attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \ > + .show = vpd_attr_show \ > +} > + > +VPD_ATTR_RO(PN); > +VPD_ATTR_RO(EC); > +VPD_ATTR_RO(MN); > +VPD_ATTR_RO(SN); > +VPD_ATTR_RO(V0); > +VPD_ATTR_RO(V1); > +VPD_ATTR_RO(V2); > +VPD_ATTR_RO(V3); > +VPD_ATTR_RO(V4); > +VPD_ATTR_RO(V5); > +VPD_ATTR_RO(V6); > +VPD_ATTR_RO(V7); > +VPD_ATTR_RO(V8); > +VPD_ATTR_RO(V9); > +VPD_ATTR_RO(VA); > +VPD_ATTR_RO(VB); > +VPD_ATTR_RO(VC); > +VPD_ATTR_RO(VD); > +VPD_ATTR_RO(VE); > +VPD_ATTR_RO(VF); > +VPD_ATTR_RO(VG); > +VPD_ATTR_RO(VH); > +VPD_ATTR_RO(VI); > +VPD_ATTR_RO(VJ); > +VPD_ATTR_RO(VK); > +VPD_ATTR_RO(VL); > +VPD_ATTR_RO(VM); > +VPD_ATTR_RO(VN); > +VPD_ATTR_RO(VO); > +VPD_ATTR_RO(VP); > +VPD_ATTR_RO(VQ); > +VPD_ATTR_RO(VR); > +VPD_ATTR_RO(VS); > +VPD_ATTR_RO(VT); > +VPD_ATTR_RO(VU); > +VPD_ATTR_RO(VV); > +VPD_ATTR_RO(VW); > +VPD_ATTR_RO(VX); > +VPD_ATTR_RO(VY); > +VPD_ATTR_RO(VZ); > + > +static struct attribute *vpd_attributes[] = { > + &vpdPN.attr, > + &vpdEC.attr, > + &vpdMN.attr, > + &vpdSN.attr, > + &vpdV0.attr, > + &vpdV1.attr, > + &vpdV2.attr, > + &vpdV3.attr, > + &vpdV4.attr, > + &vpdV5.attr, > + &vpdV6.attr, > + &vpdV7.attr, > + &vpdV8.attr, > + &vpdV9.attr, > + &vpdVA.attr, > + &vpdVB.attr, > + &vpdVC.attr, > + &vpdVD.attr, > + &vpdVE.attr, > + &vpdVF.attr, > + &vpdVG.attr, > + &vpdVH.attr, > + &vpdVI.attr, > + &vpdVJ.attr, > + &vpdVK.attr, > + &vpdVL.attr, > + &vpdVM.attr, > + &vpdVN.attr, > + &vpdVO.attr, > + &vpdVP.attr, > + &vpdVQ.attr, > + &vpdVR.attr, > + &vpdVS.attr, > + &vpdVT.attr, > + &vpdVU.attr, > + &vpdVV.attr, > + &vpdVW.attr, > + &vpdVX.attr, > + &vpdVY.attr, > + &vpdVZ.attr, > + NULL, > +}; > + > +static struct attribute_group vpd_attr_group = { > + .name = "vpdr", > + .attrs = vpd_attributes, > + .is_visible = vpd_attr_exist, > +}; > + > + > +static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len) > +{ > + u8 tag[3]; > + int rc, tlen; > + > + *len = 0; > + /* Quirk Atheros cards, reading VPD hangs system for 20s */ > + if (dev->vendor == PCI_VENDOR_ID_ATHEROS || > + dev->vendor == PCI_VENDOR_ID_ATTANSIC) > + return -ENOENT; I'm not really sure this is the right place for this type of quirk. If this is really an issue maybe we should just disable VPD for these devices. Otherwise there isn't anything to stop someone from going in and reading the VPD region via the existing VPD interfaces. > + rc = pci_read_vpd(dev, off, 1, tag); > + if (rc != 1) > + return -ENOENT; > + if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F) > + return -ENOENT; > + if (tag[0] & PCI_VPD_LRDT) { > + rc = pci_read_vpd(dev, off+1, 2, tag+1); > + if (rc != 2) > + return -ENOENT; > + tlen = pci_vpd_lrdt_size(tag) + > + PCI_VPD_LRDT_TAG_SIZE; > + } else { > + tlen = pci_vpd_srdt_size(tag) + > + PCI_VPD_SRDT_TAG_SIZE; > + tag[0] &= ~PCI_VPD_SRDT_LEN_MASK; > + } > + /* Verify VPD tag fits in area */ > + if (tlen + off > dev->vpd->len) > + return -ENOENT; > + *len = tlen; > + return tag[0]; > +} > + > +static int pci_load_vpdr(struct pci_dev *dev) > +{ > + int rlen, ilen, tag, rc; > + > + /* Check for VPD-I and VPD-R tag */ > + tag = pci_get_vpd_tag(dev, 0, &ilen); > + if (tag != PCI_VPD_LRDT_ID_STRING) > + return -ENOENT; > + tag = pci_get_vpd_tag(dev, ilen, &rlen); > + if (tag != PCI_VPD_LRDT_RO_DATA) > + return -ENOENT; > + > + rlen -= PCI_VPD_LRDT_TAG_SIZE; > + dev->vpdr_len = rlen; > + dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC); > + if (dev->vpdr_data == NULL) > + return -ENOMEM; > + Why not cache the ID string as well? Seems like it might be a field people would want to read on a regular basis in order to find out what is there. > + rc = pci_read_vpd(dev, ilen + PCI_VPD_LRDT_TAG_SIZE, > + rlen, dev->vpdr_data); > + if (rc != rlen) > + goto error; > + if (sysfs_create_group(&dev->dev.kobj, &vpd_attr_group)) > + goto error; > + return 0; > + error: > + kfree(dev->vpdr_data); > + dev->vpdr_len = 0; > + return -ENOENT; > +} > + This bit here needs to reset vpdr_data back to NULL. Otherwise you could cause memory corruption via a double free in your two clean-up routines called out below. > static ssize_t reset_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > @@ -1340,6 +1544,8 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev) > return retval; > } > dev->vpd->attr = attr; > + > + pci_load_vpdr(dev); > } > > /* Active State Power Management */ > @@ -1356,6 +1562,11 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev) > error: > pcie_aspm_remove_sysfs_dev_files(dev); > if (dev->vpd && dev->vpd->attr) { > + if (dev->vpdr_data) { > + sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group); > + kfree(dev->vpdr_data); > + dev->vpdr_data = NULL; > + } > sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); > kfree(dev->vpd->attr); > } > @@ -1438,6 +1649,11 @@ err: > static void pci_remove_capabilities_sysfs(struct pci_dev *dev) > { > if (dev->vpd && dev->vpd->attr) { > + if (dev->vpdr_data) { > + sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group); > + kfree(dev->vpdr_data); > + dev->vpdr_data = NULL; > + } > sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); > kfree(dev->vpd->attr); > } > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 6ae25aa..699cf11 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -372,6 +372,8 @@ struct pci_dev { > const struct attribute_group **msi_irq_groups; > #endif > struct pci_vpd *vpd; > + int vpdr_len; > + u8 *vpdr_data; > #ifdef CONFIG_PCI_ATS > union { > struct pci_sriov *sriov; /* SR-IOV capability related */ Instead of placing vpdr_len outside of the data why not just leave it in there? It seems like you could then make use of some of the same parsing logic regardless of if you are working with the cached copy or the actual VPD. ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-29 17:48 ` Alexander Duyck @ 2015-12-29 19:01 ` Jordan_Hargrave 2015-12-29 20:26 ` Alexander Duyck 0 siblings, 1 reply; 16+ messages in thread From: Jordan_Hargrave @ 2015-12-29 19:01 UTC (permalink / raw) To: alexander.duyck Cc: hare, bhelgaas, mkubecek, shane.seymour, linux-pci, linux-kernel, helgaas >On Mon, Dec 28, 2015 at 9:29 PM, <Jordan_Hargrave@dell.com> wrote: >> I had posted a patch recently to enable exposing the VPD-R valyes to sysfs. I need access >> to these to parse into systemd for network naming (biosdevname style names). >> >> >> The VPD-R is a readonly area contained within the PCI Vital Product >> data. There are some standard and vendor-specific keys stored in >> this region. >> >> PN = Part Number >> SN = Serial Number >> MN = Manufacturer ID >> Vx = Vendor-specific (x=0..9 A..Z) >> >> Biosdevname/Systemd will use these VPD keys for determining Network >> partitioning and port numbers for NIC cards > >Can you please repost this as a patch instead of as a reply to our >thread about VPD size. The fact is the subject is misleading as your >patch isn't actually related to VPD sizing. > I had already posted this a few weeks back but never got any feedback. [PATCH] Create sysfs entries for VPD-R keys https://marc.info/?l=linux-pci&m=144959803316031&w=2 >> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com> >> --- >> drivers/pci/pci-sysfs.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/pci.h | 2 + >> 2 files changed, 218 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c >> index eead54c..4966ece 100644 >> --- a/drivers/pci/pci-sysfs.c >> +++ b/drivers/pci/pci-sysfs.c >> @@ -1295,6 +1295,210 @@ static struct bin_attribute pcie_config_attr = { >> .write = pci_write_config, >> }; >> >> +static umode_t vpd_attr_exist(struct kobject *kobj, >> + struct attribute *attr, int n) >> +{ >> + struct device *dev; >> + struct pci_dev *pdev; >> + const char *name; >> + int i; >> + >> + dev = container_of(kobj, struct device, kobj); >> + pdev = to_pci_dev(dev); >> + >> + name = attr->name; >> + if (pdev->vpdr_data == NULL) >> + return 0; >> + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, >> + pdev->vpdr_len, name); >> + return (i >= 0 ? S_IRUGO : 0); >> +} >> + > >So I assume there is another patch that implements >pci_vpd_find_info_keyword so that it can go through the vpdr_data and >parse it? > That's already an existing function in drivers/pci/vpd.c >> +static ssize_t vpd_attr_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct pci_dev *pdev; >> + const char *name; >> + char kv_data[257] = { 0 }; >> + int i, len; >> + >> + pdev = to_pci_dev(dev); >> + name = attr->attr.name; >> + if (pdev->vpdr_data == NULL) >> + return 0; >> + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, >> + pdev->vpdr_len, name); >> + if (i >= 0) { >> + len = pci_vpd_info_field_size(&pdev->vpdr_data[i]); >> + memcpy(kv_data, pdev->vpdr_data + i + >> + PCI_VPD_INFO_FLD_HDR_SIZE, len); >> + return scnprintf(buf, PAGE_SIZE, "%s\n", kv_data); >> + } >> + return 0; >> +} >> + >> +#define VPD_ATTR_RO(x) \ >> +struct device_attribute vpd ## x = { \ >> + .attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \ >> + .show = vpd_attr_show \ >> +} >> + >> +VPD_ATTR_RO(PN); >> +VPD_ATTR_RO(EC); >> +VPD_ATTR_RO(MN); >> +VPD_ATTR_RO(SN); >> +VPD_ATTR_RO(V0); >> +VPD_ATTR_RO(V1); >> +VPD_ATTR_RO(V2); >> +VPD_ATTR_RO(V3); >> +VPD_ATTR_RO(V4); >> +VPD_ATTR_RO(V5); >> +VPD_ATTR_RO(V6); >> +VPD_ATTR_RO(V7); >> +VPD_ATTR_RO(V8); >> +VPD_ATTR_RO(V9); >> +VPD_ATTR_RO(VA); >> +VPD_ATTR_RO(VB); >> +VPD_ATTR_RO(VC); >> +VPD_ATTR_RO(VD); >> +VPD_ATTR_RO(VE); >> +VPD_ATTR_RO(VF); >> +VPD_ATTR_RO(VG); >> +VPD_ATTR_RO(VH); >> +VPD_ATTR_RO(VI); >> +VPD_ATTR_RO(VJ); >> +VPD_ATTR_RO(VK); >> +VPD_ATTR_RO(VL); >> +VPD_ATTR_RO(VM); >> +VPD_ATTR_RO(VN); >> +VPD_ATTR_RO(VO); >> +VPD_ATTR_RO(VP); >> +VPD_ATTR_RO(VQ); >> +VPD_ATTR_RO(VR); >> +VPD_ATTR_RO(VS); >> +VPD_ATTR_RO(VT); >> +VPD_ATTR_RO(VU); >> +VPD_ATTR_RO(VV); >> +VPD_ATTR_RO(VW); >> +VPD_ATTR_RO(VX); >> +VPD_ATTR_RO(VY); >> +VPD_ATTR_RO(VZ); >> + >> +static struct attribute *vpd_attributes[] = { >> + &vpdPN.attr, >> + &vpdEC.attr, >> + &vpdMN.attr, >> + &vpdSN.attr, >> + &vpdV0.attr, >> + &vpdV1.attr, >> + &vpdV2.attr, >> + &vpdV3.attr, >> + &vpdV4.attr, >> + &vpdV5.attr, >> + &vpdV6.attr, >> + &vpdV7.attr, >> + &vpdV8.attr, >> + &vpdV9.attr, >> + &vpdVA.attr, >> + &vpdVB.attr, >> + &vpdVC.attr, >> + &vpdVD.attr, >> + &vpdVE.attr, >> + &vpdVF.attr, >> + &vpdVG.attr, >> + &vpdVH.attr, >> + &vpdVI.attr, >> + &vpdVJ.attr, >> + &vpdVK.attr, >> + &vpdVL.attr, >> + &vpdVM.attr, >> + &vpdVN.attr, >> + &vpdVO.attr, >> + &vpdVP.attr, >> + &vpdVQ.attr, >> + &vpdVR.attr, >> + &vpdVS.attr, >> + &vpdVT.attr, >> + &vpdVU.attr, >> + &vpdVV.attr, >> + &vpdVW.attr, >> + &vpdVX.attr, >> + &vpdVY.attr, >> + &vpdVZ.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group vpd_attr_group = { >> + .name = "vpdr", >> + .attrs = vpd_attributes, >> + .is_visible = vpd_attr_exist, >> +}; >> + >> + >> +static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len) >> +{ >> + u8 tag[3]; >> + int rc, tlen; >> + >> + *len = 0; >> + /* Quirk Atheros cards, reading VPD hangs system for 20s */ >> + if (dev->vendor == PCI_VENDOR_ID_ATHEROS || >> + dev->vendor == PCI_VENDOR_ID_ATTANSIC) >> + return -ENOENT; > >I'm not really sure this is the right place for this type of quirk. >If this is really an issue maybe we should just disable VPD for these >devices. Otherwise there isn't anything to stop someone from going in >and reading the VPD region via the existing VPD interfaces. > This could be moved elsewhere. There's a similar quirk for megaraid cards that was posted recently. [PATCH v4] pci: Limit VPD length for megaraid_sas adapter >> + rc = pci_read_vpd(dev, off, 1, tag); >> + if (rc != 1) >> + return -ENOENT; >> + if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F) >> + return -ENOENT; >> + if (tag[0] & PCI_VPD_LRDT) { >> + rc = pci_read_vpd(dev, off+1, 2, tag+1); >> + if (rc != 2) >> + return -ENOENT; >> + tlen = pci_vpd_lrdt_size(tag) + >> + PCI_VPD_LRDT_TAG_SIZE; >> + } else { >> + tlen = pci_vpd_srdt_size(tag) + >> + PCI_VPD_SRDT_TAG_SIZE; >> + tag[0] &= ~PCI_VPD_SRDT_LEN_MASK; >> + } >> + /* Verify VPD tag fits in area */ >> + if (tlen + off > dev->vpd->len) >> + return -ENOENT; >> + *len = tlen; >> + return tag[0]; >> +} >> + >> +static int pci_load_vpdr(struct pci_dev *dev) >> +{ >> + int rlen, ilen, tag, rc; >> + >> + /* Check for VPD-I and VPD-R tag */ >> + tag = pci_get_vpd_tag(dev, 0, &ilen); >> + if (tag != PCI_VPD_LRDT_ID_STRING) >> + return -ENOENT; >> + tag = pci_get_vpd_tag(dev, ilen, &rlen); >> + if (tag != PCI_VPD_LRDT_RO_DATA) >> + return -ENOENT; >> + >> + rlen -= PCI_VPD_LRDT_TAG_SIZE; >> + dev->vpdr_len = rlen; >> + dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC); >> + if (dev->vpdr_data == NULL) >> + return -ENOMEM; >> + > >Why not cache the ID string as well? Seems like it might be a field >people would want to read on a regular basis in order to find out what >is there. > I could add that. What should attribute be called, 'vpdinfo', 'vpdi', etc? >> + rc = pci_read_vpd(dev, ilen + PCI_VPD_LRDT_TAG_SIZE, >> + rlen, dev->vpdr_data); >> + if (rc != rlen) >> + goto error; >> + if (sysfs_create_group(&dev->dev.kobj, &vpd_attr_group)) >> + goto error; >> + return 0; >> + error: >> + kfree(dev->vpdr_data); >> + dev->vpdr_len = 0; >> + return -ENOENT; >> +} >> + > >This bit here needs to reset vpdr_data back to NULL. Otherwise you >could cause memory corruption via a double free in your two clean-up >routines called out below. Ok will fix that. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-29 19:01 ` Jordan_Hargrave @ 2015-12-29 20:26 ` Alexander Duyck 0 siblings, 0 replies; 16+ messages in thread From: Alexander Duyck @ 2015-12-29 20:26 UTC (permalink / raw) To: Jordan_Hargrave Cc: Hannes Reinecke, Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas On Tue, Dec 29, 2015 at 11:01 AM, <Jordan_Hargrave@dell.com> wrote: >>On Mon, Dec 28, 2015 at 9:29 PM, <Jordan_Hargrave@dell.com> wrote: >>> I had posted a patch recently to enable exposing the VPD-R valyes to sysfs. I need access >>> to these to parse into systemd for network naming (biosdevname style names). >>> >>> >>> The VPD-R is a readonly area contained within the PCI Vital Product >>> data. There are some standard and vendor-specific keys stored in >>> this region. >>> >>> PN = Part Number >>> SN = Serial Number >>> MN = Manufacturer ID >>> Vx = Vendor-specific (x=0..9 A..Z) >>> >>> Biosdevname/Systemd will use these VPD keys for determining Network >>> partitioning and port numbers for NIC cards >> >>Can you please repost this as a patch instead of as a reply to our >>thread about VPD size. The fact is the subject is misleading as your >>patch isn't actually related to VPD sizing. >> > > I had already posted this a few weeks back but never got any feedback. > > [PATCH] Create sysfs entries for VPD-R keys > https://marc.info/?l=linux-pci&m=144959803316031&w=2 Next time I would recommend resubmitting with the people who might be interested in the Cc instead of just throwing it into an existing thread as it really just muddies the waters for the existing thread to have it branch off like this. >>> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com> >>> --- >>> drivers/pci/pci-sysfs.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/pci.h | 2 + >>> 2 files changed, 218 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c >>> index eead54c..4966ece 100644 >>> --- a/drivers/pci/pci-sysfs.c >>> +++ b/drivers/pci/pci-sysfs.c >>> @@ -1295,6 +1295,210 @@ static struct bin_attribute pcie_config_attr = { >>> .write = pci_write_config, >>> }; >>> >>> +static umode_t vpd_attr_exist(struct kobject *kobj, >>> + struct attribute *attr, int n) >>> +{ >>> + struct device *dev; >>> + struct pci_dev *pdev; >>> + const char *name; >>> + int i; >>> + >>> + dev = container_of(kobj, struct device, kobj); >>> + pdev = to_pci_dev(dev); >>> + >>> + name = attr->name; >>> + if (pdev->vpdr_data == NULL) >>> + return 0; >>> + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, >>> + pdev->vpdr_len, name); >>> + return (i >= 0 ? S_IRUGO : 0); >>> +} >>> + >> >>So I assume there is another patch that implements >>pci_vpd_find_info_keyword so that it can go through the vpdr_data and >>parse it? >> > > That's already an existing function in drivers/pci/vpd.c I see that now. Just had to do a bit of grepping through the code. Looks like previously it was used by mostly the Broadcom network device drivers. >>> +static ssize_t vpd_attr_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct pci_dev *pdev; >>> + const char *name; >>> + char kv_data[257] = { 0 }; >>> + int i, len; >>> + >>> + pdev = to_pci_dev(dev); >>> + name = attr->attr.name; >>> + if (pdev->vpdr_data == NULL) >>> + return 0; >>> + i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0, >>> + pdev->vpdr_len, name); >>> + if (i >= 0) { >>> + len = pci_vpd_info_field_size(&pdev->vpdr_data[i]); >>> + memcpy(kv_data, pdev->vpdr_data + i + >>> + PCI_VPD_INFO_FLD_HDR_SIZE, len); >>> + return scnprintf(buf, PAGE_SIZE, "%s\n", kv_data); >>> + } >>> + return 0; >>> +} >>> + >>> +#define VPD_ATTR_RO(x) \ >>> +struct device_attribute vpd ## x = { \ >>> + .attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \ >>> + .show = vpd_attr_show \ >>> +} >>> + >>> +VPD_ATTR_RO(PN); >>> +VPD_ATTR_RO(EC); >>> +VPD_ATTR_RO(MN); >>> +VPD_ATTR_RO(SN); >>> +VPD_ATTR_RO(V0); >>> +VPD_ATTR_RO(V1); >>> +VPD_ATTR_RO(V2); >>> +VPD_ATTR_RO(V3); >>> +VPD_ATTR_RO(V4); >>> +VPD_ATTR_RO(V5); >>> +VPD_ATTR_RO(V6); >>> +VPD_ATTR_RO(V7); >>> +VPD_ATTR_RO(V8); >>> +VPD_ATTR_RO(V9); >>> +VPD_ATTR_RO(VA); >>> +VPD_ATTR_RO(VB); >>> +VPD_ATTR_RO(VC); >>> +VPD_ATTR_RO(VD); >>> +VPD_ATTR_RO(VE); >>> +VPD_ATTR_RO(VF); >>> +VPD_ATTR_RO(VG); >>> +VPD_ATTR_RO(VH); >>> +VPD_ATTR_RO(VI); >>> +VPD_ATTR_RO(VJ); >>> +VPD_ATTR_RO(VK); >>> +VPD_ATTR_RO(VL); >>> +VPD_ATTR_RO(VM); >>> +VPD_ATTR_RO(VN); >>> +VPD_ATTR_RO(VO); >>> +VPD_ATTR_RO(VP); >>> +VPD_ATTR_RO(VQ); >>> +VPD_ATTR_RO(VR); >>> +VPD_ATTR_RO(VS); >>> +VPD_ATTR_RO(VT); >>> +VPD_ATTR_RO(VU); >>> +VPD_ATTR_RO(VV); >>> +VPD_ATTR_RO(VW); >>> +VPD_ATTR_RO(VX); >>> +VPD_ATTR_RO(VY); >>> +VPD_ATTR_RO(VZ); >>> + >>> +static struct attribute *vpd_attributes[] = { >>> + &vpdPN.attr, >>> + &vpdEC.attr, >>> + &vpdMN.attr, >>> + &vpdSN.attr, >>> + &vpdV0.attr, >>> + &vpdV1.attr, >>> + &vpdV2.attr, >>> + &vpdV3.attr, >>> + &vpdV4.attr, >>> + &vpdV5.attr, >>> + &vpdV6.attr, >>> + &vpdV7.attr, >>> + &vpdV8.attr, >>> + &vpdV9.attr, >>> + &vpdVA.attr, >>> + &vpdVB.attr, >>> + &vpdVC.attr, >>> + &vpdVD.attr, >>> + &vpdVE.attr, >>> + &vpdVF.attr, >>> + &vpdVG.attr, >>> + &vpdVH.attr, >>> + &vpdVI.attr, >>> + &vpdVJ.attr, >>> + &vpdVK.attr, >>> + &vpdVL.attr, >>> + &vpdVM.attr, >>> + &vpdVN.attr, >>> + &vpdVO.attr, >>> + &vpdVP.attr, >>> + &vpdVQ.attr, >>> + &vpdVR.attr, >>> + &vpdVS.attr, >>> + &vpdVT.attr, >>> + &vpdVU.attr, >>> + &vpdVV.attr, >>> + &vpdVW.attr, >>> + &vpdVX.attr, >>> + &vpdVY.attr, >>> + &vpdVZ.attr, >>> + NULL, >>> +}; >>> + >>> +static struct attribute_group vpd_attr_group = { >>> + .name = "vpdr", >>> + .attrs = vpd_attributes, >>> + .is_visible = vpd_attr_exist, >>> +}; >>> + >>> + >>> +static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len) >>> +{ >>> + u8 tag[3]; >>> + int rc, tlen; >>> + >>> + *len = 0; >>> + /* Quirk Atheros cards, reading VPD hangs system for 20s */ >>> + if (dev->vendor == PCI_VENDOR_ID_ATHEROS || >>> + dev->vendor == PCI_VENDOR_ID_ATTANSIC) >>> + return -ENOENT; >> >>I'm not really sure this is the right place for this type of quirk. >>If this is really an issue maybe we should just disable VPD for these >>devices. Otherwise there isn't anything to stop someone from going in >>and reading the VPD region via the existing VPD interfaces. >> > > This could be moved elsewhere. There's a similar quirk for megaraid cards that was posted recently. > [PATCH v4] pci: Limit VPD length for megaraid_sas adapter > >>> + rc = pci_read_vpd(dev, off, 1, tag); >>> + if (rc != 1) >>> + return -ENOENT; >>> + if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F) >>> + return -ENOENT; >>> + if (tag[0] & PCI_VPD_LRDT) { >>> + rc = pci_read_vpd(dev, off+1, 2, tag+1); >>> + if (rc != 2) >>> + return -ENOENT; >>> + tlen = pci_vpd_lrdt_size(tag) + >>> + PCI_VPD_LRDT_TAG_SIZE; >>> + } else { >>> + tlen = pci_vpd_srdt_size(tag) + >>> + PCI_VPD_SRDT_TAG_SIZE; >>> + tag[0] &= ~PCI_VPD_SRDT_LEN_MASK; >>> + } >>> + /* Verify VPD tag fits in area */ >>> + if (tlen + off > dev->vpd->len) >>> + return -ENOENT; >>> + *len = tlen; >>> + return tag[0]; >>> +} >>> + >>> +static int pci_load_vpdr(struct pci_dev *dev) >>> +{ >>> + int rlen, ilen, tag, rc; >>> + >>> + /* Check for VPD-I and VPD-R tag */ >>> + tag = pci_get_vpd_tag(dev, 0, &ilen); >>> + if (tag != PCI_VPD_LRDT_ID_STRING) >>> + return -ENOENT; >>> + tag = pci_get_vpd_tag(dev, ilen, &rlen); >>> + if (tag != PCI_VPD_LRDT_RO_DATA) >>> + return -ENOENT; >>> + >>> + rlen -= PCI_VPD_LRDT_TAG_SIZE; >>> + dev->vpdr_len = rlen; >>> + dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC); >>> + if (dev->vpdr_data == NULL) >>> + return -ENOMEM; >>> + >> >>Why not cache the ID string as well? Seems like it might be a field >>people would want to read on a regular basis in order to find out what >>is there. >> > I could add that. What should attribute be called, 'vpdinfo', 'vpdi', etc? I'd say keep it simple. Maybe something like 'id-string' since that is the name of the tag. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv3 0/2] PCI: Safe VPD access @ 2015-12-17 7:59 Hannes Reinecke 2015-12-17 7:59 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 0 siblings, 1 reply; 16+ messages in thread From: Hannes Reinecke @ 2015-12-17 7:59 UTC (permalink / raw) To: Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke PCI VPD suffers from two problems: it has a very rudimentary interface and it relies on correctly formatted data. And essentially it provides a direct channel into the card hardware. In other words, plenty of chances to mess things up. With the original implementation we would just read the VPD space, blissfully ignorant of the data formatting of the VPD data. This would work if the VPD space happens to be properly implemented. However, I've had several reports where a simple 'cat' on the vpd attribute would return garbage (if you're lucky), trigger a timeout with a kernel warning (which actually triggered this patchset), or hang your machine (if you're really unlucky). So this patchset validates the VPD data, setting the VPD attribute to the correct size or disabling VPD access altogether if no valid data is found. Hannes Reinecke (2): pci: Update VPD definitions pci: Update VPD size with correct length drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 27 +++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 2 deletions(-) -- 1.8.5.6 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] pci: Update VPD size with correct length 2015-12-17 7:59 [PATCHv3 0/2] PCI: Safe VPD access Hannes Reinecke @ 2015-12-17 7:59 ` Hannes Reinecke 2015-12-17 11:06 ` Seymour, Shane M ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Hannes Reinecke @ 2015-12-17 7:59 UTC (permalink / raw) To: Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke, Bjorn Helgaas 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 + * + */ +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; } -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-17 7:59 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke @ 2015-12-17 11:06 ` Seymour, Shane M 2015-12-17 11:10 ` kbuild test robot 2015-12-17 17:13 ` Alexander Duyck 2 siblings, 0 replies; 16+ messages in thread From: Seymour, Shane M @ 2015-12-17 11:06 UTC (permalink / raw) To: Hannes Reinecke, Bjorn Helgaas Cc: Alexander Duyck, Michal Kubecek, linux-pci, linux-kernel, Bjorn Helgaas 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-17 7:59 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 2015-12-17 11:06 ` Seymour, Shane M @ 2015-12-17 11:10 ` kbuild test robot 2015-12-17 17:13 ` Alexander Duyck 2 siblings, 0 replies; 16+ messages in thread From: kbuild test robot @ 2015-12-17 11:10 UTC (permalink / raw) To: Hannes Reinecke Cc: kbuild-all, Bjorn Helgaas, Alexander Duyck, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Hannes Reinecke, Bjorn Helgaas [-- Attachment #1: Type: text/plain, Size: 1818 bytes --] 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 [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 6125 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-17 7:59 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 2015-12-17 11:06 ` Seymour, Shane M 2015-12-17 11:10 ` kbuild test robot @ 2015-12-17 17:13 ` Alexander Duyck 2015-12-18 7:44 ` Hannes Reinecke 2 siblings, 1 reply; 16+ messages in thread From: Alexander Duyck @ 2015-12-17 17:13 UTC (permalink / raw) To: Hannes Reinecke Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pci: Update VPD size with correct length 2015-12-17 17:13 ` Alexander Duyck @ 2015-12-18 7:44 ` Hannes Reinecke 0 siblings, 0 replies; 16+ messages in thread From: Hannes Reinecke @ 2015-12-18 7:44 UTC (permalink / raw) To: Alexander Duyck Cc: Bjorn Helgaas, Michal Kubecek, Shane M. Seymour, linux-pci, linux-kernel, Bjorn Helgaas 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 -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-12-29 20:26 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-18 8:35 [PATCHv4 0/2] PCI: Safe VPD access Hannes Reinecke 2015-12-18 8:35 ` [PATCH 1/2] pci: Update VPD definitions Hannes Reinecke 2015-12-18 8:35 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 2015-12-18 13:49 ` Alexander Duyck 2015-12-18 13:57 ` Hannes Reinecke 2015-12-18 14:02 ` Alexander Duyck 2015-12-18 14:14 ` Hannes Reinecke 2015-12-29 5:29 ` Jordan_Hargrave 2015-12-29 17:48 ` Alexander Duyck 2015-12-29 19:01 ` Jordan_Hargrave 2015-12-29 20:26 ` Alexander Duyck -- strict thread matches above, loose matches on Subject: below -- 2015-12-17 7:59 [PATCHv3 0/2] PCI: Safe VPD access Hannes Reinecke 2015-12-17 7:59 ` [PATCH 2/2] pci: Update VPD size with correct length Hannes Reinecke 2015-12-17 11:06 ` Seymour, Shane M 2015-12-17 11:10 ` kbuild test robot 2015-12-17 17:13 ` Alexander Duyck 2015-12-18 7:44 ` Hannes Reinecke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).