| Message ID | f527283c-3e4e-5574-e917-519a6cc0e8b9@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/VPD: Improve handling binary sysfs attribute | expand |
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 > >
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 >> >>
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)
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(-)