From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "jaegeuk@kernel.org" <jaegeuk@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
CC: "jaegeuk@google.com" <jaegeuk@google.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 2/2] scsi: ufs: use sysfs entry for health info
Thread-Topic: [PATCH 2/2] scsi: ufs: use sysfs entry for health info
Thread-Index: AQHTeQR3YHXxyvYD2kKll5j3lHDaLqNLKLkA
Date: Tue, 19 Dec 2017 21:07:31 +0000
Message-ID: <1513717650.2535.13.camel@wdc.com>
References: <20171219200254.23120-1-jaegeuk@kernel.org>
         <20171219200254.23120-2-jaegeuk@kernel.org>
In-Reply-To: <20171219200254.23120-2-jaegeuk@kernel.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <2019607DEF07F648AC99EA7E290603B5@namprd04.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 260ceb1e-8f43-425e-44cd-08d547248523
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Dec 2017 21:07:31.9150
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0401MB1534
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id vBJL7hQR024857
Content-Length: 2989
Lines: 88

On Tue, 2017-12-19 at 12:02 -0800, Jaegeuk Kim wrote:
> This patch introduces sysfs entries to show the information.

What information does "the information" refer to?

Regarding the patch title: I think this patch introduces new sysfs attributes
instead of using existing sysfs entries. If so, please reflect this in the patch
title.

>  # cat /sys/devices/soc/1da4000.ufshc/health/eol
>  # cat /sys/devices/soc/1da4000.ufshc/health/length
>  # cat /sys/devices/soc/1da4000.ufshc/health/lifetimeA
>  # cat /sys/devices/soc/1da4000.ufshc/health/lifetimeB
>  # cat /sys/devices/soc/1da4000.ufshc/health/type

What is the meaning of the above shell commands in the context of the patch
description?

> struct desc_field_offset health_desc_field_name[] = {
> 	{"bLength",             0x00, BYTE},
> 	{"bDescriptorType",     0x01, BYTE},
> 	{"bPreEOLInfo",         0x02, BYTE},
> 	{"bDeviceLifeTimeEstA", 0x03, BYTE},
> 	{"bDeviceLifeTimeEstB", 0x04, BYTE}
> };

Why has the above data been mentioned in the patch description?

> bPreEOLInfo
>  - 00h: Not defined
>  - 01h: Normal
>  - 02h: Warning
>  - 03h: Critical
> 
> bDeviceLifeTimeEstA
>  - 00h: Not defined
>  - 01h:  0% ~ 10% device life time used
>  - 02h: 10% ~ 20% device life time used
>  - 03h: 20% ~ 30% device life time used
>  - 04h: 30% ~ 40% device life time used
>  - 05h: 40% ~ 50% device life time used
>  - 06h: 50% ~ 60% device life time used
>  - 07h: 60% ~ 70% device life time used
>  - 08h: 70% ~ 80% device life time used
>  - 09h: 80% ~ 90% device life time used
>  - 0Ah: 90% ~ 100% device life time used
>  - 0Bh: Exceeded its maximum estimated device life time

Again, why has the above information been mentioned in the patch description?
 
> +static ssize_t health_attr_show(struct device *dev,
> +				struct health_attr *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	int buff_len = hba->desc_size.health_desc;
> +	u8 desc_buf[hba->desc_size.health_desc];

Is desc_buf[] a variable-length array? If so, how big can
hba->desc_size.health_desc be? Can that number have a negative value?

> +	return scnprintf(buf, PAGE_SIZE, "0x%02x", desc_buf[attr->bytes]);

Please check whether attr->bytes falls inside the bounds of the desc_buf[] array
before using that value as an index.

> +#define HEALTH_ATTR_RO(_name, _bytes)					\
> +static struct health_attr ufs_health_##_name = {			\
> +	.attr = {.name = __stringify(_name), .mode = 0444},		\
> +	.show = health_attr_show,					\
> +	.bytes = _bytes,						\
> +}
> +
> +HEALTH_ATTR_RO(length, 0);
> +HEALTH_ATTR_RO(type, 1);
> +HEALTH_ATTR_RO(eol, 2);
> +HEALTH_ATTR_RO(lifetimeA, 3);
> +HEALTH_ATTR_RO(lifetimeB, 4);

The above makes clear that the value stored in the structure member with the name
"bytes" represents an array index. Please choose a better name for that structure
member.

Additionally, since this patch introduces new sysfs attributes, why doesn't it
add any documentation under Documentation/ABI/?

Thanks,

Bart.