linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shanker R Donthineni <sdonthineni@nvidia.com>
To: Amey Narkhede <ameynarkhede03@gmail.com>,
	Bjorn Helgaas <bhelgaas@google.com>
Cc: <alex.williamson@redhat.com>,
	Raphael Norwitz <raphael.norwitz@nutanix.com>,
	<linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<kw@linux.com>, Sinan Kaya <okaya@kernel.org>,
	Len Brown <lenb@kernel.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH v14 5/9] PCI: Allow userspace to query and set device reset mechanism
Date: Wed, 4 Aug 2021 22:55:45 -0500	[thread overview]
Message-ID: <81810acc-b3b6-1ca7-4f03-aa3c33b80b40@nvidia.com> (raw)
In-Reply-To: <20210804204201.1282-6-ameynarkhede03@gmail.com>

Hi Amey,

On 8/4/21 3:41 PM, Amey Narkhede wrote:
> External email: Use caution opening links or attachments
>
>
> 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>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  19 ++++
>  drivers/pci/pci-sysfs.c                 |   1 +
>  drivers/pci/pci.c                       | 116 ++++++++++++++++++++++++
>  drivers/pci/pci.h                       |   2 +
>  4 files changed, 138 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ef00fada2efb..ef66b62bf025 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -121,6 +121,25 @@ 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 <ameynarkhede03@gmail.com>
> +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. Initially reading
> +               this file will give names of the device supported reset
> +               methods and their ordering. After write, this file will
> +               give names and ordering of currently enabled reset methods.
> +               Writing the name or space 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 <mst@redhat.com>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 316f70c3e3b4..54ee7193b463 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1491,6 +1491,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,
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8a516e9ca316..994426b2b502 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5132,6 +5132,122 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>         { pci_reset_bus_function, .name = "bus" },
>  };
>
> +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, m;
> +
> +       for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
> +               m = pdev->reset_methods[i];
> +               if (!m)
> +                       break;
> +
> +               len += sysfs_emit_at(buf, len, "%s%s", len ? " " : "",
> +                                    pci_reset_fn_methods[m].name);
> +       }
> +
> +       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)
> +{
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       int i, m, n = 0;
> +       char *name, *options = NULL;
> +
> +       if (count >= (PAGE_SIZE - 1))
> +               return -EINVAL;
> +
> +       if (sysfs_streq(buf, "")) {
> +               goto free_and_exit;
> +       }
> +
> +       if (sysfs_streq(buf, "default")) {
> +               pci_init_reset_methods(pdev);
> +               return count;
> +       }
> +
> +       options = kstrndup(buf, count, GFP_KERNEL);
> +       if (!options)
> +               return -ENOMEM;
> +
> +       while ((name = strsep(&options, " ")) != NULL) {
> +               if (sysfs_streq(name, ""))
> +                       continue;
> +
> +               name = strim(name);
> +
> +               for (m = 1; m < PCI_NUM_RESET_METHODS; m++) {
> +                       if (sysfs_streq(name, pci_reset_fn_methods[m].name))
> +                               break;
> +               }
> +
> +               if (m == PCI_NUM_RESET_METHODS) {
> +                       pci_warn(pdev, "Skip invalid reset method '%s'", name);
> +                       continue;
> +               }
> +
> +               for (i = 0; i < n; i++) {
> +                       if (pdev->reset_methods[i] == m)
> +                               break;
> +               }
> +
> +               if (i < n)
> +                       continue;
> +
> +               if (pci_reset_fn_methods[m].reset_fn(pdev, 1)) {
> +                       pci_warn(pdev, "Unsupported reset method '%s'", name);
> +                       continue;
> +               }
> +
> +               pdev->reset_methods[n++] = m;
> +               BUG_ON(n == PCI_NUM_RESET_METHODS);
> +       }
> +
> +free_and_exit:
> +       kfree(options);
No need to initialize 'options' to NULL if you move label 'free_and_exit) to after kfree().
and also improves the code readability. 

> +       /* All the reset methods are invalid */
> +       if (n == 0 && m == PCI_NUM_RESET_METHODS)
> +               return -EINVAL;

Coding bug, uninitialized variable 'm' reference. It contains garbage value
if the user writes NULL string to 'reset_method'. Bjorn also suggested in v10
discard junk/invalid reset methods and return OK to user in case of errors.

Either remove the above two lines or initialize variable 'm' to zero at the
beginning the function to fix the issue.

> +       pdev->reset_methods[n] = 0;
> +       if (pdev->reset_methods[0] == 0) {
> +               pci_warn(pdev, "All device reset methods disabled by user");
> +       } else if ((pdev->reset_methods[0] != 1) &&
> +                  !pci_reset_fn_methods[1].reset_fn(pdev, 1)) {
> +               pci_warn(pdev, "Device specific reset disabled/de-prioritized by user");
> +       }
> +       return count;
> +}
>


  reply	other threads:[~2021-08-05  3:55 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04 20:41 [PATCH v14 0/9] PCI: Expose and manage PCI device reset Amey Narkhede
2021-08-04 20:41 ` [PATCH v14 1/9] PCI: Cache PCIe FLR capability Amey Narkhede
2021-08-04 20:41 ` [PATCH v14 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods Amey Narkhede
2021-08-04 20:41 ` [PATCH v14 3/9] PCI: Add new array for keeping track of ordering of " Amey Narkhede
2021-08-04 20:41 ` [PATCH v14 4/9] PCI: Remove reset_fn field from pci_dev Amey Narkhede
2021-08-04 20:41 ` [PATCH v14 5/9] PCI: Allow userspace to query and set device reset mechanism Amey Narkhede
2021-08-05  3:55   ` Shanker R Donthineni [this message]
2021-08-04 20:41 ` [PATCH v14 6/9] PCI: Define a function to set ACPI_COMPANION in pci_dev Amey Narkhede
2021-08-05  3:19   ` Shanker R Donthineni
2021-08-04 20:41 ` [PATCH v14 7/9] PCI: Setup ACPI fwnode early and at the same time with OF Amey Narkhede
2021-08-04 20:42 ` [PATCH v14 8/9] PCI: Add support for ACPI _RST reset method Amey Narkhede
2021-08-04 20:42 ` [PATCH v14 9/9] PCI: Change the type of probe argument in reset functions Amey Narkhede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=81810acc-b3b6-1ca7-4f03-aa3c33b80b40@nvidia.com \
    --to=sdonthineni@nvidia.com \
    --cc=alex.williamson@redhat.com \
    --cc=ameynarkhede03@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=kw@linux.com \
    --cc=lenb@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=okaya@kernel.org \
    --cc=raphael.norwitz@nutanix.com \
    --cc=rjw@rjwysocki.net \
    --subject='Re: [PATCH v14 5/9] PCI: Allow userspace to query and set device reset mechanism' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).