lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 20 Dec 2017 11:33:38 -0800
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     Bart Van Assche <Bart.VanAssche@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "jaegeuk@...gle.com" <jaegeuk@...gle.com>
Subject: Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

On 12/20, gregkh@...uxfoundation.org wrote:
> On Tue, Dec 19, 2017 at 02:46:44PM -0800, Jaegeuk Kim wrote:
> > >From 3368207da5988b8fed4e41e6c0f49a60ac014222 Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <jaegeuk@...gle.com>
> > Date: Tue, 26 Sep 2017 20:53:48 -0700
> > Subject: [PATCH 2/2] scsi: ufs: introduce sysfs entries exposing UFS health
> >  info
> > 
> > This patch adds a new sysfs group, namely health, via:
> > 
> >    /sys/devices/soc/X.ufshc/health/
> > 
> > This directory contains the below entries, each of which shows an 8-bytes
> > hex number representing different meanings defined by JEDEC specfication.
> > 
> > Users can simply read these entries to check how their underlying flash
> > storage is getting reached out to its end of life. For example, if
> > lifetimeA shows 0xb, it would be the right time to consider device swap.
> > 
> >  - length
> >    : must be 25h
> > 
> >  - type
> >    : must be 09h
> > 
> >  - eol
> >    00h: Not defined
> >    01h: Normal
> >    02h: Warning
> >    03h: Critical
> > 
> >  - lifetimeA/B
> >    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
> > 
> > Cc: Greg KH <gregkh@...uxfoundation.org>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@...gle.com>
> > ---
> >  Documentation/ABI/testing/sysfs-devices-soc-ufs | 25 +++++++++
> >  MAINTAINERS                                     |  1 +
> >  drivers/scsi/ufs/ufs.h                          |  2 +
> >  drivers/scsi/ufs/ufshcd.c                       | 69 ++++++++++++++++++++++++-
> >  drivers/scsi/ufs/ufshcd.h                       |  1 +
> >  5 files changed, 97 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-devices-soc-ufs
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-devices-soc-ufs b/Documentation/ABI/testing/sysfs-devices-soc-ufs
> > new file mode 100644
> > index 000000000000..313771a383e4
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-soc-ufs
> > @@ -0,0 +1,25 @@
> > +What:		/sys/devices/soc/X.ufshc/health
> > +Date:		September 2017
> > +contact:	Jaegeuk Kim <jaegeuk@...gle.com>
> > +Description:
> > +		This directory contains health information reported by UFS.
> > +		- length must be 25h
> > +		- type must be 09h
> > +		- eol represent
> > +		  00h: Not defined
> > +		  01h: Normal
> > +		  02h: Warning
> > +		  03h: Critical
> > +		- lifetimeA/B
> > +		  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
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aa71ab52fd76..947034319bb4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13999,6 +13999,7 @@ M:	Vinayak Holikatti <vinholikatti@...il.com>
> >  L:	linux-scsi@...r.kernel.org
> >  S:	Supported
> >  F:	Documentation/scsi/ufs.txt
> > +F:	Documentation/ABI/testing/sysfs-devices-soc-ufs
> >  F:	drivers/scsi/ufs/
> >  
> >  UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER DWC HOOKS
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index 54deeb754db5..1af541d56c7d 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -154,6 +154,7 @@ enum desc_idn {
> >  	QUERY_DESC_IDN_RFU_1		= 0x6,
> >  	QUERY_DESC_IDN_GEOMETRY		= 0x7,
> >  	QUERY_DESC_IDN_POWER		= 0x8,
> > +	QUERY_DESC_IDN_HEALTH		= 0x9,
> >  	QUERY_DESC_IDN_MAX,
> >  };
> >  
> > @@ -169,6 +170,7 @@ enum ufs_desc_def_size {
> >  	QUERY_DESC_INTERCONNECT_DEF_SIZE	= 0x06,
> >  	QUERY_DESC_GEOMETRY_DEF_SIZE		= 0x44,
> >  	QUERY_DESC_POWER_DEF_SIZE		= 0x62,
> > +	QUERY_DESC_HEALTH_DEF_SIZE		= 0x25,
> >  };
> >  
> >  /* Unit descriptor parameters offsets in bytes*/
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 12ff7daebb00..5cbb08fff0f4 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -2991,6 +2991,9 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
> >  	case QUERY_DESC_IDN_RFU_1:
> >  		*desc_len = 0;
> >  		break;
> > +	case QUERY_DESC_IDN_HEALTH:
> > +		*desc_len = hba->desc_size.health_desc;
> > +		break;
> >  	default:
> >  		*desc_len = 0;
> >  		return -EINVAL;
> > @@ -6298,6 +6301,11 @@ static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
> >  		&hba->desc_size.geom_desc);
> >  	if (err)
> >  		hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
> > +
> > +	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_HEALTH, 0,
> > +		&hba->desc_size.health_desc);
> > +	if (err)
> > +		hba->desc_size.health_desc = QUERY_DESC_HEALTH_DEF_SIZE;
> >  }
> >  
> >  static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
> > @@ -6308,6 +6316,7 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
> >  	hba->desc_size.conf_desc = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> >  	hba->desc_size.unit_desc = QUERY_DESC_UNIT_DEF_SIZE;
> >  	hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
> > +	hba->desc_size.health_desc = QUERY_DESC_HEALTH_DEF_SIZE;
> >  }
> >  
> >  /**
> > @@ -7688,14 +7697,72 @@ static const struct attribute_group ufshcd_attr_group = {
> >  	.attrs = ufshcd_attrs,
> >  };
> >  
> > +struct health_attr {
> > +	struct attribute attr;
> > +	ssize_t (*show)(struct device *dev,
> > +				struct health_attr *attr, char *buf);
> > +	int byte_offset;
> > +};
> > +
> > +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];
> > +	int err;
> > +
> > +	if (attr->byte_offset >= buff_len)
> > +		return 0;
> > +
> > +	pm_runtime_get_sync(hba->dev);
> > +	err = ufshcd_read_desc(hba, QUERY_DESC_IDN_HEALTH, 0,
> > +					desc_buf, buff_len);
> > +	pm_runtime_put_sync(hba->dev);
> > +	if (err)
> > +		return 0;
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "0x%02x", desc_buf[attr->byte_offset]);
> > +}
> > +
> > +#define HEALTH_ATTR_RO(_name, _byte_offset)				\
> > +static struct health_attr ufs_health_##_name = {			\
> > +	.attr = {.name = __stringify(_name), .mode = 0444},		\
> > +	.show = health_attr_show,					\
> > +	.byte_offset = _byte_offset,					\
> > +}
> 
> This is a nice "hack" to make things simpler, but I worry about not
> using the "normal" __ATTR_* macros here.  But I can't think of a simpler
> way to do this now, except if you had a table of strings and offsets
> that you then used to look up to get the offset based on the name of the
> attribute.

I gave it a try to use DEVICE_ATTR_RO() in v3, so could you please take a look
at it?

Thanks,

> 
> Anyway, nice job, I'm just rambling:
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

Powered by blists - more mailing lists