Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 3 Aug 2021 12:04:24 +0530
From:   Amey Narkhede <ameynarkhede03@gmail.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, alex.williamson@redhat.com,
        Raphael Norwitz <raphael.norwitz@nutanix.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        kw@linux.com, Shanker Donthineni <sdonthineni@nvidia.com>,
        Sinan Kaya <okaya@kernel.org>, Len Brown <lenb@kernel.org>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH v13 5/9] PCI: Allow userspace to query and set device
 reset mechanism
Message-ID: <20210803063424.aybmwdxxj6bt2iax@archlinux>
References: <20210801142518.1224-6-ameynarkhede03@gmail.com>
 <20210802225559.GA1472320@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210802225559.GA1472320@bjorn-Precision-5520>
Precedence: bulk

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

[...]