scsi: ufs: introduce static sysfs entries

PATCH Superseded Jaegeuk Kim 3 participants 10 messages Dec 19, 2017 – Dec 20, 2017 SCSI

lore

Share & tools

lore

Jaegeuk Kim

author

2017-12-19 20:03 UTC #0 lore raw

PATCH 1/2 scsi: ufs: introduce static sysfs entries

From: Jaegeuk Kim <[email protected]>

This patch introduces attribute group to show existing sysfs entries.

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 48 +++++++++++++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h |  2 --
 2 files changed, 19 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +19 ~~-29~~

index 011c3369082c..12ff7daebb00 100644
@@ -7605,7 +7605,7 @@ static inline ssize_t ufshcd_pm_lvl_store(struct device *dev,
 	return count;
 }
 
-static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
+static ssize_t rpm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7634,24 +7634,13 @@ static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_rpm_lvl_store(struct device *dev,
+static ssize_t rpm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, true);
 }
 
-static void ufshcd_add_rpm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->rpm_lvl_attr.show = ufshcd_rpm_lvl_show;
-	hba->rpm_lvl_attr.store = ufshcd_rpm_lvl_store;
-	sysfs_attr_init(&hba->rpm_lvl_attr.attr);
-	hba->rpm_lvl_attr.attr.name = "rpm_lvl";
-	hba->rpm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->rpm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for rpm_lvl\n");
-}
-
-static ssize_t ufshcd_spm_lvl_show(struct device *dev,
+static ssize_t spm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7680,33 +7669,34 @@ static ssize_t ufshcd_spm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_spm_lvl_store(struct device *dev,
+static ssize_t spm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, false);
 }
 
-static void ufshcd_add_spm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->spm_lvl_attr.show = ufshcd_spm_lvl_show;
-	hba->spm_lvl_attr.store = ufshcd_spm_lvl_store;
-	sysfs_attr_init(&hba->spm_lvl_attr.attr);
-	hba->spm_lvl_attr.attr.name = "spm_lvl";
-	hba->spm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->spm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for spm_lvl\n");
-}
+static DEVICE_ATTR_RW(rpm_lvl);
+static DEVICE_ATTR_RW(spm_lvl);
+
+static struct attribute *ufshcd_attrs[] = {
+	&dev_attr_rpm_lvl.attr,
+	&dev_attr_spm_lvl.attr,
+	NULL
+};
+
+static const struct attribute_group ufshcd_attr_group = {
+	.attrs = ufshcd_attrs,
+};
 
 static inline void ufshcd_add_sysfs_nodes(struct ufs_hba *hba)
 {
-	ufshcd_add_rpm_lvl_sysfs_nodes(hba);
-	ufshcd_add_spm_lvl_sysfs_nodes(hba);
+	if (sysfs_create_group(&hba->dev->kobj, &ufshcd_attr_group))
+		dev_err(hba->dev, "Failed to create sysfs group\n");
 }
 
 static inline void ufshcd_remove_sysfs_nodes(struct ufs_hba *hba)
 {
-	device_remove_file(hba->dev, &hba->rpm_lvl_attr);
-	device_remove_file(hba->dev, &hba->spm_lvl_attr);
+	sysfs_remove_group(&hba->dev->kobj, &ufshcd_attr_group);
 }
 
 /**

drivers/scsi/ufs/ufshcd.h +0 -2

index 1332e544da92..56e26eb15185 100644
@@ -526,8 +526,6 @@ struct ufs_hba {
 	enum ufs_pm_level rpm_lvl;
 	/* Desired UFS power management level during system PM */
 	enum ufs_pm_level spm_lvl;
-	struct device_attribute rpm_lvl_attr;
-	struct device_attribute spm_lvl_attr;
 	int pm_op_in_progress;
 
 	struct ufshcd_lrb *lrb;

Signature · 1 line

2.15.0.531.g2ccb3012c9-goog

Jaegeuk Kim

author

2017-12-19 20:03 UTC #1 lore raw

PATCH 2/2 scsi: ufs: use sysfs entry for health info

From: Jaegeuk Kim <[email protected]>

This patch introduces sysfs entries to show the information.

 # 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

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

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

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 drivers/scsi/ufs/ufs.h    |  2 ++
 drivers/scsi/ufs/ufshcd.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshcd.h |  1 +
 3 files changed, 68 insertions(+), 1 deletion(-)

drivers/scsi/ufs/ufs.h +2 -0

index 54deeb754db5..1af541d56c7d 100644
@@ -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*/

drivers/scsi/ufs/ufshcd.c +65 -1

97 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +1 -0

