[PATCH v4 00/11] PCI VPD access fixes

[PATCH v4 00/11] PCI VPD access fixes

[Bjorn Helgaas]

Hi Hannes,

This is a revision of your v3 series:
http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de

Here's the description from your v3 posting:

  the current PCI VPD page access assumes that the entire possible VPD
  data is readable. However, the spec only guarantees a VPD data up to
  the 'end' marker, with everything beyond that being undefined.
  This causes a system lockup on certain devices.

  With this patch we always set the VPD sysfs attribute size to '0', and
  calculate the available VPD size on the first access.
  If no valid data can be read an I/O error is returned.

  I've also included the patch from Babu to blacklists devices which
  are known to lockup when accessing the VPD data.

I tweaked a few things, mostly whitespace and printk changes.  The
appended diff shows the changes I made.

I added some patches on top to clean up and simplify the VPD code.
These shouldn't make any functional difference unless I've made a
mistake.  I've built these, but I don't really have a way to test
them.

I am still waiting for bugzilla links from Babu for the blacklist
patch.

Bjorn

---

Babu Moger (1):
      FIXME need bugzilla link

Bjorn Helgaas (7):
      PCI: Use bitfield instead of bool for struct pci_vpd_pci22.busy
      PCI: Move pci_read_vpd() and pci_write_vpd() close to other VPD code
      PCI: Move pci_vpd_release() from header file to pci/access.c
      PCI: Remove struct pci_vpd_ops.release function pointer
      PCI: Rename VPD symbols to remove unnecessary "pci22"
      PCI: Fold struct pci_vpd_pci22 into struct pci_vpd
      PCI: Sleep rather than busy-wait for VPD access completion

Hannes Reinecke (3):
      PCI: Update VPD definitions
      PCI: Allow access to VPD attributes with size 0
      PCI: Determine actual VPD size on first access


 drivers/pci/access.c    |  240 ++++++++++++++++++++++++++++++-----------------
 drivers/pci/pci-sysfs.c |   22 +++-
 drivers/pci/pci.h       |   16 ++-
 drivers/pci/probe.c     |    2 
 drivers/pci/quirks.c    |   29 ++++++
 include/linux/pci.h     |   27 +++++
 6 files changed, 231 insertions(+), 105 deletions(-)


Below are the changes I made to your v3 series:

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 8b6f5a2..4850f06 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -283,9 +283,9 @@ struct pci_vpd_pci22 {
 	struct pci_vpd base;
 	struct mutex lock;
 	u16	flag;
+	u8	cap;
 	u8	busy:1;
 	u8	valid:1;
-	u8	cap;
 };
 
 /**
@@ -311,9 +311,9 @@ static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_size)
 			    (tag == PCI_VPD_LTIN_RW_DATA)) {
 				if (pci_read_vpd(dev, off+1, 2,
 						 &header[1]) != 2) {
-					dev_dbg(&dev->dev,
-						"invalid large VPD tag %02x size at offset %zu",
-						tag, off + 1);
+					dev_warn(&dev->dev,
+						 "invalid large VPD tag %02x size at offset %zu",
+						 tag, off + 1);
 					return 0;
 				}
 				off += PCI_VPD_LRDT_TAG_SIZE +
@@ -325,15 +325,17 @@ static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_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);
+			dev_warn(&dev->dev,
+				 "invalid %s VPD tag %02x at offset %zu",
+				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
+				 tag, off);
 			return 0;
 		}
 	}
@@ -366,7 +368,7 @@ static int pci_vpd_pci22_wait(struct pci_dev *dev)
 			return ret;
 
 		if ((status & PCI_VPD_ADDR_F) == vpd->flag) {
-			vpd->busy = false;
+			vpd->busy = 0;
 			return 0;
 		}
 
@@ -393,16 +395,18 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
-	if (!vpd->valid && vpd->base.len > 0) {
-		vpd->valid = true;
+	if (!vpd->valid) {
+		vpd->valid = 1;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}
+
 	if (vpd->base.len == 0)
 		return -EIO;
 
+	if (pos >= vpd->base.len)
+		return 0;
+
 	if (end > vpd->base.len) {
-		if (pos > vpd->base.len)
-			return 0;
 		end = vpd->base.len;
 		count = end - pos;
 	}
@@ -422,7 +426,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 						 pos & ~3);
 		if (ret < 0)
 			break;
-		vpd->busy = true;
+		vpd->busy = 1;
 		vpd->flag = PCI_VPD_ADDR_F;
 		ret = pci_vpd_pci22_wait(dev);
 		if (ret < 0)
@@ -459,10 +463,11 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
-	if (!vpd->valid && vpd->base.len > 0) {
-		vpd->valid = true;
+	if (!vpd->valid) {
+		vpd->valid = 1;
 		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
 	}
+
 	if (vpd->base.len == 0)
 		return -EIO;
 
@@ -492,7 +497,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 		if (ret < 0)
 			break;
 
-		vpd->busy = true;
+		vpd->busy = 1;
 		vpd->flag = 0;
 		ret = pci_vpd_pci22_wait(dev);
 		if (ret < 0)
@@ -572,8 +577,8 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 		vpd->base.ops = &pci_vpd_pci22_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
-	vpd->busy = false;
-	vpd->valid = false;
+	vpd->busy = 0;
+	vpd->valid = 0;
 	dev->vpd = &vpd->base;
 	return 0;
 }
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index df1178f..626c3b2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2135,45 +2135,31 @@ static void quirk_via_cx700_pci_parking_caching(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, 0x324e, quirk_via_cx700_pci_parking_caching);
 
 /*
- * A read/write to sysfs entry ('/sys/bus/pci/devices/<id>/vpd')
- * will dump 32k of data. The default length is set as 32768.
- * Reading a full 32k will cause an access beyond the VPD end tag.
- * The system behaviour at that point is mostly unpredictable.
- * Apparently, some vendors have not implemented this VPD headers properly.
- * Adding a generic function disable vpd data for these buggy adapters
- * Add the DECLARE_PCI_FIXUP_FINAL line below with the specific with
- * vendor and device of interest to use this quirk.
+ * If a device follows the VPD format spec, the PCI core will not read or
+ * write past the VPD End Tag.  But some vendors do not follow the VPD
+ * format spec, so we can't tell how much data is safe to access.  Devices
+ * may behave unpredictably if we access too much.  Blacklist these devices
+ * so we don't touch VPD at all.
  */
 static void quirk_blacklist_vpd(struct pci_dev *dev)
 {
 	if (dev->vpd) {
 		dev->vpd->len = 0;
-		dev_warn(&dev->dev, "PCI vpd access has been disabled due to firmware bug\n");
+		dev_warn(&dev->dev, FW_BUG "VPD access disabled\n");
 	}
 }
 
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d,
-		quirk_blacklist_vpd);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f,
-		quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f, quirk_blacklist_vpd);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
 		quirk_blacklist_vpd);
 

[PATCH v4 01/11] PCI: Update VPD definitions

[Bjorn Helgaas]

From: Hannes Reinecke <hare@suse.de>

The 'end' tag is actually 0x0f; it's the representation as a small resource
data type tag that's 0x78 (i.e., shifted by 3).  Correct PCI_VPD_STIN_END
and PCI_VPD_SRDT_END accordingly.

Also, add helper functions to extract the resource data type tags for both
large and small resource data types.

[bhelgaas: changelog]
Signed-off-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Duyck <alexander.duyck@gmail.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 27df4a6..49ad85c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1834,12 +1834,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
@@ -1863,6 +1864,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
  *
@@ -1874,6 +1886,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
  *

[PATCH v4 02/11] PCI: Allow access to VPD attributes with size 0

[Bjorn Helgaas]

From: Hannes Reinecke <hare@suse.de>

