diff mbox series

[v5,4/7] PCI/sysfs: Allow userspace to query and set device reset mechanism

Message ID 20210529192527.2708-5-ameynarkhede03@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Expose and manage PCI device reset | expand

Commit Message

ameynarkhede03 May 29, 2021, 7:25 p.m. UTC 
Add reset_method sysfs attribute to enable user to
query and set user preferred device reset methods and
their ordering.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Co-developed-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-pci |  16 ++++
 drivers/pci/pci-sysfs.c                 | 105 ++++++++++++++++++++++++
 2 files changed, 121 insertions(+)

Comments

Shanker Donthineni June 2, 2021, 10:17 p.m. UTC | #1 
On 5/29/21 2:25 PM, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to
> query and set user preferred device reset methods and
> their ordering.
>
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Co-developed-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
Tested-by: Shanker Donthineni <sdonthineni@nvidia.com>
Krzysztof Wilczyński June 6, 2021, 12:58 p.m. UTC | #2 
Hi Amey and Shanker,

[...]
> +static ssize_t reset_method_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t len = 0;
> +	int i, prio;
> +
> +	for (prio = PCI_RESET_METHODS_NUM; prio; prio--) {
> +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++) {
> +			if (prio == pdev->reset_methods[i]) {
> +				len += sysfs_emit_at(buf, len, "%s%s",
> +						     len ? "," : "",
> +						     pci_reset_fn_methods[i].name);
> +				break;
> +			}
> +		}
> +
> +		if (i == PCI_RESET_METHODS_NUM)
> +			break;
> +	}
> +
> +	return len;
> +}

Make sure to include trailing newline when exposing values through the
sysfs object to the userspace in the above show() function.

