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

[Alexander Duyck]

On Mon, Dec 28, 2015 at 9:29 PM,  <Jordan_Hargrave@xxxxxxxx> wrote:
>>On 12/18/2015 03:02 PM, Alexander Duyck wrote:
>>> On Fri, Dec 18, 2015 at 5:57 AM, Hannes Reinecke <hare@xxxxxxx> wrote:
>>>> On 12/18/2015 02:49 PM, Alexander Duyck wrote:
>>>>>
>>>>> On Fri, Dec 18, 2015 at 12:35 AM, Hannes Reinecke <hare@xxxxxxx> wrote:
>>>>>>
>>>>>> PCI-2.2 VPD entries have a maximum size of 32k, but might actually
>>>>>> be smaller than that. To figure out the actual size one has to read
>>>>>> the VPD area until the 'end marker' is reached.
>>>>>> Trying to read VPD data beyond that marker results in 'interesting'
>>>>>> effects, from simple read errors to crashing the card. And to make
>>>>>> matters worse not every PCI card implements this properly, leaving
>>>>>> us with no 'end' marker or even completely invalid data.
>>>>>> This path modifies the size of the VPD attribute to the available
>>>>>> size, or set it to '0' if no valid data could be read.
>>>>>
>>>>>
>>>>> This isn't what I had in mind.  There is no need to add an f0 version
>>>>> of the size function.  The size for all functions other than function
>>>>> 0 when the F0 flag is set is 0.  We aren't going to be reading their
>>>>> VPD, we only read the VPD region of function 0.
>>>>>
>>>> Ah. (I'm a bit confused about the proposed action for VPD other than
>>>> function 0).
>>>> So the idea here is to _disallow_ access to VPDs from functions other than
>>>> '0' unless these functions have different PCI IDs?
>>>
>>> If you take a look at the F0 functions what they do is bypass the VPD
>>> of the functions other than function 0.  As such setting the size to 0
>>> should really have no effect since the VPD of the function isn't
>>> actually read if the F0 flag is set.
>>>
>>Setting the size to '0' effectively inhibits you to read the VPD
>>data. So if we were to return '0' for PCI devices with the F0 bit
>>set we will never ever to be able to read (or write) _any_ VPD data
>>for that PCI device/function.
>>Which would be rendering all these F0 accessors pointless, and we
>>might as well remove them.
>>
>
> I had posted a patch recently to enable exposing the VPD-R valyes to sysfs.  I need access
> to these to parse into systemd for network naming (biosdevname style names).
>
>
> The VPD-R is a readonly area contained within the PCI Vital Product
> data.  There are some standard and vendor-specific keys stored in
> this region.
>
> PN = Part Number
> SN = Serial Number
> MN = Manufacturer ID
> Vx = Vendor-specific (x=0..9 A..Z)
>
> Biosdevname/Systemd will use these VPD keys for determining Network
> partitioning and port numbers for NIC cards

Can you please repost this as a patch instead of as a reply to our
thread about VPD size.  The fact is the subject is misleading as your
patch isn't actually related to VPD sizing.

> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@xxxxxxxx>
> ---
>  drivers/pci/pci-sysfs.c |  216 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |    2 +
>  2 files changed, 218 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index eead54c..4966ece 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1295,6 +1295,210 @@ static struct bin_attribute pcie_config_attr = {
>         .write = pci_write_config,
>  };
>
> +static umode_t vpd_attr_exist(struct kobject *kobj,
> +                             struct attribute *attr, int n)
> +{
> +       struct device *dev;
> +       struct pci_dev *pdev;
> +       const char *name;
> +       int i;
> +
> +       dev = container_of(kobj, struct device, kobj);
> +       pdev = to_pci_dev(dev);
> +
> +       name = attr->name;
> +       if (pdev->vpdr_data == NULL)
> +               return 0;
> +       i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0,
> +                                     pdev->vpdr_len, name);
> +       return (i >= 0 ? S_IRUGO : 0);
> +}
> +

So I assume there is another patch that implements
pci_vpd_find_info_keyword so that it can go through the vpdr_data and
parse it?

