diff mbox

Create sysfs entries for PCI VPDI and VPDR tags

Message ID 1455825895-10951-1-git-send-email-Jordan_Hargrave@dell.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jordan_Hargrave@Dell.com Feb. 18, 2016, 8:04 p.m. UTC
The VPD-R is a readonly area of the PCI Vital Product Data region.
There are some standard keywords for serial number, manufacturer,
and vendor-specific values.  Dell Servers use a vendor-specific
tag to store number of ports and port mapping of partitioned NICs.

info = VPD-Info string
PN = Part Number
SN = Serial Number
MN = Manufacturer ID
Vx = Vendor-specific (x=0..9 A..Z)

This creates a sysfs subdirectory in the pci device: vpdattr with
'info', 'EC', 'SN', 'V0', etc. files containing the tag values.

Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
---
 drivers/pci/pci-sysfs.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)

Comments

Hannes Reinecke Feb. 19, 2016, 10 a.m. UTC | #1
On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
> The VPD-R is a readonly area of the PCI Vital Product Data region.
> There are some standard keywords for serial number, manufacturer,
> and vendor-specific values.  Dell Servers use a vendor-specific
> tag to store number of ports and port mapping of partitioned NICs.
> 
> info = VPD-Info string
> PN = Part Number
> SN = Serial Number
> MN = Manufacturer ID
> Vx = Vendor-specific (x=0..9 A..Z)
> 
> This creates a sysfs subdirectory in the pci device: vpdattr with
> 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Hmm. Can we first get an agreement on the PCI VPD parsing patches
I've posted earlier?
VPD parsing is really tricky, and we should aim on making the
read_vpd function robust enough before we begin putting things into
sysfs.

Also, I'm not utterly keen on this patchset.
The sysfs space is blown up with tiny pieces of information, which
can easily gotten via lspci, too.

Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
which case the entire sysfs attribute setup would be invalided.
How do you propose to handle that?

Cheers,

Hannes
jordan hargrave Feb. 19, 2016, 2:07 p.m. UTC | #2
On Fri, Feb 19, 2016 at 4:00 AM, Hannes Reinecke <hare@suse.de> wrote:
>
> On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
> > The VPD-R is a readonly area of the PCI Vital Product Data region.
> > There are some standard keywords for serial number, manufacturer,
> > and vendor-specific values.  Dell Servers use a vendor-specific
> > tag to store number of ports and port mapping of partitioned NICs.
> >
> > info = VPD-Info string
> > PN = Part Number
> > SN = Serial Number
> > MN = Manufacturer ID
> > Vx = Vendor-specific (x=0..9 A..Z)
> >
> > This creates a sysfs subdirectory in the pci device: vpdattr with
> > 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
> >
> > Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> Hmm. Can we first get an agreement on the PCI VPD parsing patches
> I've posted earlier?
> VPD parsing is really tricky, and we should aim on making the
> read_vpd function robust enough before we begin putting things into
> sysfs.
>
> Also, I'm not utterly keen on this patchset.
> The sysfs space is blown up with tiny pieces of information, which
> can easily gotten via lspci, too.
>
> Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
> which case the entire sysfs attribute setup would be invalided.
> How do you propose to handle that?
>

This patch only reads the attributes from VPD-I and VPD-R areas, not
the VPD-W (read write) area.
The VPD-W data is located after the VPD-I and VPD-R area  So nothing
in these attributes should change.

The main reason I want this is for replacing biosdevname (ethernet
naming) functionality and getting
the same functionality into the kernel and systemd.  Systemd doesn't
want to do vpd parsing, and
reading the vpd can take a very long time on some devices, causing
systemd to timeout.  Another
disadvantage of it being in userspace is for devices using SR-IOV.  In
those devices the vpd only
exists for the physfn devices but not the virtual devices.  A
userspace program device will have to
read the entire VPD for each physical and virtual PCI device.

Logic is something like this:
if (open("/sys/bus/pci/devices/X/physfn/vpd", O_RDONLY) < 0)
   if (open("/sys/bus/pci/devices/X/vpd", O_RDONLY) < 0)
     return;
}
parsevpd(fd);

