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;
IronPort-SDR: Y8LpZ8I2sdQh9lNRVNPkkQRbkQgUxVT4EMM38WivqJZT1SGW7AE00AC42OHUKQrPq6te1btKk1
 sRrgCNqdzI//zintKJLLwtbEkTa3mATDZSB/jWLBRUV5EtroKax2CDMRDnbweRMVC+4w6Gq/or
 r8Un5CrKR/efdvDARzgY+mvRA9/8ZIssLuDqYIeehBiVH2XkslRNIq59i3UtvQSy+bp8UQ+WoF
 d76x6HlHwzudVMA+n5alcZ4ngSmA1TZeGgtStZ+ye2mZIjdgk8GydE3CXbJwJofz1uraGwEm3R
 KdU=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Daniel Wagner <dwagner@suse.de>
CC:     "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        Hannes Reinecke <hare@suse.de>,
        Chao Leng <lengchao@huawei.com>,
        Victor Gladkov <Victor.Gladkov@kioxia.com>
Subject: Re: [PATCH] nvme: Export fast_io_fail_tmo to sysfs
Thread-Topic: [PATCH] nvme: Export fast_io_fail_tmo to sysfs
Thread-Index: AQHXJi+J720hbwseC0WZ8xLPt2l/cg==
Date:   Wed, 31 Mar 2021 20:34:31 +0000
Message-ID: <BYAPR04MB49658F0DDA22B253F98C6F3A867C9@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20210331131228.131118-1-dwagner@suse.de>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b2cfa361-350e-42bd-d201-08d8f48463c3
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Mar 2021 20:34:31.8613
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: rvDeFEbKzle6qGUJyH8kb/1eWlG7KoSUjATR4A0pgsbJytiVvLPCksLUxDj5mPDtRrjz7O8+lgQJFk9gowy8HAEhTnYL9PToQRbnsII1Ugc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5592
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Daniel,=0A=
=0A=
On 3/31/21 06:12, Daniel Wagner wrote:=0A=
> Commit 8c4dfea97f15 ("nvme-fabrics: reject I/O to offline device")=0A=
> introduced fast_io_fail_tmo but didn't export the value to sysfs. That=0A=
> means the value is hard coded during compile time. Export the timeout=0A=
> value to user space via sysfs to allow runtime configuration.=0A=
>=0A=
> Cc: Victor Gladkov <Victor.Gladkov@kioxia.com>=0A=
> Signed-off-by: Daniel Wagner <dwagner@suse.de>=0A=
> ---=0A=
>=0A=
> This patch is against nvme-5.13=0A=
>=0A=
> BTW, checkpatch complains with=0A=
>=0A=
>   WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Co=
nsider using octal permissions '0644'.=0A=
=0A=
For now keep the current style.=0A=
=0A=
>=0A=
> Is this something we want to adapt to?=0A=
>=0A=
>  drivers/nvme/host/core.c | 31 +++++++++++++++++++++++++++++++=0A=
>  1 file changed, 31 insertions(+)=0A=
>=0A=
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c=0A=
> index 40215a0246e4..c8de0e37c7d9 100644=0A=
> --- a/drivers/nvme/host/core.c=0A=
> +++ b/drivers/nvme/host/core.c=0A=
> @@ -3696,6 +3696,36 @@ static ssize_t nvme_ctrl_reconnect_delay_store(str=
uct device *dev,=0A=
>  static DEVICE_ATTR(reconnect_delay, S_IRUGO | S_IWUSR,=0A=
>  	nvme_ctrl_reconnect_delay_show, nvme_ctrl_reconnect_delay_store);=0A=
>  =0A=
> +static ssize_t nvme_ctrl_fast_io_fail_tmo_show(struct device *dev,=0A=
> +		struct device_attribute *attr, char *buf)=0A=
> +{=0A=
> +	struct nvme_ctrl *ctrl =3D dev_get_drvdata(dev);=0A=
> +=0A=
> +	if (ctrl->opts->fast_io_fail_tmo =3D=3D -1)=0A=
> +		return sprintf(buf, "off\n");=0A=
> +	return sprintf(buf, "%d\n", ctrl->opts->fast_io_fail_tmo);=0A=
=0A=
do we need snprintf() for 2nd ?=0A=
=0A=
> +}=0A=
> +=0A=
> +static ssize_t nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,=0A=
> +		struct device_attribute *attr, const char *buf, size_t count)=0A=
> +{=0A=
> +	struct nvme_ctrl *ctrl =3D dev_get_drvdata(dev);=0A=
> +	struct nvmf_ctrl_options *opts =3D ctrl->opts;=0A=
> +	int fast_io_fail_tmo, err;=0A=
> +=0A=
> +	err =3D kstrtoint(buf, 10, &fast_io_fail_tmo);=0A=
> +	if (err)=0A=
> +		return -EINVAL;=0A=
> +=0A=
=0A=
since you are returning an error, you can remove next else if, this also=0A=
removes the extra line after above return. Something like this on the top=
=0A=
of yours totally untested :-=0A=
=0A=
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c=0A=
index c8de0e37c7d9..afa0a6790b52 100644=0A=
--- a/drivers/nvme/host/core.c=0A=
+++ b/drivers/nvme/host/core.c=0A=
@@ -3717,7 +3717,7 @@ static ssize_t=0A=
nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,=0A=
        if (err)=0A=
                return -EINVAL;=0A=
 =0A=
-       else if (fast_io_fail_tmo < 0)=0A=
+       if (fast_io_fail_tmo < 0)=0A=
                opts->fast_io_fail_tmo =3D -1;=0A=
        else=0A=
                opts->fast_io_fail_tmo =3D fast_io_fail_tmo;=0A=
=0A=
> +	else if (fast_io_fail_tmo < 0)=0A=
> +		opts->fast_io_fail_tmo =3D -1;=0A=
> +	else=0A=
> +		opts->fast_io_fail_tmo =3D fast_io_fail_tmo;=0A=
> +	return count;=0A=
> +}=0A=
> +static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,=0A=
> +	nvme_ctrl_fast_io_fail_tmo_show, nvme_ctrl_fast_io_fail_tmo_store);=0A=
> +=0A=
>  static struct attribute *nvme_dev_attrs[] =3D {=0A=
>  	&dev_attr_reset_controller.attr,=0A=
>  	&dev_attr_rescan_controller.attr,=0A=
> @@ -3715,6 +3745,7 @@ static struct attribute *nvme_dev_attrs[] =3D {=0A=
>  	&dev_attr_hostid.attr,=0A=
>  	&dev_attr_ctrl_loss_tmo.attr,=0A=
>  	&dev_attr_reconnect_delay.attr,=0A=
> +	&dev_attr_fast_io_fail_tmo.attr,=0A=
>  	NULL=0A=
>  };=0A=
>  =0A=
> -- 2.29.2=0A=
=0A=
=0A=