> +static ssize_t vpd_attr_show(struct device *dev,
> +                            struct device_attribute *attr, char *buf)
> +{
> +       struct pci_dev *pdev;
> +       const char *name;
> +       char kv_data[257] = { 0 };
> +       int i, len;
> +
> +       pdev = to_pci_dev(dev);
> +       name = attr->attr.name;
> +       if (pdev->vpdr_data == NULL)
> +               return 0;
> +       i = pci_vpd_find_info_keyword(pdev->vpdr_data, 0,
> +                                     pdev->vpdr_len, name);
> +       if (i >= 0) {
> +               len = pci_vpd_info_field_size(&pdev->vpdr_data[i]);
> +               memcpy(kv_data, pdev->vpdr_data + i +
> +                      PCI_VPD_INFO_FLD_HDR_SIZE, len);
> +               return scnprintf(buf, PAGE_SIZE, "%s\n", kv_data);
> +       }
> +       return 0;
> +}
> +
> +#define VPD_ATTR_RO(x) \
> +struct device_attribute vpd ## x = { \
> +       .attr = { .name = #x, .mode = S_IRUGO | S_IWUSR }, \
> +       .show = vpd_attr_show \
> +}
> +
> +VPD_ATTR_RO(PN);
> +VPD_ATTR_RO(EC);
> +VPD_ATTR_RO(MN);
> +VPD_ATTR_RO(SN);
> +VPD_ATTR_RO(V0);
> +VPD_ATTR_RO(V1);
> +VPD_ATTR_RO(V2);
> +VPD_ATTR_RO(V3);
> +VPD_ATTR_RO(V4);
> +VPD_ATTR_RO(V5);
> +VPD_ATTR_RO(V6);
> +VPD_ATTR_RO(V7);
> +VPD_ATTR_RO(V8);
> +VPD_ATTR_RO(V9);
> +VPD_ATTR_RO(VA);
> +VPD_ATTR_RO(VB);
> +VPD_ATTR_RO(VC);
> +VPD_ATTR_RO(VD);
> +VPD_ATTR_RO(VE);
> +VPD_ATTR_RO(VF);
> +VPD_ATTR_RO(VG);
> +VPD_ATTR_RO(VH);
> +VPD_ATTR_RO(VI);
> +VPD_ATTR_RO(VJ);
> +VPD_ATTR_RO(VK);
> +VPD_ATTR_RO(VL);
> +VPD_ATTR_RO(VM);
> +VPD_ATTR_RO(VN);
> +VPD_ATTR_RO(VO);
> +VPD_ATTR_RO(VP);
> +VPD_ATTR_RO(VQ);
> +VPD_ATTR_RO(VR);
> +VPD_ATTR_RO(VS);
> +VPD_ATTR_RO(VT);
> +VPD_ATTR_RO(VU);
> +VPD_ATTR_RO(VV);
> +VPD_ATTR_RO(VW);
> +VPD_ATTR_RO(VX);
> +VPD_ATTR_RO(VY);
> +VPD_ATTR_RO(VZ);
> +
> +static struct attribute *vpd_attributes[] = {
> +       &vpdPN.attr,
> +       &vpdEC.attr,
> +       &vpdMN.attr,
> +       &vpdSN.attr,
> +       &vpdV0.attr,
> +       &vpdV1.attr,
> +       &vpdV2.attr,
> +       &vpdV3.attr,
> +       &vpdV4.attr,
> +       &vpdV5.attr,
> +       &vpdV6.attr,
> +       &vpdV7.attr,
> +       &vpdV8.attr,
> +       &vpdV9.attr,
> +       &vpdVA.attr,
> +       &vpdVB.attr,
> +       &vpdVC.attr,
> +       &vpdVD.attr,
> +       &vpdVE.attr,
> +       &vpdVF.attr,
> +       &vpdVG.attr,
> +       &vpdVH.attr,
> +       &vpdVI.attr,
> +       &vpdVJ.attr,
> +       &vpdVK.attr,
> +       &vpdVL.attr,
> +       &vpdVM.attr,
> +       &vpdVN.attr,
> +       &vpdVO.attr,
> +       &vpdVP.attr,
> +       &vpdVQ.attr,
> +       &vpdVR.attr,
> +       &vpdVS.attr,
> +       &vpdVT.attr,
> +       &vpdVU.attr,
> +       &vpdVV.attr,
> +       &vpdVW.attr,
> +       &vpdVX.attr,
> +       &vpdVY.attr,
> +       &vpdVZ.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group vpd_attr_group = {
> +       .name = "vpdr",
> +       .attrs = vpd_attributes,
> +       .is_visible = vpd_attr_exist,
> +};
> +
> +
> +static int pci_get_vpd_tag(struct pci_dev *dev, int off, int *len)
> +{
> +       u8  tag[3];
> +       int rc, tlen;
> +
> +       *len = 0;
> +       /* Quirk Atheros cards, reading VPD hangs system for 20s */
> +       if (dev->vendor == PCI_VENDOR_ID_ATHEROS ||
> +           dev->vendor == PCI_VENDOR_ID_ATTANSIC)
> +               return -ENOENT;

I'm not really sure this is the right place for this type of quirk.
If this is really an issue maybe we should just disable VPD for these
devices.  Otherwise there isn't anything to stop someone from going in
and reading the VPD region via the existing VPD interfaces.

> +       rc = pci_read_vpd(dev, off, 1, tag);
> +       if (rc != 1)
> +               return -ENOENT;
> +       if (tag[0] == 0x00 || tag[0] == 0xFF || tag[0] == 0x7F)
> +               return -ENOENT;
> +       if (tag[0] & PCI_VPD_LRDT) {
> +               rc = pci_read_vpd(dev, off+1, 2, tag+1);
> +               if (rc != 2)
> +                       return -ENOENT;
> +               tlen = pci_vpd_lrdt_size(tag) +
> +                       PCI_VPD_LRDT_TAG_SIZE;
> +       } else {
> +               tlen = pci_vpd_srdt_size(tag) +
> +                       PCI_VPD_SRDT_TAG_SIZE;
> +               tag[0] &= ~PCI_VPD_SRDT_LEN_MASK;
> +       }
> +       /* Verify VPD tag fits in area */
> +       if (tlen + off > dev->vpd->len)
> +               return -ENOENT;
> +       *len = tlen;
> +       return tag[0];
> +}
> +
> +static int pci_load_vpdr(struct pci_dev *dev)
> +{
> +       int rlen, ilen, tag, rc;
> +
> +       /* Check for VPD-I and VPD-R tag */
> +       tag = pci_get_vpd_tag(dev, 0, &ilen);
> +       if (tag != PCI_VPD_LRDT_ID_STRING)
> +               return -ENOENT;
> +       tag = pci_get_vpd_tag(dev, ilen, &rlen);
> +       if (tag != PCI_VPD_LRDT_RO_DATA)
> +               return -ENOENT;
> +
> +       rlen -= PCI_VPD_LRDT_TAG_SIZE;
> +       dev->vpdr_len = rlen;
> +       dev->vpdr_data = kzalloc(rlen, GFP_ATOMIC);
> +       if (dev->vpdr_data == NULL)
> +               return -ENOMEM;
> +

Why not cache the ID string as well?  Seems like it might be a field
people would want to read on a regular basis in order to find out what
is there.

> +       rc = pci_read_vpd(dev, ilen + PCI_VPD_LRDT_TAG_SIZE,
> +                         rlen, dev->vpdr_data);
> +       if (rc != rlen)
> +               goto error;
> +       if (sysfs_create_group(&dev->dev.kobj, &vpd_attr_group))
> +               goto error;
> +       return 0;
> + error:
> +       kfree(dev->vpdr_data);
> +       dev->vpdr_len = 0;
> +       return -ENOENT;
> +}
> +

This bit here needs to reset vpdr_data back to NULL.  Otherwise you
could cause memory corruption via a double free in your two clean-up
routines called out below.

>  static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>                            const char *buf, size_t count)
>  {
> @@ -1340,6 +1544,8 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>                         return retval;
>                 }
>                 dev->vpd->attr = attr;
> +
> +               pci_load_vpdr(dev);
>         }
>
>         /* Active State Power Management */
> @@ -1356,6 +1562,11 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  error:
>         pcie_aspm_remove_sysfs_dev_files(dev);
>         if (dev->vpd && dev->vpd->attr) {
> +               if (dev->vpdr_data) {
> +                       sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group);
> +                       kfree(dev->vpdr_data);
> +                       dev->vpdr_data = NULL;
> +               }
>                 sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
>                 kfree(dev->vpd->attr);
>         }
> @@ -1438,6 +1649,11 @@ err:
>  static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>  {
>         if (dev->vpd && dev->vpd->attr) {
> +               if (dev->vpdr_data) {
> +                       sysfs_remove_group(&dev->dev.kobj, &vpd_attr_group);
> +                       kfree(dev->vpdr_data);
> +                       dev->vpdr_data = NULL;
> +               }
>                 sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
>                 kfree(dev->vpd->attr);
>         }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6ae25aa..699cf11 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -372,6 +372,8 @@ struct pci_dev {
>         const struct attribute_group **msi_irq_groups;
>  #endif
>         struct pci_vpd *vpd;
> +       int vpdr_len;
> +       u8 *vpdr_data;
>  #ifdef CONFIG_PCI_ATS
>         union {
>                 struct pci_sriov *sriov;        /* SR-IOV capability related */

Instead of placing vpdr_len outside of the data why not just leave it
in there?  It seems like you could then make use of some of the same
parsing logic regardless of if you are working with the cached copy or
the actual VPD.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/