[PATCH 0/3] PCI VPD access fixes

[PATCH 0/3] PCI VPD access fixes

[Hannes Reinecke]

Hi all,

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 calculate the actual VPD size, or set it to '0'
if no valid VPD data could be read.

Hannes Reinecke (3):
  pci: Update VPD definitions
  pci: Update VPD size with correct length
  pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set

 drivers/pci/access.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci-sysfs.c | 20 +++++++++------
 include/linux/pci.h     | 27 ++++++++++++++++++--
 3 files changed, 104 insertions(+), 11 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] 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 d86378c..5d61c36 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1836,12 +1836,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
@@ -1865,6 +1866,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
  *
@@ -1876,6 +1888,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/3] 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 the VPD attribute couldn't 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..ce1b0cb 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -475,6 +475,61 @@ 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) {
+					dev_dbg(&dev->dev,
+						"invalid large vpd tag %02x "
+						"size at offset %zu",
+						tag, off + 1);
+					break;
+				}
+				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 +552,8 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	vpd->cap = cap;
 	vpd->busy = false;
 	dev->vpd = &vpd->base;
+	if (!(dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0))
+		vpd->base.len = pci_vpd_pci22_size(dev);
 	return 0;
 }
 
-- 
1.8.5.6

[PATCH 3/3] pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set

[Hannes Reinecke]

If the VPD data is accessed via another PCI function than 0
the access is redirected to function 0. However, we shouldn't
calculate the actual size as we did for function 0, as there
is no guarantee that function 0 is already initialized and
VPD access is very costly and error prone.

So to avoid all this we set the size to '0' in sysfs, but only return
the valid bytes from the underlying function 0.

Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/pci/access.c    | 11 ++++++++++-
 drivers/pci/pci-sysfs.c | 20 ++++++++++++--------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ce1b0cb..9fe19be 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -337,9 +337,16 @@ 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 (end > vpd->base.len) {
+		if (pos > vpd->base.len)
+			return 0;
+		end = vpd->base.len;
+		count = end - pos;
+	}
+
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
@@ -554,6 +561,8 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	dev->vpd = &vpd->base;
 	if (!(dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0))
 		vpd->base.len = pci_vpd_pci22_size(dev);
+	else
+		vpd->base.len = 0;
 	return 0;
 }
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index eead54c..de327c3 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -772,10 +772,12 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj,
 	struct pci_dev *dev =
 		to_pci_dev(container_of(kobj, struct device, 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);
 }
@@ -787,10 +789,12 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj,
 	struct pci_dev *dev =
 		to_pci_dev(container_of(kobj, struct device, 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);
 }
-- 
1.8.5.6

Re: [PATCH 0/3] PCI VPD access fixes

[Babu Moger]

Hannes,
These patches cause some other issues. I still have to figure
out what is going on here. I have seen same problem before also with your
older patches. That is the reason I thought setting the vpd length to 0
is an easy solution.  Here are is what panic stack looks like. I am
looking at it now. Let me know if you see anything obvious here.

