PCI VPD access fixes
v3 cover PCI VPD access fixes
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.
Changes to v2:
- Spelling fixes as suggested by Bjorn Helgaas
- Update to correct vpd size in pci_vpc_pci22_write
- Pass in modified VPD size to pci_vpd_pci22_size() to make quirk_brcm_570x_limit_vpd work
Babu Moger (1):
pci: Blacklist vpd access for buggy devices
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 | 84 +++++++++++++++++++++++++++++++++++++++++++++++-- drivers/pci/pci-sysfs.c | 22 +++++++------ drivers/pci/quirks.c | 43 +++++++++++++++++++++++++ include/linux/pci.h | 27 ++++++++++++++-- 4 files changed, 162 insertions(+), 14 deletions(-)
Signature · 1 line
1.8.5.6
PATCH v3 1/4 pci: Update VPD definitions
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.
--- include/linux/pci.h | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
include/linux/pci.h +25 -2
@@ -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 *
Signature · 1 line
1.8.5.6
PATCH v3 2/4 pci: allow access to VPD attributes with size '0'
In reply to Hannes ReineckeIt 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'.
--- drivers/pci/pci-sysfs.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/pci/pci-sysfs.c +12 -8
@@ -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); }
Signature · 1 line
1.8.5.6
PATCH v3 3/4 pci: Determine actual VPD size on first access
In reply to Hannes ReineckePCI-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 patch tries 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'.
--- drivers/pci/access.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++-- drivers/pci/pci-sysfs.c | 2 +- 2 files changed, 82 insertions(+), 4 deletions(-)
drivers/pci/access.c +81 -3
@@ -283,10 +283,63 @@ struct pci_vpd_pci22 {struct pci_vpd base;struct mutex lock;u16 flag; -bool busy; + u8 busy:1; + u8 valid:1;u8 cap; }; +/** +* 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_dbg (&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_dbg (&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 +390,23 @@ 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 = true; + vpd->base.len =pci_vpd_pci22_size (dev, vpd->base.len); + } +if (vpd->base.len ==0 ) +return -EIO ; + +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 ;
@@ -389,7 +456,17 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t countloff_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 = true; + 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 +573,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)mutex_init (&vpd->lock); vpd->cap = cap; vpd->busy = false; + vpd->valid = false; dev->vpd = &vpd->base;return 0 ; }
drivers/pci/pci-sysfs.c +1 -1
@@ -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;
Signature · 1 line
1.8.5.6
PATCH v3 4/4 pci: Blacklist vpd access for buggy devices
In reply to Hannes ReineckeFrom: Babu Moger <[email protected]>
Reading or Writing of PCI VPD data causes system panic. We saw this problem by running "lspci -vvv" in the beginning. However this can be easily reproduced by running
cat /sys/bus/devices/XX../vpd
As even a simple read on any VPD data triggers a system lockup on certain cards this patch implements a PCI quirk to disabling VPD acces altogether.
This is done by setting the by setting the vpd length to '0' initially for those devices, which will then inhibit access completely..
--- drivers/pci/access.c | 4 ++-- drivers/pci/quirks.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-)
drivers/pci/access.c +2 -2
@@ -393,7 +393,7 @@ 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) { +if (!vpd->valid && vpd->base.len >0 ) { vpd->valid = true; vpd->base.len =pci_vpd_pci22_size (dev, vpd->base.len); }
@@ -459,7 +459,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t countif (pos <0 || (pos &3 ) || (count &3 ))return -EINVAL ; -if (!vpd->valid) { +if (!vpd->valid && vpd->base.len >0 ) { vpd->valid = true; vpd->base.len =pci_vpd_pci22_size (dev, vpd->base.len); }
drivers/pci/quirks.c +43 -0
49 lines not on this page · raw message
Signature · 1 line
1.8.5.6
Re: [PATCHv3 4/4] pci: Blacklist vpd access for buggy devices
Quoted 7 lines · Hannes Reinecke
From: Babu Moger <[email protected]>
Reading or Writing of PCI VPD data causes system panic. We saw this problem by running "lspci -vvv" in the beginning. However this can be easily reproduced by running
cat/sys/bus/devices/XX../vpd
Quoted 6 lines
As even a simple read on any VPD data triggers a system lockup on certain cards this patch implements a PCI quirk to disabling VPD acces altogether.
This is done by setting the by setting the vpd length to '0' initially for those devices, which will then inhibit access completely..
IIRC, Babu was going to open bugzillas with details about the actual falures. I'd like references to them here.
Quoted 23 lines
Cc Alexander Duyck <[email protected]>Cc Bjorn Helgaas <[email protected]>Signed-off-by Babu Moger <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/pci/access.c | 4 ++-- drivers/pci/quirks.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-)drivers/pci/access.c +1
-1@@ -393,7 +393,7 @@ 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) { +if (!vpd->valid && vpd->base.len >0 ) { vpd->valid = true; vpd->base.len =pci_vpd_pci22_size (dev, vpd->base.len); }
drivers/pci/access.c static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count · line 462
Quoted 5 lines
drivers/pci/access.c +1
-1@@ -459,7 +459,7 @@ static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t countif (pos <0 || (pos &3 ) || (count &3 ))return -EINVAL ; -if (!vpd->valid) { +if (!vpd->valid && vpd->base.len >0 ) {
I think these changes to pci_vpd_pci22_read() and pci_vpd_pci22_write() are unnecessary because pci_vpd_pci22_size() already returns 0 when you pass 0 to it.
Quoted 64 lines
drivers/pci/access.c +0
-0vpd->valid = true; vpd->base.len =pci_vpd_pci22_size (dev, vpd->base.len); }drivers/pci/quirks.c +43
-0@@ -2135,6 +2135,49 @@ 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. +*/ +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" ); + } +} + +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 Signature · 6 lines
1.8.5.6 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Using dyn debug also tested that this message comes out as expected for the LSI card (and it did):
[ 289.539523] 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 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.
---
Quoted 49 lines
-----Original Message-----
From: [email protected] [mailto:linux-pci- [email protected]] On Behalf Of Hannes Reinecke
Sent: Monday, February 15, 2016 7:42 PM
To: Bjorn Helgaas
Cc: Alexander Duyck; [email protected]; Babu Moger; Jordan Hargrave; Hannes Reinecke
Subject: [PATCHv3 0/4] PCI VPD access fixesthe 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.
Changes to v2:
- Spelling fixes as suggested by Bjorn Helgaas
- Update to correct vpd size inpci_vpc_pci22_write
- Pass in modified VPD size topci_vpd_pci22_size()to makequirk_brcm_570x_limit_vpdworkBabu Moger (1):
pci: Blacklist vpd access for buggy devicesHannes Reinecke (3):
pci: Update VPD definitions
pci: allow access to VPD attributes with size '0'
pci: Determine actual VPD size on first accessdrivers/pci/access.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++-- drivers/pci/pci-sysfs.c | 22 +++++++------ drivers/pci/quirks.c | 43 +++++++++++++++++++++++++ include/linux/pci.h | 27 ++++++++++++++-- 4 files changed, 162 insertions(+), 14 deletions(-)--
1.8.5.6--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html