It is not always possible to determine the actual size of the VPD
data, so allow access to them if the size is set to '0'.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
---
 drivers/pci/pci-sysfs.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 95d9e7b..a730f54 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -769,10 +769,12 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj,
 {
 	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
 
-	if (off > bin_attr->size)
-		count = 0;
-	else if (count > bin_attr->size - off)
-		count = bin_attr->size - off;
+	if (bin_attr->size > 0) {
+		if (off > bin_attr->size)
+			count = 0;
+		else if (count > bin_attr->size - off)
+			count = bin_attr->size - off;
+	}
 
 	return pci_read_vpd(dev, off, count, buf);
 }
@@ -783,10 +785,12 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj,
 {
 	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
 
-	if (off > bin_attr->size)
-		count = 0;
-	else if (count > bin_attr->size - off)
-		count = bin_attr->size - off;
+	if (bin_attr->size > 0) {
+		if (off > bin_attr->size)
+			count = 0;
+		else if (count > bin_attr->size - off)
+			count = bin_attr->size - off;
+	}
 
 	return pci_write_vpd(dev, off, count, buf);
 }

[PATCH v4 03/11] PCI: Use bitfield instead of bool for struct pci_vpd_pci22.busy

[Bjorn Helgaas]

Make struct pci_vpd_pci22.busy a 1-bit field instead of a bool.  We intend
to add another flag, and two bitfields are cheaper than two bools.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 8c05b5c..a7f0069 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -283,8 +283,8 @@ struct pci_vpd_pci22 {
 	struct pci_vpd base;
 	struct mutex lock;
 	u16	flag;
-	bool	busy;
 	u8	cap;
+	u8	busy:1;
 };
 
 /*
@@ -313,7 +313,7 @@ static int pci_vpd_pci22_wait(struct pci_dev *dev)
 			return ret;
 
 		if ((status & PCI_VPD_ADDR_F) == vpd->flag) {
-			vpd->busy = false;
+			vpd->busy = 0;
 			return 0;
 		}
 
@@ -355,7 +355,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 						 pos & ~3);
 		if (ret < 0)
 			break;
-		vpd->busy = true;
+		vpd->busy = 1;
 		vpd->flag = PCI_VPD_ADDR_F;
 		ret = pci_vpd_pci22_wait(dev);
 		if (ret < 0)
@@ -415,7 +415,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 		if (ret < 0)
 			break;
 
-		vpd->busy = true;
+		vpd->busy = 1;
 		vpd->flag = 0;
 		ret = pci_vpd_pci22_wait(dev);
 		if (ret < 0)
@@ -495,7 +495,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 		vpd->base.ops = &pci_vpd_pci22_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
-	vpd->busy = false;
+	vpd->busy = 0;
 	dev->vpd = &vpd->base;
 	return 0;
 }

[PATCH v4 04/11] PCI: Determine actual VPD size on first access

[Bjorn Helgaas]

From: Hannes Reinecke <hare@suse.de>

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.

Per spec, reading outside of the VPD space is "not allowed."  In practice,
it may cause simple read errors or even crash the card.  To make matters
worse not every PCI card implements this properly, leaving us with no 'end'
marker or even completely invalid data.

Try to determine the size of the VPD data when it's first accessed.  If no
valid data can be read an I/O error will be returned when reading or
writing the sysfs attribute.

As the amount of VPD data is unknown initially the size of the sysfs
attribute will always be set to '0'.

[bhelgaas: changelog, use 0/1 (not false/true) for bitfield, tweak
pci_vpd_pci22_read() error checking]
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
---
 drivers/pci/access.c    |   87 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci-sysfs.c |    2 +
 2 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index a7f0069..4850f06 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -285,8 +285,63 @@ struct pci_vpd_pci22 {
 	u16	flag;
 	u8	cap;
 	u8	busy:1;
+	u8	valid:1;
 };
 
+/**
+ * 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 old_size)
+{
+	size_t off = 0;
+	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
+
+	while (off < old_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) {
+					dev_warn(&dev->dev,
+						 "invalid large VPD tag %02x size at offset %zu",
+						 tag, off + 1);
+					return 0;
+				}
+				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_warn(&dev->dev,
+				 "invalid %s VPD tag %02x at offset %zu",
+				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
+				 tag, off);
+			return 0;
+		}
+	}
+	return 0;
+}
+
 /*
  * Wait for last operation to complete.
  * This code has to spin since there is no other notification from the PCI
@@ -337,9 +392,25 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
-	if (pos < 0 || pos > vpd->base.len || end > vpd->base.len)
+	if (pos < 0)
 		return -EINVAL;
 
+	if (!vpd->valid) {
+		vpd->valid = 1;
+		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+	}
+
+	if (vpd->base.len == 0)
+		return -EIO;
+
+	if (pos >= vpd->base.len)
+		return 0;
+
+	if (end > vpd->base.len) {
+		end = vpd->base.len;
+		count = end - pos;
+	}
+
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
@@ -389,7 +460,18 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	loff_t end = pos + count;
 	int ret = 0;
 
-	if (pos < 0 || (pos & 3) || (count & 3) || end > vpd->base.len)
+	if (pos < 0 || (pos & 3) || (count & 3))
+		return -EINVAL;
+
+	if (!vpd->valid) {
+		vpd->valid = 1;
+		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+	}
+
+	if (vpd->base.len == 0)
+		return -EIO;
+
+	if (end > vpd->base.len)
 		return -EINVAL;
 
 	if (mutex_lock_killable(&vpd->lock))
@@ -496,6 +578,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = 0;
+	vpd->valid = 0;
 	dev->vpd = &vpd->base;
 	return 0;
 }
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index a730f54..ed39c09 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1323,7 +1323,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 			return -ENOMEM;
 
 		sysfs_bin_attr_init(attr);
-		attr->size = dev->vpd->len;
+		attr->size = 0;
 		attr->attr.name = "vpd";
 		attr->attr.mode = S_IRUSR | S_IWUSR;
 		attr->read = read_vpd_attr;

[PATCH v4 05/11] FIXME need bugzilla link

[Bjorn Helgaas]

From: Babu Moger <babu.moger@oracle.com>

PCI: Prevent VPD access for buggy devices

On some devices, reading or writing VPD data causes a system panic.
This can be easily reproduced by running "lspci -vvv" or
"cat /sys/bus/devices/XX../vpd".

Blacklist these devices so we don't access VPD data at all.

[bhelgaas: changelog, comment, drop pci/access.c changes]
Signed-off-by: Babu Moger <babu.moger@oracle.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
---
 drivers/pci/quirks.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0575a1e..626c3b2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2135,6 +2135,35 @@ static void quirk_via_cx700_pci_parking_caching(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, 0x324e, quirk_via_cx700_pci_parking_caching);
 
 /*
+ * If a device follows the VPD format spec, the PCI core will not read or
+ * write past the VPD End Tag.  But some vendors do not follow the VPD
+ * format spec, so we can't tell how much data is safe to access.  Devices
+ * may behave unpredictably if we access too much.  Blacklist these devices
+ * so we don't touch VPD at all.
+ */
+static void quirk_blacklist_vpd(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		dev->vpd->len = 0;
+		dev_warn(&dev->dev, FW_BUG "VPD access disabled\n");
+	}
+}
+
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f, quirk_blacklist_vpd);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
+		quirk_blacklist_vpd);
+
+/*
  * For Broadcom 5706, 5708, 5709 rev. A nics, any read beyond the
  * VPD end tag will hang the device.  This problem was initially
  * observed when a vpd entry was created in sysfs

[PATCH v4 06/11] PCI: Move pci_read_vpd() and pci_write_vpd() close to other VPD code

[Bjorn Helgaas]

pci_read_vpd() and pci_write_vpd() were stranded in the middle of config
accessor functions.  Move them close to the other VPD code in the file.
No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |   62 ++++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 4850f06..4c4c734 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -174,38 +174,6 @@ struct pci_ops *pci_bus_set_ops(struct pci_bus *bus, struct pci_ops *ops)
 }
 EXPORT_SYMBOL(pci_bus_set_ops);
 
-/**
- * pci_read_vpd - Read one entry from Vital Product Data
- * @dev:	pci device struct
- * @pos:	offset in vpd space
- * @count:	number of bytes to read
- * @buf:	pointer to where to store result
- *
- */
-ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
-{
-	if (!dev->vpd || !dev->vpd->ops)
-		return -ENODEV;
-	return dev->vpd->ops->read(dev, pos, count, buf);
-}
-EXPORT_SYMBOL(pci_read_vpd);
-
-/**
- * pci_write_vpd - Write entry to Vital Product Data
- * @dev:	pci device struct
- * @pos:	offset in vpd space
- * @count:	number of bytes to write
- * @buf:	buffer containing write data
- *
- */
-ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
-{
-	if (!dev->vpd || !dev->vpd->ops)
-		return -ENODEV;
-	return dev->vpd->ops->write(dev, pos, count, buf);
-}
-EXPORT_SYMBOL(pci_write_vpd);
-
 /*
  * 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
@@ -277,6 +245,36 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 /* VPD access through PCI 2.2+ VPD capability */
 
