| Message ID | 1452684335-46107-3-git-send-email-hare@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi Hannes, On Wed, Jan 13, 2016 at 12:25:33PM +0100, Hannes Reinecke wrote: > 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> > --- > 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 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; > + } I'm trying to figure out why we do any of this checking here. It seems like we could do all of it in pci_read_vpd(). Where is bin_attr->size set? I assume this is the "attr" allocated in pci_create_capabilities_sysfs(). I see that before your series, we set "attr->size = dev->vpd->len" there, but after patch 3/4 of your series, we set it to 0 there, and I don't see a place that sets it to anything else. > 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 > > -- > 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
On 02/09/2016 09:53 PM, Bjorn Helgaas wrote: > Hi Hannes, > > On Wed, Jan 13, 2016 at 12:25:33PM +0100, Hannes Reinecke wrote: >> 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> >> --- >> 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 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; >> + } > > I'm trying to figure out why we do any of this checking here. It > seems like we could do all of it in pci_read_vpd(). > > Where is bin_attr->size set? I assume this is the "attr" allocated in > pci_create_capabilities_sysfs(). I see that before your series, we > set "attr->size = dev->vpd->len" there, but after patch 3/4 of your > series, we set it to 0 there, and I don't see a place that sets it to > anything else. > That is entirely correct. One of the joys of sysfs binary attributes. In general, sysfs binary attributes are allowed to have a size of '0', but still can provide data when read. The size of '0' is just an indicator for "I don't know how much data I can provide, figure out yourself". (Cf fs/sysfs/file.c:sysfs_kf_bin_read() for details here) In our case we're not using that function, but rather our own pci_read_vpd(), which always exposes a size. So this patch just takes the same reasoning from sysfs_kf_bin_read(), and sets the size to '0', as initially we simply do _not_ know how much data will be provided. Cheers, Hannes
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); }
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> --- drivers/pci/pci-sysfs.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)