Re: [PATCH v13 5/9] PCI: Allow userspace to query and set device reset mechanism

[Amey Narkhede <ameynarkhede03@gmail.com>]

On 21/08/02 05:55PM, Bjorn Helgaas wrote:
> On Sun, Aug 01, 2021 at 07:55:14PM +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 <alex.williamson@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > ---
> > +
[...]
> > +static ssize_t reset_method_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf, size_t count)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int i = 0;
> > +	char *name, *options = NULL;
> > +
> > +	if (count >= (PAGE_SIZE - 1))
> > +		return -EINVAL;
> > +
> > +	if (sysfs_streq(buf, "")) {
> > +		pdev->reset_methods[0] = 0;
> > +		pci_warn(pdev, "All device reset methods disabled by user");
> > +		return count;
> > +	}
>
> I think it's possible for the user to disable all reset methods by
> supplying only junk.  Maybe this check could be moved to the end of
> the function to catch both the "empty input" and the "input contains
> only junk" cases?
>
Supplying only junk doesn't disable the reset. It returns -EINVAL as it
will go in following while loop. The check m == PCI_NUM_RESET_METHODS
returns -EINVAL

> > +	if (sysfs_streq(buf, "default")) {
> > +		pci_init_reset_methods(pdev);
> > +		return count;
> > +	}
> > +
> > +	options = kstrndup(buf, count, GFP_KERNEL);
> > +	if (!options)
> > +		return -ENOMEM;
> > +
>
>   i = 0;
>
> here so it's nearer the loop it controls.
>
> > +	while ((name = strsep(&options, " ")) != NULL) {
> > +		int m;
> > +
> > +		if (sysfs_streq(name, ""))
> > +			continue;
> > +
> > +		name = strim(name);
> > +
> > +		for (m = 1; m < PCI_NUM_RESET_METHODS && i < PCI_NUM_RESET_METHODS; m++) {
> > +			if (sysfs_streq(name, pci_reset_fn_methods[m].name) &&
> > +			    !pci_reset_fn_methods[m].reset_fn(pdev, 1)) {
> > +				pdev->reset_methods[i++] = m;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (m == PCI_NUM_RESET_METHODS) {
> > +			kfree(options);
> > +			return -EINVAL;
>
> In this case, I think we have actually updated pdev->reset_methods[],
> but we still return -EINVAL, right?  If we decide to silently ignore
> unrecognized methods, we probably should return success here.
>
Is it okay to do that? I hope it won't cause any trouble for user
scripts

Thanks,
Amey

[...]