Specifically it is parsing one of the Vx attributes for a 'DCM' or
'DC2' string that contain a mapping from
NIC ports and partitions to PCI device

> 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Reinecke Feb. 19, 2016, 2:18 p.m. UTC | #3
On 02/19/2016 03:07 PM, Jordan Hargrave wrote:
> On Fri, Feb 19, 2016 at 4:00 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
>>> The VPD-R is a readonly area of the PCI Vital Product Data region.
>>> There are some standard keywords for serial number, manufacturer,
>>> and vendor-specific values.  Dell Servers use a vendor-specific
>>> tag to store number of ports and port mapping of partitioned NICs.
>>>
>>> info = VPD-Info string
>>> PN = Part Number
>>> SN = Serial Number
>>> MN = Manufacturer ID
>>> Vx = Vendor-specific (x=0..9 A..Z)
>>>
>>> This creates a sysfs subdirectory in the pci device: vpdattr with
>>> 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
>>>
>>> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
>> Hmm. Can we first get an agreement on the PCI VPD parsing patches
>> I've posted earlier?
>> VPD parsing is really tricky, and we should aim on making the
>> read_vpd function robust enough before we begin putting things into
>> sysfs.
>>
>> Also, I'm not utterly keen on this patchset.
>> The sysfs space is blown up with tiny pieces of information, which
>> can easily gotten via lspci, too.
>>
>> Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
>> which case the entire sysfs attribute setup would be invalided.
>> How do you propose to handle that?
>>
> 
> This patch only reads the attributes from VPD-I and VPD-R areas, not
> the VPD-W (read write) area.
> The VPD-W data is located after the VPD-I and VPD-R area  So nothing
> in these attributes should change.
> 
Ah. Ok.

> The main reason I want this is for replacing biosdevname (ethernet
> naming) functionality and getting the same functionality into the
> kernel and systemd.  Systemd doesn't want to do vpd parsing, and
> reading the vpd can take a very long time on some devices, causing
> systemd to timeout.  Another disadvantage of it being in userspace
> is for devices using SR-IOV.  In those devices the vpd only
> exists for the physfn devices but not the virtual devices.  A
> userspace program device will have to read the entire VPD for
> each physical and virtual PCI device.
> 
> Logic is something like this:
> if (open("/sys/bus/pci/devices/X/physfn/vpd", O_RDONLY) < 0)
>    if (open("/sys/bus/pci/devices/X/vpd", O_RDONLY) < 0)
>      return;
> }
> parsevpd(fd);
> 
> Specifically it is parsing one of the Vx attributes for a 'DCM' or
> 'DC2' string that contain a mapping from
> NIC ports and partitions to PCI device
> 
Well, unfortunately you just gave a very good reason to _not_
include this into the kernel:

> reading the vpd can take a very long time on some devices, causing

If we were to put your patch in, we would need to read the VPD
_during each boot_, thereby slowing down the booting process noticeably.
Plus the additional risk of locking up during boot for misbehaving
PCI devices. Probably not something we should be doing.

I would rather have it delegated to some helper function/program
invoked from udev; with my latest patchset we always will have
well-behaved VPD information so it's easy to just read the vpd
attribute from sysfs.
There still might be a lag, but surely not so long as if to timeout
udev. And if we still encounter these devices I would mark them as
broken via the blacklist and skip VPD reading for those.

Cheers,