+/**
+ * pci_read_vpd - Read one entry from Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to read
+ * @buf:	pointer to where to store result
+ */
+ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
+{
+	if (!dev->vpd || !dev->vpd->ops)
+		return -ENODEV;
+	return dev->vpd->ops->read(dev, pos, count, buf);
+}
+EXPORT_SYMBOL(pci_read_vpd);
+
+/**
+ * pci_write_vpd - Write entry to Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to write
+ * @buf:	buffer containing write data
+ */
+ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
+{
+	if (!dev->vpd || !dev->vpd->ops)
+		return -ENODEV;
+	return dev->vpd->ops->write(dev, pos, count, buf);
+}
+EXPORT_SYMBOL(pci_write_vpd);
+
 #define PCI_VPD_PCI22_SIZE (PCI_VPD_ADDR_MASK + 1)
 
 struct pci_vpd_pci22 {

[PATCH v4 07/11] PCI: Move pci_vpd_release() from header file to pci/access.c

[Bjorn Helgaas]

Move pci_vpd_release() so it's next to the other VPD functions.  This puts
it next to pci_vpd_pci22_init(), which allocates the space freed by
pci_vpd_release().

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    6 ++++++
 drivers/pci/pci.h    |    6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 4c4c734..ca42a33 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -581,6 +581,12 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	return 0;
 }
 
+void pci_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd)
+		dev->vpd->ops->release(dev);
+}
+
 /**
  * pci_cfg_access_lock - Lock PCI config reads/writes
  * @dev:	pci device struct
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9a1660f..52e86b0 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -107,11 +107,7 @@ struct pci_vpd {
 };
 
 int pci_vpd_pci22_init(struct pci_dev *dev);
-static inline void pci_vpd_release(struct pci_dev *dev)
-{
-	if (dev->vpd)
-		dev->vpd->ops->release(dev);
-}
+void pci_vpd_release(struct pci_dev *dev);
 
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS

[PATCH v4 08/11] PCI: Remove struct pci_vpd_ops.release function pointer

[Bjorn Helgaas]

The struct pci_vpd_ops.release function pointer is always
pci_vpd_pci22_release(), so there's no need for the flexibility of a
function pointer.

Inline the pci_vpd_pci22_release() body into pci_vpd_release() and remove
pci_vpd_pci22_release() and the struct pci_vpd_ops.release function
pointer.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    9 +--------
 drivers/pci/pci.h    |    1 -
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ca42a33..68ed22a 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -508,15 +508,9 @@ out:
 	return ret ? ret : count;
 }
 
-static void pci_vpd_pci22_release(struct pci_dev *dev)
-{
-	kfree(container_of(dev->vpd, struct pci_vpd_pci22, base));
-}
-
 static const struct pci_vpd_ops pci_vpd_pci22_ops = {
 	.read = pci_vpd_pci22_read,
 	.write = pci_vpd_pci22_write,
-	.release = pci_vpd_pci22_release,
 };
 
 static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
@@ -552,7 +546,6 @@ static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count,
 static const struct pci_vpd_ops pci_vpd_f0_ops = {
 	.read = pci_vpd_f0_read,
 	.write = pci_vpd_f0_write,
-	.release = pci_vpd_pci22_release,
 };
 
 int pci_vpd_pci22_init(struct pci_dev *dev)
@@ -584,7 +577,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 void pci_vpd_release(struct pci_dev *dev)
 {
 	if (dev->vpd)
-		dev->vpd->ops->release(dev);
+		kfree(container_of(dev->vpd, struct pci_vpd_pci22, base));
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 52e86b0..b3e9daa 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -97,7 +97,6 @@ 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);
-	void (*release)(struct pci_dev *dev);
 };
 
 struct pci_vpd {

[PATCH v4 09/11] PCI: Rename VPD symbols to remove unnecessary "pci22"

[Bjorn Helgaas]

There's only one kind of VPD, so we don't need to qualify it as "the
version described by PCI spec rev 2.2."

Rename the following symbols to remove unnecessary "pci22":

  PCI_VPD_PCI22_SIZE	-> PCI_VPD_MAX_SIZE
  pci_vpd_pci22_size()	-> pci_vpd_size()
  pci_vpd_pci22_wait()	-> pci_vpd_wait()
  pci_vpd_pci22_read()	-> pci_vpd_read()
  pci_vpd_pci22_write()	-> pci_vpd_write()
  pci_vpd_pci22_ops	-> pci_vpd_ops
  pci_vpd_pci22_init()	-> pci_vpd_init()

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |   38 +++++++++++++++++++-------------------
 drivers/pci/pci.h    |    2 +-
 drivers/pci/probe.c  |    2 +-
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 68ed22a..ee205de 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -275,7 +275,7 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void
 }
 EXPORT_SYMBOL(pci_write_vpd);
 
-#define PCI_VPD_PCI22_SIZE (PCI_VPD_ADDR_MASK + 1)
+#define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1)
 
 struct pci_vpd_pci22 {
 	struct pci_vpd base;
@@ -291,7 +291,7 @@ struct pci_vpd_pci22 {
  * @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 old_size)
+static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 {
 	size_t off = 0;
 	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
@@ -348,7 +348,7 @@ static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_size)
  *
  * Returns 0 on success, negative values indicate error.
  */
-static int pci_vpd_pci22_wait(struct pci_dev *dev)
+static int pci_vpd_wait(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
@@ -381,8 +381,8 @@ static int pci_vpd_pci22_wait(struct pci_dev *dev)
 	}
 }
 
-static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
-				  void *arg)
+static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
+			    void *arg)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
@@ -395,7 +395,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+		vpd->base.len = pci_vpd_size(dev, vpd->base.len);
 	}
 
 	if (vpd->base.len == 0)
@@ -412,7 +412,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
-	ret = pci_vpd_pci22_wait(dev);
+	ret = pci_vpd_wait(dev);
 	if (ret < 0)
 		goto out;
 
@@ -426,7 +426,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
 			break;
 		vpd->busy = 1;
 		vpd->flag = PCI_VPD_ADDR_F;
-		ret = pci_vpd_pci22_wait(dev);
+		ret = pci_vpd_wait(dev);
 		if (ret < 0)
 			break;
 
@@ -449,8 +449,8 @@ out:
 	return ret ? ret : count;
 }
 
-static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count,
-				   const void *arg)
+static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
+			     const void *arg)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
@@ -463,7 +463,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
+		vpd->base.len = pci_vpd_size(dev, vpd->base.len);
 	}
 
 	if (vpd->base.len == 0)
