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:   Thu, 20 May 2021 18:37:37 +0200
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Amey Narkhede <ameynarkhede03@gmail.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, alex.williamson@redhat.com,
        raphael.norwitz@nutanix.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RESEND v2 5/7] PCI/sysfs: Allow userspace to query and
 set device reset mechanism
Message-ID: <20210520163737.GD641812@rocinante.localdomain>
References: <20210519235426.99728-1-ameynarkhede03@gmail.com>
 <20210519235426.99728-6-ameynarkhede03@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210519235426.99728-6-ameynarkhede03@gmail.com>
Precedence: bulk

Hi Amey,

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

The sysfs_streq() is nice, indeed.

> +	while ((name = strsep((char **)&buf, ",")) != NULL) {

I believe we could make this parsing a little bit more resilient,
especially since we are handling user input and this cannot ever be
really fully trusted, so for example:

  while ((name = strsep((char **)&buf, ","))) {
	if !(strlen(name))  <--- sysfs_streq() could be used here too.
		continue;

	name = strim(name); <--- remove leading and trailing whitespaces, if any.
	(...)

[...]
> +	if (reset_methods[0] &&
> +	    reset_methods[0] != PCI_RESET_FN_METHODS)
> +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");

What would be difference between disabling and de-prioritizing, is there
be a way for us to distinguish between the two?  I was wondering if we
could, notify the user when the device specific reset is disable or when
it has been de-prioritized?

Krzysztof