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;
From:   Raphael Norwitz <raphael.norwitz@nutanix.com>
To:     Amey Narkhede <ameynarkhede03@gmail.com>
CC:     Bjorn Helgaas <bhelgaas@google.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        Raphael Norwitz <raphael.norwitz@nutanix.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kw@linux.com" <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 v9 4/8] PCI/sysfs: Allow userspace to query and set device
 reset mechanism
Thread-Topic: [PATCH v9 4/8] PCI/sysfs: Allow userspace to query and set
 device reset mechanism
Thread-Index: AQHXcaktjT4W9oc3UEOmIGnwEgPzR6s+BoMA
Date:   Sun, 11 Jul 2021 16:51:02 +0000
Message-ID: <20210711165056.GA30721@raphael-debian-dev>
References: <20210705142138.2651-1-ameynarkhede03@gmail.com>
 <20210705142138.2651-5-ameynarkhede03@gmail.com>
In-Reply-To: <20210705142138.2651-5-ameynarkhede03@gmail.com>
Accept-Language: en-US
Content-Language: en-US
user-agent: Mutt/1.10.1 (2018-07-13)
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?WacBp6kc9PrjGBc90hViMWH2yMz6woBrwxbBtGc4ahomgKsqeoSfdp2E+LfC?=
 =?us-ascii?Q?sgylQleUhMEIzpb2kNshRtJGpk+od9S5vofOq1o7BaMxXwuMmgf4WWoKx6hG?=
 =?us-ascii?Q?IgghC8UtkcRdYET3om/qu+Vn8HFJ3Bji6CCLqUMeJve77Q7KggwS/mSztC6C?=
 =?us-ascii?Q?nff5PaeesVoDE9QZNBECc3LJ6n3Qx6J+aUxrnKgAMbS2d4Bl6BDauBtr5Gye?=
 =?us-ascii?Q?SY4o+ReYE1arpXglUwIrOzc2O31rFsKoZPrsrl4cGQL8jO34vd47zFVDNbqw?=
 =?us-ascii?Q?a4t0bfCbyr8j2nAnNfY9ZXsV3t+8j0WoLOZhAZh/SV3fK3LOAt0FtMHaIDtr?=
 =?us-ascii?Q?sdI/1WmMr/BId9afLs9Uc5dSmjPmJxqJk4IqlxUHCscDB+2i0ImYf66X36rH?=
 =?us-ascii?Q?wCNl/AGH/5alvbk8oClywmJKUCDR3LWnKi9Z8WqXi9jPd0ikmjJqYgCYnLi4?=
 =?us-ascii?Q?Dni9/5o4zssvlD5t++XHjLwkVt/O3LTa0jKw8zO7hP7cWoPvBFdoUS/netei?=
 =?us-ascii?Q?Vr1MEoZn/aEgIkBNadpB/qUY5W/chSycTEmcc1ztc2db50G0DKKQy6RT/5IS?=
 =?us-ascii?Q?wWy0vEltUvEih78KWvioXo5iM+O5Fo8le8LVHNyQ/sq7yHC25NQDQ9/Rqk8w?=
 =?us-ascii?Q?DY13HFysdGpRYSqAiuLItby8EmjS2Le5OvSnlFD23uWSB3nrO58mqDFO1gN7?=
 =?us-ascii?Q?eAGNXA51HRmNVgrex+K/Zlxzlq1nXMh4+0y2AEFNRV5Ri0IlwT/9iCA+lHaw?=
 =?us-ascii?Q?m1eG7uDKpfANWLT7D2w5ffDShraO02LLNSjprggtuoDZ9Dor18wCgskhxwgM?=
 =?us-ascii?Q?fZNPaKMWRokfxR0rCNChgi+thkBSonXukqhIMBQl7olZK1tJ9qxqZDEF81xV?=
 =?us-ascii?Q?I8OsGUSMDn7PvsslTlaqwO7CvJKQgX/D4RSwt0BOstJjcIb1xMf0qzycirqg?=
 =?us-ascii?Q?su714z6Ka0Grehi4Fm35aERWKbUt1EhEr6tZ8xIzF2xlUqwJAwymaaTE+6GM?=
 =?us-ascii?Q?o700DFplKkyEfPyVVLGuELLdhxbGqEgAAZhP6RLHc6vpk37LV3bohQjT47K5?=
 =?us-ascii?Q?2malBQzo0NGbbM/7VigVwER0Sr3Q4/zNCBNE0rWeADterBzL+WC8ghD17n0i?=
 =?us-ascii?Q?ELZauvYUs5Xoqc6AOLfS6fb6qj1b3ViAOUMGtYDlBNqa487IWJpyTDQXm5XR?=
 =?us-ascii?Q?g30gVTGc3ry/IhUA0BX9qH/vMN6CuHy7EItk2eN0WBWMioaYqNdxlWTe/GK9?=
 =?us-ascii?Q?0MX3JGBN02uWwXYD8ROSt7qSYH/9awl9QQWvi5+gjTNLIf3aFLKEhQDrvK3S?=
 =?us-ascii?Q?jJocORRXVPQFhCMq+hCE5tHt?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <A959285D80C9E446ACA6FE9D346CF7D8@namprd02.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nutanix.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4543.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b20617e3-74c6-45bb-017f-08d9448c1134
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jul 2021 16:51:02.3137
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: bb047546-786f-4de1-bd75-24e5b6f79043
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: t3sPu4UMshBUNV+k9DUcK8g641j0mWdDVisquKZoEXoOuVJVjbb2koaAD8OF6kgq71luBHw/Nj1d7C2F+ymC+xnkRU8dXjt0gp3cQtrcLNU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR02MB7241
X-Proofpoint-ORIG-GUID: -o39XnhOpTBypQStR5uMu34AU6bD2oN9
X-Proofpoint-GUID: -o39XnhOpTBypQStR5uMu34AU6bD2oN9
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-07-11_10:2021-07-09,2021-07-11 signatures=0
X-Proofpoint-Spam-Reason: safe
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Jul 05, 2021 at 07:51:34PM +0530, Amey Narkhede wrote:
> Add reset_method sysfs attribute to enable user to query and set user
> preferred device reset methods and their ordering.
>=20
> 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>