@@ -475,7 +475,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
-	ret = pci_vpd_pci22_wait(dev);
+	ret = pci_vpd_wait(dev);
 	if (ret < 0)
 		goto out;
 
@@ -497,7 +497,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
 
 		vpd->busy = 1;
 		vpd->flag = 0;
-		ret = pci_vpd_pci22_wait(dev);
+		ret = pci_vpd_wait(dev);
 		if (ret < 0)
 			break;
 
@@ -508,9 +508,9 @@ out:
 	return ret ? ret : count;
 }
 
-static const struct pci_vpd_ops pci_vpd_pci22_ops = {
-	.read = pci_vpd_pci22_read,
-	.write = pci_vpd_pci22_write,
+static const struct pci_vpd_ops pci_vpd_ops = {
+	.read = pci_vpd_read,
+	.write = pci_vpd_write,
 };
 
 static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
@@ -548,7 +548,7 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
 	.write = pci_vpd_f0_write,
 };
 
-int pci_vpd_pci22_init(struct pci_dev *dev)
+int pci_vpd_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
 	u8 cap;
@@ -561,11 +561,11 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	if (!vpd)
 		return -ENOMEM;
 
-	vpd->base.len = PCI_VPD_PCI22_SIZE;
+	vpd->base.len = PCI_VPD_MAX_SIZE;
 	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
 		vpd->base.ops = &pci_vpd_f0_ops;
 	else
-		vpd->base.ops = &pci_vpd_pci22_ops;
+		vpd->base.ops = &pci_vpd_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = 0;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b3e9daa..6191703 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -105,7 +105,7 @@ struct pci_vpd {
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
 };
 
-int pci_vpd_pci22_init(struct pci_dev *dev);
+int pci_vpd_init(struct pci_dev *dev);
 void pci_vpd_release(struct pci_dev *dev);
 
 /* PCI /proc functions */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..39b0174 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1608,7 +1608,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_pm_init(dev);
 
 	/* Vital Product Data */
-	pci_vpd_pci22_init(dev);
+	pci_vpd_init(dev);
 
 	/* Alternative Routing-ID Forwarding */
 	pci_configure_ari(dev);

[PATCH v4 10/11] PCI: Fold struct pci_vpd_pci22 into struct pci_vpd

[Bjorn Helgaas]

We only support one flavor of VPD, so there's no need to complicate things
by having a "generic" struct pci_vpd and a more specific struct
pci_vpd_pci22.

Fold struct pci_vpd_pci22 directly into struct pci_vpd.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |   46 +++++++++++++++++-----------------------------
 drivers/pci/pci.h    |    7 ++++++-
 2 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ee205de..cae5462 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -277,15 +277,6 @@ EXPORT_SYMBOL(pci_write_vpd);
 
 #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1)
 
-struct pci_vpd_pci22 {
-	struct pci_vpd base;
-	struct mutex lock;
-	u16	flag;
-	u8	cap;
-	u8	busy:1;
-	u8	valid:1;
-};
-
 /**
  * pci_vpd_size - determine actual size of Vital Product Data
  * @dev:	pci device struct
@@ -350,8 +341,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
  */
 static int pci_vpd_wait(struct pci_dev *dev)
 {
-	struct pci_vpd_pci22 *vpd =
-		container_of(dev->vpd, struct pci_vpd_pci22, base);
+	struct pci_vpd *vpd = dev->vpd;
 	unsigned long timeout = jiffies + HZ/20 + 2;
 	u16 status;
 	int ret;
@@ -384,8 +374,7 @@ static int pci_vpd_wait(struct pci_dev *dev)
 static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 			    void *arg)
 {
-	struct pci_vpd_pci22 *vpd =
-		container_of(dev->vpd, struct pci_vpd_pci22, base);
+	struct pci_vpd *vpd = dev->vpd;
 	int ret;
 	loff_t end = pos + count;
 	u8 *buf = arg;
@@ -395,17 +384,17 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->base.len = pci_vpd_size(dev, vpd->base.len);
+		vpd->len = pci_vpd_size(dev, vpd->len);
 	}
 
-	if (vpd->base.len == 0)
+	if (vpd->len == 0)
 		return -EIO;
 
-	if (pos >= vpd->base.len)
+	if (pos > vpd->len)
 		return 0;
 
-	if (end > vpd->base.len) {
-		end = vpd->base.len;
+	if (end > vpd->len) {
+		end = vpd->len;
 		count = end - pos;
 	}
 
@@ -452,8 +441,7 @@ out:
 static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 			     const void *arg)
 {
-	struct pci_vpd_pci22 *vpd =
-		container_of(dev->vpd, struct pci_vpd_pci22, base);
+	struct pci_vpd *vpd = dev->vpd;
 	const u8 *buf = arg;
 	loff_t end = pos + count;
 	int ret = 0;
@@ -463,13 +451,13 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->base.len = pci_vpd_size(dev, vpd->base.len);
+		vpd->len = pci_vpd_size(dev, vpd->len);
 	}
 
-	if (vpd->base.len == 0)
+	if (vpd->len == 0)
 		return -EIO;
 
-	if (end > vpd->base.len)
+	if (end > vpd->len)
 		return -EINVAL;
 
 	if (mutex_lock_killable(&vpd->lock))
