[PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute

[PATCH 0/2] PCI/VPD: Improve handling binary sysfs attribute

[Heiner Kallweit]

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

[PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions

[Heiner Kallweit]

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

[PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute

[Heiner Kallweit]

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

Re: [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions

[Bjorn Helgaas]

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
> 
> 

Re: [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute

[Bjorn Helgaas]

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
> 
> 

Re: [PATCH 1/2] PCI/VPD: Remove dead code from sysfs access functions

[Heiner Kallweit]

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
>>
>>

Re: [PATCH 2/2] PCI/VPD: Improve handling binary sysfs attribute

[Heiner Kallweit]

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
>>
>>