[...]
> +static ssize_t reset_method_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
[...]
> +
> +	while ((name = strsep((char **)&buf, ",")) != NULL) {
[...]

This is something that I wonder could benefit from the following:

  char *options, *end;

  if (count >= (PAGE_SIZE - 1))
	return -EINVAL;
  
  options = kstrndup(buf, count, GFP_KERNEL);
  if (!options)
  	return -ENOMEM;
  
  while ((name = strsep(&options, ",")) != NULL) {
  	...
  }
  
  ...
  
  kfree(options);

Why?  To avoid changing the string buffer that has been passed to
reset_method_store() as strsep() while parsing will update the content
of the buffer.  The cast to (char **), aside of most definitely allowing
to suppress the probable compiler warning, will also allow for what
should be a technically a constant string (to which we got a pointer to)
to be modified.  I am not sure how much could this be of a problem, but
I would try not to do it, if possible.

[...]
> +set_reset_methods:
> +	memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(reset_method);

A small nitpikc: customary there is no space (a newline) between the
function and the macro, the macro follows immediately after.

	Krzysztof
Shanker Donthineni June 6, 2021, 2:27 p.m. UTC | #3 
Hi Krzysztof,

On 6/6/21 7:58 AM, Krzysztof Wilczyński wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Amey and Shanker,
>
> [...]
>> +static ssize_t reset_method_show(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              char *buf)
>> +{
>> +     struct pci_dev *pdev = to_pci_dev(dev);
>> +     ssize_t len = 0;
>> +     int i, prio;
>> +
>> +     for (prio = PCI_RESET_METHODS_NUM; prio; prio--) {
>> +             for (i = 0; i < PCI_RESET_METHODS_NUM; i++) {
>> +                     if (prio == pdev->reset_methods[i]) {
>> +                             len += sysfs_emit_at(buf, len, "%s%s",
>> +                                                  len ? "," : "",
>> +                                                  pci_reset_fn_methods[i].name);
>> +                             break;
>> +                     }
>> +             }
>> +
>> +             if (i == PCI_RESET_METHODS_NUM)
>> +                     break;
>> +     }
>> +
>> +     return len;
>> +}
> Make sure to include trailing newline when exposing values through the
> sysfs object to the userspace in the above show() function.

Agree new line is needed. Will fix it.
> [...]
>> +static ssize_t reset_method_store(struct device *dev,
>> +                               struct device_attribute *attr,
>> +                               const char *buf, size_t count)
> [...]
>> +
>> +     while ((name = strsep((char **)&buf, ",")) != NULL) {
> [...]
>
> This is something that I wonder could benefit from the following:
>
>   char *options, *end;
>
>   if (count >= (PAGE_SIZE - 1))
>         return -EINVAL;
>
>   options = kstrndup(buf, count, GFP_KERNEL);
>   if (!options)
>         return -ENOMEM;
>
>   while ((name = strsep(&options, ",")) != NULL) {
>         ...
>   }
>
>   ...
>
>   kfree(options);
>
> Why?  To avoid changing the string buffer that has been passed to
> reset_method_store() as strsep() while parsing will update the content
> of the buffer.  The cast to (char **), aside of most definitely allowing
> to suppress the probable compiler warning, will also allow for what
> should be a technically a constant string (to which we got a pointer to)
> to be modified.  I am not sure how much could this be of a problem, but
> I would try not to do it, if possible.

Thanks, will use temporary buffer for parsing string.
>
> [...]
>> +set_reset_methods:
>> +     memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(reset_method);
> A small nitpikc: customary there is no space (a newline) between the
> function and the macro, the macro follows immediately after.

Will fix it.
>         Krzysztof
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ef00fada2..cf6dbbb3c 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -121,6 +121,22 @@  Description:
 		child buses, and re-discover devices removed earlier
 		from this part of the device tree.
 
+What:		/sys/bus/pci/devices/.../reset_method
+Date:		March 2021
+Contact:	Amey Narkhede <ameynarkhede03@gmail.com>
+Description:
+		Some devices allow an individual function to be reset
+		without affecting other functions in the same slot.
+		For devices that have this support, a file named reset_method
+		will be present in sysfs. Reading this file will give names
+		of the device supported reset methods and their ordering.
+		Writing the name or comma separated list of names of any of
+		the device supported reset methods to this file will set the
+		reset methods and their ordering to be used when resetting
+		the device. Writing empty string to this file will disable
+		ability to reset the device and writing "default" will return
+		to the original value.
+
 What:		/sys/bus/pci/devices/.../reset
 Date:		July 2009
 Contact:	Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 316f70c3e..04b3d6565 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1334,6 +1334,110 @@  static const struct attribute_group pci_dev_rom_attr_group = {
 	.is_bin_visible = pci_dev_rom_attr_is_visible,
 };
 
+static ssize_t reset_method_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	ssize_t len = 0;
+	int i, prio;
+
+	for (prio = PCI_RESET_METHODS_NUM; prio; prio--) {
+		for (i = 0; i < PCI_RESET_METHODS_NUM; i++) {
+			if (prio == pdev->reset_methods[i]) {
+				len += sysfs_emit_at(buf, len, "%s%s",
+						     len ? "," : "",
+						     pci_reset_fn_methods[i].name);
+				break;
+			}
+		}
+
+		if (i == PCI_RESET_METHODS_NUM)
+			break;
+	}
+
+	return len;
+}
+
+static ssize_t reset_method_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	u8 reset_methods[PCI_RESET_METHODS_NUM];
+	struct pci_dev *pdev = to_pci_dev(dev);
+	u8 prio = PCI_RESET_METHODS_NUM;
+	char *name;
+	int i;
+
+	/*
+	 * Initialize reset_method such that 0xff indicates
+	 * supported but not currently enabled reset methods
+	 * as we only use priority values which are within
+	 * the range of PCI_RESET_FN_METHODS array size
+	 */
+	for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
+		reset_methods[i] = pdev->reset_methods[i] ? 0xff : 0;
+
+	if (sysfs_streq(buf, "")) {
+		pci_warn(pdev, "All device reset methods disabled by user");
+		goto set_reset_methods;
+	}
+
+	if (sysfs_streq(buf, "default")) {
+		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
+			reset_methods[i] = reset_methods[i] ? prio-- : 0;
+		goto set_reset_methods;
+	}
+
+	while ((name = strsep((char **)&buf, ",")) != NULL) {
+		if (sysfs_streq(name, ""))
+			continue;
+
+		name = strim(name);
+
+		for (i = 0; i < PCI_RESET_METHODS_NUM; i++) {
+			if (reset_methods[i] &&
+			    sysfs_streq(name, pci_reset_fn_methods[i].name)) {
+				reset_methods[i] = prio--;
+				break;
+			}
+		}
+		if (i == PCI_RESET_METHODS_NUM)
+			return -EINVAL;
+	}
+
+	if (reset_methods[0] &&
+	    reset_methods[0] != PCI_RESET_METHODS_NUM)
+		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
+
+set_reset_methods:
+	memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
+	return count;
+}
+
+static DEVICE_ATTR_RW(reset_method);
+
+static struct attribute *pci_dev_reset_method_attrs[] = {
+	&dev_attr_reset_method.attr,
+	NULL,
+};
+
+static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
+						    struct attribute *a, int n)
+{
+	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
+
+	if (!pci_reset_supported(pdev))
+		return 0;
+
+	return a->mode;
+}
+
+static const struct attribute_group pci_dev_reset_method_attr_group = {
+	.attrs = pci_dev_reset_method_attrs,
+	.is_visible = pci_dev_reset_method_attr_is_visible,
+};
+
 static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
@@ -1491,6 +1595,7 @@  const struct attribute_group *pci_dev_groups[] = {
 	&pci_dev_config_attr_group,
 	&pci_dev_rom_attr_group,
 	&pci_dev_reset_attr_group,
+	&pci_dev_reset_method_attr_group,
 	&pci_dev_vpd_attr_group,
 #ifdef CONFIG_DMI
 	&pci_dev_smbios_attr_group,