@@ -550,7 +538,7 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
 
 int pci_vpd_init(struct pci_dev *dev)
 {
-	struct pci_vpd_pci22 *vpd;
+	struct pci_vpd *vpd;
 	u8 cap;
 
 	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
@@ -561,23 +549,23 @@ int pci_vpd_init(struct pci_dev *dev)
 	if (!vpd)
 		return -ENOMEM;
 
-	vpd->base.len = PCI_VPD_MAX_SIZE;
+	vpd->len = PCI_VPD_MAX_SIZE;
 	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
-		vpd->base.ops = &pci_vpd_f0_ops;
+		vpd->ops = &pci_vpd_f0_ops;
 	else
-		vpd->base.ops = &pci_vpd_ops;
+		vpd->ops = &pci_vpd_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = 0;
 	vpd->valid = 0;
-	dev->vpd = &vpd->base;
+	dev->vpd = vpd;
 	return 0;
 }
 
 void pci_vpd_release(struct pci_dev *dev)
 {
 	if (dev->vpd)
-		kfree(container_of(dev->vpd, struct pci_vpd_pci22, base));
+		kfree(dev->vpd);
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6191703..d0fb934 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -100,9 +100,14 @@ struct pci_vpd_ops {
 };
 
 struct pci_vpd {
-	unsigned int len;
 	const struct pci_vpd_ops *ops;
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
+	struct mutex	lock;
+	unsigned int	len;
+	u16		flag;
+	u8		cap;
+	u8		busy:1;
+	u8		valid:1;
 };
 
 int pci_vpd_init(struct pci_dev *dev);

[PATCH v4 11/11] PCI: Sleep rather than busy-wait for VPD access completion

[Bjorn Helgaas]

Use usleep_range() instead of udelay() while waiting for a VPD access to
complete.  This is not a performance path, so no need to hog the CPU.

Rationale for usleep_range() parameters:

  We clear PCI_VPD_ADDR_F for a read (or set it for a write), then wait for
  the device to change it.  For a device that updates PCI_VPD_ADDR between
  our config write and subsequent config read, we won't sleep at all and
  can get the device's maximum rate.

  Sleeping a minimum of 10 usec per 4-byte access limits throughput to
  about 400Kbytes/second.  VPD is small (32K bytes at most), and most
  devices use only a fraction of that.

  We back off exponentially up to 1024 usec per iteration.  If we reach
  1024, we've already waited up to 1008 usec (16 + 32 + ...  + 512), so if
  we miss an update and wait an extra 1024 usec, we can still get about 1/2
  of the device's maximum rate.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index cae5462..68cad94 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -342,14 +342,15 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 static int pci_vpd_wait(struct pci_dev *dev)
 {
 	struct pci_vpd *vpd = dev->vpd;
-	unsigned long timeout = jiffies + HZ/20 + 2;
+	unsigned long timeout = jiffies + msecs_to_jiffies(50);
+	unsigned long max_sleep = 16;
 	u16 status;
 	int ret;
 
 	if (!vpd->busy)
 		return 0;
 
-	for (;;) {
+	while (time_before(jiffies, timeout)) {
 		ret = pci_user_read_config_word(dev, vpd->cap + PCI_VPD_ADDR,
 						&status);
 		if (ret < 0)
@@ -360,15 +361,16 @@ static int pci_vpd_wait(struct pci_dev *dev)
 			return 0;
 		}
 
-		if (time_after(jiffies, timeout)) {
-			dev_printk(KERN_DEBUG, &dev->dev, "vpd r/w failed.  This is likely a firmware bug on this device.  Contact the card vendor for a firmware update\n");
-			return -ETIMEDOUT;
-		}
 		if (fatal_signal_pending(current))
 			return -EINTR;
-		if (!cond_resched())
-			udelay(10);
+
+		usleep_range(10, max_sleep);
+		if (max_sleep < 1024)
+			max_sleep *= 2;
 	}
+
+	dev_warn(&dev->dev, "VPD access failed.  This is likely a firmware bug on this device.  Contact the card vendor for a firmware update\n");
+	return -ETIMEDOUT;
 }
 
 static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,

Re: [PATCH v4 03/11] PCI: Use bitfield instead of bool for struct pci_vpd_pci22.busy

[Hannes Reinecke]

On 02/23/2016 08:46 AM, Bjorn Helgaas wrote:
> Make struct pci_vpd_pci22.busy a 1-bit field instead of a bool.  We intend
> to add another flag, and two bitfields are cheaper than two bools.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 06/11] PCI: Move pci_read_vpd() and pci_write_vpd() close to other VPD code

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> pci_read_vpd() and pci_write_vpd() were stranded in the middle of config
> accessor functions.  Move them close to the other VPD code in the file.
> No functional change.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 07/11] PCI: Move pci_vpd_release() from header file to pci/access.c

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> Move pci_vpd_release() so it's next to the other VPD functions.  This puts
> it next to pci_vpd_pci22_init(), which allocates the space freed by
> pci_vpd_release().
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 08/11] PCI: Remove struct pci_vpd_ops.release function pointer

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> The struct pci_vpd_ops.release function pointer is always
> pci_vpd_pci22_release(), so there's no need for the flexibility of a
> function pointer.
> 
> Inline the pci_vpd_pci22_release() body into pci_vpd_release() and remove
> pci_vpd_pci22_release() and the struct pci_vpd_ops.release function
> pointer.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 09/11] PCI: Rename VPD symbols to remove unnecessary "pci22"

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> There's only one kind of VPD, so we don't need to qualify it as "the
> version described by PCI spec rev 2.2."
> 
> Rename the following symbols to remove unnecessary "pci22":
> 
>   PCI_VPD_PCI22_SIZE	-> PCI_VPD_MAX_SIZE
>   pci_vpd_pci22_size()	-> pci_vpd_size()
>   pci_vpd_pci22_wait()	-> pci_vpd_wait()
>   pci_vpd_pci22_read()	-> pci_vpd_read()
>   pci_vpd_pci22_write()	-> pci_vpd_write()
>   pci_vpd_pci22_ops	-> pci_vpd_ops
>   pci_vpd_pci22_init()	-> pci_vpd_init()
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 10/11] PCI: Fold struct pci_vpd_pci22 into struct pci_vpd

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> We only support one flavor of VPD, so there's no need to complicate things
> by having a "generic" struct pci_vpd and a more specific struct
> pci_vpd_pci22.
> 
> Fold struct pci_vpd_pci22 directly into struct pci_vpd.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 11/11] PCI: Sleep rather than busy-wait for VPD access completion

[Hannes Reinecke]

On 02/23/2016 08:47 AM, Bjorn Helgaas wrote:
> Use usleep_range() instead of udelay() while waiting for a VPD access to
> complete.  This is not a performance path, so no need to hog the CPU.
> 
> Rationale for usleep_range() parameters:
> 
>   We clear PCI_VPD_ADDR_F for a read (or set it for a write), then wait for
>   the device to change it.  For a device that updates PCI_VPD_ADDR between
>   our config write and subsequent config read, we won't sleep at all and
>   can get the device's maximum rate.
> 
>   Sleeping a minimum of 10 usec per 4-byte access limits throughput to
>   about 400Kbytes/second.  VPD is small (32K bytes at most), and most
>   devices use only a fraction of that.
> 
>   We back off exponentially up to 1024 usec per iteration.  If we reach
>   1024, we've already waited up to 1008 usec (16 + 32 + ...  + 512), so if
>   we miss an update and wait an extra 1024 usec, we can still get about 1/2
>   of the device's maximum rate.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 00/11] PCI VPD access fixes

[Bjorn Helgaas]

Hi Hannes,

Thanks for taking a look at the rest of these.

On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
> Hi Hannes,
> 
> This is a revision of your v3 series:
> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
> 
> Here's the description from your v3 posting:
> 
>   the current PCI VPD page access assumes that the entire possible VPD
>   data is readable. However, the spec only guarantees a VPD data up to
>   the 'end' marker, with everything beyond that being undefined.
>   This causes a system lockup on certain devices.
> 
>   With this patch we always set the VPD sysfs attribute size to '0', and
>   calculate the available VPD size on the first access.
>   If no valid data can be read an I/O error is returned.

Just to see if I have this right: the VPD file size in sysfs will
always appear as zero, regardless of whether it has been read or
written, right?  I don't think the user-visible size should change.

Bjorn

Re: [PATCH v4 00/11] PCI VPD access fixes

[Bjorn Helgaas]

On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
> Hi Hannes,
> 
> This is a revision of your v3 series:
> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
> 
> Here's the description from your v3 posting:
> 
>   the current PCI VPD page access assumes that the entire possible VPD
>   data is readable. However, the spec only guarantees a VPD data up to
>   the 'end' marker, with everything beyond that being undefined.
>   This causes a system lockup on certain devices.
> 
>   With this patch we always set the VPD sysfs attribute size to '0', and
>   calculate the available VPD size on the first access.
>   If no valid data can be read an I/O error is returned.
> 
>   I've also included the patch from Babu to blacklists devices which
>   are known to lockup when accessing the VPD data.
> 
> I tweaked a few things, mostly whitespace and printk changes.  The
> appended diff shows the changes I made.
> 
> I added some patches on top to clean up and simplify the VPD code.
> These shouldn't make any functional difference unless I've made a
> mistake.  I've built these, but I don't really have a way to test
> them.
> 
> I am still waiting for bugzilla links from Babu for the blacklist
> patch.
> 
> Bjorn
> 
> ---
> 
> Babu Moger (1):
>       FIXME need bugzilla link
> 
> Bjorn Helgaas (7):
>       PCI: Use bitfield instead of bool for struct pci_vpd_pci22.busy
>       PCI: Move pci_read_vpd() and pci_write_vpd() close to other VPD code
>       PCI: Move pci_vpd_release() from header file to pci/access.c
>       PCI: Remove struct pci_vpd_ops.release function pointer
>       PCI: Rename VPD symbols to remove unnecessary "pci22"
>       PCI: Fold struct pci_vpd_pci22 into struct pci_vpd
>       PCI: Sleep rather than busy-wait for VPD access completion
> 
> Hannes Reinecke (3):
>       PCI: Update VPD definitions
>       PCI: Allow access to VPD attributes with size 0
>       PCI: Determine actual VPD size on first access
> 
> 
>  drivers/pci/access.c    |  240 ++++++++++++++++++++++++++++++-----------------
>  drivers/pci/pci-sysfs.c |   22 +++-
>  drivers/pci/pci.h       |   16 ++-
>  drivers/pci/probe.c     |    2 
>  drivers/pci/quirks.c    |   29 ++++++
>  include/linux/pci.h     |   27 +++++
>  6 files changed, 231 insertions(+), 105 deletions(-)