Hannes
Jordan_Hargrave@Dell.com Feb. 19, 2016, 7:44 p.m. UTC | #4
>On 02/19/2016 03:07 PM, Jordan Hargrave wrote:
>> On Fri, Feb 19, 2016 at 4:00 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>
>>> On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
>>>> The VPD-R is a readonly area of the PCI Vital Product Data region.
>>>> There are some standard keywords for serial number, manufacturer,
>>>> and vendor-specific values.  Dell Servers use a vendor-specific
>>>> tag to store number of ports and port mapping of partitioned NICs.
>>>>
>>>> info = VPD-Info string
>>>> PN = Part Number
>>>> SN = Serial Number
>>>> MN = Manufacturer ID
>>>> Vx = Vendor-specific (x=0..9 A..Z)
>>>>
>>>> This creates a sysfs subdirectory in the pci device: vpdattr with
>>>> 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
>>>>
>>>> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
>>> Hmm. Can we first get an agreement on the PCI VPD parsing patches
>>> I've posted earlier?
>>> VPD parsing is really tricky, and we should aim on making the
>>> read_vpd function robust enough before we begin putting things into
>>> sysfs.
>>>
>>> Also, I'm not utterly keen on this patchset.
>>> The sysfs space is blown up with tiny pieces of information, which
>>> can easily gotten via lspci, too.
>>>
>>> Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
>>> which case the entire sysfs attribute setup would be invalided.
>>> How do you propose to handle that?
>>>
>>
>> This patch only reads the attributes from VPD-I and VPD-R areas, not
>> the VPD-W (read write) area.
>> The VPD-W data is located after the VPD-I and VPD-R area  So nothing
>> in these attributes should change.
>>
>Ah. Ok.
>
>> The main reason I want this is for replacing biosdevname (ethernet
>> naming) functionality and getting the same functionality into the
>> kernel and systemd.  Systemd doesn't want to do vpd parsing, and
>> reading the vpd can take a very long time on some devices, causing
>> systemd to timeout.  Another disadvantage of it being in userspace
>> is for devices using SR-IOV.  In those devices the vpd only
>> exists for the physfn devices but not the virtual devices.  A
>> userspace program device will have to read the entire VPD for
>> each physical and virtual PCI device.
>>
>> Logic is something like this:
>> if (open("/sys/bus/pci/devices/X/physfn/vpd", O_RDONLY) < 0)
>>    if (open("/sys/bus/pci/devices/X/vpd", O_RDONLY) < 0)
>>      return;
>> }
>> parsevpd(fd);
>>
>> Specifically it is parsing one of the Vx attributes for a 'DCM' or
>> 'DC2' string that contain a mapping from
>> NIC ports and partitions to PCI device
>>
>Well, unfortunately you just gave a very good reason to _not_
>include this into the kernel:

The delay isn't a huge amount on any of the devices I've seen. The
Mellanox cards I have are the slowest.  Here's some timing tests I've done,
using this patch vs a readvpd utility.  I also compared it with lspci.

@@@ Read individual Broadcom
(time ./readvpd 0000:01:00.0 > /dev/null) &>>log
real	0m0.003s
user	0m0.002s
sys	0m0.002s

@@@ Read individual Mellanox
(time ./readvpd 0000:04:00.0 > /dev/null) &>>log
real	0m0.071s
user	0m0.001s
sys	0m0.070s

@@@ Read individual Broadcom using lspci
(time lspci -vvv -s 0000:01:00.0 > /dev/null) &>>log
real	0m0.036s
user	0m0.017s
sys	0m0.019s

@@@ Read individual Mellanox using lspci
(time lspci -vvv -s 0000:04:00.0 > /dev/null) &>>log
real	0m1.213s  <--- SLOW!!!!
user	0m0.012s
sys	0m1.201s

