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

Commit Message

ameynarkhede03 June 8, 2021, 5:48 a.m. UTC

Add reset_method sysfs attribute to enable user to
query and set user preferred device reset methods and
their ordering.

Co-developed-by: Alex Williamson <[e-mail]>
Signed-off-by: Alex Williamson <[e-mail]>
Signed-off-by: Amey Narkhede <[e-mail]>
---
 Documentation/ABI/testing/sysfs-bus-pci |  16 ++++
 drivers/pci/pci-sysfs.c                 | 118 ++++++++++++++++++++++++
 2 files changed, 134 insertions(+)

Comments

Raphael Norwitz June 9, 2021, 9:57 p.m. UTC | #1

On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to
> query and set user preferred device reset methods and
> their ordering.
> 
> Co-developed-by: Alex Williamson <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>
> Signed-off-by: Amey Narkhede <[e-mail]>

Reviewed-by: Raphael Norwitz <[e-mail]>

> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  16 ++++
>  drivers/pci/pci-sysfs.c                 | 118 ++++++++++++++++++++++++
>  2 files changed, 134 insertions(+)
> 
> 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 <[e-mail]>
> +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 <[e-mail]>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 316f70c3e..52def79aa 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1334,6 +1334,123 @@ 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;
> +	}
> +

Don't you still need to ensure you add the newline even if there are no
reset methods set? If the len is zero why don't we need the newline?

Otherwise looks good.