I added Hannes' reviewed-by and applied these, with the exception of
"PCI: Prevent VPD access for buggy devices" (I'm waiting for bugzilla
links for those quirks), to pci/vpd for v4.6.

Bjorn

Re: [PATCH v4 00/11] PCI VPD access fixes

[Hannes Reinecke]

On 02/23/2016 10:07 PM, Bjorn Helgaas wrote:
> Hi Hannes,
> 
> Thanks for taking a look at the rest of these.
> 
> On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
>> Hi Hannes,
>>
>> This is a revision of your v3 series:
>> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
>>
>> Here's the description from your v3 posting:
>>
>>   the current PCI VPD page access assumes that the entire possible VPD
>>   data is readable. However, the spec only guarantees a VPD data up to
>>   the 'end' marker, with everything beyond that being undefined.
>>   This causes a system lockup on certain devices.
>>
>>   With this patch we always set the VPD sysfs attribute size to '0', and
>>   calculate the available VPD size on the first access.
>>   If no valid data can be read an I/O error is returned.
> 
> Just to see if I have this right: the VPD file size in sysfs will
> always appear as zero, regardless of whether it has been read or
> written, right?  I don't think the user-visible size should change.
> 
That is correct.
As the actual size is evaluated on the first access, we don't have it
available when creating the sysfs attribute itself.
And when using the nominal size of 32k some bright program might try to
jump to somewhere in the middle of the data, which will make calculating
the validity of this horribly complex.
Setting it to '0' is an easy way of avoiding this kinda games.

So yes, there will be a user-visible change, but it shouldn't affect the
programs accessing this attribute.
lspci works happily with these changes

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 00/11] PCI VPD access fixes

[Bjorn Helgaas]

On Wed, Feb 24, 2016 at 05:48:33AM +0800, Hannes Reinecke wrote:
> On 02/23/2016 10:07 PM, Bjorn Helgaas wrote:
> > Hi Hannes,
> > 
> > Thanks for taking a look at the rest of these.
> > 
> > On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
> >> Hi Hannes,
> >>
> >> This is a revision of your v3 series:
> >> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
> >>
> >> Here's the description from your v3 posting:
> >>
> >>   the current PCI VPD page access assumes that the entire possible VPD
> >>   data is readable. However, the spec only guarantees a VPD data up to
> >>   the 'end' marker, with everything beyond that being undefined.
> >>   This causes a system lockup on certain devices.
> >>
> >>   With this patch we always set the VPD sysfs attribute size to '0', and
> >>   calculate the available VPD size on the first access.
> >>   If no valid data can be read an I/O error is returned.
> > 
> > Just to see if I have this right: the VPD file size in sysfs will
> > always appear as zero, regardless of whether it has been read or
> > written, right?  I don't think the user-visible size should change.
> > 
> That is correct.
> As the actual size is evaluated on the first access, we don't have it
> available when creating the sysfs attribute itself.
> And when using the nominal size of 32k some bright program might try to
> jump to somewhere in the middle of the data, which will make calculating
> the validity of this horribly complex.
> Setting it to '0' is an easy way of avoiding this kinda games.
> 
> So yes, there will be a user-visible change, but it shouldn't affect the
> programs accessing this attribute.
> lspci works happily with these changes

What is the user-visible change?  Here's what I'm thinking.  If we do
this:

  ls -l /sys/.../vpd
  dd if=/sys/.../vpd bs=1 count=1
  ls -l /sys/.../vpd

Do we see different sizes from the two "ls" invocations?  My thought
is that we should see '0' both times, because I don't really think
that output should change depending on previous actions of this user
or other users.

I though you were confirming that we do always see '0', but then you
mentioned a user-visible change; is there a different change you're
thinking of?

Bjorn

Re: [PATCH v4 00/11] PCI VPD access fixes

[Hannes Reinecke]

On 02/24/2016 06:36 AM, Bjorn Helgaas wrote:
> On Wed, Feb 24, 2016 at 05:48:33AM +0800, Hannes Reinecke wrote:
>> On 02/23/2016 10:07 PM, Bjorn Helgaas wrote:
>>> Hi Hannes,
>>>
>>> Thanks for taking a look at the rest of these.
>>>
>>> On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
>>>> Hi Hannes,
>>>>
>>>> This is a revision of your v3 series:
>>>> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
>>>>
>>>> Here's the description from your v3 posting:
>>>>
>>>>   the current PCI VPD page access assumes that the entire possible VPD
>>>>   data is readable. However, the spec only guarantees a VPD data up to
>>>>   the 'end' marker, with everything beyond that being undefined.
>>>>   This causes a system lockup on certain devices.
>>>>
>>>>   With this patch we always set the VPD sysfs attribute size to '0', and
>>>>   calculate the available VPD size on the first access.
>>>>   If no valid data can be read an I/O error is returned.
>>>
>>> Just to see if I have this right: the VPD file size in sysfs will
>>> always appear as zero, regardless of whether it has been read or
>>> written, right?  I don't think the user-visible size should change.
>>>
>> That is correct.
>> As the actual size is evaluated on the first access, we don't have it
>> available when creating the sysfs attribute itself.
>> And when using the nominal size of 32k some bright program might try to
>> jump to somewhere in the middle of the data, which will make calculating
>> the validity of this horribly complex.
>> Setting it to '0' is an easy way of avoiding this kinda games.
>>
>> So yes, there will be a user-visible change, but it shouldn't affect the
>> programs accessing this attribute.
>> lspci works happily with these changes
> 
> What is the user-visible change?  Here's what I'm thinking.  If we do
> this:
> 
>   ls -l /sys/.../vpd
>   dd if=/sys/.../vpd bs=1 count=1
>   ls -l /sys/.../vpd
> 
> Do we see different sizes from the two "ls" invocations?  My thought
> is that we should see '0' both times, because I don't really think
> that output should change depending on previous actions of this user
> or other users.
> 
Originally we have:

# ls -l 0000:07:00.0/vpd
-rw------- 1 root root 32768 Feb 24 01:29 0000:07:00.0/vpd

and with this patchset we have:

# ls -l 0000:07:00.0/vpd
-rw------- 1 root root 0 Feb 24 01:29 0000:07:00.0/vpd

So only programs doing a 'stat' on the device node will see a difference.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v4 00/11] PCI VPD access fixes

[Bjorn Helgaas]

