[PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[Arthur Simchaev]

Currently the string descriptors sysfs entries are printing in ascii
format. According to Jedec UFS spec the string descriptors data is
Unicode and need-not be ascii convertible. Therefore in case the device
string descriptor contains non ascii convertible characters, it will
produce a wrong output. In order to fix this issue, the new
string descriptors entries will added to sysfs directory.
Those entries will show the string descriptors in raw format

Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 44 ++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-sysfs.c               | 34 +++++++++++++++++------
 2 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index d1bc23c..f6d6a46 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -561,6 +561,50 @@ Description:	This file contains a product revision string. The full
 
 		The file is read only.
 
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/manufacturer_name_raw
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This file contains a device manufactureer name string
+		as raw data. The full information about the descriptor
+		could be found at UFS specifications 2.1.
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/product_name_raw
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This file contains a product name string as raw data.
+		The full information about the descriptor could be found at
+		UFS specifications 2.1.
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/oem_id_raw
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This file contains a OEM ID string as raw data.
+		The full information about the descriptor could be found at
+		UFS specifications 2.1.
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/serial_number_raw
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This file contains a device serial number string
+		as raw data. The full information about the descriptor could be
+		found at UFS specifications 2.1.
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/product_revision_raw
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This file contains a product revision string as raw data.
+		The full information about the descriptor could be found at
+		UFS specifications 2.1.
+
+		The file is read only.
 
 What:		/sys/class/scsi_device/*/device/unit_descriptor/boot_lun_id
 Date:		February 2018
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index acc54f5..f1407ff 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -658,7 +658,7 @@ static const struct attribute_group ufs_sysfs_power_descriptor_group = {
 	.attrs = ufs_sysfs_power_descriptor,
 };
 
-#define UFS_STRING_DESCRIPTOR(_name, _pname)				\
+#define UFS_STRING_DESCRIPTOR(_name, _pname, _is_ascii)		\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
 {									\
@@ -690,10 +690,18 @@ static ssize_t _name##_show(struct device *dev,				\
 	kfree(desc_buf);						\
 	desc_buf = NULL;						\
 	ret = ufshcd_read_string_desc(hba, index, &desc_buf,		\
-				      SD_ASCII_STD);			\
+				      _is_ascii);			\
 	if (ret < 0)							\
 		goto out;						\
-	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
+	if (_is_ascii) {						\
+		ret = sysfs_emit(buf, "%s\n", desc_buf);		\
+	} else {							\
+		int i;							\
+									\
+		for (i = 0; i < desc_buf[0]; i++)			\
+			hex_byte_pack(buf + i * 2, desc_buf[i]);	\
+		ret = sysfs_emit(buf, "%s\n", buf);			\
+	}			\
 out:									\
 	pm_runtime_put_sync(hba->dev);					\
 	kfree(desc_buf);						\
@@ -702,11 +710,16 @@ out:									\
 }									\
 static DEVICE_ATTR_RO(_name)
 
-UFS_STRING_DESCRIPTOR(manufacturer_name, _MANF_NAME);
-UFS_STRING_DESCRIPTOR(product_name, _PRDCT_NAME);
-UFS_STRING_DESCRIPTOR(oem_id, _OEM_ID);
-UFS_STRING_DESCRIPTOR(serial_number, _SN);
-UFS_STRING_DESCRIPTOR(product_revision, _PRDCT_REV);
+UFS_STRING_DESCRIPTOR(manufacturer_name, _MANF_NAME, 1);
+UFS_STRING_DESCRIPTOR(product_name, _PRDCT_NAME, 1);
+UFS_STRING_DESCRIPTOR(oem_id, _OEM_ID, 1);
+UFS_STRING_DESCRIPTOR(serial_number, _SN, 1);
+UFS_STRING_DESCRIPTOR(product_revision, _PRDCT_REV, 1);
+UFS_STRING_DESCRIPTOR(manufacturer_name_raw, _MANF_NAME, 0);
+UFS_STRING_DESCRIPTOR(product_name_raw, _PRDCT_NAME, 0);
+UFS_STRING_DESCRIPTOR(oem_id_raw, _OEM_ID, 0);
+UFS_STRING_DESCRIPTOR(serial_number_raw, _SN, 0);
+UFS_STRING_DESCRIPTOR(product_revision_raw, _PRDCT_REV, 0);
 
 static struct attribute *ufs_sysfs_string_descriptors[] = {
 	&dev_attr_manufacturer_name.attr,
@@ -714,6 +727,11 @@ static struct attribute *ufs_sysfs_string_descriptors[] = {
 	&dev_attr_oem_id.attr,
 	&dev_attr_serial_number.attr,
 	&dev_attr_product_revision.attr,
+	&dev_attr_manufacturer_name_raw.attr,
+	&dev_attr_product_name_raw.attr,
+	&dev_attr_oem_id_raw.attr,
+	&dev_attr_serial_number_raw.attr,
+	&dev_attr_product_revision_raw.attr,
 	NULL,
 };
 
-- 
2.7.4

RE: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[Arthur Simchaev]

Hi Martin

Could you please consider to take this patch?

Regards
Arthur

> -----Original Message-----
> From: Arthur Simchaev <Arthur.Simchaev@wdc.com>
> Sent: Monday, February 15, 2021 7:41 PM
> To: James E . J . Bottomley <jejb@linux.vnet.ibm.com>; Martin K . Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: alim.akhtar@samsung.com; Bean Huo <beanhuo@micron.com>; Arthur
> Simchaev <Arthur.Simchaev@wdc.com>
> Subject: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data
> 
> Currently the string descriptors sysfs entries are printing in ascii
> format. According to Jedec UFS spec the string descriptors data is
> Unicode and need-not be ascii convertible. Therefore in case the device
> string descriptor contains non ascii convertible characters, it will
> produce a wrong output. In order to fix this issue, the new
> string descriptors entries will added to sysfs directory.
> Those entries will show the string descriptors in raw format
> 
> Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 44
> ++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufs-sysfs.c               | 34 +++++++++++++++++------
>  2 files changed, 70 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> b/Documentation/ABI/testing/sysfs-driver-ufs
> index d1bc23c..f6d6a46 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -561,6 +561,50 @@ Description:	This file contains a product revision
> string. The full
> 
>  		The file is read only.
> 
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/string_descriptors/manufacture
> r_name_raw
> +Date:		February 2021
> +Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
> +Description:	This file contains a device manufactureer name string
> +		as raw data. The full information about the descriptor
> +		could be found at UFS specifications 2.1.
> +
> +		The file is read only.
> +
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/string_descriptors/product_na
> me_raw
> +Date:		February 2021
> +Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
> +Description:	This file contains a product name string as raw data.
> +		The full information about the descriptor could be found at
> +		UFS specifications 2.1.
> +
> +		The file is read only.
> +
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/string_descriptors/oem_id_raw
> +Date:		February 2021
> +Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
> +Description:	This file contains a OEM ID string as raw data.
> +		The full information about the descriptor could be found at
> +		UFS specifications 2.1.
> +
> +		The file is read only.
> +
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/string_descriptors/serial_numb
> er_raw
> +Date:		February 2021
> +Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
> +Description:	This file contains a device serial number string
> +		as raw data. The full information about the descriptor could
> be
> +		found at UFS specifications 2.1.
> +
> +		The file is read only.
> +
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/string_descriptors/product_revi
> sion_raw
> +Date:		February 2021
> +Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
> +Description:	This file contains a product revision string as raw data.
> +		The full information about the descriptor could be found at
> +		UFS specifications 2.1.
> +
> +		The file is read only.
> 
>  What:
> 	/sys/class/scsi_device/*/device/unit_descriptor/boot_lun_id
>  Date:		February 2018
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index acc54f5..f1407ff 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -658,7 +658,7 @@ static const struct attribute_group
> ufs_sysfs_power_descriptor_group = {
>  	.attrs = ufs_sysfs_power_descriptor,
>  };
> 
> -#define UFS_STRING_DESCRIPTOR(_name, _pname)
> 	\
> +#define UFS_STRING_DESCRIPTOR(_name, _pname, _is_ascii)
> 	\
>  static ssize_t _name##_show(struct device *dev,
> 	\
>  	struct device_attribute *attr, char *buf)			\
>  {									\
> @@ -690,10 +690,18 @@ static ssize_t _name##_show(struct device *dev,
> 				\
>  	kfree(desc_buf);						\
>  	desc_buf = NULL;						\
>  	ret = ufshcd_read_string_desc(hba, index, &desc_buf,		\
> -				      SD_ASCII_STD);			\
> +				      _is_ascii);			\
>  	if (ret < 0)							\
>  		goto out;						\
> -	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
> +	if (_is_ascii) {						\
> +		ret = sysfs_emit(buf, "%s\n", desc_buf);		\
> +	} else {							\
> +		int i;							\
> +									\
> +		for (i = 0; i < desc_buf[0]; i++)			\
> +			hex_byte_pack(buf + i * 2, desc_buf[i]);	\
> +		ret = sysfs_emit(buf, "%s\n", buf);			\
> +	}			\
>  out:									\
>  	pm_runtime_put_sync(hba->dev);
> 	\
>  	kfree(desc_buf);						\
> @@ -702,11 +710,16 @@ out:
> 			\
>  }									\
>  static DEVICE_ATTR_RO(_name)
> 
> -UFS_STRING_DESCRIPTOR(manufacturer_name, _MANF_NAME);
> -UFS_STRING_DESCRIPTOR(product_name, _PRDCT_NAME);
> -UFS_STRING_DESCRIPTOR(oem_id, _OEM_ID);
> -UFS_STRING_DESCRIPTOR(serial_number, _SN);
> -UFS_STRING_DESCRIPTOR(product_revision, _PRDCT_REV);
> +UFS_STRING_DESCRIPTOR(manufacturer_name, _MANF_NAME, 1);
> +UFS_STRING_DESCRIPTOR(product_name, _PRDCT_NAME, 1);
> +UFS_STRING_DESCRIPTOR(oem_id, _OEM_ID, 1);
> +UFS_STRING_DESCRIPTOR(serial_number, _SN, 1);
> +UFS_STRING_DESCRIPTOR(product_revision, _PRDCT_REV, 1);
> +UFS_STRING_DESCRIPTOR(manufacturer_name_raw, _MANF_NAME, 0);
> +UFS_STRING_DESCRIPTOR(product_name_raw, _PRDCT_NAME, 0);
> +UFS_STRING_DESCRIPTOR(oem_id_raw, _OEM_ID, 0);
> +UFS_STRING_DESCRIPTOR(serial_number_raw, _SN, 0);
> +UFS_STRING_DESCRIPTOR(product_revision_raw, _PRDCT_REV, 0);
> 
>  static struct attribute *ufs_sysfs_string_descriptors[] = {
>  	&dev_attr_manufacturer_name.attr,
> @@ -714,6 +727,11 @@ static struct attribute
> *ufs_sysfs_string_descriptors[] = {
>  	&dev_attr_oem_id.attr,
>  	&dev_attr_serial_number.attr,
>  	&dev_attr_product_revision.attr,
> +	&dev_attr_manufacturer_name_raw.attr,
> +	&dev_attr_product_name_raw.attr,
> +	&dev_attr_oem_id_raw.attr,
> +	&dev_attr_serial_number_raw.attr,
> +	&dev_attr_product_revision_raw.attr,
>  	NULL,
>  };
> 
> --
> 2.7.4

Re: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[Martin K. Petersen]

Hi Arthur!

> Could you please consider to take this patch?

The patch needs some reviews. I suggest you repost.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[Bart Van Assche]

On 2/15/21 9:40 AM, Arthur Simchaev wrote:
> -#define UFS_STRING_DESCRIPTOR(_name, _pname)				\
> +#define UFS_STRING_DESCRIPTOR(_name, _pname, _is_ascii)		\
>  static ssize_t _name##_show(struct device *dev,				\
>  	struct device_attribute *attr, char *buf)			\
>  {									\
> @@ -690,10 +690,18 @@ static ssize_t _name##_show(struct device *dev,				\
>  	kfree(desc_buf);						\
>  	desc_buf = NULL;						\
>  	ret = ufshcd_read_string_desc(hba, index, &desc_buf,		\
> -				      SD_ASCII_STD);			\
> +				      _is_ascii);			\
>  	if (ret < 0)							\
>  		goto out;						\
> -	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
> +	if (_is_ascii) {						\
> +		ret = sysfs_emit(buf, "%s\n", desc_buf);		\
> +	} else {							\
> +		int i;							\
> +									\
> +		for (i = 0; i < desc_buf[0]; i++)			\
> +			hex_byte_pack(buf + i * 2, desc_buf[i]);	\
> +		ret = sysfs_emit(buf, "%s\n", buf);			\
> +	}			\
>  out:									\
>  	pm_runtime_put_sync(hba->dev);					\
>  	kfree(desc_buf);						\