> +	if (len)
> +		len += sysfs_emit_at(buf, len, "\n");
> +
> +	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, *options;
> +	int i;
> +
> +	if (count >= (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	options = kstrndup(buf, count, GFP_KERNEL);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	/*
> +	 * 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
> +	 */

NIT: missing period in above comment.

> +	for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> +		reset_methods[i] = pdev->reset_methods[i] ? 0xff : 0;
> +
> +	if (sysfs_streq(options, "")) {
> +		pci_warn(pdev, "All device reset methods disabled by user");
> +		goto set_reset_methods;
> +	}
> +
> +	if (sysfs_streq(options, "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(&options, ",")) != 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) {
> +			kfree(options);
> +			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:
> +	kfree(options);
> +	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 +1608,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,
> -- 
> 2.31.1
> 
>

Shanker Donthineni June 9, 2021, 10:36 p.m. UTC | #2

Hi Raphael,

On 6/9/21 4:57 PM, Raphael Norwitz wrote:
>> +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;
>> +     }
>> +
> Don't you still need to ensure you add the newline even if there are no
> reset methods set? If the len is zero why don't we need the newline?
>
> Otherwise looks good.
>

sysfs entry 'reset_method' will not be visible if there are no reset methods.

Raphael Norwitz June 9, 2021, 10:48 p.m. UTC | #3

Yes - I got this one wrong.

Nevermind, looks good. Just the punctuation NIT.

On Wed, Jun 09, 2021 at 05:36:26PM -0500, Shanker R Donthineni wrote:
> Hi Raphael,
> 
> On 6/9/21 4:57 PM, Raphael Norwitz wrote:
> >> +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;
> >> +     }
> >> +
> > Don't you still need to ensure you add the newline even if there are no
> > reset methods set? If the len is zero why don't we need the newline?
> >
> > Otherwise looks good.
> >
> 
> sysfs entry 'reset_method' will not be visible if there are no reset methods.
> 
>

Shanker Donthineni June 10, 2021, 8:16 p.m. UTC | #4

On 6/8/21 12:48 AM, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to
> query and set user preferred device reset methods and
> their ordering.
>
> Co-developed-by: Alex Williamson <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>
> Signed-off-by: Amey Narkhede <[e-mail]>

Tested-by: Shanker Donthineni <[e-mail]>

Bjorn Helgaas June 18, 2021, 8 p.m. UTC | #5

On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to
> query and set user preferred device reset methods and
> their ordering.

Rewrap to fill 75 columns (also apply to other patches if applicable,
e.g., 3/8 looks like it could use it).

2/8 looks like it's missing a blank line between paragraphs.

> Co-developed-by: Alex Williamson <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>
> Signed-off-by: Amey Narkhede <[e-mail]>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  16 ++++
>  drivers/pci/pci-sysfs.c                 | 118 ++++++++++++++++++++++++
>  2 files changed, 134 insertions(+)
> 
> 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 <[e-mail]>
> +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.

Rewrap to fill or add a blank line if "For devices ..." is supposed to
start a new paragraph.

My guess is you intend reading to show the *currently enabled* reset
methods, not the entire "supported" set?  So if a user has disabled
one of them, it no longer appears when you read the file?

> +
>  What:		/sys/bus/pci/devices/.../reset
>  Date:		July 2009
>  Contact:	Michael S. Tsirkin <[e-mail]>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 316f70c3e..52def79aa 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1334,6 +1334,123 @@ 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;
> +	}

I'm guessing that if you adopt the alternate reset_methods[] encoding,
this nested loop becomes a single loop and "prio" goes away?

> +	if (len)
> +		len += sysfs_emit_at(buf, len, "\n");
> +
> +	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, *options;
> +	int i;

Reorder decls with to_pci_dev(dev) first, then in order of use.

> +	if (count >= (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	options = kstrndup(buf, count, GFP_KERNEL);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	/*
> +	 * 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;

I'm hoping the 0xff trick goes away with the alternate encoding?

> +	if (sysfs_streq(options, "")) {
> +		pci_warn(pdev, "All device reset methods disabled by user");
> +		goto set_reset_methods;
> +	}

I think you can get this case out of the way early with no kstrndup(),
no goto, etc.

> +	if (sysfs_streq(options, "default")) {
> +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> +		goto set_reset_methods;
> +	}

If you use pci_init_reset_methods() here, you can also get this case
out of the way early.

> +	while ((name = strsep(&options, ",")) != 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) {
> +			kfree(options);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (reset_methods[0] &&
> +	    reset_methods[0] != PCI_RESET_METHODS_NUM)
> +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");

Is there a specific reason for this warning?  Is it just telling the
user that he might have shot himself in the foot?  Not sure that's
necessary.

> +set_reset_methods:
> +	kfree(options);
> +	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;

I think this _is_visible method is executed only once, at
device_add()-time.  That means if a device doesn't support any resets
at that time, "reset_method" will not be visible, and there will be no
way to ever enable a reset method at run-time.  I assume that's OK;
just double-checking.

> +
> +	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 +1608,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,
> -- 
> 2.31.1
>

ameynarkhede03 June 19, 2021, 1:59 p.m. UTC | #6

On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > Add reset_method sysfs attribute to enable user to
> > query and set user preferred device reset methods and
> > their ordering.
>
> Rewrap to fill 75 columns (also apply to other patches if applicable,
> e.g., 3/8 looks like it could use it).
>
> 2/8 looks like it's missing a blank line between paragraphs.
>
> > Co-developed-by: Alex Williamson <[e-mail]>
> > Signed-off-by: Alex Williamson <[e-mail]>
> > Signed-off-by: Amey Narkhede <[e-mail]>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci |  16 ++++
> >  drivers/pci/pci-sysfs.c                 | 118 ++++++++++++++++++++++++
> >  2 files changed, 134 insertions(+)
> >
> > 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 <[e-mail]>
> > +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.
>
> Rewrap to fill or add a blank line if "For devices ..." is supposed to
> start a new paragraph.
>
> My guess is you intend reading to show the *currently enabled* reset
> methods, not the entire "supported" set?  So if a user has disabled
> one of them, it no longer appears when you read the file?
>
> > +
> >  What:		/sys/bus/pci/devices/.../reset
> >  Date:		July 2009
> >  Contact:	Michael S. Tsirkin <[e-mail]>
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 316f70c3e..52def79aa 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1334,6 +1334,123 @@ 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;
> > +	}
>
> I'm guessing that if you adopt the alternate reset_methods[] encoding,
> this nested loop becomes a single loop and "prio" goes away?
>
> > +	if (len)
> > +		len += sysfs_emit_at(buf, len, "\n");
> > +
> > +	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, *options;
> > +	int i;
>
> Reorder decls with to_pci_dev(dev) first, then in order of use.
>
> > +	if (count >= (PAGE_SIZE - 1))
> > +		return -EINVAL;
> > +
> > +	options = kstrndup(buf, count, GFP_KERNEL);
> > +	if (!options)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * 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;
>
> I'm hoping the 0xff trick goes away with the alternate encoding?
>
> > +	if (sysfs_streq(options, "")) {
> > +		pci_warn(pdev, "All device reset methods disabled by user");
> > +		goto set_reset_methods;
> > +	}
>
> I think you can get this case out of the way early with no kstrndup(),
> no goto, etc.
>
> > +	if (sysfs_streq(options, "default")) {
> > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > +		goto set_reset_methods;
> > +	}
>
> If you use pci_init_reset_methods() here, you can also get this case
> out of the way early.
>
The problem with alternate encoding is we won't be able to know if
one of the reset methods was disabled previously. For example,

# cat reset_methods
flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
# echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
# cat reset_methods
bus

Now if an user wants to enable flr

# echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
OR
# echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]

either they need to write "default" first then flr or we will need to
reprobe reset methods each time when user writes to reset_method attribute.


> > +	while ((name = strsep(&options, ",")) != 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) {
> > +			kfree(options);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (reset_methods[0] &&
> > +	    reset_methods[0] != PCI_RESET_METHODS_NUM)
> > +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
>
> Is there a specific reason for this warning?  Is it just telling the
> user that he might have shot himself in the foot?  Not sure that's
> necessary.
>
I think generally presence of device specific reset method means other
methods are potentially broken. Is it okay to skip this?

> > +set_reset_methods:
> > +	kfree(options);
> > +	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;
>
> I think this _is_visible method is executed only once, at
> device_add()-time.  That means if a device doesn't support any resets
> at that time, "reset_method" will not be visible, and there will be no
> way to ever enable a reset method at run-time.  I assume that's OK;
> just double-checking.
>
Correct. Its similar to exisitng reset_fn bitfield which is removed in this
patch series.
[...]

Thanks,
Amey

Bjorn Helgaas June 21, 2021, 1:01 p.m. UTC | #7

On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > Add reset_method sysfs attribute to enable user to
> > > query and set user preferred device reset methods and
> > > their ordering.

> > > +	if (sysfs_streq(options, "default")) {
> > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > +		goto set_reset_methods;
> > > +	}
> >
> > If you use pci_init_reset_methods() here, you can also get this case
> > out of the way early.
> >
> The problem with alternate encoding is we won't be able to know if
> one of the reset methods was disabled previously. For example,
> 
> # cat reset_methods
> flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> # cat reset_methods
> bus
> 
> Now if an user wants to enable flr
> 
> # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> OR
> # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> 
> either they need to write "default" first then flr or we will need to
> reprobe reset methods each time when user writes to reset_method attribute.

Not sure I completely understand the problem here.  I think relying on
previous state that is invisible to the user is a little problematic
because it's hard for the user to predict what will happen.

If the user enables a method that was previously "disabled" because
the probe failed, won't the reset method itself just fail with
-ENOTTY?  Is that a problem?

> > > +	while ((name = strsep(&options, ",")) != 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) {
> > > +			kfree(options);
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > > +	if (reset_methods[0] &&
> > > +	    reset_methods[0] != PCI_RESET_METHODS_NUM)
> > > +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
> >
> > Is there a specific reason for this warning?  Is it just telling the
> > user that he might have shot himself in the foot?  Not sure that's
> > necessary.
> >
> I think generally presence of device specific reset method means other
> methods are potentially broken. Is it okay to skip this?

We might want a warning at reset-time if all the methods failed,
because that means we may leak state between users.  Maybe we also
want one here, if *all* reset methods are disabled.  I don't really
like special treatment of device-specific methods here because it
depends on the assumption that "device-specific means all other resets
are broken."  That's hard to maintain.

Bjorn

ameynarkhede03 June 21, 2021, 5:28 p.m. UTC | #8

On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > Add reset_method sysfs attribute to enable user to
> > > > query and set user preferred device reset methods and
> > > > their ordering.
>
> > > > +	if (sysfs_streq(options, "default")) {
> > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > +		goto set_reset_methods;
> > > > +	}
> > >
> > > If you use pci_init_reset_methods() here, you can also get this case
> > > out of the way early.
> > >
> > The problem with alternate encoding is we won't be able to know if
> > one of the reset methods was disabled previously. For example,
> >
> > # cat reset_methods
> > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > # cat reset_methods
> > bus
> >
> > Now if an user wants to enable flr
> >
> > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > OR
> > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> >
> > either they need to write "default" first then flr or we will need to
> > reprobe reset methods each time when user writes to reset_method attribute.
>
> Not sure I completely understand the problem here.  I think relying on
> previous state that is invisible to the user is a little problematic
> because it's hard for the user to predict what will happen.
>
> If the user enables a method that was previously "disabled" because
> the probe failed, won't the reset method itself just fail with
> -ENOTTY?  Is that a problem?
>
I think I didn't explain this correctly. With current implementation
its not necessary to explicitly set *order of availabe* reset methods.
User can directly write a single supported reset method only and then perform
the reset. Side effect of that is other methods are disabled if user
writes single or less than available number of supported reset method.
Current implementation is able to handle this case but with new encoding
we'll need to reprobe reset methods everytime because we have no way
of distingushing supported and currently enabled reset method.

Alternate way of doing this is using 2 bitmaps as outlined here by
Shanker https://marc.info/?l=linux-kernel&m=162428773101702&w=2
> > > > +	while ((name = strsep(&options, ",")) != 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) {
> > > > +			kfree(options);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (reset_methods[0] &&
> > > > +	    reset_methods[0] != PCI_RESET_METHODS_NUM)
> > > > +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
> > >
> > > Is there a specific reason for this warning?  Is it just telling the
> > > user that he might have shot himself in the foot?  Not sure that's
> > > necessary.
> > >
> > I think generally presence of device specific reset method means other
> > methods are potentially broken. Is it okay to skip this?
>
> We might want a warning at reset-time if all the methods failed,
> because that means we may leak state between users.  Maybe we also
> want one here, if *all* reset methods are disabled.  I don't really
> like special treatment of device-specific methods here because it
> depends on the assumption that "device-specific means all other resets
> are broken."  That's hard to maintain.
>
> Bjorn
Makes sense. I'll update this.

Thanks,
Amey

Bjorn Helgaas June 21, 2021, 7:07 p.m. UTC | #9

On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > Add reset_method sysfs attribute to enable user to
> > > > > query and set user preferred device reset methods and
> > > > > their ordering.
> >
> > > > > +	if (sysfs_streq(options, "default")) {
> > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > +		goto set_reset_methods;
> > > > > +	}
> > > >
> > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > out of the way early.
> > > >
> > > The problem with alternate encoding is we won't be able to know if
> > > one of the reset methods was disabled previously. For example,
> > >
> > > # cat reset_methods
> > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > # cat reset_methods
> > > bus
> > >
> > > Now if an user wants to enable flr
> > >
> > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > OR
> > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > >
> > > either they need to write "default" first then flr or we will need to
> > > reprobe reset methods each time when user writes to reset_method attribute.
> >
> > Not sure I completely understand the problem here.  I think relying on
> > previous state that is invisible to the user is a little problematic
> > because it's hard for the user to predict what will happen.
> >
> > If the user enables a method that was previously "disabled" because
> > the probe failed, won't the reset method itself just fail with
> > -ENOTTY?  Is that a problem?
> >
> I think I didn't explain this correctly. With current implementation
> its not necessary to explicitly set *order of availabe* reset methods.
> User can directly write a single supported reset method only and then perform
> the reset. Side effect of that is other methods are disabled if user
> writes single or less than available number of supported reset method.
> Current implementation is able to handle this case but with new encoding
> we'll need to reprobe reset methods everytime because we have no way
> of distingushing supported and currently enabled reset method.

I'm confused.  I thought the point of the nested loops to find the
highest priority enabled reset method was to allow the user to control
the order.  The sysfs doc says writing "reset_method" sets the "reset
methods and their ordering."

It seems complicated to track "supported" and "enabled" separately,
and I don't know what the benefit is.  If we write "reset_method" to
enable reset X, can we just probe reset X to see if it's supported?

Bjorn

ameynarkhede03 June 21, 2021, 7:33 p.m. UTC | #10

On 21/06/21 02:07PM, Bjorn Helgaas wrote:
> On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> > On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > > Add reset_method sysfs attribute to enable user to
> > > > > > query and set user preferred device reset methods and
> > > > > > their ordering.
> > >
> > > > > > +	if (sysfs_streq(options, "default")) {
> > > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > > +		goto set_reset_methods;
> > > > > > +	}
> > > > >
> > > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > > out of the way early.
> > > > >
> > > > The problem with alternate encoding is we won't be able to know if
> > > > one of the reset methods was disabled previously. For example,
> > > >
> > > > # cat reset_methods
> > > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > > # cat reset_methods
> > > > bus
> > > >
> > > > Now if an user wants to enable flr
> > > >
> > > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > > OR
> > > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > > >
> > > > either they need to write "default" first then flr or we will need to
> > > > reprobe reset methods each time when user writes to reset_method attribute.
> > >
> > > Not sure I completely understand the problem here.  I think relying on
> > > previous state that is invisible to the user is a little problematic
> > > because it's hard for the user to predict what will happen.
> > >
> > > If the user enables a method that was previously "disabled" because
> > > the probe failed, won't the reset method itself just fail with
> > > -ENOTTY?  Is that a problem?
> > >
> > I think I didn't explain this correctly. With current implementation
> > its not necessary to explicitly set *order of availabe* reset methods.
> > User can directly write a single supported reset method only and then perform
> > the reset. Side effect of that is other methods are disabled if user
> > writes single or less than available number of supported reset method.
> > Current implementation is able to handle this case but with new encoding
> > we'll need to reprobe reset methods everytime because we have no way
> > of distingushing supported and currently enabled reset method.
>
> I'm confused.  I thought the point of the nested loops to find the
> highest priority enabled reset method was to allow the user to control
> the order.  The sysfs doc says writing "reset_method" sets the "reset
> methods and their ordering."
>
> It seems complicated to track "supported" and "enabled" separately,
> and I don't know what the benefit is.  If we write "reset_method" to
> enable reset X, can we just probe reset X to see if it's supported?
>
> Bjorn
Although final result is same whether user writes a supported reset method or
their ordering that is,
# echo bus > reset_methods
and
# echo bus,flr > reset_methods

are the same but in the first version, users don't have to explicitly
set the ordering if they just want to perform bus reset.
Current implementation allows the flexibility for switching between
first and second option.

Does this address your doubt?

Thanks,
Amey

Bjorn Helgaas June 23, 2021, 12:06 p.m. UTC | #11

On Tue, Jun 22, 2021 at 01:03:07AM +0530, Amey Narkhede wrote:
> On 21/06/21 02:07PM, Bjorn Helgaas wrote:
> > On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> > > On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > > > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > > > Add reset_method sysfs attribute to enable user to
> > > > > > > query and set user preferred device reset methods and
> > > > > > > their ordering.
> > > >
> > > > > > > +	if (sysfs_streq(options, "default")) {
> > > > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > > > +		goto set_reset_methods;
> > > > > > > +	}
> > > > > >
> > > > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > > > out of the way early.
> > > > > >
> > > > > The problem with alternate encoding is we won't be able to know if
> > > > > one of the reset methods was disabled previously. For example,
> > > > >
> > > > > # cat reset_methods
> > > > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > > > # cat reset_methods
> > > > > bus
> > > > >
> > > > > Now if an user wants to enable flr
> > > > >
> > > > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > > > OR
> > > > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > > > >
> > > > > either they need to write "default" first then flr or we will need to
> > > > > reprobe reset methods each time when user writes to reset_method attribute.
> > > >
> > > > Not sure I completely understand the problem here.  I think relying on
> > > > previous state that is invisible to the user is a little problematic
> > > > because it's hard for the user to predict what will happen.
> > > >
> > > > If the user enables a method that was previously "disabled" because
> > > > the probe failed, won't the reset method itself just fail with
> > > > -ENOTTY?  Is that a problem?
> > > >
> > > I think I didn't explain this correctly. With current implementation
> > > its not necessary to explicitly set *order of availabe* reset methods.
> > > User can directly write a single supported reset method only and then perform
> > > the reset. Side effect of that is other methods are disabled if user
> > > writes single or less than available number of supported reset method.
> > > Current implementation is able to handle this case but with new encoding
> > > we'll need to reprobe reset methods everytime because we have no way
> > > of distingushing supported and currently enabled reset method.
> >
> > I'm confused.  I thought the point of the nested loops to find the
> > highest priority enabled reset method was to allow the user to control
> > the order.  The sysfs doc says writing "reset_method" sets the "reset
> > methods and their ordering."
> >
> > It seems complicated to track "supported" and "enabled" separately,
> > and I don't know what the benefit is.  If we write "reset_method" to
> > enable reset X, can we just probe reset X to see if it's supported?
>
> Although final result is same whether user writes a supported reset method or
> their ordering that is,
> # echo bus > reset_methods
> and
> # echo bus,flr > reset_methods
> 
> are the same but in the first version, users don't have to explicitly
> set the ordering if they just want to perform bus reset.
> Current implementation allows the flexibility for switching between
> first and second option.

Sorry, I can't quite make sense of the above.

Your doc implies the following are different:

  # echo bus,flr > reset_methods
  # echo flr,bus > reset_methods

Are they?  If you don't need to provide control over the order of
trying resets, this can all be simplified quite a bit.

Bjorn

ameynarkhede03 June 23, 2021, 2:07 p.m. UTC | #12

On 21/06/23 07:06AM, Bjorn Helgaas wrote:
> On Tue, Jun 22, 2021 at 01:03:07AM +0530, Amey Narkhede wrote:
> > On 21/06/21 02:07PM, Bjorn Helgaas wrote:
> > > On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> > > > On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > > > > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > > > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > > > > Add reset_method sysfs attribute to enable user to
> > > > > > > > query and set user preferred device reset methods and
> > > > > > > > their ordering.
> > > > >
> > > > > > > > +	if (sysfs_streq(options, "default")) {
> > > > > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > > > > +		goto set_reset_methods;
> > > > > > > > +	}
> > > > > > >
> > > > > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > > > > out of the way early.
> > > > > > >
> > > > > > The problem with alternate encoding is we won't be able to know if
> > > > > > one of the reset methods was disabled previously. For example,
> > > > > >
> > > > > > # cat reset_methods
> > > > > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > > > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > > > > # cat reset_methods
> > > > > > bus
> > > > > >
> > > > > > Now if an user wants to enable flr
> > > > > >
> > > > > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > > > > OR
> > > > > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > > > > >
> > > > > > either they need to write "default" first then flr or we will need to
> > > > > > reprobe reset methods each time when user writes to reset_method attribute.
> > > > >
> > > > > Not sure I completely understand the problem here.  I think relying on
> > > > > previous state that is invisible to the user is a little problematic
> > > > > because it's hard for the user to predict what will happen.
> > > > >
> > > > > If the user enables a method that was previously "disabled" because
> > > > > the probe failed, won't the reset method itself just fail with
> > > > > -ENOTTY?  Is that a problem?
> > > > >
> > > > I think I didn't explain this correctly. With current implementation
> > > > its not necessary to explicitly set *order of availabe* reset methods.
> > > > User can directly write a single supported reset method only and then perform
> > > > the reset. Side effect of that is other methods are disabled if user
> > > > writes single or less than available number of supported reset method.
> > > > Current implementation is able to handle this case but with new encoding
> > > > we'll need to reprobe reset methods everytime because we have no way
> > > > of distingushing supported and currently enabled reset method.
> > >
> > > I'm confused.  I thought the point of the nested loops to find the
> > > highest priority enabled reset method was to allow the user to control
> > > the order.  The sysfs doc says writing "reset_method" sets the "reset
> > > methods and their ordering."
> > >
> > > It seems complicated to track "supported" and "enabled" separately,
> > > and I don't know what the benefit is.  If we write "reset_method" to
> > > enable reset X, can we just probe reset X to see if it's supported?
> >
> > Although final result is same whether user writes a supported reset method or
> > their ordering that is,
> > # echo bus > reset_methods
> > and
> > # echo bus,flr > reset_methods
> >
> > are the same but in the first version, users don't have to explicitly
> > set the ordering if they just want to perform bus reset.
> > Current implementation allows the flexibility for switching between
> > first and second option.
>
> Sorry, I can't quite make sense of the above.
>
> Your doc implies the following are different:
>
>   # echo bus,flr > reset_methods
>   # echo flr,bus > reset_methods
>
> Are they?  If you don't need to provide control over the order of
> trying resets, this can all be simplified quite a bit.
>
> Bjorn
The v1 of the patch series allowed only single reset method to be
written instead of ordering of all supported reset methods.
With your example, current implementation allows both writing single
value and list of supported reset methods.

# echo bus > reset_methods
and
# echo bus,flr > reset_methods

OR

# echo flr > reset_methods
and
echo flr,bus > reset_methods

Its more of a preference than a functional point. Ultimately the
__pci_reset_function_locked() function will only perform 'bus' reset in
this example because we make sure 'reset_methods' file only contains
valid device supported reset methods all the time so
__pci_reset_function_locked() won't go into -ENOTTY case but with new
encoding theres no way to make sure `reset_methods`sysfs attribute will
contain valid supported reset methods all the time because of the reset
methods can be masked implicitly if user uses first option of writing
only single value.

My point is current implementation allows more flexibility for the user.

Thanks,
Amey

Alex Williamson June 23, 2021, 5:21 p.m. UTC | #13

On Mon, 21 Jun 2021 08:01:35 -0500
Bjorn Helgaas <[e-mail]> wrote:

> On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > On 21/06/18 03:00PM, Bjorn Helgaas wrote:  
> > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:  
> > > > +	while ((name = strsep(&options, ",")) != 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) {
> > > > +			kfree(options);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (reset_methods[0] &&
> > > > +	    reset_methods[0] != PCI_RESET_METHODS_NUM)
> > > > +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");  
> > >
> > > Is there a specific reason for this warning?  Is it just telling the
> > > user that he might have shot himself in the foot?  Not sure that's
> > > necessary.
> > >  
> > I think generally presence of device specific reset method means other
> > methods are potentially broken. Is it okay to skip this?  
> 
> We might want a warning at reset-time if all the methods failed,
> because that means we may leak state between users.  Maybe we also
> want one here, if *all* reset methods are disabled.  I don't really
> like special treatment of device-specific methods here because it
> depends on the assumption that "device-specific means all other resets
> are broken."  That's hard to maintain.

I'd say the device specific reset is special.  The device itself can
support a number of resets and they're theoretically all equivalent,
it's a policy decision which to use.  But the device specific reset is
a software provided reset.  Someone has specifically gone to the
trouble to create a reset mechanism that is in some way better than the
other methods.  Not using that one by default sure feels like something
worthy of leaving a breadcrumb in dmesg for debugging.  Thanks,

Alex

ameynarkhede03 June 23, 2021, 5:56 p.m. UTC | #14

On 21/06/23 07:37PM, Amey Narkhede wrote:
> On 21/06/23 07:06AM, Bjorn Helgaas wrote:
> > On Tue, Jun 22, 2021 at 01:03:07AM +0530, Amey Narkhede wrote:
> > > On 21/06/21 02:07PM, Bjorn Helgaas wrote:
> > > > On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> > > > > On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > > > > > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > > > > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > > > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > > > > > Add reset_method sysfs attribute to enable user to
> > > > > > > > > query and set user preferred device reset methods and
> > > > > > > > > their ordering.
> > > > > >
> > > > > > > > > +	if (sysfs_streq(options, "default")) {
> > > > > > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > > > > > +		goto set_reset_methods;
> > > > > > > > > +	}
> > > > > > > >
> > > > > > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > > > > > out of the way early.
> > > > > > > >
> > > > > > > The problem with alternate encoding is we won't be able to know if
> > > > > > > one of the reset methods was disabled previously. For example,
> > > > > > >
> > > > > > > # cat reset_methods
> > > > > > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > > > > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > > > > > # cat reset_methods
> > > > > > > bus
> > > > > > >
> > > > > > > Now if an user wants to enable flr
> > > > > > >
> > > > > > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > > > > > OR
> > > > > > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > > > > > >
> > > > > > > either they need to write "default" first then flr or we will need to
> > > > > > > reprobe reset methods each time when user writes to reset_method attribute.
> > > > > >
> > > > > > Not sure I completely understand the problem here.  I think relying on
> > > > > > previous state that is invisible to the user is a little problematic
> > > > > > because it's hard for the user to predict what will happen.
> > > > > >
> > > > > > If the user enables a method that was previously "disabled" because
> > > > > > the probe failed, won't the reset method itself just fail with
> > > > > > -ENOTTY?  Is that a problem?
> > > > > >
> > > > > I think I didn't explain this correctly. With current implementation
> > > > > its not necessary to explicitly set *order of availabe* reset methods.
> > > > > User can directly write a single supported reset method only and then perform
> > > > > the reset. Side effect of that is other methods are disabled if user
> > > > > writes single or less than available number of supported reset method.
> > > > > Current implementation is able to handle this case but with new encoding
> > > > > we'll need to reprobe reset methods everytime because we have no way
> > > > > of distingushing supported and currently enabled reset method.
> > > >
> > > > I'm confused.  I thought the point of the nested loops to find the
> > > > highest priority enabled reset method was to allow the user to control
> > > > the order.  The sysfs doc says writing "reset_method" sets the "reset
> > > > methods and their ordering."
> > > >
> > > > It seems complicated to track "supported" and "enabled" separately,
> > > > and I don't know what the benefit is.  If we write "reset_method" to
> > > > enable reset X, can we just probe reset X to see if it's supported?
> > >
> > > Although final result is same whether user writes a supported reset method or
> > > their ordering that is,
> > > # echo bus > reset_methods
> > > and
> > > # echo bus,flr > reset_methods
> > >
> > > are the same but in the first version, users don't have to explicitly
> > > set the ordering if they just want to perform bus reset.
> > > Current implementation allows the flexibility for switching between
> > > first and second option.
> >
> > Sorry, I can't quite make sense of the above.
> >
> > Your doc implies the following are different:
> >
> >   # echo bus,flr > reset_methods
> >   # echo flr,bus > reset_methods
> >
> > Are they?  If you don't need to provide control over the order of
> > trying resets, this can all be simplified quite a bit.
> >
> > Bjorn
> The v1 of the patch series allowed only single reset method to be
> written instead of ordering of all supported reset methods.
> With your example, current implementation allows both writing single
> value and list of supported reset methods.
>
> # echo bus > reset_methods
> and
> # echo bus,flr > reset_methods
>
> OR
>
> # echo flr > reset_methods
> and
> echo flr,bus > reset_methods
>
# echo flr,bus and echo bus,flr are different.

> Its more of a preference than a functional point. Ultimately the
> __pci_reset_function_locked() function will only perform 'bus' reset in
> this example because we make sure 'reset_methods' file only contains
> valid device supported reset methods all the time so
> __pci_reset_function_locked() won't go into -ENOTTY case but with new
Oops I'm wrong here. __pci_reset_function_locked() can return -ENOTTY
and follow through if a reset fails.

Rest of the point should hold.
> encoding theres no way to make sure `reset_methods`sysfs attribute will
> contain valid supported reset methods all the time because of the reset
> methods can be masked implicitly if user uses first option of writing
> only single value.
>
> My point is current implementation allows more flexibility for the user.
>
> Thanks,
> Amey

Bjorn Helgaas June 24, 2021, 12:15 p.m. UTC | #15

On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to
> query and set user preferred device 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.

> +	while ((name = strsep(&options, ",")) != 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) {
> +			kfree(options);
> +			return -EINVAL;
> +		}
> +	}

Asking again since we didn't get this clarified before.  The above
tells me that "reset_methods" allows the user to control the *order*
in which we try reset methods.

Consider the following two uses:

  (1) # echo bus,flr > reset_methods

  (2) # echo flr,bus > reset_methods

Do these have the same effect or not?

If "reset_methods" allows control over the order, I expect them to be
different: (1) would try a bus reset and, if the bus reset failed, an
FLR, while (2) would try an FLR and, if the FLR failed, a bus reset.

ameynarkhede03 June 24, 2021, 3:12 p.m. UTC | #16

On 21/06/24 07:15AM, Bjorn Helgaas wrote:
> On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > Add reset_method sysfs attribute to enable user to
> > query and set user preferred device 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.
>
> > +	while ((name = strsep(&options, ",")) != 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) {
> > +			kfree(options);
> > +			return -EINVAL;
> > +		}
> > +	}
>
> Asking again since we didn't get this clarified before.  The above
> tells me that "reset_methods" allows the user to control the *order*
> in which we try reset methods.
>
> Consider the following two uses:
>
>   (1) # echo bus,flr > reset_methods
>
>   (2) # echo flr,bus > reset_methods
>
> Do these have the same effect or not?
>
They have different effect.
> If "reset_methods" allows control over the order, I expect them to be
> different: (1) would try a bus reset and, if the bus reset failed, an
> FLR, while (2) would try an FLR and, if the FLR failed, a bus reset.
Exactly you are right.

Now the point I was presenting was with new encoding we have to write
list of *all of the supported reset methods* in order for example, in
above example flr,bus or bus,flr. We can't just write 'flr' or 'bus'
then switch back to writing flr,bus/bus,flr(these have different effect
as mentioned earlier).

Basically with new encoding an user can't write subset of reset methods
they have to write list of *all* supported methods everytime.

Thanks,
Amey

Bjorn Helgaas June 24, 2021, 4:56 p.m. UTC | #17

On Thu, Jun 24, 2021 at 08:42:42PM +0530, Amey Narkhede wrote:
> On 21/06/24 07:15AM, Bjorn Helgaas wrote:
> > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > Add reset_method sysfs attribute to enable user to
> > > query and set user preferred device 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.
> >
> > > +	while ((name = strsep(&options, ",")) != 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) {
> > > +			kfree(options);
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> >
> > Asking again since we didn't get this clarified before.  The above
> > tells me that "reset_methods" allows the user to control the
> > *order* in which we try reset methods.
> >
> > Consider the following two uses:
> >
> >   (1) # echo bus,flr > reset_methods
> >
> >   (2) # echo flr,bus > reset_methods
> >
> > Do these have the same effect or not?
> >
> They have different effect.

I asked about this because Shanker's idea [1] of using two bitmaps
only keeps track of which resets are *enabled*.  It does not keep
track of the *ordering*.  Since you want to control the ordering, I
think we need more state than just the supported/enabled bitmaps.

> > If "reset_methods" allows control over the order, I expect them to
> > be different: (1) would try a bus reset and, if the bus reset
> > failed, an FLR, while (2) would try an FLR and, if the FLR failed,
> > a bus reset.
>
> Exactly you are right.
> 
> Now the point I was presenting was with new encoding we have to
> write list of *all of the supported reset methods* in order for
> example, in above example flr,bus or bus,flr. We can't just write
> 'flr' or 'bus' then switch back to writing flr,bus/bus,flr (these
> have different effect as mentioned earlier).

It sounds like you're saying this sequence can't work:

  # echo flr > reset_methods
  # echo bus,flr > reset_methods

But I'm afraid you'll have to walk me through the reasons why this
can't be made to work.

> Basically with new encoding an user can't write subset of reset
> methods they have to write list of *all* supported methods
> everytime.

Why does the user have to write all supported methods?  Is that to
preserve the fact that "cat reset_methods" always shows all the
supported methods so the user knows what's available?

I'm wondering why we can't do something like this (pidgin code):

  if (option == "default") {
    pci_init_reset_methods(dev);
    return;
  }

  n = 0;
  foreach method in option {
    i = lookup_reset_method(method);
    if (pci_reset_methods[i].reset_fn(dev, PROBE) == 0)
      dev->reset_methods[n++] = i;           # method i supported
  }
  dev->reset_methods[n++] = 0;               # end of supported methods

If we did something like the above, the user could always find the
list of all methods supported by a device by doing this:

  # echo default > reset_methods
  # cat reset_methods

Yes, this does call the "probe" methods several times.  I don't think
that's necessarily a problem.

Bjorn

[1] https://lore.kernel.org/r/[e-mail]

Shanker Donthineni June 24, 2021, 5:20 p.m. UTC | #18

On 6/24/21 11:56 AM, Bjorn Helgaas wrote:
> Why does the user have to write all supported methods?  Is that to
> preserve the fact that "cat reset_methods" always shows all the
> supported methods so the user knows what's available?
>
> I'm wondering why we can't do something like this (pidgin code):
>
>   if (option == "default") {
>     pci_init_reset_methods(dev);
>     return;
>   }
>
>   n = 0;
>   foreach method in option {
>     i = lookup_reset_method(method);
>     if (pci_reset_methods[i].reset_fn(dev, PROBE) == 0)
>       dev->reset_methods[n++] = i;           # method i supported
>   }
>   dev->reset_methods[n++] = 0;               # end of supported methods
>
> If we did something like the above, the user could always find the
> list of all methods supported by a device by doing this:
>
>   # echo default > reset_methods
>   # cat reset_methods
>
> Yes, this does call the "probe" methods several times.  I don't think
> that's necessarily a problem.
Agree, I don't think admin/user will change reset methods frequently and no
side effects or performance impacts on probing multiple times.   

We should support enabling partially ordered reset methods and restore
default methods either by re-probing resets or remember supported
resets in pci_dev.

ameynarkhede03 June 24, 2021, 5:28 p.m. UTC | #19

On 21/06/24 11:56AM, Bjorn Helgaas wrote:
> On Thu, Jun 24, 2021 at 08:42:42PM +0530, Amey Narkhede wrote:
> > On 21/06/24 07:15AM, Bjorn Helgaas wrote:
> > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > Add reset_method sysfs attribute to enable user to
> > > > query and set user preferred device 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.
> > >
> > > > +	while ((name = strsep(&options, ",")) != 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) {
> > > > +			kfree(options);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > >
> > > Asking again since we didn't get this clarified before.  The above
> > > tells me that "reset_methods" allows the user to control the
> > > *order* in which we try reset methods.
> > >
> > > Consider the following two uses:
> > >
> > >   (1) # echo bus,flr > reset_methods
> > >
> > >   (2) # echo flr,bus > reset_methods
> > >
> > > Do these have the same effect or not?
> > >
> > They have different effect.
>
> I asked about this because Shanker's idea [1] of using two bitmaps
> only keeps track of which resets are *enabled*.  It does not keep
> track of the *ordering*.  Since you want to control the ordering, I
> think we need more state than just the supported/enabled bitmaps.
>
> > > If "reset_methods" allows control over the order, I expect them to
> > > be different: (1) would try a bus reset and, if the bus reset
> > > failed, an FLR, while (2) would try an FLR and, if the FLR failed,
> > > a bus reset.
> >
> > Exactly you are right.
> >
> > Now the point I was presenting was with new encoding we have to
> > write list of *all of the supported reset methods* in order for
> > example, in above example flr,bus or bus,flr. We can't just write
> > 'flr' or 'bus' then switch back to writing flr,bus/bus,flr (these
> > have different effect as mentioned earlier).
>
> It sounds like you're saying this sequence can't work:
>
>   # echo flr > reset_methods
# dev->reset_methods = [3, 0, 0, ..]
>   # echo bus,flr > reset_methods
# to get dev->reset_methods = [6, 3, 0, ...]
we'll need to probe reset methods here.
>
> But I'm afraid you'll have to walk me through the reasons why this
> can't be made to work.
I wrote incomplete description. It can work but we'll need to probe
everytime which involves reading different capabilities(PCI_CAP_ID_AF,
PCI_PM_CTRL etc) from device. With current encoding we just have to
probe at the begining.
>
> > Basically with new encoding an user can't write subset of reset
> > methods they have to write list of *all* supported methods
> > everytime.
>
> Why does the user have to write all supported methods?  Is that to
> preserve the fact that "cat reset_methods" always shows all the
> supported methods so the user knows what's available?
>
> I'm wondering why we can't do something like this (pidgin code):
>
>   if (option == "default") {
>     pci_init_reset_methods(dev);
>     return;
>   }
>
>   n = 0;
>   foreach method in option {
>     i = lookup_reset_method(method);
>     if (pci_reset_methods[i].reset_fn(dev, PROBE) == 0)
Repeatedly calling probe might have some impact as it involves reading
device registers as explained earlier.
>       dev->reset_methods[n++] = i;           # method i supported
>   }
>   dev->reset_methods[n++] = 0;               # end of supported methods
>
> If we did something like the above, the user could always find the
> list of all methods supported by a device by doing this:
>
>   # echo default > reset_methods
>   # cat reset_methods
>
This is one solution for current problem with new encoding.
> Yes, this does call the "probe" methods several times.  I don't think
> that's necessarily a problem.
I thought this would be a problem because of your earlier suggestion
of caching flr capability to avoid probing multiple times. In this case
we'll need to read different device registers multiple times. With
current encoding we don't have to do that multiple times.

Thanks,
Amey
>
> Bjorn
>
> [1] https://lore.kernel.org/r/[e-mail]

Bjorn Helgaas June 24, 2021, 5:59 p.m. UTC | #20

On Thu, Jun 24, 2021 at 10:58:06PM +0530, Amey Narkhede wrote:
> On 21/06/24 11:56AM, Bjorn Helgaas wrote:
> > On Thu, Jun 24, 2021 at 08:42:42PM +0530, Amey Narkhede wrote:
> > > On 21/06/24 07:15AM, Bjorn Helgaas wrote:
> > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > Add reset_method sysfs attribute to enable user to
> > > > > query and set user preferred device 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.
> > > >
> > > > > +	while ((name = strsep(&options, ",")) != 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) {
> > > > > +			kfree(options);
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	}
> > > >
> > > > Asking again since we didn't get this clarified before.  The above
> > > > tells me that "reset_methods" allows the user to control the
> > > > *order* in which we try reset methods.
> > > >
> > > > Consider the following two uses:
> > > >
> > > >   (1) # echo bus,flr > reset_methods
> > > >
> > > >   (2) # echo flr,bus > reset_methods
> > > >
> > > > Do these have the same effect or not?
> > > >
> > > They have different effect.
> >
> > I asked about this because Shanker's idea [1] of using two bitmaps
> > only keeps track of which resets are *enabled*.  It does not keep
> > track of the *ordering*.  Since you want to control the ordering, I
> > think we need more state than just the supported/enabled bitmaps.
> >
> > > > If "reset_methods" allows control over the order, I expect them to
> > > > be different: (1) would try a bus reset and, if the bus reset
> > > > failed, an FLR, while (2) would try an FLR and, if the FLR failed,
> > > > a bus reset.
> > >
> > > Exactly you are right.
> > >
> > > Now the point I was presenting was with new encoding we have to
> > > write list of *all of the supported reset methods* in order for
> > > example, in above example flr,bus or bus,flr. We can't just write
> > > 'flr' or 'bus' then switch back to writing flr,bus/bus,flr (these
> > > have different effect as mentioned earlier).
> >
> > It sounds like you're saying this sequence can't work:
> >
> >   # echo flr > reset_methods
>
> # dev->reset_methods = [3, 0, 0, ..]
>
> >   # echo bus,flr > reset_methods
>
> # to get dev->reset_methods = [6, 3, 0, ...]
> we'll need to probe reset methods here.
>
> > But I'm afraid you'll have to walk me through the reasons why this
> > can't be made to work.
>
> I wrote incomplete description. It can work but we'll need to probe
> everytime which involves reading different capabilities(PCI_CAP_ID_AF,
> PCI_PM_CTRL etc) from device. With current encoding we just have to
> probe at the begining.
>
> > > Basically with new encoding an user can't write subset of reset
> > > methods they have to write list of *all* supported methods
> > > everytime.
> >
> > Why does the user have to write all supported methods?  Is that to
> > preserve the fact that "cat reset_methods" always shows all the
> > supported methods so the user knows what's available?
> >
> > I'm wondering why we can't do something like this (pidgin code):
> >
> >   if (option == "default") {
> >     pci_init_reset_methods(dev);
> >     return;
> >   }
> >
> >   n = 0;
> >   foreach method in option {
> >     i = lookup_reset_method(method);
> >     if (pci_reset_methods[i].reset_fn(dev, PROBE) == 0)
>
> Repeatedly calling probe might have some impact as it involves reading
> device registers as explained earlier.
>
> >       dev->reset_methods[n++] = i;           # method i supported
> >   }
> >   dev->reset_methods[n++] = 0;               # end of supported methods
> >
> > If we did something like the above, the user could always find the
> > list of all methods supported by a device by doing this:
> >
> >   # echo default > reset_methods
> >   # cat reset_methods
>
> This is one solution for current problem with new encoding.
>
> > Yes, this does call the "probe" methods several times.  I don't think
> > that's necessarily a problem.
>
> I thought this would be a problem because of your earlier suggestion
> of caching flr capability to avoid probing multiple times. In this case
> we'll need to read different device registers multiple times. With
> current encoding we don't have to do that multiple times.

I don't think it's a problem to run "probe" methods when we're setting
the enabled reset methods (either at enumeration-time or when we write
to "reset_methods").  These are low-frequency events and I don't think
there's any performance issue.

I don't think we should have to run "probe" methods every time we call
pci_reset_function().

I suggested a dev->has_flr bit for two reasons:

  1) It avoids reading PCI_EXP_DEVCAP every time a driver calls
     pcie_reset_flr(), and

  2) It gives a nice opportunity for quirks to disable FLR for devices
     where it's broken.

> > [1] https://lore.kernel.org/r/[e-mail]

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 <[e-mail]>
+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 <[e-mail]>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 316f70c3e..52def79aa 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1334,6 +1334,123 @@  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;
+	}
+
+	if (len)
+		len += sysfs_emit_at(buf, len, "\n");
+
+	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, *options;
+	int i;
+
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
+
+	options = kstrndup(buf, count, GFP_KERNEL);
+	if (!options)
+		return -ENOMEM;
+
+	/*
+	 * 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(options, "")) {
+		pci_warn(pdev, "All device reset methods disabled by user");
+		goto set_reset_methods;
+	}
+
+	if (sysfs_streq(options, "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(&options, ",")) != 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) {
+			kfree(options);
+			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:
+	kfree(options);
+	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 +1608,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,