On Wed, Feb 24, 2016 at 08:30:38AM +0800, Hannes Reinecke wrote:
> On 02/24/2016 06:36 AM, Bjorn Helgaas wrote:
> > On Wed, Feb 24, 2016 at 05:48:33AM +0800, Hannes Reinecke wrote:
> >> On 02/23/2016 10:07 PM, Bjorn Helgaas wrote:
> >>> Hi Hannes,
> >>>
> >>> Thanks for taking a look at the rest of these.
> >>>
> >>> On Mon, Feb 22, 2016 at 06:46:23PM -0600, Bjorn Helgaas wrote:
> >>>> Hi Hannes,
> >>>>
> >>>> This is a revision of your v3 series:
> >>>> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
> >>>>
> >>>> Here's the description from your v3 posting:
> >>>>
> >>>>   the current PCI VPD page access assumes that the entire possible VPD
> >>>>   data is readable. However, the spec only guarantees a VPD data up to
> >>>>   the 'end' marker, with everything beyond that being undefined.
> >>>>   This causes a system lockup on certain devices.
> >>>>
> >>>>   With this patch we always set the VPD sysfs attribute size to '0', and
> >>>>   calculate the available VPD size on the first access.
> >>>>   If no valid data can be read an I/O error is returned.
> >>>
> >>> Just to see if I have this right: the VPD file size in sysfs will
> >>> always appear as zero, regardless of whether it has been read or
> >>> written, right?  I don't think the user-visible size should change.
> >>>
> >> That is correct.
> >> As the actual size is evaluated on the first access, we don't have it
> >> available when creating the sysfs attribute itself.
> >> And when using the nominal size of 32k some bright program might try to
> >> jump to somewhere in the middle of the data, which will make calculating
> >> the validity of this horribly complex.
> >> Setting it to '0' is an easy way of avoiding this kinda games.
> >>
> >> So yes, there will be a user-visible change, but it shouldn't affect the
> >> programs accessing this attribute.
> >> lspci works happily with these changes
> > 
> > What is the user-visible change?  Here's what I'm thinking.  If we do
> > this:
> > 
> >   ls -l /sys/.../vpd
> >   dd if=/sys/.../vpd bs=1 count=1
> >   ls -l /sys/.../vpd
> > 
> > Do we see different sizes from the two "ls" invocations?  My thought
> > is that we should see '0' both times, because I don't really think
> > that output should change depending on previous actions of this user
> > or other users.
> > 
> Originally we have:
> 
> # ls -l 0000:07:00.0/vpd
> -rw------- 1 root root 32768 Feb 24 01:29 0000:07:00.0/vpd
> 
> and with this patchset we have:
> 
> # ls -l 0000:07:00.0/vpd
> -rw------- 1 root root 0 Feb 24 01:29 0000:07:00.0/vpd
> 
> So only programs doing a 'stat' on the device node will see a difference.

Oh, I think I see: you mean there's a user-visible difference between
the current tree and the tree with your patches applied.

I was hoping that on a single kernel, the "vpd" attribute size was
always the same, regardless of whether anybody had read or written it.

If we always report zero size for all "vpd" files, I think that's OK.

Bjorn

RE: [PATCH v4 00/11] PCI VPD access fixes

[Seymour, Shane M]

UmV0ZXN0ZWQgdjQuIEZvciB0aGUgc2VyaWVzIHRlc3RlZCB0d28gUENJZSBjYXJkcyB0aGF0IHN1
cHBvcnQgVlBEOg0KDQowZDowMC4wIFNlcmlhbCBBdHRhY2hlZCBTQ1NJIGNvbnRyb2xsZXI6IExT
SSBMb2dpYyAvIFN5bWJpb3MgTG9naWMgU0FTMjMwOCBQQ0ktRXhwcmVzcyBGdXNpb24tTVBUIFNB
Uy0yIChyZXYgMDUpDQoxZTowMC4wIEZpYnJlIENoYW5uZWw6IFFMb2dpYyBDb3JwLiBJU1AyNDMy
LWJhc2VkIDRHYiBGaWJyZSBDaGFubmVsIHRvIFBDSSBFeHByZXNzIEhCQSAocmV2IDAzKQ0KDQpU
aGUgZmlyc3QgY2FyZCBleHBvc2VzIDMyS2lCIG9mIE5VTCBieXRlcyBhcyBWUEQgYW5kIHRoZSBz
ZWNvbmQgMTU0IGJ5dGVzIG9mIG5vbi1OVUwgZGF0YSByZXBlYXRpbmcgZXZlcnkgNEtpQiBmb3Ig
MzJLaUIgYmVmb3JlIHRoZSBjaGFuZ2UuIEFmdGVyIHRoZSBjaGFuZ2UgdGhlIGZpcnN0ICBjYXJk
IHJldHVybnMgbm8gZGF0YSBmb3IgVlBEIChzaW5jZSBpdCBjb250YWlucyBvbmx5IE5VTCBieXRl
cykgYW5kIHRoZSBzZWNvbmQgdGhlIGV4cGVjdGVkIDE1NCBieXRlcyBvZiBkYXRhLg0KDQpUaGUg
Zm9sbG93aW5nIHdhcm5pbmcgbWVzc2FnZSBjb21lcyBvdXQgYXMgZXhwZWN0ZWQgZm9yIHRoZSBM
U0kgY2FyZCAoY2hhbmdlZCBmcm9tIGR5biBkZWJ1ZyBjb250cm9sbGVkIG1lc3NhZ2UpOg0KDQpb
ICA2MTQuNDc1OTg0XSBtcHQzc2FzIDAwMDA6MGQ6MDAuMDogaW52YWxpZCBzaG9ydCBWUEQgdGFn
IDAwIGF0IG9mZnNldCAxDQoNCmxzcGNpIHdvcmtzIGNvcnJlY3RseSB3aXRoIHRoZSBwYXRjaCBp
biB0aGUga2VybmVsLiBUaGUgb25seSBjaGFuZ2Ugb24gdGhlIHN5c3RlbSAoYXBhcnQgZnJvbSB0
aGUgc2l6ZSBvZiB0aGUgdnBkIGZpbGUpIHdhcyB0aGF0IHdpdGggdGhlIExTSSBjYXJkIGxzcGNp
IHdpdGggLXZ2dnYgYXMgcm9vdCBpdCBub3cgc2F5cyAiTm90IHJlYWRhYmxlIiAgaW5zdGVhZCBv
ZiAiVW5rbm93biBzbWFsbCByZXNvdXJjZSB0eXBlIDAwLCB3aWxsIG5vdCBkZWNvZGUgbW9yZS4i
IGluIHRoZSBWUEQgc2VjdGlvbi4NCi0tLQ0KVGVzdGVkLWJ5OiBTaGFuZSBTZXltb3VyIDxzaGFu
ZS5zZXltb3VyQGhwZS5jb20+DQo=

Re: [PATCH v4 00/11] PCI VPD access fixes

[Babu Moger]

Hi Bjorn,

On 2/22/2016 6:46 PM, Bjorn Helgaas wrote:
> Hi Hannes,
> 
> This is a revision of your v3 series:
> http://lkml.kernel.org/r/1455525722-122040-1-git-send-email-hare@suse.de
> 
> Here's the description from your v3 posting:
> 
>   the current PCI VPD page access assumes that the entire possible VPD
>   data is readable. However, the spec only guarantees a VPD data up to
>   the 'end' marker, with everything beyond that being undefined.
>   This causes a system lockup on certain devices.
> 
>   With this patch we always set the VPD sysfs attribute size to '0', and
>   calculate the available VPD size on the first access.
>   If no valid data can be read an I/O error is returned.
> 
>   I've also included the patch from Babu to blacklists devices which
>   are known to lockup when accessing the VPD data.
> 
> I tweaked a few things, mostly whitespace and printk changes.  The
> appended diff shows the changes I made.
> 
> I added some patches on top to clean up and simplify the VPD code.
> These shouldn't make any functional difference unless I've made a
> mistake.  I've built these, but I don't really have a way to test
> them.
> 
> I am still waiting for bugzilla links from Babu for the blacklist
> patch.

 Sorry. I was on vacation. Just back in office today. Here is the 
 Bugzilla link.  https://bugzilla.kernel.org/show_bug.cgi?id=110681

     