Hex data needs to be parsed before it can be used by any software. Has
it been considered to make the "raw" attributes binary attributes
instead of hex-encoded binary? See also sysfs_create_bin_file().

Thanks,

Bart.

RE: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data

[Arthur Simchaev]

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Wednesday, March 17, 2021 5:31 AM
> To: Arthur Simchaev <Arthur.Simchaev@wdc.com>; James E . J . Bottomley
> <jejb@linux.vnet.ibm.com>; Martin K . Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: alim.akhtar@samsung.com; Bean Huo <beanhuo@micron.com>
> Subject: Re: [PATCH v2] scsi: ufs: sysfs: Print string descriptors as raw data
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> On 2/15/21 9:40 AM, Arthur Simchaev wrote:
> > -#define UFS_STRING_DESCRIPTOR(_name, _pname)                         \
> > +#define UFS_STRING_DESCRIPTOR(_name, _pname, _is_ascii)              \
> >  static ssize_t _name##_show(struct device *dev,                              \
> >       struct device_attribute *attr, char *buf)                       \
> >  {                                                                    \
> > @@ -690,10 +690,18 @@ static ssize_t _name##_show(struct device *dev,
> \
> >       kfree(desc_buf);                                                \
> >       desc_buf = NULL;                                                \
> >       ret = ufshcd_read_string_desc(hba, index, &desc_buf,            \
> > -                                   SD_ASCII_STD);                    \
> > +                                   _is_ascii);                       \
> >       if (ret < 0)                                                    \
> >               goto out;                                               \
> > -     ret = sysfs_emit(buf, "%s\n", desc_buf);                        \
> > +     if (_is_ascii) {                                                \
> > +             ret = sysfs_emit(buf, "%s\n", desc_buf);                \
> > +     } else {                                                        \
> > +             int i;                                                  \
> > +                                                                     \
> > +             for (i = 0; i < desc_buf[0]; i++)                       \
> > +                     hex_byte_pack(buf + i * 2, desc_buf[i]);        \
> > +             ret = sysfs_emit(buf, "%s\n", buf);                     \
> > +     }                       \
> >  out:                                                                 \
> >       pm_runtime_put_sync(hba->dev);                                  \
> >       kfree(desc_buf);                                                \
> 
> Hex data needs to be parsed before it can be used by any software. Has
> it been considered to make the "raw" attributes binary attributes
> instead of hex-encoded binary? See also sysfs_create_bin_file().
> 
> Thanks,
> 
> Bart.

Hi Bart,
Thank you for your comments.

The typical use case that originate this issue, is of some flash vendor's field engineer reading the serial part number.
All other string descriptors are less of an issue.

The current Jedec spec allows the serial number may not be ascii convertible . For example:
 - ufshcd_read_string_desc(bool asci = false):  00 1d 00 20 00 95 00 20 00 ec 00 84 00 5b 00 14
 - ufshcd_read_string_desc(bool asci = true):  "  ]  "

Therefore, upon reading the "raw" serial number, the user can verify the data integrity.

How about just applying this change to the serial number sysfs entry, and drop all others?

Regards
Arthur