linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: ameynarkhede03@gmail.com
Cc: bhelgaas@google.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, alex.williamson@redhat.com,
	raphael.norwitz@nutanix.com
Subject: Re: [PATCH 4/4] PCI/sysfs: Allow userspace to query and set device reset mechanism
Date: Mon, 15 Mar 2021 00:55:45 +0100	[thread overview]
Message-ID: <20210314235545.girtrazsdxtrqo2q@pali> (raw)
In-Reply-To: <20210312173452.3855-5-ameynarkhede03@gmail.com>

On Friday 12 March 2021 23:04:52 ameynarkhede03@gmail.com wrote:
> From: Amey Narkhede <ameynarkhede03@gmail.com>
> 
> Add reset_methods_enabled bitmap to struct pci_dev to
> keep track of user preferred device reset mechanisms.
> Add reset_method sysfs attribute to query and set
> user preferred device reset mechanisms.
> 
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> ---
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
>  Documentation/ABI/testing/sysfs-bus-pci | 15 ++++++
>  drivers/pci/pci-sysfs.c                 | 66 +++++++++++++++++++++++--
>  drivers/pci/pci.c                       |  3 +-
>  include/linux/pci.h                     |  2 +
>  4 files changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 25c9c3977..ae53ecd2e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -121,6 +121,21 @@ 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. Reading this file will give names
> +		of the device supported reset methods. Currently used methods
> +		are enclosed in brackets. Writing the name of any of the device
> +		supported reset method to this file will set the reset method to
> +		be used when resetting the device. Writing "none" to this file
> +		will disable ability to reset the device and writing "default"
> +		will return to the original value.
> +

Hello Amey!

I think that this API does not work for PCIe Hot Reset (=PCI secondary
bus reset) and PCIe Warm Reset.

First reset method is bound to the bus, not device and therefore kernel
does not have to see any registered device. So there would be no
"reset_method" sysfs file, and also no "reset" sysfs file. But PCIe Hot
Reset is in most cases needed when buggy card is not registered on bus,
to trigger this reset. And with this API this is not possible.

PCIe Warm Reset is done by PERST# signal. When signal is asserted then
device is in reset state and therefore is not registered. So again
kernel does not have to see registered device.

Moreover for mPCIe form factor cards, boards can share one PERST# signal
with more PCIe cards and control this signal via GPIO. So asserting
PERST# GPIO can trigger Warm reset for more PCIe cards, not just one. It
depends on board or topology.

So... I do not think that current approach with "reset_method" sysfs
entry bound to the PCI device does not work for PCI secondary bus reset
and also cannot be used for implementing PCIe Warm Reset.

I would rather suggest to re-design and prepare a new API which would
work also with PCIe Hot Reset and PCIe Warm Reset.

This "reset" sysfs file can work only with PCI Function Level Reset or
some PM or device specific reset. But not with reset types which are
more like slot or bus orientated.