@@@ Read each network device with 'real' VPD.  This should be equivalent to the boot time delay, at least for network devices with VPD
(time for X in /sys/class/net/*/device ; do PF=$(readlink -f $X); SBDF=$(basename $PF) ; if [ -e $PF/vpdattr ] ; then echo ==== $X ; ./readvpd $SBDF > /dev/null; fi ; done) &>> log
==== /sys/class/net/eno1/device
==== /sys/class/net/eno2/device
==== /sys/class/net/eno3/device
==== /sys/class/net/eno4/device
==== /sys/class/net/eno5/device
==== /sys/class/net/eno6/device
==== /sys/class/net/enp4s0d1/device
==== /sys/class/net/enp4s0/device

real	0m0.319s
user	0m0.033s
sys	0m0.295s

@@@ Read each network device, including SR-IOV
(time for X in /sys/class/net/*/device ; do PF=$(readlink -f $X); if [ -e $PF/physfn ] ; then PF=$(readlink -f $PF/physfn) ; fi ; SBDF=$(basename $PF) ; if [ -e $PF/vpdattr ] ; then echo ==== $X ; ./readvpd $SBDF > /dev/null; fi ; done) &>> log
==== /sys/class/net/eno1/device
==== /sys/class/net/eno2/device
==== /sys/class/net/eno3/device
==== /sys/class/net/eno4/device
==== /sys/class/net/eno5/device
==== /sys/class/net/eno6/device
==== /sys/class/net/enp4s0d1/device
==== /sys/class/net/enp4s0/device
==== /sys/class/net/enp4s0f1d1/device (SR-IOV)
==== /sys/class/net/enp4s0f1/device (SR-IOV)
==== /sys/class/net/enp4s0f2d1/device (SR-IOV)
==== /sys/class/net/enp4s0f2/device (SR-IOV)
==== /sys/class/net/enp4s0f3d1/device (SR-IOV)
==== /sys/class/net/enp4s0f3/device (SR-IOV)
==== /sys/class/net/enp4s0f4d1/device (SR-IOV)
==== /sys/class/net/enp4s0f4/device (SR-IOV)
==== /sys/class/net/enp4s0f5d1/device (SR-IOV)
==== /sys/class/net/enp4s0f5/device (SR-IOV)
==== /sys/class/net/enp4s0f6d1/device (SR-IOV)
==== /sys/class/net/enp4s0f6/device (SR-IOV)
==== /sys/class/net/enp4s0f7d1/device (SR-IOV)
==== /sys/class/net/enp4s0f7/device (SR-IOV)
==== /sys/class/net/enp4s1d1/device (SR-IOV)
==== /sys/class/net/enp4s1/device (SR-IOV)

real	0m1.449s
user	0m0.047s
sys	0m1.412s

This is much slower as it has to re-read/parse the VPD data for each SR-IOV device