index 56e26eb15185..ba44cbcf755e 100644
@@ -229,6 +229,7 @@ struct ufs_desc_size {
 	int interc_desc;
 	int unit_desc;
 	int conf_desc;
+	int health_desc;
 };
 
 /**

Signature · 1 line

2.15.0.531.g2ccb3012c9-goog

Bart Van Assche

2017-12-19 21:07 UTC #2 lore raw

Re: [PATCH 2/2] scsi: ufs: use sysfs entry for health info

Quoted 1 line · Jaegeuk Kim

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.

Quoted 5 lines

 # 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?

Quoted 7 lines

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?

Quoted 6 lines

bPreEOLInfo
- 00h: Not defined
- 01h: Normal
- 02h: Warning
- 03h: Critical

Quoted 13 lines

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?

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7712

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +6 -0

+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?

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7722

Quoted 1 line

drivers/scsi/ufs/ufshcd.c +1 -0

+	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.

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7736

Quoted 12 lines

drivers/scsi/ufs/ufshcd.c +12 -0

+#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.

Jaegeuk Kim

author

2017-12-19 22:45 UTC #3 lore raw

Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

Quoted 48 lines · Bart Van Assche

Quoted 1 line · Jaegeuk Kim
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.
Quoted 5 lines
 # 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?
Quoted 7 lines
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?
Quoted 6 lines
bPreEOLInfo
- 00h: Not defined
- 01h: Normal
- 02h: Warning
- 03h: Critical
Quoted 13 lines
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

Quoted 1 line

Again, why has the above information been mentioned in the patch description?

Let me send v2.

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7712
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
+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?

IIUC, it is given by UFS which must be valid. Otherwise, it should be QUERY_DESC_HEALTH_DEF_SIZE which is valid again. This is similarly being done in other sysfs entries here.

Quoted 5 lines

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7722
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +1 -0
+	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.

Okay.

Quoted 17 lines

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7736
Quoted 13 lines
drivers/scsi/ufs/ufshcd.c +12 -0
+#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.

Changed to byte_offset.

Quoted 2 lines

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

Added.

Thanks,

Quoted 4 lines

Thanks,
Bart.

Jaegeuk Kim

author

2017-12-19 22:46 UTC #4 lore raw

Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

In reply to Bart Van Assche

Quoted 1 line

From 3368207da5988b8fed4e41e6c0f49a60ac014222 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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 <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 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

Documentation/ABI/testing/sysfs-devices-soc-ufs new file +25 -0

new file mode 100644
index 000000000000..313771a383e4
@@ -0,0 +1,25 @@
+What:		/sys/devices/soc/X.ufshc/health
+Date:		September 2017
+contact:	Jaegeuk Kim <[email protected]>
+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

MAINTAINERS +1 -0

index aa71ab52fd76..947034319bb4 100644
@@ -13999,6 +13999,7 @@ M:	Vinayak Holikatti <[email protected]>
 L:	[email protected]
 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

drivers/scsi/ufs/ufs.h +2 -0

index 54deeb754db5..1af541d56c7d 100644
@@ -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*/

drivers/scsi/ufs/ufshcd.c +68 -1

100 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +1 -0

index 56e26eb15185..ba44cbcf755e 100644
@@ -229,6 +229,7 @@ struct ufs_desc_size {
 	int interc_desc;
 	int unit_desc;
 	int conf_desc;
+	int health_desc;
 };
 
 /**

Signature · 1 line

2.15.0.531.g2ccb3012c9-goog

Bart Van Assche

2017-12-19 23:53 UTC #5 lore raw

Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

Quoted 6 lines · Jaegeuk Kim

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/

Thanks for the quick respin. This looks a lot better to me. I will leave it to a UFS expert to do an in-depth review.

Bart.

[email protected]

Reviewed-by

2017-12-20 09:26 UTC #6 lore raw

Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

In reply to Jaegeuk Kim

Quoted 179 lines · Jaegeuk Kim

Quoted 1 line

From 3368207da5988b8fed4e41e6c0f49a60ac014222 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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.

- length
: must be 25h

- type
: must be 09h

- eol
00h: Not defined
01h: Normal
02h: Warning
03h: Critical

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 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

Documentation/ABI/testing/sysfs-devices-soc-ufs new file +25 -0

new file mode 100644
index 000000000000..313771a383e4
@@ -0,0 +1,25 @@
+What:		/sys/devices/soc/X.ufshc/health
+Date:		September 2017
+contact:	Jaegeuk Kim <[email protected]>
+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

MAINTAINERS +1 -0

index aa71ab52fd76..947034319bb4 100644
@@ -13999,6 +13999,7 @@ M:	Vinayak Holikatti <[email protected]>
 L:	[email protected]
 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

drivers/scsi/ufs/ufs.h +2 -0

index 54deeb754db5..1af541d56c7d 100644
@@ -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*/