>  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 78d2c130c..3cd06d1c0 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1304,6 +1304,59 @@ static const struct bin_attribute pcie_config_attr = {
>  	.write = pci_write_config,
>  };
> 
> +static ssize_t reset_method_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	const struct pci_reset_fn_method *reset;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0, reset = pci_reset_fn_methods; reset->reset_fn; i++, reset++) {
> +		if (pdev->reset_methods_enabled & (1 << i))
> +			len += sysfs_emit_at(buf, len, "[%s] ", reset->name);
> +		else if (pdev->reset_methods & (1 << i))
> +			len += sysfs_emit_at(buf, len, "%s ", reset->name);
> +	}
> +
> +	return len;
> +}
> +
> +static ssize_t reset_method_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	const struct pci_reset_fn_method *reset = pci_reset_fn_methods;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	u8 reset_mechanism;
> +	int i = 0;
> +
> +	/* Writing none disables reset */
> +	if (sysfs_streq(buf, "none")) {
> +		reset_mechanism = 0;
> +	} else if (sysfs_streq(buf, "default")) {
> +		/* Writing default returns to initial value */
> +		reset_mechanism = pdev->reset_methods;
> +	} else {
> +		reset_mechanism = 0;
> +		for (; reset->reset_fn; i++, reset++) {
> +			if (sysfs_streq(buf, reset->name)) {
> +				reset_mechanism = 1 << i;
> +				break;
> +			}
> +		}
> +		if (!reset_mechanism || !(pdev->reset_methods & reset_mechanism))
> +			return -EINVAL;
> +	}
> +
> +	pdev->reset_methods_enabled = reset_mechanism;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(reset_method);
> +
>  static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  			   const char *buf, size_t count)
>  {
> @@ -1337,11 +1390,16 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  	if (dev->reset_methods) {
>  		retval = device_create_file(&dev->dev, &dev_attr_reset);
>  		if (retval)
> -			goto error;
> +			goto err_reset;
> +		retval = device_create_file(&dev->dev, &dev_attr_reset_method);
> +		if (retval)
> +			goto err_method;
>  	}
>  	return 0;
> 
> -error:
> +err_method:
> +	device_remove_file(&dev->dev, &dev_attr_reset);
> +err_reset:
>  	pcie_vpd_remove_sysfs_dev_files(dev);
>  	return retval;
>  }
> @@ -1417,8 +1475,10 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>  static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>  {
>  	pcie_vpd_remove_sysfs_dev_files(dev);
> -	if (dev->reset_methods)
> +	if (dev->reset_methods) {
>  		device_remove_file(&dev->dev, &dev_attr_reset);
> +		device_remove_file(&dev->dev, &dev_attr_reset_method);
> +	}
>  }
> 
>  /**
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b7f6c6588..81cebea56 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5106,7 +5106,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>  	might_sleep();
> 
>  	for (i = 0, reset = pci_reset_fn_methods; reset->reset_fn; i++, reset++) {
> -		if (!(dev->reset_methods & (1 << i)))
> +		if (!(dev->reset_methods_enabled & (1 << i)))
>  			continue;
> 
>  		/*
> @@ -5153,6 +5153,7 @@ void pci_init_reset_methods(struct pci_dev *dev)
>  		else if (rc != -ENOTTY)
>  			break;
>  	}
> +	dev->reset_methods_enabled = dev->reset_methods;
>  }
> 
>  /**
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a2f003f4e..400f614e0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -335,6 +335,8 @@ struct pci_dev {
>  	 * See pci_reset_fn_methods array in pci.c
>  	 */
>  	u8 __bitwise reset_methods;		/* bitmap for device supported reset capabilities */
> +	/* bitmap for user enabled and device supported reset capabilities */
> +	u8 __bitwise reset_methods_enabled;
>  #ifdef CONFIG_PCIEAER
>  	u16		aer_cap;	/* AER capability offset */
>  	struct aer_stats *aer_stats;	/* AER stats for this device */
> --
> 2.30.2

  reply	other threads:[~2021-03-14 23:56 UTC|newest]

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-12 17:34 [PATCH 0/4] Expose and manage PCI device reset ameynarkhede03
2021-03-12 17:34 ` [PATCH 1/4] PCI: Refactor pcie_flr to follow calling convention of other reset methods ameynarkhede03
2021-03-12 17:34 ` [PATCH 2/4] PCI: Add new bitmap for keeping track of supported reset mechanisms ameynarkhede03
2021-03-14 23:51   ` Pali Rohár
2021-03-12 17:34 ` [PATCH 3/4] PCI: Remove reset_fn field from pci_dev ameynarkhede03
2021-03-14 23:52   ` Pali Rohár
2021-03-12 17:34 ` [PATCH 4/4] PCI/sysfs: Allow userspace to query and set device reset mechanism ameynarkhede03
2021-03-14 23:55   ` Pali Rohár [this message]
2021-03-15 13:43     ` Amey Narkhede
2021-03-15 13:52       ` Pali Rohár
2021-03-15 14:34         ` Alex Williamson
2021-03-15 14:52           ` Pali Rohár
2021-03-15 15:03             ` Alex Williamson
2021-03-17 19:02               ` Pali Rohár
2021-03-17 19:15                 ` Alex Williamson
2021-03-17 19:24                   ` Pali Rohár
2021-03-17 19:32                     ` Alex Williamson
2021-03-17 19:40                       ` Pali Rohár
2021-03-17 20:00                         ` Alex Williamson
2021-03-17 20:13                           ` Pali Rohár
2021-03-18 14:31                             ` Amey Narkhede
2021-03-23 14:34                               ` Pali Rohár
2021-03-23 14:44                                 ` Alex Williamson
2021-03-23 15:32                                   ` Amey Narkhede
2021-03-23 16:06                                     ` Alex Williamson
2021-03-23 16:15                                       ` Alex Williamson
2021-03-15 15:07           ` Leon Romanovsky
2021-03-15 15:33             ` Amey Narkhede
2021-03-15 16:29               ` Alex Williamson
2021-03-15 18:32                 ` Raphael Norwitz
2021-03-17  4:20                   ` Leon Romanovsky
2021-03-17 10:24                     ` Amey Narkhede
2021-03-17 11:02                       ` Leon Romanovsky
2021-03-17 11:23                         ` Amey Narkhede
2021-03-17 11:47                           ` Leon Romanovsky
2021-03-17 13:17                             ` Amey Narkhede
2021-03-17 13:58                               ` Leon Romanovsky
2021-03-17 17:31                                 ` Alex Williamson
2021-03-18  9:09                                   ` Leon Romanovsky
2021-03-18 14:22                                     ` Amey Narkhede
2021-03-18 14:57                                       ` Leon Romanovsky
2021-03-18 17:01                                         ` Amey Narkhede
2021-03-18 17:35                                           ` Leon Romanovsky
2021-03-18 17:43                                             ` Amey Narkhede
2021-03-18 18:14                                               ` Enrico Weigelt, metux IT consult
2021-03-19 13:05                                               ` Leon Romanovsky
2021-03-19 15:23                                                 ` Amey Narkhede
2021-03-19 15:37                                                   ` Leon Romanovsky
2021-03-19 15:53                                                     ` Amey Narkhede
2021-03-18 17:58                                             ` Enrico Weigelt, metux IT consult
2021-03-19 13:07                                               ` Leon Romanovsky
2021-03-18 16:39                                     ` Alex Williamson
2021-03-18 17:22                                       ` Leon Romanovsky
2021-03-18 17:38                                         ` Amey Narkhede
2021-03-18 18:34                                         ` Enrico Weigelt, metux IT consult
2021-03-19 12:59                                           ` Leon Romanovsky
2021-03-19 13:48                                             ` Enrico Weigelt, metux IT consult
2021-03-19 15:51                                               ` Leon Romanovsky
2021-03-19 15:57                                             ` Bjorn Helgaas
2021-03-19 16:24                                               ` Leon Romanovsky
2021-03-19 16:23                                             ` Alex Williamson
2021-03-20  9:10                                               ` Leon Romanovsky
2021-03-20 14:59                                                 ` Alex Williamson
2021-03-21  8:40                                                   ` Leon Romanovsky
2021-03-21 14:57                                                     ` Amey Narkhede
2021-03-22 17:10                                                     ` Alex Williamson
2021-03-24 10:03                                                       ` Leon Romanovsky
2021-03-24 14:37                                                         ` Alex Williamson
2021-03-24 15:13                                                           ` Leon Romanovsky
2021-03-24 17:17                                                             ` Alex Williamson
2021-03-25  8:37                                                               ` Leon Romanovsky
2021-03-25 14:55                                                                 ` Alex Williamson
2021-03-25 16:09                                                                   ` Leon Romanovsky
2021-03-25 17:22                                                                     ` Amey Narkhede
2021-03-25 17:36                                                                       ` Leon Romanovsky
2021-03-25 17:53                                                                     ` Alex Williamson
2021-03-26  6:40                                                                       ` Leon Romanovsky
2021-03-26  9:18                                                                         ` Krzysztof Wilczyński
2021-03-26 12:54                                                                           ` Leon Romanovsky
2021-03-26 14:20                                                                         ` Alex Williamson
2021-03-27  6:02                                                                           ` Leon Romanovsky
2021-03-25 16:26                                                                 ` Amey Narkhede
2021-03-25 16:46                                                                   ` Leon Romanovsky
2021-03-18 17:51     ` Enrico Weigelt, metux IT consult
     [not found] ` <20210312112043.3f2954e3@omen.home.shazbot.org>
2021-03-12 18:40   ` [PATCH 0/4] Expose and manage PCI device reset Amey Narkhede
2021-03-12 18:58     ` Krzysztof Wilczyński
2021-03-12 19:06       ` Amey Narkhede
2021-03-12 19:20         ` Krzysztof Wilczyński
2021-03-13  2:02     ` Raphael Norwitz
2021-03-14 12:09 ` Leon Romanovsky

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=20210314235545.girtrazsdxtrqo2q@pali \
    --to=pali@kernel.org \
    --cc=alex.williamson@redhat.com \
    --cc=ameynarkhede03@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=raphael.norwitz@nutanix.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).