By contrast, here is using cached kernel entries (including virtual devices)
(time for X in /sys/class/net/*/device ; do PF=$(readlink -f $X); if [ -e $PF/physfn ] ; then PF=$(readlink -f $PF/physfn) ; fi ; if [ -e $PF/vpdattr ] ; then echo ==== $X ; cat $PF/vpdattr/* > /dev/null; fi ; done) &> log
==== /sys/class/net/eno1/device
==== /sys/class/net/eno2/device
==== /sys/class/net/eno3/device
==== /sys/class/net/eno4/device
==== /sys/class/net/eno5/device
==== /sys/class/net/eno6/device
==== /sys/class/net/enp4s0d1/device
==== /sys/class/net/enp4s0/device
==== /sys/class/net/enp4s0f1d1/device
==== /sys/class/net/enp4s0f1/device
==== /sys/class/net/enp4s0f2d1/device
==== /sys/class/net/enp4s0f2/device
==== /sys/class/net/enp4s0f3d1/device
==== /sys/class/net/enp4s0f3/device
==== /sys/class/net/enp4s0f4d1/device
==== /sys/class/net/enp4s0f4/device
==== /sys/class/net/enp4s0f5d1/device
==== /sys/class/net/enp4s0f5/device
==== /sys/class/net/enp4s0f6d1/device
==== /sys/class/net/enp4s0f6/device
==== /sys/class/net/enp4s0f7d1/device
==== /sys/class/net/enp4s0f7/device
==== /sys/class/net/enp4s1d1/device
==== /sys/class/net/enp4s1/device

real	0m0.212s
user	0m0.050s
sys	0m0.175s

>> reading the vpd can take a very long time on some devices, causing
>
>If we were to put your patch in, we would need to read the VPD
>_during each boot_, thereby slowing down the booting process noticeably.
>Plus the additional risk of locking up during boot for misbehaving
>PCI devices. Probably not something we should be doing.
>
>I would rather have it delegated to some helper function/program
>invoked from udev; with my latest patchset we always will have
>well-behaved VPD information so it's easy to just read the vpd
>attribute from sysfs.
>There still might be a lag, but surely not so long as if to timeout
>udev. And if we still encounter these devices I would mark them as
>broken via the blacklist and skip VPD reading for those.
>
>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)
>--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 10, 2016, 9:26 p.m. UTC | #5
On Fri, Feb 19, 2016 at 08:07:13AM -0600, Jordan Hargrave wrote:
> On Fri, Feb 19, 2016 at 4:00 AM, Hannes Reinecke <hare@suse.de> wrote:
> >
> > On 02/18/2016 09:04 PM, Jordan Hargrave wrote:
> > > The VPD-R is a readonly area of the PCI Vital Product Data region.
> > > There are some standard keywords for serial number, manufacturer,
> > > and vendor-specific values.  Dell Servers use a vendor-specific
> > > tag to store number of ports and port mapping of partitioned NICs.
> > >
> > > info = VPD-Info string
> > > PN = Part Number
> > > SN = Serial Number
> > > MN = Manufacturer ID
> > > Vx = Vendor-specific (x=0..9 A..Z)
> > >
> > > This creates a sysfs subdirectory in the pci device: vpdattr with
> > > 'info', 'EC', 'SN', 'V0', etc. files containing the tag values.
> > >
> > > Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> > Hmm. Can we first get an agreement on the PCI VPD parsing patches
> > I've posted earlier?
> > VPD parsing is really tricky, and we should aim on making the
> > read_vpd function robust enough before we begin putting things into
> > sysfs.
> >
> > Also, I'm not utterly keen on this patchset.
> > The sysfs space is blown up with tiny pieces of information, which
> > can easily gotten via lspci, too.
> >
> > Also, to my knowledge it's perfectly valid to _write_ to the VPD, in
> > which case the entire sysfs attribute setup would be invalided.
> > How do you propose to handle that?
> >
> 
> This patch only reads the attributes from VPD-I and VPD-R areas, not
> the VPD-W (read write) area.
> The VPD-W data is located after the VPD-I and VPD-R area  So nothing
> in these attributes should change.
> 
> The main reason I want this is for replacing biosdevname (ethernet
> naming) functionality and getting
> the same functionality into the kernel and systemd.  Systemd doesn't
> want to do vpd parsing, and
> reading the vpd can take a very long time on some devices, causing
> systemd to timeout.  

Your patch really does two things: (1) caches the VPDI and VPD-R data
data, and (2) adds sysfs entries to parse items out of that cached
data.  The speedup is from the caching, not the kernel parsing.

Maybe we should consider exporting the raw cached VPDI and VPD-R data
via sysfs and having user-space parse it.

> Another
> disadvantage of it being in userspace is for devices using SR-IOV.  In
> those devices the vpd only
> exists for the physfn devices but not the virtual devices.  A
> userspace program device will have to
> read the entire VPD for each physical and virtual PCI device.

What's the SR-IOV connection?  Is there something in the SR-IOV spec
that says VFs can't have VPD?

> Logic is something like this:
> if (open("/sys/bus/pci/devices/X/physfn/vpd", O_RDONLY) < 0)
>    if (open("/sys/bus/pci/devices/X/vpd", O_RDONLY) < 0)
>      return;
> }
> parsevpd(fd);
> 
> Specifically it is parsing one of the Vx attributes for a 'DCM' or
> 'DC2' string that contain a mapping from
> NIC ports and partitions to PCI device
> 
> > 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)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d750870..e5f3779 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1295,6 +1295,230 @@  static struct bin_attribute pcie_config_attr = {
 	.write = pci_write_config,
 };
 
+static umode_t vpd_attr_exist(struct kobject *kobj,
+			      struct attribute *attr, int n)
+{
+	struct device *dev;
+	struct pci_dev *pdev;
+	int i;
+
+	dev = container_of(kobj, struct device, kobj);
+	pdev = to_pci_dev(dev);
+
+	if (!strcmp(attr->name, "info"))
+		return pdev->vpdi_data ? S_IRUGO : 0;
+	if (pdev->vpdr_data == NULL)
+		return 0;
+	i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0,
+				      pdev->vpdr_len,
+				      attr->name);
+	return i >= 0 ? S_IRUGO : 0;
+}
+
+static ssize_t vpd_attr_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev;
+	int i, len;
+
+	pdev = to_pci_dev(dev);
+	if (!strcmp(attr->attr.name, "info")) {
+		if (pdev->vpdi_data == NULL)
+			return 0;
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+				 pdev->vpdi_data);
+	}
+	if (pdev->vpdr_data == NULL)
+		return 0;
+	i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0,
+				      pdev->vpdr_len,
+				      attr->attr.name);
+	if (i >= 0) {
+		len = pci_vpd_info_field_size(&pdev->vpdr_data[i]);
+		return scnprintf(buf, PAGE_SIZE, "%.*s\n", len,
+				 pdev->vpdr_data + i +
+				 PCI_VPD_INFO_FLD_HDR_SIZE);
+	}
+	return 0;
+}
+
+#define VPD_ATTR_RO(x) \
+static struct device_attribute vpd ## x = { \
+	.attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \
+	.show = vpd_attr_show \
+}
+VPD_ATTR_RO(info);
+VPD_ATTR_RO(PN);
+VPD_ATTR_RO(EC);
+VPD_ATTR_RO(MN);
+VPD_ATTR_RO(SN);
+VPD_ATTR_RO(V0);
+VPD_ATTR_RO(V1);
+VPD_ATTR_RO(V2);
+VPD_ATTR_RO(V3);
+VPD_ATTR_RO(V4);
+VPD_ATTR_RO(V5);
+VPD_ATTR_RO(V6);
+VPD_ATTR_RO(V7);
+VPD_ATTR_RO(V8);
+VPD_ATTR_RO(V9);
+VPD_ATTR_RO(VA);
+VPD_ATTR_RO(VB);
+VPD_ATTR_RO(VC);
+VPD_ATTR_RO(VD);
+VPD_ATTR_RO(VE);
+VPD_ATTR_RO(VF);
+VPD_ATTR_RO(VG);
+VPD_ATTR_RO(VH);
+VPD_ATTR_RO(VI);
+VPD_ATTR_RO(VJ);
+VPD_ATTR_RO(VK);
+VPD_ATTR_RO(VL);
+VPD_ATTR_RO(VM);
+VPD_ATTR_RO(VN);
+VPD_ATTR_RO(VO);
+VPD_ATTR_RO(VP);
+VPD_ATTR_RO(VQ);
+VPD_ATTR_RO(VR);
+VPD_ATTR_RO(VS);
+VPD_ATTR_RO(VT);
+VPD_ATTR_RO(VU);
+VPD_ATTR_RO(VV);
+VPD_ATTR_RO(VW);
+VPD_ATTR_RO(VX);
+VPD_ATTR_RO(VY);
+VPD_ATTR_RO(VZ);
+
+static struct attribute *vpd_attributes[] = {
+	&vpdinfo.attr,
+	&vpdPN.attr,
+	&vpdEC.attr,
+	&vpdMN.attr,
+	&vpdSN.attr,
+	&vpdV0.attr,
+	&vpdV1.attr,
+	&vpdV2.attr,
+	&vpdV3.attr,
+	&vpdV4.attr,
+	&vpdV5.attr,
+	&vpdV6.attr,
+	&vpdV7.attr,
+	&vpdV8.attr,
+	&vpdV9.attr,
+	&vpdVA.attr,
+	&vpdVB.attr,
+	&vpdVC.attr,
+	&vpdVD.attr,
+	&vpdVE.attr,
+	&vpdVF.attr,
+	&vpdVG.attr,
+	&vpdVH.attr,
+	&vpdVI.attr,
+	&vpdVJ.attr,
+	&vpdVK.attr,
+	&vpdVL.attr,
+	&vpdVM.attr,
+	&vpdVN.attr,
+	&vpdVO.attr,
+	&vpdVP.attr,
+	&vpdVQ.attr,
+	&vpdVR.attr,
+	&vpdVS.attr,
+	&vpdVT.attr,
+	&vpdVU.attr,
+	&vpdVV.attr,
+	&vpdVW.attr,
+	&vpdVX.attr,
+	&vpdVY.attr,
+	&vpdVZ.attr,
+	NULL,
+};
+
+static struct attribute_group vpd_attr_group = {
+	.name = "vpdattr",
+	.attrs = vpd_attributes,
+	.is_visible = vpd_attr_exist,
+};
+
+
+static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len)
+{
+	u8  tag[3];
+	int rc, tlen;
+
+	*len = 0;
+	rc = pci_read_vpd(dev, off, 1, tag);
+	if (rc != 1)
+		return -ENOENT;
+	/* Ignore invalid/end tags */
+	if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F)
+		return -ENOENT;
+	if (tag[0] & PCI_VPD_LRDT) {
+		/* Large tag */
+		rc = pci_read_vpd(dev, off+1, 2, tag+1);
+		if (rc != 2)
+			return -ENOENT;
+		tlen = pci_vpd_lrdt_size(tag) +
+			PCI_VPD_LRDT_TAG_SIZE;
+	} else {
+		/* Small tag (shouldn't happen in VPD...) */
+		tlen = pci_vpd_srdt_size(tag) +
+			PCI_VPD_SRDT_TAG_SIZE;
+		tag[0] &= ~PCI_VPD_SRDT_LEN_MASK;
+	}
+	/* Verify VPD tag fits in area */
+	if (tlen + off > dev->vpd->len)
+		return -ENOENT;
+	*len = tlen;
+	return tag[0];
+}
+
+static int pci_load_vpdr(struct pci_dev *dev)
+{
+	int rlen, ilen, tag, rc;
+
+	/* Check for VPD-I tag */
+	tag = pci_get_vpd_tag(dev, 0, &ilen);
+	if (tag != PCI_VPD_LRDT_ID_STRING)
+		return -ENOENT;
+
+	/* Read VPDI string */
+	ilen -= PCI_VPD_LRDT_TAG_SIZE;
+	dev->vpdi_data = kzalloc(ilen + 1, GFP_ATOMIC);
+	if (dev->vpdi_data == NULL)
+		return  -ENOMEM;
+	rc = pci_read_vpd(dev, PCI_VPD_LRDT_TAG_SIZE,
+			  ilen, dev->vpdi_data);
+	if (rc != ilen)
+		goto error;
+	ilen += PCI_VPD_LRDT_TAG_SIZE;
+
+	/* Check for VPD-R tag */
+	tag = pci_get_vpd_tag(dev, ilen, &rlen);
+	if (tag != PCI_VPD_LRDT_RO_DATA)
+		return -ENOENT;
+
+	/* Read VPDR area */
+	rlen -= PCI_VPD_LRDT_TAG_SIZE;
+	dev->vpdr_len = rlen;
+	dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC);
+	if (dev->vpdr_data == NULL)
+		return -ENOMEM;
+
+	rc = pci_read_vpd(dev, ilen + PCI_VPD_LRDT_TAG_SIZE,
+			  rlen, dev->vpdr_data);
+	if (rc != rlen)
+		goto error;
+	if (sysfs_create_group(&dev->dev.kobj, &vpd_attr_group))
+		goto error;
+	return 0;
+ error:
+	kfree(dev->vpdi_data);
+	kfree(dev->vpdr_data);
+	dev->vpdr_data = NULL;
+	return -ENOENT;
+}
+
 static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
@@ -1340,6 +1564,8 @@  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 			return retval;
 		}
 		dev->vpd->attr = attr;
+
+		pci_load_vpdr(dev);
 	}
 
 	/* Active State Power Management */
@@ -1356,6 +1582,13 @@  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 error:
 	pcie_aspm_remove_sysfs_dev_files(dev);
 	if (dev->vpd && dev->vpd->attr) {
+		if (dev->vpdr_data) {
+			sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group);
+			kfree(dev->vpdr_data);
+			dev->vpdr_data = NULL;
+		}
+		kfree(dev->vpdi_data);
+		dev->vpdi_data = NULL;
 		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
 		kfree(dev->vpd->attr);
 	}
@@ -1438,6 +1671,13 @@  err:
 static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 {
 	if (dev->vpd && dev->vpd->attr) {
+		if (dev->vpdr_data) {
+			sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group);
+			kfree(dev->vpdr_data);
+			dev->vpdr_data = NULL;
+		}
+		kfree(dev->vpdi_data);
+		dev->vpdi_data = NULL;
 		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
 		kfree(dev->vpd->attr);
 	}