> 
> Bjorn
> 
> ---
> 
> Babu Moger (1):
>       FIXME need bugzilla link
> 
> Bjorn Helgaas (7):
>       PCI: Use bitfield instead of bool for struct pci_vpd_pci22.busy
>       PCI: Move pci_read_vpd() and pci_write_vpd() close to other VPD code
>       PCI: Move pci_vpd_release() from header file to pci/access.c
>       PCI: Remove struct pci_vpd_ops.release function pointer
>       PCI: Rename VPD symbols to remove unnecessary "pci22"
>       PCI: Fold struct pci_vpd_pci22 into struct pci_vpd
>       PCI: Sleep rather than busy-wait for VPD access completion
> 
> Hannes Reinecke (3):
>       PCI: Update VPD definitions
>       PCI: Allow access to VPD attributes with size 0
>       PCI: Determine actual VPD size on first access
> 
> 
>  drivers/pci/access.c    |  240 ++++++++++++++++++++++++++++++-----------------
>  drivers/pci/pci-sysfs.c |   22 +++-
>  drivers/pci/pci.h       |   16 ++-
>  drivers/pci/probe.c     |    2 
>  drivers/pci/quirks.c    |   29 ++++++
>  include/linux/pci.h     |   27 +++++
>  6 files changed, 231 insertions(+), 105 deletions(-)
> 
> 
> Below are the changes I made to your v3 series:
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 8b6f5a2..4850f06 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -283,9 +283,9 @@ struct pci_vpd_pci22 {
>  	struct pci_vpd base;
>  	struct mutex lock;
>  	u16	flag;
> +	u8	cap;
>  	u8	busy:1;
>  	u8	valid:1;
> -	u8	cap;
>  };
>  
>  /**
> @@ -311,9 +311,9 @@ static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_size)
>  			    (tag == PCI_VPD_LTIN_RW_DATA)) {
>  				if (pci_read_vpd(dev, off+1, 2,
>  						 &header[1]) != 2) {
> -					dev_dbg(&dev->dev,
> -						"invalid large VPD tag %02x size at offset %zu",
> -						tag, off + 1);
> +					dev_warn(&dev->dev,
> +						 "invalid large VPD tag %02x size at offset %zu",
> +						 tag, off + 1);
>  					return 0;
>  				}
>  				off += PCI_VPD_LRDT_TAG_SIZE +
> @@ -325,15 +325,17 @@ static size_t pci_vpd_pci22_size(struct pci_dev *dev, size_t old_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);
> +			dev_warn(&dev->dev,
> +				 "invalid %s VPD tag %02x at offset %zu",
> +				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
> +				 tag, off);
>  			return 0;
>  		}
>  	}
> @@ -366,7 +368,7 @@ static int pci_vpd_pci22_wait(struct pci_dev *dev)
>  			return ret;
>  
>  		if ((status & PCI_VPD_ADDR_F) == vpd->flag) {
> -			vpd->busy = false;
> +			vpd->busy = 0;
>  			return 0;
>  		}
>  
> @@ -393,16 +395,18 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
>  	if (pos < 0)
>  		return -EINVAL;
>  
> -	if (!vpd->valid && vpd->base.len > 0) {
> -		vpd->valid = true;
> +	if (!vpd->valid) {
> +		vpd->valid = 1;
>  		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
>  	}
> +
>  	if (vpd->base.len == 0)
>  		return -EIO;
>  
> +	if (pos >= vpd->base.len)
> +		return 0;
> +
>  	if (end > vpd->base.len) {
> -		if (pos > vpd->base.len)
> -			return 0;
>  		end = vpd->base.len;
>  		count = end - pos;
>  	}
> @@ -422,7 +426,7 @@ static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
>  						 pos & ~3);
>  		if (ret < 0)
>  			break;
> -		vpd->busy = true;
> +		vpd->busy = 1;
>  		vpd->flag = PCI_VPD_ADDR_F;
>  		ret = pci_vpd_pci22_wait(dev);
>  		if (ret < 0)
> @@ -459,10 +463,11 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
>  	if (pos < 0 || (pos & 3) || (count & 3))
>  		return -EINVAL;
>  
> -	if (!vpd->valid && vpd->base.len > 0) {
> -		vpd->valid = true;
> +	if (!vpd->valid) {
> +		vpd->valid = 1;
>  		vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
>  	}
> +
>  	if (vpd->base.len == 0)
>  		return -EIO;
>  
> @@ -492,7 +497,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count
>  		if (ret < 0)
>  			break;
>  
> -		vpd->busy = true;
> +		vpd->busy = 1;
>  		vpd->flag = 0;
>  		ret = pci_vpd_pci22_wait(dev);
>  		if (ret < 0)
> @@ -572,8 +577,8 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>  		vpd->base.ops = &pci_vpd_pci22_ops;
>  	mutex_init(&vpd->lock);
>  	vpd->cap = cap;
> -	vpd->busy = false;
> -	vpd->valid = false;
> +	vpd->busy = 0;
> +	vpd->valid = 0;
>  	dev->vpd = &vpd->base;
>  	return 0;
>  }
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index df1178f..626c3b2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2135,45 +2135,31 @@ static void quirk_via_cx700_pci_parking_caching(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, 0x324e, quirk_via_cx700_pci_parking_caching);
>  
>  /*
> - * A read/write to sysfs entry ('/sys/bus/pci/devices/<id>/vpd')
> - * will dump 32k of data. The default length is set as 32768.
> - * Reading a full 32k will cause an access beyond the VPD end tag.
> - * The system behaviour at that point is mostly unpredictable.
> - * Apparently, some vendors have not implemented this VPD headers properly.
> - * Adding a generic function disable vpd data for these buggy adapters
> - * Add the DECLARE_PCI_FIXUP_FINAL line below with the specific with
> - * vendor and device of interest to use this quirk.
> + * If a device follows the VPD format spec, the PCI core will not read or
> + * write past the VPD End Tag.  But some vendors do not follow the VPD
> + * format spec, so we can't tell how much data is safe to access.  Devices
> + * may behave unpredictably if we access too much.  Blacklist these devices
> + * so we don't touch VPD at all.
>   */
>  static void quirk_blacklist_vpd(struct pci_dev *dev)
>  {
>  	if (dev->vpd) {
>  		dev->vpd->len = 0;
> -		dev_warn(&dev->dev, "PCI vpd access has been disabled due to firmware bug\n");
> +		dev_warn(&dev->dev, FW_BUG "VPD access disabled\n");
>  	}
>  }
>  
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d,
> -		quirk_blacklist_vpd);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f,
> -		quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0060, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x007c, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0413, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0078, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0079, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0073, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x0071, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005b, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x002f, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005d, quirk_blacklist_vpd);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LSI_LOGIC, 0x005f, quirk_blacklist_vpd);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, PCI_ANY_ID,
>  		quirk_blacklist_vpd);
>  
> 

Re: [PATCH v4 00/11] PCI VPD access fixes

[Babu Moger]

On 2/23/2016 10:52 PM, Seymour, Shane M wrote:
> Retested v4. For the series tested two PCIe cards that support VPD:
> 
> 0d:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
> 1e:00.0 Fibre Channel: QLogic Corp. ISP2432-based 4Gb Fibre Channel to PCI Express HBA (rev 03)
> 
> The first card exposes 32KiB of NUL bytes as VPD and the second 154 bytes of non-NUL data repeating every 4KiB for 32KiB before the change. After the change the first  card returns no data for VPD (since it contains only NUL bytes) and the second the expected 154 bytes of data.
> 
> The following warning message comes out as expected for the LSI card (changed from dyn debug controlled message):
> 
> [  614.475984] mpt3sas 0000:0d:00.0: invalid short VPD tag 00 at offset 1
> 
> lspci works correctly with the patch in the kernel. The only change on the system (apart from the size of the vpd file) was that with the LSI card lspci with -vvvv as root it now says "Not readable"  instead of "Unknown small resource type 00, will not decode more." in the VPD section.
> ---
> Tested-by: Shane Seymour <shane.seymour@hpe.com>
> 

I have also retested the V4 series. Did not see any side effects.
Once again here is the bug I opened for this issue.
https://bugzilla.kernel.org/show_bug.cgi?id=110681
---
Tested-by: Babu Moger <babu.moger@oracle.com>