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@xxxxxxxxxx>
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@xxxxxxxxx>
> > ---
> > +
[...]
> > +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
[...]
