* [PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute @ 2021-01-07 21:47 Heiner Kallweit 2021-01-07 21:48 ` [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions Heiner Kallweit 2021-01-07 21:48 ` [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit 0 siblings, 2 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-01-07 21:47 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci Since 104daa71b396 ("PCI: Determine actual VPD size on first access") there's nothing that keeps us from using a static attribute. This allows to significantly simplify the code. Heiner Kallweit (2): PCI/VPD: Remove dead code from sysfs access functions PCI/VPD: Improve handling binary sysfs attribute drivers/pci/vpd.c | 56 ++++++++++------------------------------------- 1 file changed, 11 insertions(+), 45 deletions(-) -- 2.30.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions 2021-01-07 21:47 [PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit @ 2021-01-07 21:48 ` Heiner Kallweit 2021-02-02 23:59 ` Bjorn Helgaas 2021-01-07 21:48 ` [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit 1 sibling, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2021-01-07 21:48 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci Since 104daa71b396 ("PCI: Determine actual VPD size on first access") attribute size is set to 0 (unlimited). So let's remove this now dead code. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/vpd.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index 7915d10f9..a3fd09105 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -403,13 +403,6 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, { struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); - 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); } @@ -419,13 +412,6 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, { struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); - 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); } -- 2.30.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions 2021-01-07 21:48 ` [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions Heiner Kallweit @ 2021-02-02 23:59 ` Bjorn Helgaas 2021-02-03 7:17 ` Heiner Kallweit 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2021-02-02 23:59 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Bjorn Helgaas, linux-pci On Thu, Jan 07, 2021 at 10:48:15PM +0100, Heiner Kallweit wrote: > Since 104daa71b396 ("PCI: Determine actual VPD size on first access") > attribute size is set to 0 (unlimited). So let's remove this now > dead code. Doesn't the 104daa71b396 commit log say "attr->size == 0" means the size is unknown (not unlimited)? But I don't think vpd.c does anything at all with attr->size other than set it to zero. Is there some reason we can't remove the "attr->size = 0" assignment, too? Maybe the sysfs attribute code uses it? But I don't see vpd.c doing anything that would contribute to that. Sorry, I'm just puzzled. > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/pci/vpd.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 7915d10f9..a3fd09105 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -403,13 +403,6 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, > { > struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); > > - 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); > } > > @@ -419,13 +412,6 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, > { > struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); > > - 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); > } > > -- > 2.30.0 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions 2021-02-02 23:59 ` Bjorn Helgaas @ 2021-02-03 7:17 ` Heiner Kallweit 0 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-02-03 7:17 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci On 03.02.2021 00:59, Bjorn Helgaas wrote: > On Thu, Jan 07, 2021 at 10:48:15PM +0100, Heiner Kallweit wrote: >> Since 104daa71b396 ("PCI: Determine actual VPD size on first access") >> attribute size is set to 0 (unlimited). So let's remove this now >> dead code. > > Doesn't the 104daa71b396 commit log say "attr->size == 0" means the > size is unknown (not unlimited)? > If attr->size == 0, then sysfs code will not check whether a read/write access is out of bounds. It expects that the respective driver returns an error or less than the requested data when reading beyond the actual size. And that's exactly what we do in the VPD code. > But I don't think vpd.c does anything at all with attr->size other > than set it to zero. Is there some reason we can't remove the > "attr->size = 0" assignment, too? > We could rely on zero-initialization of the allocated memory. But leaving this assignment makes clear that sysfs code doesn't have to check length restrictions. > Maybe the sysfs attribute code uses it? But I don't see vpd.c doing > anything that would contribute to that. > Yes, attr->size is used by sysfs code only. > Sorry, I'm just puzzled. > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/pci/vpd.c | 14 -------------- >> 1 file changed, 14 deletions(-) >> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >> index 7915d10f9..a3fd09105 100644 >> --- a/drivers/pci/vpd.c >> +++ b/drivers/pci/vpd.c >> @@ -403,13 +403,6 @@ static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, >> { >> struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); >> >> - 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); >> } >> >> @@ -419,13 +412,6 @@ static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, >> { >> struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); >> >> - 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); >> } >> >> -- >> 2.30.0 >> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute 2021-01-07 21:47 [PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit 2021-01-07 21:48 ` [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions Heiner Kallweit @ 2021-01-07 21:48 ` Heiner Kallweit 2021-02-03 0:09 ` Bjorn Helgaas 1 sibling, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2021-01-07 21:48 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci Since 104daa71b396 ("PCI: Determine actual VPD size on first access") there's nothing that keeps us from using a static attribute. This allows to significantly simplify the code. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/vpd.c | 42 +++++++++++------------------------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index a3fd09105..9ef8f400e 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -21,7 +21,6 @@ struct pci_vpd_ops { struct pci_vpd { const struct pci_vpd_ops *ops; - struct bin_attribute *attr; /* Descriptor for sysfs VPD entry */ struct mutex lock; unsigned int len; u16 flag; @@ -397,57 +396,38 @@ void pci_vpd_release(struct pci_dev *dev) kfree(dev->vpd); } -static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, - struct bin_attribute *bin_attr, char *buf, - loff_t off, size_t count) +static ssize_t vpd_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) { struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); return pci_read_vpd(dev, off, count, buf); } -static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, - struct bin_attribute *bin_attr, char *buf, - loff_t off, size_t count) +static ssize_t vpd_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) { struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); return pci_write_vpd(dev, off, count, buf); } +static const BIN_ATTR_RW(vpd, 0); + void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev) { - int retval; - struct bin_attribute *attr; - if (!dev->vpd) return; - attr = kzalloc(sizeof(*attr), GFP_ATOMIC); - if (!attr) - return; - - sysfs_bin_attr_init(attr); - attr->size = 0; - attr->attr.name = "vpd"; - attr->attr.mode = S_IRUSR | S_IWUSR; - attr->read = read_vpd_attr; - attr->write = write_vpd_attr; - retval = sysfs_create_bin_file(&dev->dev.kobj, attr); - if (retval) { - kfree(attr); - return; - } - - dev->vpd->attr = attr; + if (sysfs_create_bin_file(&dev->dev.kobj, &bin_attr_vpd)) + pci_warn(dev, "can't create VPD sysfs file\n"); } void pcie_vpd_remove_sysfs_dev_files(struct pci_dev *dev) { - if (dev->vpd && dev->vpd->attr) { - sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); - kfree(dev->vpd->attr); - } + sysfs_remove_bin_file(&dev->dev.kobj, &bin_attr_vpd); } int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt) -- 2.30.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute 2021-01-07 21:48 ` [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit @ 2021-02-03 0:09 ` Bjorn Helgaas 2021-02-03 8:03 ` Heiner Kallweit 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2021-02-03 0:09 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Bjorn Helgaas, linux-pci On Thu, Jan 07, 2021 at 10:48:55PM +0100, Heiner Kallweit wrote: > Since 104daa71b396 ("PCI: Determine actual VPD size on first access") > there's nothing that keeps us from using a static attribute. > This allows to significantly simplify the code. I love this! A few comments below. > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/pci/vpd.c | 42 +++++++++++------------------------------- > 1 file changed, 11 insertions(+), 31 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index a3fd09105..9ef8f400e 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -21,7 +21,6 @@ struct pci_vpd_ops { > > struct pci_vpd { > const struct pci_vpd_ops *ops; > - struct bin_attribute *attr; /* Descriptor for sysfs VPD entry */ > struct mutex lock; > unsigned int len; > u16 flag; > @@ -397,57 +396,38 @@ void pci_vpd_release(struct pci_dev *dev) > kfree(dev->vpd); > } > > -static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, > - struct bin_attribute *bin_attr, char *buf, > - loff_t off, size_t count) > +static ssize_t vpd_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t off, size_t count) > { > struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); > > return pci_read_vpd(dev, off, count, buf); > } > > -static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, > - struct bin_attribute *bin_attr, char *buf, > - loff_t off, size_t count) > +static ssize_t vpd_write(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t off, size_t count) > { > struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); > > return pci_write_vpd(dev, off, count, buf); > } > > +static const BIN_ATTR_RW(vpd, 0); Wow, I'm surprised that there are only 50ish uses of BIN_ATTR_*(): s390/crypto, w1/slaves/..., and a few other places. It always makes me wonder when we're one of a small number of callers. Seems OK though. > void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev) > { > - int retval; > - struct bin_attribute *attr; > - > if (!dev->vpd) > return; > > - attr = kzalloc(sizeof(*attr), GFP_ATOMIC); > - if (!attr) > - return; > - > - sysfs_bin_attr_init(attr); > - attr->size = 0; > - attr->attr.name = "vpd"; > - attr->attr.mode = S_IRUSR | S_IWUSR; > - attr->read = read_vpd_attr; > - attr->write = write_vpd_attr; > - retval = sysfs_create_bin_file(&dev->dev.kobj, attr); > - if (retval) { > - kfree(attr); > - return; > - } > - > - dev->vpd->attr = attr; Above is awesome. Also maybe confirms that we could remove the "attr->size = 0" assignment in the previous patch? > + if (sysfs_create_bin_file(&dev->dev.kobj, &bin_attr_vpd)) > + pci_warn(dev, "can't create VPD sysfs file\n"); Not sure we need this. Can't we use the .is_visible() thing and an attribute group to get rid of the explicit sysfs_create_bin_file()? > } > > void pcie_vpd_remove_sysfs_dev_files(struct pci_dev *dev) > { > - if (dev->vpd && dev->vpd->attr) { > - sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); > - kfree(dev->vpd->attr); > - } > + sysfs_remove_bin_file(&dev->dev.kobj, &bin_attr_vpd); > } > > int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt) > -- > 2.30.0 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute 2021-02-03 0:09 ` Bjorn Helgaas @ 2021-02-03 8:03 ` Heiner Kallweit 0 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2021-02-03 8:03 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci On 03.02.2021 01:09, Bjorn Helgaas wrote: > On Thu, Jan 07, 2021 at 10:48:55PM +0100, Heiner Kallweit wrote: >> Since 104daa71b396 ("PCI: Determine actual VPD size on first access") >> there's nothing that keeps us from using a static attribute. >> This allows to significantly simplify the code. > > I love this! A few comments below. > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/pci/vpd.c | 42 +++++++++++------------------------------- >> 1 file changed, 11 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >> index a3fd09105..9ef8f400e 100644 >> --- a/drivers/pci/vpd.c >> +++ b/drivers/pci/vpd.c >> @@ -21,7 +21,6 @@ struct pci_vpd_ops { >> >> struct pci_vpd { >> const struct pci_vpd_ops *ops; >> - struct bin_attribute *attr; /* Descriptor for sysfs VPD entry */ >> struct mutex lock; >> unsigned int len; >> u16 flag; >> @@ -397,57 +396,38 @@ void pci_vpd_release(struct pci_dev *dev) >> kfree(dev->vpd); >> } >> >> -static ssize_t read_vpd_attr(struct file *filp, struct kobject *kobj, >> - struct bin_attribute *bin_attr, char *buf, >> - loff_t off, size_t count) >> +static ssize_t vpd_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *bin_attr, char *buf, >> + loff_t off, size_t count) >> { >> struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); >> >> return pci_read_vpd(dev, off, count, buf); >> } >> >> -static ssize_t write_vpd_attr(struct file *filp, struct kobject *kobj, >> - struct bin_attribute *bin_attr, char *buf, >> - loff_t off, size_t count) >> +static ssize_t vpd_write(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *bin_attr, char *buf, >> + loff_t off, size_t count) >> { >> struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj)); >> >> return pci_write_vpd(dev, off, count, buf); >> } >> >> +static const BIN_ATTR_RW(vpd, 0); > > Wow, I'm surprised that there are only 50ish uses of BIN_ATTR_*(): > s390/crypto, w1/slaves/..., and a few other places. It always makes > me wonder when we're one of a small number of callers. Seems OK > though. > >> void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev) >> { >> - int retval; >> - struct bin_attribute *attr; >> - >> if (!dev->vpd) >> return; >> >> - attr = kzalloc(sizeof(*attr), GFP_ATOMIC); >> - if (!attr) >> - return; >> - >> - sysfs_bin_attr_init(attr); >> - attr->size = 0; >> - attr->attr.name = "vpd"; >> - attr->attr.mode = S_IRUSR | S_IWUSR; >> - attr->read = read_vpd_attr; >> - attr->write = write_vpd_attr; >> - retval = sysfs_create_bin_file(&dev->dev.kobj, attr); >> - if (retval) { >> - kfree(attr); >> - return; >> - } >> - >> - dev->vpd->attr = attr; > > Above is awesome. Also maybe confirms that we could remove the > "attr->size = 0" assignment in the previous patch? > Technically this assignment has never been needed, because the attribute memory is kzalloc'ed. But it makes clear to the reader that sysfs code isn't supposed to do any length checks. >> + if (sysfs_create_bin_file(&dev->dev.kobj, &bin_attr_vpd)) >> + pci_warn(dev, "can't create VPD sysfs file\n"); > > Not sure we need this. Can't we use the .is_visible() thing and an > attribute group to get rid of the explicit sysfs_create_bin_file()? > Nice. Let me do this, I'll send a v2 of the series. >> } >> >> void pcie_vpd_remove_sysfs_dev_files(struct pci_dev *dev) >> { >> - if (dev->vpd && dev->vpd->attr) { >> - sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr); >> - kfree(dev->vpd->attr); >> - } >> + sysfs_remove_bin_file(&dev->dev.kobj, &bin_attr_vpd); >> } >> >> int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt) >> -- >> 2.30.0 >> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-03 8:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-07 21:47 [PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit 2021-01-07 21:48 ` [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions Heiner Kallweit 2021-02-02 23:59 ` Bjorn Helgaas 2021-02-03 7:17 ` Heiner Kallweit 2021-01-07 21:48 ` [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute Heiner Kallweit 2021-02-03 0:09 ` Bjorn Helgaas 2021-02-03 8:03 ` Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).