drivers/scsi/ufs/ufshcd.c +33 -0

index 12ff7daebb00..5cbb08fff0f4 100644
@@ -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;

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7733

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +10 -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.

Anyway, nice job, I'm just rambling:

Reviewed-by Greg Kroah-Hartman <[email protected]>

Jaegeuk Kim

author

2017-12-20 19:33 UTC #7 lore raw

Re: [PATCH 2/2 v2] scsi: ufs: use sysfs entry for health info

Quoted 188 lines · On 12/20, [email protected] wrote:

Quoted 187 lines · Jaegeuk Kim

Quoted 1 line

From 3368207da5988b8fed4e41e6c0f49a60ac014222 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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.

- length
: must be 25h

- type
: must be 09h

- eol
00h: Not defined
01h: Normal
02h: Warning
03h: Critical

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 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

Documentation/ABI/testing/sysfs-devices-soc-ufs new file +25 -0

new file mode 100644
index 000000000000..313771a383e4
@@ -0,0 +1,25 @@
+What:		/sys/devices/soc/X.ufshc/health
+Date:		September 2017
+contact:	Jaegeuk Kim <[email protected]>
+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

MAINTAINERS +1 -0

index aa71ab52fd76..947034319bb4 100644
@@ -13999,6 +13999,7 @@ M:	Vinayak Holikatti <[email protected]>
 L:	[email protected]
 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

drivers/scsi/ufs/ufs.h +2 -0

index 54deeb754db5..1af541d56c7d 100644
@@ -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*/

drivers/scsi/ufs/ufshcd.c +41 -0

index 12ff7daebb00..5cbb08fff0f4 100644
@@ -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,					\

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static const struct attribute_group ufshcd_attr_group = { · line 7733
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	.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,

Quoted 4 lines

Anyway, nice job, I'm just rambling:
Reviewed-by Greg Kroah-Hartman <[email protected]>

Greg KH

Acked-by on v1 1/2

2017-12-20 08:02 UTC #8 lore raw

In reply to Jaegeuk Kim

Quoted 74 lines · Jaegeuk Kim

From: Jaegeuk Kim <[email protected]>

This patch introduces attribute group to show existing sysfs entries.

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 48 +++++++++++++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h |  2 --
 2 files changed, 19 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +6 ~~-25~~

index 011c3369082c..12ff7daebb00 100644
@@ -7605,7 +7605,7 @@ static inline ssize_t ufshcd_pm_lvl_store(struct device *dev,
 	return count;
 }
 
-static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
+static ssize_t rpm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7634,24 +7634,13 @@ static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_rpm_lvl_store(struct device *dev,
+static ssize_t rpm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, true);
 }
 
-static void ufshcd_add_rpm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->rpm_lvl_attr.show = ufshcd_rpm_lvl_show;
-	hba->rpm_lvl_attr.store = ufshcd_rpm_lvl_store;
-	sysfs_attr_init(&hba->rpm_lvl_attr.attr);
-	hba->rpm_lvl_attr.attr.name = "rpm_lvl";
-	hba->rpm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->rpm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for rpm_lvl\n");
-}
-
-static ssize_t ufshcd_spm_lvl_show(struct device *dev,
+static ssize_t spm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7680,33 +7669,34 @@ static ssize_t ufshcd_spm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_spm_lvl_store(struct device *dev,
+static ssize_t spm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, false);
 }
 
-static void ufshcd_add_spm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->spm_lvl_attr.show = ufshcd_spm_lvl_show;
-	hba->spm_lvl_attr.store = ufshcd_spm_lvl_store;
-	sysfs_attr_init(&hba->spm_lvl_attr.attr);
-	hba->spm_lvl_attr.attr.name = "spm_lvl";
-	hba->spm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->spm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for spm_lvl\n");
-}
+static DEVICE_ATTR_RW(rpm_lvl);
+static DEVICE_ATTR_RW(spm_lvl);

