[PATCH] scsi: ufs: sysfs: add is_ascii_output entry

[PATCH] scsi: ufs: sysfs: add is_ascii_output entry

[Arthur Simchaev]

From: Arthur Simchaev <arthur.simchaev@wdc.com>

Currently the string descriptors sysfs entries are printing in ascii
format. According to Jedec UFS spec the string descriptors data is
Unicode and may 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
"is_ascii_output" entry will be added to string descriptor's sysfs
directory. According to the entry value, the user will receive the
string descriptor output in raw or ascii data

Signed-off-by: Arthur Simchaev <Arthur.Simchaev@sandisk.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs |  9 +++++++++
 drivers/scsi/ufs/ufs-sysfs.c               | 32 ++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index d1bc23c..c7d8d8d 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -561,6 +561,15 @@ Description:	This file contains a product revision string. The full
 
 		The file is read only.
 
+What:		/sys/bus/platform/drivers/ufshcd/*/string_descriptors/is_ascii_output
+Date:		February 2021
+Contact:	Arthur Simchaev <arthur.simchaev@wdc.com>
+Description:	This entry could be used to set the string descriptor
+		output format:
+		0 will print raw string descriptor data as defined in
+		UFS JEDEC spec.
+		1 will convert the string descriptor data to ascii string
+		and print it.
 
 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..83c8104 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -9,6 +9,8 @@
 #include "ufs.h"
 #include "ufs-sysfs.h"
 
+static bool is_ascii_output = true;
+
 static const char *ufschd_uic_link_state_to_string(
 			enum uic_link_state state)
 {
@@ -693,7 +695,15 @@ static ssize_t _name##_show(struct device *dev,				\
 				      SD_ASCII_STD);			\
 	if (ret < 0)							\
 		goto out;						\
-	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
+	if (is_ascii_output) {						\
+		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);						\
@@ -708,13 +718,31 @@ UFS_STRING_DESCRIPTOR(oem_id, _OEM_ID);
 UFS_STRING_DESCRIPTOR(serial_number, _SN);
 UFS_STRING_DESCRIPTOR(product_revision, _PRDCT_REV);
 
+static ssize_t is_ascii_output_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%d\n", is_ascii_output);
+}
+
+static ssize_t is_ascii_output_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	if (kstrtobool(buf, &is_ascii_output))
+		return -EINVAL;
+	return count;
+}
+
+static DEVICE_ATTR_RW(is_ascii_output);
+
 static struct attribute *ufs_sysfs_string_descriptors[] = {
 	&dev_attr_manufacturer_name.attr,
 	&dev_attr_product_name.attr,
 	&dev_attr_oem_id.attr,
 	&dev_attr_serial_number.attr,
 	&dev_attr_product_revision.attr,
-	NULL,
+	&dev_attr_is_ascii_output.attr,
+	NULL
 };
 
 static const struct attribute_group ufs_sysfs_string_descriptors_group = {
-- 
2.7.4

Re: [PATCH] scsi: ufs: sysfs: add is_ascii_output entry

[Bart Van Assche]

On 2/10/21 2:53 AM, Arthur Simchaev wrote:
> +static bool is_ascii_output = true;

[ ... ]

>  static const char *ufschd_uic_link_state_to_string(
>  			enum uic_link_state state)
>  {
> @@ -693,7 +695,15 @@ static ssize_t _name##_show(struct device *dev,				\
>  				      SD_ASCII_STD);			\
>  	if (ret < 0)							\
>  		goto out;						\
> -	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
> +	if (is_ascii_output) {						\
> +		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);						\

Please do not introduce a mode variable but instead introduce a new
attribute such that there is one attribute for the unicode output and
one attribute for the ASCII output. Mode variables are troublesome when
e.g. two scripts try to set the mode attribute concurrently.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: sysfs: add is_ascii_output entry

[Greg KH]

On Wed, Feb 10, 2021 at 07:35:25PM -0800, Bart Van Assche wrote:
> On 2/10/21 2:53 AM, Arthur Simchaev wrote:
> > +static bool is_ascii_output = true;
> 
> [ ... ]
> 
> >  static const char *ufschd_uic_link_state_to_string(
> >  			enum uic_link_state state)
> >  {
> > @@ -693,7 +695,15 @@ static ssize_t _name##_show(struct device *dev,				\
> >  				      SD_ASCII_STD);			\
> >  	if (ret < 0)							\
> >  		goto out;						\
> > -	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
> > +	if (is_ascii_output) {						\
> > +		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);						\
> 
> Please do not introduce a mode variable but instead introduce a new
> attribute such that there is one attribute for the unicode output and
> one attribute for the ASCII output. Mode variables are troublesome when
> e.g. two scripts try to set the mode attribute concurrently.

Agreed, just make a new sysfs file, please never change the output of an
existing sysfs file, that way will guarantee confusion in userspace.

thanks,

greg k-h

RE: [PATCH] scsi: ufs: sysfs: add is_ascii_output entry

[Arthur Simchaev]

Done.

Regards
Arthur

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Sunday, February 14, 2021 11:54 AM
> To: Bart Van Assche <bvanassche@acm.org>
> Cc: 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; alim.akhtar@samsung.com; Bean Huo
> <beanhuo@micron.com>; Arthur Simchaev <Arthur.Simchaev@wdc.com>
> Subject: Re: [PATCH] scsi: ufs: sysfs: add is_ascii_output entry
> 
> 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 Wed, Feb 10, 2021 at 07:35:25PM -0800, Bart Van Assche wrote:
> > On 2/10/21 2:53 AM, Arthur Simchaev wrote:
> > > +static bool is_ascii_output = true;
> >
> > [ ... ]
> >
> > >  static const char *ufschd_uic_link_state_to_string(
> > >                     enum uic_link_state state)
> > >  {
> > > @@ -693,7 +695,15 @@ static ssize_t _name##_show(struct device *dev,
> \
> > >                                   SD_ASCII_STD);                    \
> > >     if (ret < 0)                                                    \
> > >             goto out;                                               \
> > > -   ret = sysfs_emit(buf, "%s\n", desc_buf);                        \
> > > +   if (is_ascii_output) {                                          \
> > > +           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);                                                \
> >
> > Please do not introduce a mode variable but instead introduce a new
> > attribute such that there is one attribute for the unicode output and
> > one attribute for the ASCII output. Mode variables are troublesome when
> > e.g. two scripts try to set the mode attribute concurrently.
> 
> Agreed, just make a new sysfs file, please never change the output of an
> existing sysfs file, that way will guarantee confusion in userspace.
> 
> thanks,
> 
> greg k-h