[PATCHv4 0/2] PCI: Safe VPD access

[PATCHv4 0/2] PCI: Safe VPD access

[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

[PATCH 1/2] pci: Update VPD definitions

[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

[PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

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

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

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

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

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)

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

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

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

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)

RE: [PATCH 2/2] pci: Update VPD size with correct length

[Jordan_Hargrave]

>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
>

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

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.

RE: [PATCH 2/2] pci: Update VPD size with correct length

[Jordan_Hargrave]

>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.

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

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.

[PATCHv3 0/2] PCI: Safe VPD access

[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

[PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

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

RE: [PATCH 2/2] pci: Update VPD size with correct length

[Seymour, Shane M]

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>

Re: [PATCH 2/2] pci: Update VPD size with correct length

[kbuild test robot]

[-- 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 --]

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

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

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

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)