Just some spacing NITs.

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  19 +++++
>  drivers/pci/pci-sysfs.c                 | 103 ++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
>=20
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/=
testing/sysfs-bus-pci
> index ef00fada2..43f4e33c7 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.
> =20
> +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 comma 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 316f70c3e..8a740e211 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1334,6 +1334,108 @@ static const struct attribute_group pci_dev_rom_a=
ttr_group =3D {
>  	.is_bin_visible =3D pci_dev_rom_attr_is_visible,
>  };
> =20
> +static ssize_t reset_method_show(struct device *dev,

Nit: spaces on the following two lines. "struct device_attribute *attr"
and "char *buf" should be in line with "struct device *dev"

> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct pci_dev *pdev =3D to_pci_dev(dev);
> +	ssize_t len =3D 0;
> +	int i, idx;
> +
> +	for (i =3D 0; i < PCI_NUM_RESET_METHODS; i++) {
> +		idx =3D pdev->reset_methods[i];
> +		if (!idx)
> +			break;
> +
> +		len +=3D sysfs_emit_at(buf, len, "%s%s", len ? "," : "",
> +				     pci_reset_fn_methods[idx].name);
> +	}
> +
> +	if (len)
> +		len +=3D sysfs_emit_at(buf, len, "\n");
> +
> +	return len;
> +}
> +
> +static ssize_t reset_method_store(struct device *dev,

Nit: spaces on the following two lines

> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev =3D to_pci_dev(dev);
> +	int n =3D 0;
> +	char *name, *options =3D NULL;
> +	u8 reset_methods[PCI_NUM_RESET_METHODS] =3D { 0 };
> +
> +	if (count >=3D (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	if (sysfs_streq(buf, "")) {
> +		pci_warn(pdev, "All device reset methods disabled by user");
> +		goto set_reset_methods;
> +	}
> +
> +	if (sysfs_streq(buf, "default")) {
> +		pci_init_reset_methods(pdev);
> +		return count;
> +	}
> +
> +	options =3D kstrndup(buf, count, GFP_KERNEL);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	while ((name =3D strsep(&options, ",")) !=3D NULL) {
> +		int i;
> +
> +		if (sysfs_streq(name, ""))
> +			continue;
> +
> +		name =3D strim(name);
> +
> +		for (i =3D 1; i < PCI_NUM_RESET_METHODS; i++) {
> +			if (sysfs_streq(name, pci_reset_fn_methods[i].name) &&
> +			    !pci_reset_fn_methods[i].reset_fn(pdev, 1)) {
> +				reset_methods[n++] =3D i;
> +				break;
> +			}
> +		}
> +
> +		if (i =3D=3D PCI_NUM_RESET_METHODS) {
> +			kfree(options);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (!pci_reset_fn_methods[1].reset_fn(pdev, 1) && reset_methods[0] !=3D=
 1)
> +		pci_warn(pdev, "Device specific reset disabled/de-prioritized by user"=
);
> +
> +set_reset_methods:
> +	memcpy(pdev->reset_methods, reset_methods, sizeof(reset_methods));
> +	kfree(options);
> +	return count;
> +}
> +static DEVICE_ATTR_RW(reset_method);
> +
> +static struct attribute *pci_dev_reset_method_attrs[] =3D {
> +	&dev_attr_reset_method.attr,
> +	NULL,
> +};
> +
> +static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj=
,

Nit: ditto - spacing.

> +						    struct attribute *a, int n)
> +{
> +	struct pci_dev *pdev =3D to_pci_dev(kobj_to_dev(kobj));
> +
> +	if (!pci_reset_supported(pdev))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +static const struct attribute_group pci_dev_reset_method_attr_group =3D =
{
> +	.attrs =3D pci_dev_reset_method_attrs,
> +	.is_visible =3D pci_dev_reset_method_attr_is_visible,
> +};
> +
>  static ssize_t reset_store(struct device *dev, struct device_attribute *=
attr,

Nit: spacing following line.

>  			   const char *buf, size_t count)
>  {
> @@ -1491,6 +1593,7 @@ const struct attribute_group *pci_dev_groups[] =3D =
{
>  	&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,
> --=20
> 2.32.0
>=20
> =