megasas: 06.807.10.00-rc1
megasas: Waiting for FW to come to ready state
megasas: FW now in Ready state
NON-RESUMABLE ERROR: Reporting on cpu 3
NON-RESUMABLE ERROR: TPC [0x0000000000730990] <msix_capability_init+0x250/0x360>
NON-RESUMABLE ERROR: RAW [0003100000000001:0006ccae1076de96:0000000202000080:ffffffffffffffff
NON-RESUMABLE ERROR:      0000000800030000:0000000100000000:0000000000000000:0000000000000000]
NON-RESUMABLE ERROR: handle [0x0003100000000001] stick [0x0006ccae1076de96]
NON-RESUMABLE ERROR: type [precise nonresumable]
NON-RESUMABLE ERROR: attrs [0x02000080] < ASI sp-faulted priv >
NON-RESUMABLE ERROR: raddr [0xffffffffffffffff]
NON-RESUMABLE ERROR: insn effective address [0x000008510079700c]
NON-RESUMABLE ERROR: size [0x8]
NON-RESUMABLE ERROR: asi [0x00]
CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
task: fff8000ff37bda00 ti: fff8000ff2940000 task.ti: fff8000ff2940000
TSTATE: 0000000011001607 TPC: 0000000000730990 TNPC: 0000000000730994 Y: 00000000    Tainted: G            E
TPC: <msix_capability_init+0x250/0x360>
g0: 0000000000000000 g1: fff8000ff33109c0 g2: 0000000000000000 g3: 000008510079700c
g4: fff8000ff37bda00 g5: fff8000ffe58a000 g6: fff8000ff2940000 g7: 0000085100797000
o0: fff8000ffd60d000 o1: 000000000000ffff o2: 000000000000ffff o3: 000000000111e678
o4: 000000000000001e o5: 0000000000000000 sp: fff8000ff2942711 ret_pc: 0000000000730940
RPC: <msix_capability_init+0x200/0x360>
l0: fff8000ffd60d888 l1: fff8000ff28123ec l2: fff8000ffd60d888 l3: 00000000000080d0
l4: 00000000010eace0 l5: 0000000000000001 l6: 0080000000000000 l7: 8000000000000000
i0: fff8000ffd60d000 i1: fff8000ff28123e4 i2: 0000000000000001 i3: 0000000000000000
i4: 0000000000000000 i5: 0000085100797000 i6: fff8000ff29427d1 i7: 0000000000730c08
I7: <pci_enable_msix+0x168/0x1c0>
Call Trace:
 [0000000000730c08] pci_enable_msix+0x168/0x1c0
 [0000000000730c80] pci_enable_msix_range+0x20/0x60
 [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
 [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
 [0000000000721e74] local_pci_probe+0x34/0xa0
 [0000000000721f88] pci_call_probe+0xa8/0xe0
 [0000000000722270] pci_device_probe+0x50/0x80
 [000000000079b660] really_probe+0x140/0x420
 [000000000079b984] driver_probe_device+0x44/0xa0
 [000000000079ba68] __driver_attach+0x88/0xa0
 [000000000079986c] bus_for_each_dev+0x6c/0xa0
 [000000000079b1fc] driver_attach+0x1c/0x40
 [000000000079a2bc] bus_add_driver+0x17c/0x220
 [000000000079c234] driver_register+0x74/0x120
 [000000000072235c] __pci_register_driver+0x3c/0x60
 [00000000100f00ac] megasas_init+0xac/0x1dc [megaraid_sas]
Kernel panic - not syncing: Non-resumable error.
CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
Call Trace:
 [00000000009c8314] panic+0xb4/0x248
 [0000000000429638] sun4v_nonresum_error+0xb8/0xe0
 [000000000040744c] sun4v_nonres_mondo+0xc8/0xd8
 [0000000000730990] msix_capability_init+0x250/0x360
 [0000000000730c08] pci_enable_msix+0x168/0x1c0
 [0000000000730c80] pci_enable_msix_range+0x20/0x60
 [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
 [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
 [0000000000721e74] local_pci_probe+0x34/0xa0
 [0000000000721f88] pci_call_probe+0xa8/0xe0
 [0000000000722270] pci_device_probe+0x50/0x80
 [000000000079b660] really_probe+0x140/0x420
 [000000000079b984] driver_probe_device+0x44/0xa0
 [000000000079ba68] __driver_attach+0x88/0xa0
 [000000000079986c] bus_for_each_dev+0x6c/0xa0
 [000000000079b1fc] driver_attach+0x1c/0x40
Press Stop-A (L1-A) to return to the boot prom
---[ end Kernel panic - not syncing: Non-resumable error.



On 1/12/2016 8:42 AM, Hannes Reinecke wrote:
> Hi all,
> 
> 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 calculate the actual VPD size, or set it to '0'
> if no valid VPD data could be read.
> 
> Hannes Reinecke (3):
>   pci: Update VPD definitions
>   pci: Update VPD size with correct length
>   pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set
> 
>  drivers/pci/access.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/pci/pci-sysfs.c | 20 +++++++++------
>  include/linux/pci.h     | 27 ++++++++++++++++++--
>  3 files changed, 104 insertions(+), 11 deletions(-)
> 

Re: [PATCH 0/3] PCI VPD access fixes

[Babu Moger]

On 1/12/2016 11:23 AM, Babu Moger wrote:
> Hannes,
> These patches cause some other issues. I still have to figure
> out what is going on here. I have seen same problem before also with your
> older patches. That is the reason I thought setting the vpd length to 0
> is an easy solution.  Here are is what panic stack looks like. I am
> looking at it now. Let me know if you see anything obvious here.
> 
> megasas: 06.807.10.00-rc1
> megasas: Waiting for FW to come to ready state
> megasas: FW now in Ready state
> NON-RESUMABLE ERROR: Reporting on cpu 3
> NON-RESUMABLE ERROR: TPC [0x0000000000730990] <msix_capability_init+0x250/0x360>
> NON-RESUMABLE ERROR: RAW [0003100000000001:0006ccae1076de96:0000000202000080:ffffffffffffffff
> NON-RESUMABLE ERROR:      0000000800030000:0000000100000000:0000000000000000:0000000000000000]
> NON-RESUMABLE ERROR: handle [0x0003100000000001] stick [0x0006ccae1076de96]
> NON-RESUMABLE ERROR: type [precise nonresumable]
> NON-RESUMABLE ERROR: attrs [0x02000080] < ASI sp-faulted priv >
> NON-RESUMABLE ERROR: raddr [0xffffffffffffffff]
> NON-RESUMABLE ERROR: insn effective address [0x000008510079700c]
> NON-RESUMABLE ERROR: size [0x8]
> NON-RESUMABLE ERROR: asi [0x00]
> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
> task: fff8000ff37bda00 ti: fff8000ff2940000 task.ti: fff8000ff2940000
> TSTATE: 0000000011001607 TPC: 0000000000730990 TNPC: 0000000000730994 Y: 00000000    Tainted: G            E
> TPC: <msix_capability_init+0x250/0x360>
> g0: 0000000000000000 g1: fff8000ff33109c0 g2: 0000000000000000 g3: 000008510079700c
> g4: fff8000ff37bda00 g5: fff8000ffe58a000 g6: fff8000ff2940000 g7: 0000085100797000
> o0: fff8000ffd60d000 o1: 000000000000ffff o2: 000000000000ffff o3: 000000000111e678
> o4: 000000000000001e o5: 0000000000000000 sp: fff8000ff2942711 ret_pc: 0000000000730940
> RPC: <msix_capability_init+0x200/0x360>
> l0: fff8000ffd60d888 l1: fff8000ff28123ec l2: fff8000ffd60d888 l3: 00000000000080d0
> l4: 00000000010eace0 l5: 0000000000000001 l6: 0080000000000000 l7: 8000000000000000
> i0: fff8000ffd60d000 i1: fff8000ff28123e4 i2: 0000000000000001 i3: 0000000000000000
> i4: 0000000000000000 i5: 0000085100797000 i6: fff8000ff29427d1 i7: 0000000000730c08
> I7: <pci_enable_msix+0x168/0x1c0>
> Call Trace:
>  [0000000000730c08] pci_enable_msix+0x168/0x1c0
>  [0000000000730c80] pci_enable_msix_range+0x20/0x60
>  [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>  [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>  [0000000000721e74] local_pci_probe+0x34/0xa0
>  [0000000000721f88] pci_call_probe+0xa8/0xe0
>  [0000000000722270] pci_device_probe+0x50/0x80
>  [000000000079b660] really_probe+0x140/0x420
>  [000000000079b984] driver_probe_device+0x44/0xa0
>  [000000000079ba68] __driver_attach+0x88/0xa0
>  [000000000079986c] bus_for_each_dev+0x6c/0xa0
>  [000000000079b1fc] driver_attach+0x1c/0x40
>  [000000000079a2bc] bus_add_driver+0x17c/0x220
>  [000000000079c234] driver_register+0x74/0x120
>  [000000000072235c] __pci_register_driver+0x3c/0x60
>  [00000000100f00ac] megasas_init+0xac/0x1dc [megaraid_sas]
> Kernel panic - not syncing: Non-resumable error.
> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
> Call Trace:
>  [00000000009c8314] panic+0xb4/0x248
>  [0000000000429638] sun4v_nonresum_error+0xb8/0xe0
>  [000000000040744c] sun4v_nonres_mondo+0xc8/0xd8
>  [0000000000730990] msix_capability_init+0x250/0x360
>  [0000000000730c08] pci_enable_msix+0x168/0x1c0
>  [0000000000730c80] pci_enable_msix_range+0x20/0x60
>  [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>  [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>  [0000000000721e74] local_pci_probe+0x34/0xa0
>  [0000000000721f88] pci_call_probe+0xa8/0xe0
>  [0000000000722270] pci_device_probe+0x50/0x80
>  [000000000079b660] really_probe+0x140/0x420
>  [000000000079b984] driver_probe_device+0x44/0xa0
>  [000000000079ba68] __driver_attach+0x88/0xa0
>  [000000000079986c] bus_for_each_dev+0x6c/0xa0
>  [000000000079b1fc] driver_attach+0x1c/0x40
> Press Stop-A (L1-A) to return to the boot prom
> ---[ end Kernel panic - not syncing: Non-resumable error.
> 
> 
> 
> On 1/12/2016 8:42 AM, Hannes Reinecke wrote:
>> Hi all,
>>
>> 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 calculate the actual VPD size, or set it to '0'
>> if no valid VPD data could be read.
>>
>> Hannes Reinecke (3):
>>   pci: Update VPD definitions
>>   pci: Update VPD size with correct length
>>   pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set
>>
>>  drivers/pci/access.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/pci/pci-sysfs.c | 20 +++++++++------
>>  include/linux/pci.h     | 27 ++++++++++++++++++--
>>  3 files changed, 104 insertions(+), 11 deletions(-)
>>

Hannes, I think the logic to to get the PCI vpd length seems fine. I see that 
the function pci_vpd_pci22_size returns zero seeing the invalid tag. That looks
good. However, this card(LSI Logic / Symbios Logic MegaRAID SAS 2108 controllers)
locks-up when we attempt to read pci vpd area. So when the driver tries to 
initialize the card it times out and panics eventually.

I tried to set up a new flag(PCI_DEV_FLAGS_VPD_ZERO) for this device using
 DECLARE_PCI_FIXUP_CLASS_EARLY.

Changes in pci_vpd_pci22_init

 if ((dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) ||
                (dev->dev_flags & PCI_DEV_FLAGS_VPD_ZERO))
                vpd->base.len = 0;
        else
                vpd->base.len = pci_vpd_pci22_size(dev);
        return 0;

Changes in driver/quirk.c 
static void quirk_set_vpd_sero(struct pci_dev *dev)
{
        dump_stack();
        dev->dev_flags |= PCI_DEV_FLAGS_VPD_ZERO;
}

DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
                PCI_CLASS_STORAGE_RAID, 8, quirk_set_vpd_sero);

Even this did not help. I don't know sequence of these flags setting. 
When we are inside the function pci_vpd_pci22_init none of these flags
are set up.  I need to look at these early quirks again. 

In summary we may have to blindly set the length to zero for this device.

Let me know your thoughts.

Re: [PATCH 0/3] PCI VPD access fixes

[Hannes Reinecke]

Hi Babu,

On 01/12/2016 11:15 PM, Babu Moger wrote:
>
>
> On 1/12/2016 11:23 AM, Babu Moger wrote:
>> Hannes,
>> These patches cause some other issues. I still have to figure
>> out what is going on here. I have seen same problem before also with your
>> older patches. That is the reason I thought setting the vpd length to 0
>> is an easy solution.  Here are is what panic stack looks like. I am
>> looking at it now. Let me know if you see anything obvious here.
>>
>> megasas: 06.807.10.00-rc1
>> megasas: Waiting for FW to come to ready state
>> megasas: FW now in Ready state
>> NON-RESUMABLE ERROR: Reporting on cpu 3
>> NON-RESUMABLE ERROR: TPC [0x0000000000730990] <msix_capability_init+0x250/0x360>
>> NON-RESUMABLE ERROR: RAW [0003100000000001:0006ccae1076de96:0000000202000080:ffffffffffffffff
>> NON-RESUMABLE ERROR:      0000000800030000:0000000100000000:0000000000000000:0000000000000000]
>> NON-RESUMABLE ERROR: handle [0x0003100000000001] stick [0x0006ccae1076de96]
>> NON-RESUMABLE ERROR: type [precise nonresumable]
>> NON-RESUMABLE ERROR: attrs [0x02000080] < ASI sp-faulted priv >
>> NON-RESUMABLE ERROR: raddr [0xffffffffffffffff]
>> NON-RESUMABLE ERROR: insn effective address [0x000008510079700c]
>> NON-RESUMABLE ERROR: size [0x8]
>> NON-RESUMABLE ERROR: asi [0x00]
>> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
>> task: fff8000ff37bda00 ti: fff8000ff2940000 task.ti: fff8000ff2940000
>> TSTATE: 0000000011001607 TPC: 0000000000730990 TNPC: 0000000000730994 Y: 00000000    Tainted: G            E
>> TPC: <msix_capability_init+0x250/0x360>
>> g0: 0000000000000000 g1: fff8000ff33109c0 g2: 0000000000000000 g3: 000008510079700c
>> g4: fff8000ff37bda00 g5: fff8000ffe58a000 g6: fff8000ff2940000 g7: 0000085100797000
>> o0: fff8000ffd60d000 o1: 000000000000ffff o2: 000000000000ffff o3: 000000000111e678
>> o4: 000000000000001e o5: 0000000000000000 sp: fff8000ff2942711 ret_pc: 0000000000730940
>> RPC: <msix_capability_init+0x200/0x360>
>> l0: fff8000ffd60d888 l1: fff8000ff28123ec l2: fff8000ffd60d888 l3: 00000000000080d0
>> l4: 00000000010eace0 l5: 0000000000000001 l6: 0080000000000000 l7: 8000000000000000
>> i0: fff8000ffd60d000 i1: fff8000ff28123e4 i2: 0000000000000001 i3: 0000000000000000
>> i4: 0000000000000000 i5: 0000085100797000 i6: fff8000ff29427d1 i7: 0000000000730c08
>> I7: <pci_enable_msix+0x168/0x1c0>
>> Call Trace:
>>   [0000000000730c08] pci_enable_msix+0x168/0x1c0
>>   [0000000000730c80] pci_enable_msix_range+0x20/0x60
>>   [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>>   [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>>   [0000000000721e74] local_pci_probe+0x34/0xa0
>>   [0000000000721f88] pci_call_probe+0xa8/0xe0
>>   [0000000000722270] pci_device_probe+0x50/0x80
>>   [000000000079b660] really_probe+0x140/0x420
>>   [000000000079b984] driver_probe_device+0x44/0xa0
>>   [000000000079ba68] __driver_attach+0x88/0xa0
>>   [000000000079986c] bus_for_each_dev+0x6c/0xa0
>>   [000000000079b1fc] driver_attach+0x1c/0x40
>>   [000000000079a2bc] bus_add_driver+0x17c/0x220
>>   [000000000079c234] driver_register+0x74/0x120
>>   [000000000072235c] __pci_register_driver+0x3c/0x60
>>   [00000000100f00ac] megasas_init+0xac/0x1dc [megaraid_sas]
>> Kernel panic - not syncing: Non-resumable error.
>> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
>> Call Trace:
>>   [00000000009c8314] panic+0xb4/0x248
>>   [0000000000429638] sun4v_nonresum_error+0xb8/0xe0
>>   [000000000040744c] sun4v_nonres_mondo+0xc8/0xd8
>>   [0000000000730990] msix_capability_init+0x250/0x360
>>   [0000000000730c08] pci_enable_msix+0x168/0x1c0
>>   [0000000000730c80] pci_enable_msix_range+0x20/0x60
>>   [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>>   [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>>   [0000000000721e74] local_pci_probe+0x34/0xa0
>>   [0000000000721f88] pci_call_probe+0xa8/0xe0
>>   [0000000000722270] pci_device_probe+0x50/0x80
>>   [000000000079b660] really_probe+0x140/0x420
>>   [000000000079b984] driver_probe_device+0x44/0xa0
>>   [000000000079ba68] __driver_attach+0x88/0xa0
>>   [000000000079986c] bus_for_each_dev+0x6c/0xa0
>>   [000000000079b1fc] driver_attach+0x1c/0x40
>> Press Stop-A (L1-A) to return to the boot prom
>> ---[ end Kernel panic - not syncing: Non-resumable error.
>>
>>
>>
>> On 1/12/2016 8:42 AM, Hannes Reinecke wrote:
>>> Hi all,
>>>
>>> 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 calculate the actual VPD size, or set it to '0'
>>> if no valid VPD data could be read.
>>>
>>> Hannes Reinecke (3):
>>>    pci: Update VPD definitions
>>>    pci: Update VPD size with correct length
>>>    pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set
>>>
>>>   drivers/pci/access.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   drivers/pci/pci-sysfs.c | 20 +++++++++------
>>>   include/linux/pci.h     | 27 ++++++++++++++++++--
>>>   3 files changed, 104 insertions(+), 11 deletions(-)
>>>
>
> Hannes, I think the logic to to get the PCI vpd length seems fine. I see that
> the function pci_vpd_pci22_size returns zero seeing the invalid tag. That looks
> good. However, this card(LSI Logic / Symbios Logic MegaRAID SAS 2108 controllers)
> locks-up when we attempt to read pci vpd area. So when the driver tries to
> initialize the card it times out and panics eventually.
>
WHAT? Which brain-dead engineer designed _that_?

> I tried to set up a new flag(PCI_DEV_FLAGS_VPD_ZERO) for this device using
>   DECLARE_PCI_FIXUP_CLASS_EARLY.
>
> Changes in pci_vpd_pci22_init
>
>   if ((dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) ||
>                  (dev->dev_flags & PCI_DEV_FLAGS_VPD_ZERO))
>                  vpd->base.len = 0;
>          else
>                  vpd->base.len = pci_vpd_pci22_size(dev);
>          return 0;
>
> Changes in driver/quirk.c
> static void quirk_set_vpd_sero(struct pci_dev *dev)
> {
>          dump_stack();
>          dev->dev_flags |= PCI_DEV_FLAGS_VPD_ZERO;
> }
>
> DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
>                  PCI_CLASS_STORAGE_RAID, 8, quirk_set_vpd_sero);
>
> Even this did not help. I don't know sequence of these flags setting.
> When we are inside the function pci_vpd_pci22_init none of these flags
> are set up.  I need to look at these early quirks again.
>
Yes, of course it didn't.
Thing is, setting the attribute size to '0' doesn't mean there is no 
data, it just means "we don't know how much data we have".
IE you can still read from that attribute (cf patch 3/3 in that 
series) and the mentioned stall is triggered.

Actually, I've been thinking about this, too; with my current 
patchset we don't make any difference between 'no data' (eg when 
reading past the end) and 'invalid data' (eg when the card returns 
garbage or worse)

So I guess I should modify my patch to return -EINVAL if the data is 
garbage, and integrate your patchset, too.

Let me see ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
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 0/3] PCI VPD access fixes

[Babu Moger]

Thanks Hannes. Patches look good. Tested with my LSI Logic / Symbios Logic MegaRAID SAS 2108 controller

+cc Jordan.
 
Jordan, If your card allows some access to vpd area, these patches might
work for you as well. Worst case, you might have to add your vendor/device ids
in the last(4/4) patch.
    
On 1/13/2016 2:20 AM, Hannes Reinecke wrote:
> Hi Babu,
> 
> On 01/12/2016 11:15 PM, Babu Moger wrote:
>>
>>
>> On 1/12/2016 11:23 AM, Babu Moger wrote:
>>> Hannes,
>>> These patches cause some other issues. I still have to figure
>>> out what is going on here. I have seen same problem before also with your
>>> older patches. That is the reason I thought setting the vpd length to 0
>>> is an easy solution.  Here are is what panic stack looks like. I am
>>> looking at it now. Let me know if you see anything obvious here.
>>>
>>> megasas: 06.807.10.00-rc1
>>> megasas: Waiting for FW to come to ready state
>>> megasas: FW now in Ready state
>>> NON-RESUMABLE ERROR: Reporting on cpu 3
>>> NON-RESUMABLE ERROR: TPC [0x0000000000730990] <msix_capability_init+0x250/0x360>
>>> NON-RESUMABLE ERROR: RAW [0003100000000001:0006ccae1076de96:0000000202000080:ffffffffffffffff
>>> NON-RESUMABLE ERROR:      0000000800030000:0000000100000000:0000000000000000:0000000000000000]
>>> NON-RESUMABLE ERROR: handle [0x0003100000000001] stick [0x0006ccae1076de96]
>>> NON-RESUMABLE ERROR: type [precise nonresumable]
>>> NON-RESUMABLE ERROR: attrs [0x02000080] < ASI sp-faulted priv >
>>> NON-RESUMABLE ERROR: raddr [0xffffffffffffffff]
>>> NON-RESUMABLE ERROR: insn effective address [0x000008510079700c]
>>> NON-RESUMABLE ERROR: size [0x8]
>>> NON-RESUMABLE ERROR: asi [0x00]
>>> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
>>> task: fff8000ff37bda00 ti: fff8000ff2940000 task.ti: fff8000ff2940000
>>> TSTATE: 0000000011001607 TPC: 0000000000730990 TNPC: 0000000000730994 Y: 00000000    Tainted: G            E
>>> TPC: <msix_capability_init+0x250/0x360>
>>> g0: 0000000000000000 g1: fff8000ff33109c0 g2: 0000000000000000 g3: 000008510079700c
>>> g4: fff8000ff37bda00 g5: fff8000ffe58a000 g6: fff8000ff2940000 g7: 0000085100797000
>>> o0: fff8000ffd60d000 o1: 000000000000ffff o2: 000000000000ffff o3: 000000000111e678
>>> o4: 000000000000001e o5: 0000000000000000 sp: fff8000ff2942711 ret_pc: 0000000000730940
>>> RPC: <msix_capability_init+0x200/0x360>
>>> l0: fff8000ffd60d888 l1: fff8000ff28123ec l2: fff8000ffd60d888 l3: 00000000000080d0
>>> l4: 00000000010eace0 l5: 0000000000000001 l6: 0080000000000000 l7: 8000000000000000
>>> i0: fff8000ffd60d000 i1: fff8000ff28123e4 i2: 0000000000000001 i3: 0000000000000000
>>> i4: 0000000000000000 i5: 0000085100797000 i6: fff8000ff29427d1 i7: 0000000000730c08
>>> I7: <pci_enable_msix+0x168/0x1c0>
>>> Call Trace:
>>>   [0000000000730c08] pci_enable_msix+0x168/0x1c0
>>>   [0000000000730c80] pci_enable_msix_range+0x20/0x60
>>>   [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>>>   [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>>>   [0000000000721e74] local_pci_probe+0x34/0xa0
>>>   [0000000000721f88] pci_call_probe+0xa8/0xe0
>>>   [0000000000722270] pci_device_probe+0x50/0x80
>>>   [000000000079b660] really_probe+0x140/0x420
>>>   [000000000079b984] driver_probe_device+0x44/0xa0
>>>   [000000000079ba68] __driver_attach+0x88/0xa0
>>>   [000000000079986c] bus_for_each_dev+0x6c/0xa0
>>>   [000000000079b1fc] driver_attach+0x1c/0x40
>>>   [000000000079a2bc] bus_add_driver+0x17c/0x220
>>>   [000000000079c234] driver_register+0x74/0x120
>>>   [000000000072235c] __pci_register_driver+0x3c/0x60
>>>   [00000000100f00ac] megasas_init+0xac/0x1dc [megaraid_sas]
>>> Kernel panic - not syncing: Non-resumable error.
>>> CPU: 3 PID: 325 Comm: modprobe Tainted: G            E   4.1.12-uek4-bm #5
>>> Call Trace:
>>>   [00000000009c8314] panic+0xb4/0x248
>>>   [0000000000429638] sun4v_nonresum_error+0xb8/0xe0
>>>   [000000000040744c] sun4v_nonres_mondo+0xc8/0xd8
>>>   [0000000000730990] msix_capability_init+0x250/0x360
>>>   [0000000000730c08] pci_enable_msix+0x168/0x1c0
>>>   [0000000000730c80] pci_enable_msix_range+0x20/0x60
>>>   [00000000100d7554] megasas_init_fw+0x374/0xb40 [megaraid_sas]
>>>   [00000000100d8c80] megasas_probe_one+0x4a0/0x9a0 [megaraid_sas]
>>>   [0000000000721e74] local_pci_probe+0x34/0xa0
>>>   [0000000000721f88] pci_call_probe+0xa8/0xe0
>>>   [0000000000722270] pci_device_probe+0x50/0x80
>>>   [000000000079b660] really_probe+0x140/0x420
>>>   [000000000079b984] driver_probe_device+0x44/0xa0
>>>   [000000000079ba68] __driver_attach+0x88/0xa0
>>>   [000000000079986c] bus_for_each_dev+0x6c/0xa0
>>>   [000000000079b1fc] driver_attach+0x1c/0x40
>>> Press Stop-A (L1-A) to return to the boot prom
>>> ---[ end Kernel panic - not syncing: Non-resumable error.
>>>
>>>
>>>
>>> On 1/12/2016 8:42 AM, Hannes Reinecke wrote:
>>>> Hi all,
>>>>
>>>> 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 calculate the actual VPD size, or set it to '0'
>>>> if no valid VPD data could be read.
>>>>
>>>> Hannes Reinecke (3):
>>>>    pci: Update VPD definitions
>>>>    pci: Update VPD size with correct length
>>>>    pci: set VPD size to '0' if PCI_VPD_FLAGS_VPD_REF_F0 is set
>>>>
>>>>   drivers/pci/access.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   drivers/pci/pci-sysfs.c | 20 +++++++++------
>>>>   include/linux/pci.h     | 27 ++++++++++++++++++--
>>>>   3 files changed, 104 insertions(+), 11 deletions(-)
>>>>
>>
>> Hannes, I think the logic to to get the PCI vpd length seems fine. I see that
>> the function pci_vpd_pci22_size returns zero seeing the invalid tag. That looks
>> good. However, this card(LSI Logic / Symbios Logic MegaRAID SAS 2108 controllers)
>> locks-up when we attempt to read pci vpd area. So when the driver tries to
>> initialize the card it times out and panics eventually.
>>
> WHAT? Which brain-dead engineer designed _that_?
> 
>> I tried to set up a new flag(PCI_DEV_FLAGS_VPD_ZERO) for this device using
>>   DECLARE_PCI_FIXUP_CLASS_EARLY.
>>
>> Changes in pci_vpd_pci22_init
>>
>>   if ((dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) ||
>>                  (dev->dev_flags & PCI_DEV_FLAGS_VPD_ZERO))
>>                  vpd->base.len = 0;
>>          else
>>                  vpd->base.len = pci_vpd_pci22_size(dev);
>>          return 0;
>>
>> Changes in driver/quirk.c
>> static void quirk_set_vpd_sero(struct pci_dev *dev)
>> {
>>          dump_stack();
>>          dev->dev_flags |= PCI_DEV_FLAGS_VPD_ZERO;
>> }
>>
>> DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_LSI_LOGIC, 0x0060,
>>                  PCI_CLASS_STORAGE_RAID, 8, quirk_set_vpd_sero);
>>
>> Even this did not help. I don't know sequence of these flags setting.
>> When we are inside the function pci_vpd_pci22_init none of these flags
>> are set up.  I need to look at these early quirks again.
>>
> Yes, of course it didn't.
> Thing is, setting the attribute size to '0' doesn't mean there is no data, it just means "we don't know how much data we have".
> IE you can still read from that attribute (cf patch 3/3 in that series) and the mentioned stall is triggered.
> 
> Actually, I've been thinking about this, too; with my current patchset we don't make any difference between 'no data' (eg when reading past the end) and 'invalid data' (eg when the card returns garbage or worse)
> 
> So I guess I should modify my patch to return -EINVAL if the data is garbage, and integrate your patchset, too.
> 
> Let me see ...
> 
> Cheers,
> 
> Hannes