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

[PATCH v2 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.

v2:
- switch to using PCI sysfs core code in patch 2

Heiner Kallweit (2):
  PCI/VPD: Remove dead code from sysfs access functions
  PCI/VPD: Let PCI sysfs core code handle VPD binary attribute

 drivers/pci/pci-sysfs.c | 54 +++++++++++++++++++++++---------
 drivers/pci/pci.h       |  2 --
 drivers/pci/vpd.c       | 68 -----------------------------------------
 3 files changed, 40 insertions(+), 84 deletions(-)

-- 
2.30.0

[PATCH v2 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>
---
v2:
- no changes
---
 drivers/pci/vpd.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 35559ead2..2096530ce 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 v2 1/2] PCI/VPD: Remove dead code from sysfs access functions

[Krzysztof Wilczyński]

Hi Heiner,

Thank you for cleaning this up!

On 21-02-03 09:48:03, 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.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - no changes
> ---
>  drivers/pci/vpd.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 35559ead2..2096530ce 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
> 

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof

[PATCH v2 2/2] PCI/VPD: Let PCI sysfs core code handle VPD binary 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 use PCI sysfs core code for handling the attribute.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/pci-sysfs.c | 54 ++++++++++++++++++++++++++++++-----------
 drivers/pci/pci.h       |  2 --
 drivers/pci/vpd.c       | 54 -----------------------------------------
 3 files changed, 40 insertions(+), 70 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fb072f4b3..ed2ded167 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -255,6 +255,25 @@ static ssize_t ari_enabled_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(ari_enabled);
 
+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 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 BIN_ATTR_RW(vpd, 0);
+
 static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
@@ -621,6 +640,11 @@ static struct attribute *pci_dev_attrs[] = {
 	NULL,
 };
 
+static struct bin_attribute *pci_dev_bin_attrs[] = {
+	&bin_attr_vpd,
+	NULL,
+};
+
 static struct attribute *pci_bridge_attrs[] = {
 	&dev_attr_subordinate_bus_number.attr,
 	&dev_attr_secondary_bus_number.attr,
@@ -1323,20 +1347,10 @@ static DEVICE_ATTR(reset, 0200, NULL, reset_store);
 
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
-	int retval;
-
-	pcie_vpd_create_sysfs_dev_files(dev);
-
-	if (dev->reset_fn) {
-		retval = device_create_file(&dev->dev, &dev_attr_reset);
-		if (retval)
-			goto error;
-	}
-	return 0;
+	if (!dev->reset_fn)
+		return 0;
 
-error:
-	pcie_vpd_remove_sysfs_dev_files(dev);
-	return retval;
+	return device_create_file(&dev->dev, &dev_attr_reset);
 }
 
 int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
@@ -1409,7 +1423,6 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 
 static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 {
-	pcie_vpd_remove_sysfs_dev_files(dev);
 	if (dev->reset_fn) {
 		device_remove_file(&dev->dev, &dev_attr_reset);
 		dev->reset_fn = 0;
@@ -1523,8 +1536,21 @@ static umode_t pcie_dev_attrs_are_visible(struct kobject *kobj,
 	return 0;
 }
 
+static umode_t pci_dev_bin_attrs_visible(struct kobject *kobj,
+					 struct bin_attribute *a, int n)
+{
+	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
+
+	if (a == &bin_attr_vpd && !pdev->vpd)
+		return 0;
+
+	return a->attr.mode;
+}
+
 static const struct attribute_group pci_dev_group = {
 	.attrs = pci_dev_attrs,
+	.bin_attrs = pci_dev_bin_attrs,
+	.is_bin_visible = pci_dev_bin_attrs_visible,
 };
 
 const struct attribute_group *pci_dev_groups[] = {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ef7c46613..d6ad1a3e2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -143,8 +143,6 @@ static inline bool pcie_downstream_port(const struct pci_dev *dev)
 
 int pci_vpd_init(struct pci_dev *dev);
 void pci_vpd_release(struct pci_dev *dev);
-void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev);
-void pcie_vpd_remove_sysfs_dev_files(struct pci_dev *dev);
 
 /* PCI Virtual Channel */
 int pci_save_vc_state(struct pci_dev *dev);
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 2096530ce..a33a8fd7e 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,59 +396,6 @@ 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)
-{
-	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)
-{
-	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
-
-	return pci_write_vpd(dev, off, count, buf);
-}
-
-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;
-}
-
-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);
-	}
-}
-
 int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt)
 {
 	int i;
-- 
2.30.0

Re: [PATCH v2 2/2] PCI/VPD: Let PCI sysfs core code handle VPD binary attribute

[Krzysztof Wilczyński]

Hi Heiner,

Thank you for working on this and for cleaning this up!

On 21-02-03 09:50:13, 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 use PCI sysfs core code for handling the attribute.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/pci-sysfs.c | 54 ++++++++++++++++++++++++++++++-----------
>  drivers/pci/pci.h       |  2 --
>  drivers/pci/vpd.c       | 54 -----------------------------------------
>  3 files changed, 40 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fb072f4b3..ed2ded167 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -255,6 +255,25 @@ static ssize_t ari_enabled_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(ari_enabled);
>  
> +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 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 BIN_ATTR_RW(vpd, 0);
> +
>  static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
> @@ -621,6 +640,11 @@ static struct attribute *pci_dev_attrs[] = {
>  	NULL,
>  };
>  
> +static struct bin_attribute *pci_dev_bin_attrs[] = {
> +	&bin_attr_vpd,
> +	NULL,
> +};
> +
>  static struct attribute *pci_bridge_attrs[] = {
>  	&dev_attr_subordinate_bus_number.attr,
>  	&dev_attr_secondary_bus_number.attr,
> @@ -1323,20 +1347,10 @@ static DEVICE_ATTR(reset, 0200, NULL, reset_store);
>  
>  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  {
> -	int retval;
> -
> -	pcie_vpd_create_sysfs_dev_files(dev);
> -
> -	if (dev->reset_fn) {
> -		retval = device_create_file(&dev->dev, &dev_attr_reset);
> -		if (retval)
> -			goto error;
> -	}
> -	return 0;
> +	if (!dev->reset_fn)
> +		return 0;
>  
> -error:
> -	pcie_vpd_remove_sysfs_dev_files(dev);
> -	return retval;
> +	return device_create_file(&dev->dev, &dev_attr_reset);
>  }
>  
>  int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
> @@ -1409,7 +1423,6 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>  
>  static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>  {
> -	pcie_vpd_remove_sysfs_dev_files(dev);
>  	if (dev->reset_fn) {
>  		device_remove_file(&dev->dev, &dev_attr_reset);
>  		dev->reset_fn = 0;
> @@ -1523,8 +1536,21 @@ static umode_t pcie_dev_attrs_are_visible(struct kobject *kobj,
>  	return 0;
>  }
>  
> +static umode_t pci_dev_bin_attrs_visible(struct kobject *kobj,
> +					 struct bin_attribute *a, int n)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +
> +	if (a == &bin_attr_vpd && !pdev->vpd)
> +		return 0;
> +
> +	return a->attr.mode;
> +}
> +
>  static const struct attribute_group pci_dev_group = {
>  	.attrs = pci_dev_attrs,
> +	.bin_attrs = pci_dev_bin_attrs,
> +	.is_bin_visible = pci_dev_bin_attrs_visible,
>  };
>  
>  const struct attribute_group *pci_dev_groups[] = {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ef7c46613..d6ad1a3e2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -143,8 +143,6 @@ static inline bool pcie_downstream_port(const struct pci_dev *dev)
>  
>  int pci_vpd_init(struct pci_dev *dev);
>  void pci_vpd_release(struct pci_dev *dev);
> -void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev);
> -void pcie_vpd_remove_sysfs_dev_files(struct pci_dev *dev);
>  
>  /* PCI Virtual Channel */
>  int pci_save_vc_state(struct pci_dev *dev);
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 2096530ce..a33a8fd7e 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,59 +396,6 @@ 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)
> -{
> -	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)
> -{
> -	struct pci_dev *dev = to_pci_dev(kobj_to_dev(kobj));
> -
> -	return pci_write_vpd(dev, off, count, buf);
> -}
> -
> -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;
> -}
> -
> -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);
> -	}
> -}
> -
>  int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt)
>  {
>  	int i;
> -- 
> 2.30.0
> 

This looks so much better now.

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof

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

[Bjorn Helgaas]

On Wed, Feb 03, 2021 at 09:46:58AM +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.
> 
> v2:
> - switch to using PCI sysfs core code in patch 2
> 
> Heiner Kallweit (2):
>   PCI/VPD: Remove dead code from sysfs access functions
>   PCI/VPD: Let PCI sysfs core code handle VPD binary attribute
> 
>  drivers/pci/pci-sysfs.c | 54 +++++++++++++++++++++++---------
>  drivers/pci/pci.h       |  2 --
>  drivers/pci/vpd.c       | 68 -----------------------------------------
>  3 files changed, 40 insertions(+), 84 deletions(-)

Applied to pci/vpd for v5.13, thanks!