drivers/scsi/ufs/ufshcd.c static ssize_t ufshcd_spm_lvl_show(struct device *dev, · line 7689

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +10 -0

+
+static struct attribute *ufshcd_attrs[] = {
+	&dev_attr_rpm_lvl.attr,
+	&dev_attr_spm_lvl.attr,
+	NULL
+};
+
+static const struct attribute_group ufshcd_attr_group = {
+	.attrs = ufshcd_attrs,
+};

ATTRIBUTE_GROUPS()?

drivers/scsi/ufs/ufshcd.c static ssize_t ufshcd_spm_lvl_show(struct device *dev, · line 7694

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +2 -2

 static inline void ufshcd_add_sysfs_nodes(struct ufs_hba *hba)
 {
-	ufshcd_add_rpm_lvl_sysfs_nodes(hba);
-	ufshcd_add_spm_lvl_sysfs_nodes(hba);
+	if (sysfs_create_group(&hba->dev->kobj, &ufshcd_attr_group))
+		dev_err(hba->dev, "Failed to create sysfs group\n");

That will turn this into sysfs_create_groups()

But really, you should be able to do this all "at once" And really, it should be a "default attribute group" that the driver core adds to the device, but that's outside the scope of this patchset.

So for now, this is just fine, the attribute groups work is for an add-on patch. Thanks for working to get this upstream, I'm tired of seeing all of the different variants of this driver floating around out-of-tree.

Acked-by Greg Kroah-Hartman <[email protected]>

Jaegeuk Kim

author

2017-12-20 19:46 UTC #9 lore raw

Quoted 97 lines · Greg KH

Quoted 74 lines · Jaegeuk Kim

From: Jaegeuk Kim <[email protected]>

This patch introduces attribute group to show existing sysfs entries.

Cc Greg KH <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 48 +++++++++++++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h |  2 --
 2 files changed, 19 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +6 ~~-25~~

index 011c3369082c..12ff7daebb00 100644
@@ -7605,7 +7605,7 @@ static inline ssize_t ufshcd_pm_lvl_store(struct device *dev,
 	return count;
 }
 
-static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
+static ssize_t rpm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7634,24 +7634,13 @@ static ssize_t ufshcd_rpm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_rpm_lvl_store(struct device *dev,
+static ssize_t rpm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, true);
 }
 
-static void ufshcd_add_rpm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->rpm_lvl_attr.show = ufshcd_rpm_lvl_show;
-	hba->rpm_lvl_attr.store = ufshcd_rpm_lvl_store;
-	sysfs_attr_init(&hba->rpm_lvl_attr.attr);
-	hba->rpm_lvl_attr.attr.name = "rpm_lvl";
-	hba->rpm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->rpm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for rpm_lvl\n");
-}
-
-static ssize_t ufshcd_spm_lvl_show(struct device *dev,
+static ssize_t spm_lvl_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);

@@ -7680,33 +7669,34 @@ static ssize_t ufshcd_spm_lvl_show(struct device *dev,
 	return curr_len;
 }
 
-static ssize_t ufshcd_spm_lvl_store(struct device *dev,
+static ssize_t spm_lvl_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	return ufshcd_pm_lvl_store(dev, attr, buf, count, false);
 }
 
-static void ufshcd_add_spm_lvl_sysfs_nodes(struct ufs_hba *hba)
-{
-	hba->spm_lvl_attr.show = ufshcd_spm_lvl_show;
-	hba->spm_lvl_attr.store = ufshcd_spm_lvl_store;
-	sysfs_attr_init(&hba->spm_lvl_attr.attr);
-	hba->spm_lvl_attr.attr.name = "spm_lvl";
-	hba->spm_lvl_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->spm_lvl_attr))
-		dev_err(hba->dev, "Failed to create sysfs for spm_lvl\n");
-}
+static DEVICE_ATTR_RW(rpm_lvl);
+static DEVICE_ATTR_RW(spm_lvl);

drivers/scsi/ufs/ufshcd.c static ssize_t ufshcd_spm_lvl_show(struct device *dev, · line 7689

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +10 -0

+
+static struct attribute *ufshcd_attrs[] = {
+	&dev_attr_rpm_lvl.attr,
+	&dev_attr_spm_lvl.attr,
+	NULL
+};
+
+static const struct attribute_group ufshcd_attr_group = {
+	.attrs = ufshcd_attrs,
+};

ATTRIBUTE_GROUPS()?

drivers/scsi/ufs/ufshcd.c static ssize_t ufshcd_spm_lvl_show(struct device *dev, · line 7694

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +2 -2

 static inline void ufshcd_add_sysfs_nodes(struct ufs_hba *hba)
 {
-	ufshcd_add_rpm_lvl_sysfs_nodes(hba);
-	ufshcd_add_spm_lvl_sysfs_nodes(hba);
+	if (sysfs_create_group(&hba->dev->kobj, &ufshcd_attr_group))
+		dev_err(hba->dev, "Failed to create sysfs group\n");

That will turn this into sysfs_create_groups()

Quoted 8 lines

But really, you should be able to do this all "at once" And really, it should be a "default attribute group" that the driver core adds to the device, but that's outside the scope of this patchset.
So for now, this is just fine, the attribute groups work is for an add-on patch. Thanks for working to get this upstream, I'm tired of seeing all of the different variants of this driver floating around out-of-tree.

Agreed, it's really painful to sync it across many kernels. :( Anyway, I've changed to use ATTRIBUTE_GROUPS() in v3 as well.

Thanks,