by Jaegeuk Kim

[permalink] [raw]

Subject: Re: [PATCH v3 1/9] ufs: sysfs: device descriptor

On 01/02, Stanislav Nijnikov wrote:
>
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Thursday, December 28, 2017 9:37 PM
> > To: Stanislav Nijnikov <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; Alex Lemberg <[email protected]>
> > Subject: Re: [PATCH v3 1/9] ufs: sysfs: device descriptor
> >
> > On 12/28, Stanislav Nijnikov wrote:
> > > This patch introduces a sysfs group entry for the UFS device
> > > descriptor parameters. The group adds "device_descriptor" folder under
> > > the UFS driver sysfs entry (/sys/bus/platform/drivers/ufshcd/*). The
> > > parameters are shown as hexadecimal numbers. The full information
> > > about the parameters could be found at UFS specifications 2.1.
> > >
> > > Signed-off-by: Stanislav Nijnikov <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-driver-ufs | 223
> > +++++++++++++++++++++++++++++
> > > drivers/scsi/ufs/Makefile | 3 +-
> > > drivers/scsi/ufs/ufs-sysfs.c | 170 ++++++++++++++++++++++
> > > drivers/scsi/ufs/ufs-sysfs.h | 25 ++++
> > > drivers/scsi/ufs/ufs.h | 8 ++
> > > drivers/scsi/ufs/ufshcd.c | 12 +-
> > > drivers/scsi/ufs/ufshcd.h | 4 +
> > > 7 files changed, 439 insertions(+), 6 deletions(-) create mode
> > > 100644 Documentation/ABI/testing/sysfs-driver-ufs
> > > create mode 100644 drivers/scsi/ufs/ufs-sysfs.c create mode 100644
> > > drivers/scsi/ufs/ufs-sysfs.h
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > > b/Documentation/ABI/testing/sysfs-driver-ufs
> > > new file mode 100644
> > > index 0000000..17cc4aa
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> >
> > [snip]
> >
> > > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> > > index 9310c6c..918f579 100644
> > > --- a/drivers/scsi/ufs/Makefile
> > > +++ b/drivers/scsi/ufs/Makefile
> > > @@ -3,6 +3,7 @@
> > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-
> > dwc.o
> > > tc-dwc-g210.o
> > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> > > ufshcd-dwc.o tc-dwc-g210.o
> > > obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
> > > -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
> > > +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-objs :=
> > > +ufshcd.o ufs-sysfs.o
> >
> > Why not just adding ufs-sysfs.o in the existing configuration?
>
> The kernel build robot compiles the UFS driver as a separate module.
> The existing configuration doesn't allow to add a new file to be compiled
> as a part of this module. The line like " obj-$(CONFIG_SCSI_UFSHCD) +=
> ufshcd.o ufs-sysfs.o" in the makefile will actually create 2 modules.
> This was the reason why I used EXPORT_SYMBOL in the first version.

Is there a reason to drop the first version?

>
> >
> > > obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> > > obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git
> > > a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c new file
> > > mode 100644 index 0000000..1c685f3
> > > --- /dev/null
> > > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > > @@ -0,0 +1,170 @@
> > > +/*
> > > +* UFS Device Management sysfs
> > > +*
> > > +* Copyright (C) 2017 Western Digital Corporation
> > > +*
> > > +* This program is free software; you can redistribute it and/or
> > > +* modify it under the terms of the GNU General Public License version
> > > +* 2 as published by the Free Software Foundation.
> > > +*
> > > +* This program is distributed in the hope that it will be useful, but
> > > +* WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > GNU
> > > +* General Public License for more details.
> > > +*
> > > +*/
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/string.h>
> > > +
> > > +#include "ufs.h"
> > > +#include "ufs-sysfs.h"
> > > +/* collision between the device descriptor parameter and the
> > > +definition */ #undef DEVICE_CLASS
> >
> > Does this make sense? How about attaching "_" for all the macro like
> > _DEVICE_CLASS below?
> >
>
> It's not just changing the one line that uses "DEVICE_CLASS" to use
> "_DEVICE_CLASS". It's will be necessary to add "_" to all descriptor
> parameters macros in all patches.

You should be able to do that by moving ufs_sysfs_read_desc_param() into
ufshcd.c, and remaining only the necessary header files for sysfs stuffs.

>
> > > +
> > > +enum ufs_desc_param_size {
> > > + UFS_PARAM_BYTE_SIZE = 1,
> > > + UFS_PARAM_WORD_SIZE = 2,
> > > + UFS_PARAM_DWORD_SIZE = 4,
> > > + UFS_PARAM_QWORD_SIZE = 8,
> > > +};

Must be in header file.

> > > +
> > > +static inline ssize_t ufs_sysfs_read_desc_param(
> > > + struct ufs_hba *hba, u8 desc_idn, u8 index, char *buf, u8 off,
> > > + enum ufs_desc_param_size param_size) {
> > > + int desc_len;
> > > + int ret;
> > > + u8 *desc_buf;
> > > +
> > > + if (ufshcd_map_desc_id_to_length(hba, desc_idn, &desc_len) ||
> > > + off >= desc_len)
> > > + return -EINVAL;
> > > + desc_buf = kzalloc(desc_len, GFP_ATOMIC);
> > > + if (!desc_buf)
> > > + return -ENOMEM;
> > > + ret = ufshcd_query_descriptor_retry(hba,
> > UPIU_QUERY_OPCODE_READ_DESC,
> > > + desc_idn, index, 0, desc_buf, &desc_len);
> > > + if (ret)
> >
> > Should free desc_buf here.
> >
> > > + return -EINVAL;

BTW, the existing ufshcd_read_desc_param() seems the right function for the
above work.

> > > + switch (param_size) {
> > > + case UFS_PARAM_BYTE_SIZE:
> > > + ret = sprintf(buf, "0x%02X\n", desc_buf[off]);
> > > + break;
> > > + case UFS_PARAM_WORD_SIZE:
> > > + ret = sprintf(buf, "0x%04X\n",
> > > + be16_to_cpu(*((u16 *)(desc_buf + off))));
> > > + break;
> > > + case UFS_PARAM_DWORD_SIZE:
> > > + ret = sprintf(buf, "0x%08X\n",
> > > + be32_to_cpu(*((u32 *)(desc_buf + off))));
> > > + break;
> > > + case UFS_PARAM_QWORD_SIZE:
> > > + ret = sprintf(buf, "0x%016llX\n",
> > > + be64_to_cpu(*((u64 *)(desc_buf + off))));
> > > + break;
> > > + }
> > > + kfree(desc_buf);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +#define ufs_sysfs_desc_param_show(_name, _puname, _duname,

Not a good macro definition. In addition, the patch has some coding style
errors, which requires to pass script/checkpatch.pl.

I didn't test the below fully tho, could you take a look at this change?
IMO, still, making a default group with existing sysfs entries should be done
in prior to this.

diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 918f579..1fa74c1 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -3,7 +3,6 @@
obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
-obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
-ufshcd-core-objs := ufshcd.o ufs-sysfs.o
+obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o ufs-sysfs.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 1c685f3..226eb44 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -19,71 +19,22 @@

#include "ufs.h"
#include "ufs-sysfs.h"
-/* collision between the device descriptor parameter and the definition */
-#undef DEVICE_CLASS

-enum ufs_desc_param_size {
- UFS_PARAM_BYTE_SIZE = 1,
- UFS_PARAM_WORD_SIZE = 2,
- UFS_PARAM_DWORD_SIZE = 4,
- UFS_PARAM_QWORD_SIZE = 8,
-};
-
-static inline ssize_t ufs_sysfs_read_desc_param(
- struct ufs_hba *hba, u8 desc_idn, u8 index, char *buf, u8 off,
- enum ufs_desc_param_size param_size)
-{
- int desc_len;
- int ret;
- u8 *desc_buf;
-
- if (ufshcd_map_desc_id_to_length(hba, desc_idn, &desc_len) ||
- off >= desc_len)
- return -EINVAL;
- desc_buf = kzalloc(desc_len, GFP_ATOMIC);
- if (!desc_buf)
- return -ENOMEM;
- ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
- desc_idn, index, 0, desc_buf, &desc_len);
- if (ret)
- return -EINVAL;
- switch (param_size) {
- case UFS_PARAM_BYTE_SIZE:
- ret = sprintf(buf, "0x%02X\n", desc_buf[off]);
- break;
- case UFS_PARAM_WORD_SIZE:
- ret = sprintf(buf, "0x%04X\n",
- be16_to_cpu(*((u16 *)(desc_buf + off))));
- break;
- case UFS_PARAM_DWORD_SIZE:
- ret = sprintf(buf, "0x%08X\n",
- be32_to_cpu(*((u32 *)(desc_buf + off))));
- break;
- case UFS_PARAM_QWORD_SIZE:
- ret = sprintf(buf, "0x%016llX\n",
- be64_to_cpu(*((u64 *)(desc_buf + off))));
- break;
- }
- kfree(desc_buf);
-
- return ret;
-}
-
-#define ufs_sysfs_desc_param_show(_name, _puname, _duname, _size) \
-static ssize_t _name##_show(struct device *dev, \
- struct device_attribute *attr, char *buf) \
-{ \
- struct ufs_hba *hba = dev_get_drvdata(dev); \
- return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname, \
- 0, buf, _duname##_DESC_PARAM_##_puname, \
- UFS_PARAM_##_size##_SIZE); \
-}
-
-#define UFS_DESC_PARAM(_pname, _puname, _duname, _size) \
- ufs_sysfs_desc_param_show(_pname, _puname, _duname, _size) \
- static DEVICE_ATTR_RO(_pname)
-
-#define UFS_DEVICE_DESC_PARAM(_name, _uname, _size) \
+#define UFS_DESC_PARAM_SHOW(_name, _puname, _duname, _size) \
+static ssize_t _name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return ufshcd_sysfs_read_desc_param(dev, \
+ QUERY_DESC_IDN_##_duname, \
+ 0, _duname##_DESC_PARAM_##_puname, buf, \
+ UFS_PARAM_##_size##_SIZE); \
+} \
+static DEVICE_ATTR_RO(_name)
+
+#define UFS_DESC_PARAM(_pname, _puname, _duname, _size) \
+ UFS_DESC_PARAM_SHOW(_pname, _puname, _duname, _size)
+
+#define UFS_DEVICE_DESC_PARAM(_name, _uname, _size) \
UFS_DESC_PARAM(_name, _uname, DEVICE, _size)

UFS_DEVICE_DESC_PARAM(device_type, DEVICE_TYPE, BYTE);
@@ -153,18 +104,21 @@ static const struct attribute_group *ufs_sysfs_groups[] = {
NULL,
};

-void ufs_sysfs_add_device_management(struct ufs_hba *hba)
+void ufs_sysfs_add_device_management(struct device *dev)
{
int ret;

- ret = sysfs_create_groups(&hba->dev->kobj, ufs_sysfs_groups);
+ ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
if (ret)
- dev_err(hba->dev,
+ dev_err(dev,
"%s: sysfs groups creation failed (err = %d)\n",
__func__, ret);
}

-void ufs_sysfs_remove_device_management(struct ufs_hba *hba)
+void ufs_sysfs_remove_device_management(struct device *dev)
{
- sysfs_remove_groups(&hba->dev->kobj, ufs_sysfs_groups);
+ sysfs_remove_groups(&dev->kobj, ufs_sysfs_groups);
}
+
+EXPORT_SYMBOL(ufs_sysfs_add_device_management);
+EXPORT_SYMBOL(ufs_sysfs_remove_device_management);
diff --git a/drivers/scsi/ufs/ufs-sysfs.h b/drivers/scsi/ufs/ufs-sysfs.h
index a1fc9dc..c857e20 100644
--- a/drivers/scsi/ufs/ufs-sysfs.h
+++ b/drivers/scsi/ufs/ufs-sysfs.h
@@ -16,10 +16,23 @@
#ifndef __UFS_SYSFS_H__
#define __UFS_SYSFS_H__

+#include <linux/device.h>
#include <linux/sysfs.h>

-#include "ufshcd.h"
+enum ufs_desc_param_size {
+ UFS_PARAM_BYTE_SIZE = 1,
+ UFS_PARAM_WORD_SIZE = 2,
+ UFS_PARAM_DWORD_SIZE = 4,
+ UFS_PARAM_QWORD_SIZE = 8,
+};

-void ufs_sysfs_add_device_management(struct ufs_hba *hba);
-void ufs_sysfs_remove_device_management(struct ufs_hba *hba);
+extern void ufs_sysfs_add_device_management(struct device *dev);
+extern void ufs_sysfs_remove_device_management(struct device *dev);
+
+extern ssize_t ufshcd_sysfs_read_desc_param(struct device *dev,
+ enum desc_idn desc_id,
+ u8 desc_index,
+ u8 param_offset,
+ u8 *sysfs_buf,
+ u8 param_size);
#endif
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c1c45c1..7d38e1b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2895,7 +2895,7 @@ static int __ufshcd_query_descriptor(struct ufs_hba *hba,
* The buf_len parameter will contain, on return, the length parameter
* received on the response.
*/
-int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
+static int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
enum query_opcode opcode,
enum desc_idn idn, u8 index, u8 selector,
u8 *desc_buf, int *buf_len)
@@ -3079,6 +3079,46 @@ static int ufshcd_read_desc_param(struct ufs_hba *hba,
return ret;
}

+ssize_t ufshcd_sysfs_read_desc_param(struct device *dev,
+ enum desc_idn desc_id,
+ u8 desc_index,
+ u8 param_offset,
+ u8 *sysfs_buf,
+ u8 param_size)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ u8 desc_buf[UFS_PARAM_QWORD_SIZE] = {0};
+ int ret;
+
+ if (param_size > UFS_PARAM_QWORD_SIZE)
+ return -EINVAL;
+
+ ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
+ param_offset, desc_buf, param_size);
+ if (ret)
+ return ret;
+
+ switch (param_size) {
+ case UFS_PARAM_BYTE_SIZE:
+ ret = sprintf(sysfs_buf, "0x%02X\n", *desc_buf);
+ break;
+ case UFS_PARAM_WORD_SIZE:
+ ret = sprintf(sysfs_buf, "0x%04X\n",
+ be16_to_cpu(*((u16 *)desc_buf)));
+ break;
+ case UFS_PARAM_DWORD_SIZE:
+ ret = sprintf(sysfs_buf, "0x%08X\n",
+ be32_to_cpu(*((u32 *)desc_buf)));
+ break;
+ case UFS_PARAM_QWORD_SIZE:
+ ret = sprintf(sysfs_buf, "0x%016llX\n",
+ be64_to_cpu(*((u64 *)desc_buf)));
+ break;
+ }
+ return ret;
+}
+EXPORT_SYMBOL(ufshcd_sysfs_read_desc_param);
+
static inline int ufshcd_read_desc(struct ufs_hba *hba,
enum desc_idn desc_id,
int desc_index,
@@ -7701,12 +7741,12 @@ 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);
- ufs_sysfs_add_device_management(hba);
+ ufs_sysfs_add_device_management(hba->dev);
}

static inline void ufshcd_remove_sysfs_nodes(struct ufs_hba *hba)
{
- ufs_sysfs_remove_device_management(hba);
+ ufs_sysfs_remove_device_management(hba->dev);
device_remove_file(hba->dev, &hba->rpm_lvl_attr);
device_remove_file(hba->dev, &hba->spm_lvl_attr);
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6a0ec4b..1332e54 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -841,10 +841,6 @@ static inline bool ufshcd_is_hs_mode(struct ufs_pa_layer_attr *pwr_info)
}

/* Expose Query-Request API */
-int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
- enum query_opcode opcode,
- enum desc_idn idn, u8 index, u8 selector,
- u8 *desc_buf, int *buf_len);
int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
enum flag_idn idn, bool *flag_res);
int ufshcd_hold(struct ufs_hba *hba, bool async);

2018-01-04 17:46:31

by Stanislav Nijnikov

[permalink] [raw]

Subject: RE: [PATCH v3 1/9] ufs: sysfs: device descriptor

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, January 3, 2018 3:44 AM
> To: Stanislav Nijnikov <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Alex Lemberg <[email protected]>
> Subject: Re: [PATCH v3 1/9] ufs: sysfs: device descriptor
>
> On 01/02, Stanislav Nijnikov wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:[email protected]]
> > > Sent: Thursday, December 28, 2017 9:37 PM
> > > To: Stanislav Nijnikov <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; Alex Lemberg <[email protected]>
> > > Subject: Re: [PATCH v3 1/9] ufs: sysfs: device descriptor
> > >
> > > On 12/28, Stanislav Nijnikov wrote:
> > > > This patch introduces a sysfs group entry for the UFS device
> > > > descriptor parameters. The group adds "device_descriptor" folder
> > > > under the UFS driver sysfs entry
> > > > (/sys/bus/platform/drivers/ufshcd/*). The parameters are shown as
> > > > hexadecimal numbers. The full information about the parameters could
> be found at UFS specifications 2.1.
> > > >
> > > > Signed-off-by: Stanislav Nijnikov <[email protected]>
> > > > ---
> > > > Documentation/ABI/testing/sysfs-driver-ufs | 223
> > > +++++++++++++++++++++++++++++
> > > > drivers/scsi/ufs/Makefile | 3 +-
> > > > drivers/scsi/ufs/ufs-sysfs.c | 170 ++++++++++++++++++++++
> > > > drivers/scsi/ufs/ufs-sysfs.h | 25 ++++
> > > > drivers/scsi/ufs/ufs.h | 8 ++
> > > > drivers/scsi/ufs/ufshcd.c | 12 +-
> > > > drivers/scsi/ufs/ufshcd.h | 4 +
> > > > 7 files changed, 439 insertions(+), 6 deletions(-) create mode
> > > > 100644 Documentation/ABI/testing/sysfs-driver-ufs
> > > > create mode 100644 drivers/scsi/ufs/ufs-sysfs.c create mode
> > > > 100644 drivers/scsi/ufs/ufs-sysfs.h
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > > > b/Documentation/ABI/testing/sysfs-driver-ufs
> > > > new file mode 100644
> > > > index 0000000..17cc4aa
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > >
> > > [snip]
> > >
> > > > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> > > > index 9310c6c..918f579 100644
> > > > --- a/drivers/scsi/ufs/Makefile
> > > > +++ b/drivers/scsi/ufs/Makefile
> > > > @@ -3,6 +3,7 @@
> > > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-
> > > dwc.o
> > > > tc-dwc-g210.o
> > > > obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-
> pltfrm.o
> > > > ufshcd-dwc.o tc-dwc-g210.o
> > > > obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
> > > > -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
> > > > +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-objs :=
> > > > +ufshcd.o ufs-sysfs.o
> > >
> > > Why not just adding ufs-sysfs.o in the existing configuration?
> >
> > The kernel build robot compiles the UFS driver as a separate module.
> > The existing configuration doesn't allow to add a new file to be
> > compiled as a part of this module. The line like "
> > obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o ufs-sysfs.o" in the makefile will
> actually create 2 modules.
> > This was the reason why I used EXPORT_SYMBOL in the first version.
>
> Is there a reason to drop the first version?
>
It was updated according to Greg Kroah-Hartman' notes (one of them was what is
a reason to use EXPORT_SYMBOL if functions are used only in one module).
> >
> > >
> > > > obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> > > > obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git
> > > > a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c new
> > > > file mode 100644 index 0000000..1c685f3
> > > > --- /dev/null
> > > > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > > > @@ -0,0 +1,170 @@
> > > > +/*
> > > > +* UFS Device Management sysfs
> > > > +*
> > > > +* Copyright (C) 2017 Western Digital Corporation
> > > > +*
> > > > +* This program is free software; you can redistribute it and/or
> > > > +* modify it under the terms of the GNU General Public License
> > > > +version
> > > > +* 2 as published by the Free Software Foundation.
> > > > +*
> > > > +* This program is distributed in the hope that it will be useful,
> > > > +but
> > > > +* WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> the
> > > GNU
> > > > +* General Public License for more details.
> > > > +*
> > > > +*/
> > > > +
> > > > +#include <linux/err.h>
> > > > +#include <linux/string.h>
> > > > +
> > > > +#include "ufs.h"
> > > > +#include "ufs-sysfs.h"
> > > > +/* collision between the device descriptor parameter and the
> > > > +definition */ #undef DEVICE_CLASS
> > >
> > > Does this make sense? How about attaching "_" for all the macro like
> > > _DEVICE_CLASS below?
> > >
> >
> > It's not just changing the one line that uses "DEVICE_CLASS" to use
> > "_DEVICE_CLASS". It's will be necessary to add "_" to all descriptor
> > parameters macros in all patches.
>
> You should be able to do that by moving ufs_sysfs_read_desc_param() into
> ufshcd.c, and remaining only the necessary header files for sysfs stuffs.
>

I see your point but then I'll have to put many sysfs related functions (
reading descriptor parameters, reading string descriptor, reading flags
and attributes) into the "ufshcd.c" file which I would like to avoid. And it
will be necessary to use no definitions from "ufshcd.h" in the "ufs-sysfs.c"
functions.

> >
> > > > +
> > > > +enum ufs_desc_param_size {
> > > > + UFS_PARAM_BYTE_SIZE = 1,
> > > > + UFS_PARAM_WORD_SIZE = 2,
> > > > + UFS_PARAM_DWORD_SIZE = 4,
> > > > + UFS_PARAM_QWORD_SIZE = 8,
> > > > +};
>
> Must be in header file.
>
> > > > +
> > > > +static inline ssize_t ufs_sysfs_read_desc_param(
> > > > + struct ufs_hba *hba, u8 desc_idn, u8 index, char *buf, u8 off,
> > > > + enum ufs_desc_param_size param_size) {
> > > > + int desc_len;
> > > > + int ret;
> > > > + u8 *desc_buf;
> > > > +
> > > > + if (ufshcd_map_desc_id_to_length(hba, desc_idn, &desc_len) ||
> > > > + off >= desc_len)
> > > > + return -EINVAL;
> > > > + desc_buf = kzalloc(desc_len, GFP_ATOMIC);
> > > > + if (!desc_buf)
> > > > + return -ENOMEM;
> > > > + ret = ufshcd_query_descriptor_retry(hba,
> > > UPIU_QUERY_OPCODE_READ_DESC,
> > > > + desc_idn, index, 0, desc_buf, &desc_len);
> > > > + if (ret)
> > >
> > > Should free desc_buf here.
> > >
> > > > + return -EINVAL;
>
> BTW, the existing ufshcd_read_desc_param() seems the right function for
> the above work.
>
> > > > + switch (param_size) {
> > > > + case UFS_PARAM_BYTE_SIZE:
> > > > + ret = sprintf(buf, "0x%02X\n", desc_buf[off]);
> > > > + break;
> > > > + case UFS_PARAM_WORD_SIZE:
> > > > + ret = sprintf(buf, "0x%04X\n",
> > > > + be16_to_cpu(*((u16 *)(desc_buf + off))));
> > > > + break;
> > > > + case UFS_PARAM_DWORD_SIZE:
> > > > + ret = sprintf(buf, "0x%08X\n",
> > > > + be32_to_cpu(*((u32 *)(desc_buf + off))));
> > > > + break;
> > > > + case UFS_PARAM_QWORD_SIZE:
> > > > + ret = sprintf(buf, "0x%016llX\n",
> > > > + be64_to_cpu(*((u64 *)(desc_buf + off))));
> > > > + break;
> > > > + }
> > > > + kfree(desc_buf);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +#define ufs_sysfs_desc_param_show(_name, _puname, _duname,
>
> Not a good macro definition. In addition, the patch has some coding style
> errors, which requires to pass script/checkpatch.pl.
>
> I didn't test the below fully tho, could you take a look at this change?
> IMO, still, making a default group with existing sysfs entries should be done
> in prior to this.
>
> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile index
> 918f579..1fa74c1 100644
> --- a/drivers/scsi/ufs/Makefile
> +++ b/drivers/scsi/ufs/Makefile
> @@ -3,7 +3,6 @@
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
> tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> ufshcd-dwc.o tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
> -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o -ufshcd-core-objs :=
> ufshcd.o ufs-sysfs.o
> +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o ufs-sysfs.o
> obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o diff --git
> a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index
> 1c685f3..226eb44 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -19,71 +19,22 @@
>
> #include "ufs.h"
> #include "ufs-sysfs.h"
> -/* collision between the device descriptor parameter and the definition */ -
> #undef DEVICE_CLASS
>
> -enum ufs_desc_param_size {
> - UFS_PARAM_BYTE_SIZE = 1,
> - UFS_PARAM_WORD_SIZE = 2,
> - UFS_PARAM_DWORD_SIZE = 4,
> - UFS_PARAM_QWORD_SIZE = 8,
> -};
> -
> -static inline ssize_t ufs_sysfs_read_desc_param(
> - struct ufs_hba *hba, u8 desc_idn, u8 index, char *buf, u8 off,
> - enum ufs_desc_param_size param_size)
> -{
> - int desc_len;
> - int ret;
> - u8 *desc_buf;
> -
> - if (ufshcd_map_desc_id_to_length(hba, desc_idn, &desc_len) ||
> - off >= desc_len)
> - return -EINVAL;
> - desc_buf = kzalloc(desc_len, GFP_ATOMIC);
> - if (!desc_buf)
> - return -ENOMEM;
> - ret = ufshcd_query_descriptor_retry(hba,
> UPIU_QUERY_OPCODE_READ_DESC,
> - desc_idn, index, 0, desc_buf, &desc_len);
> - if (ret)
> - return -EINVAL;
> - switch (param_size) {
> - case UFS_PARAM_BYTE_SIZE:
> - ret = sprintf(buf, "0x%02X\n", desc_buf[off]);
> - break;
> - case UFS_PARAM_WORD_SIZE:
> - ret = sprintf(buf, "0x%04X\n",
> - be16_to_cpu(*((u16 *)(desc_buf + off))));
> - break;
> - case UFS_PARAM_DWORD_SIZE:
> - ret = sprintf(buf, "0x%08X\n",
> - be32_to_cpu(*((u32 *)(desc_buf + off))));
> - break;
> - case UFS_PARAM_QWORD_SIZE:
> - ret = sprintf(buf, "0x%016llX\n",
> - be64_to_cpu(*((u64 *)(desc_buf + off))));
> - break;
> - }
> - kfree(desc_buf);
> -
> - return ret;
> -}
> -
> -#define ufs_sysfs_desc_param_show(_name, _puname, _duname, _size)
> \
> -static ssize_t _name##_show(struct device *dev, \
> - struct device_attribute *attr, char *buf) \
> -{ \
> - struct ufs_hba *hba = dev_get_drvdata(dev); \
> - return ufs_sysfs_read_desc_param(hba,
> QUERY_DESC_IDN_##_duname, \
> - 0, buf, _duname##_DESC_PARAM_##_puname, \
> - UFS_PARAM_##_size##_SIZE); \
> -}
> -
> -#define UFS_DESC_PARAM(_pname, _puname, _duname, _size)
> \
> - ufs_sysfs_desc_param_show(_pname, _puname, _duname, _size)
> \
> - static DEVICE_ATTR_RO(_pname)
> -
> -#define UFS_DEVICE_DESC_PARAM(_name, _uname, _size) \
> +#define UFS_DESC_PARAM_SHOW(_name, _puname, _duname, _size)
> \
> +static ssize_t _name##_show(struct device *dev,
> \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + return ufshcd_sysfs_read_desc_param(dev, \
> + QUERY_DESC_IDN_##_duname,
> \
> + 0, _duname##_DESC_PARAM_##_puname, buf,
> \
> + UFS_PARAM_##_size##_SIZE);
> \
> +} \
> +static DEVICE_ATTR_RO(_name)
> +
> +#define UFS_DESC_PARAM(_pname, _puname, _duname, _size)
> \
> + UFS_DESC_PARAM_SHOW(_pname, _puname, _duname, _size)
> +
> +#define UFS_DEVICE_DESC_PARAM(_name, _uname, _size)
> \
> UFS_DESC_PARAM(_name, _uname, DEVICE, _size)
>
> UFS_DEVICE_DESC_PARAM(device_type, DEVICE_TYPE, BYTE); @@ -153,18
> +104,21 @@ static const struct attribute_group *ufs_sysfs_groups[] = {
> NULL,
> };
>
> -void ufs_sysfs_add_device_management(struct ufs_hba *hba)
> +void ufs_sysfs_add_device_management(struct device *dev)
> {
> int ret;
>
> - ret = sysfs_create_groups(&hba->dev->kobj, ufs_sysfs_groups);
> + ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
> if (ret)
> - dev_err(hba->dev,
> + dev_err(dev,
> "%s: sysfs groups creation failed (err = %d)\n",
> __func__, ret);
> }
>
> -void ufs_sysfs_remove_device_management(struct ufs_hba *hba)
> +void ufs_sysfs_remove_device_management(struct device *dev)
> {
> - sysfs_remove_groups(&hba->dev->kobj, ufs_sysfs_groups);
> + sysfs_remove_groups(&dev->kobj, ufs_sysfs_groups);
> }
> +
> +EXPORT_SYMBOL(ufs_sysfs_add_device_management);
> +EXPORT_SYMBOL(ufs_sysfs_remove_device_management);
> diff --git a/drivers/scsi/ufs/ufs-sysfs.h b/drivers/scsi/ufs/ufs-sysfs.h index
> a1fc9dc..c857e20 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.h
> +++ b/drivers/scsi/ufs/ufs-sysfs.h
> @@ -16,10 +16,23 @@
> #ifndef __UFS_SYSFS_H__
> #define __UFS_SYSFS_H__
>
> +#include <linux/device.h>
> #include <linux/sysfs.h>
>
> -#include "ufshcd.h"
> +enum ufs_desc_param_size {
> + UFS_PARAM_BYTE_SIZE = 1,
> + UFS_PARAM_WORD_SIZE = 2,
> + UFS_PARAM_DWORD_SIZE = 4,
> + UFS_PARAM_QWORD_SIZE = 8,
> +};
>
> -void ufs_sysfs_add_device_management(struct ufs_hba *hba); -void
> ufs_sysfs_remove_device_management(struct ufs_hba *hba);
> +extern void ufs_sysfs_add_device_management(struct device *dev);
> extern
> +void ufs_sysfs_remove_device_management(struct device *dev);
> +
> +extern ssize_t ufshcd_sysfs_read_desc_param(struct device *dev,
> + enum desc_idn desc_id,
> + u8 desc_index,
> + u8 param_offset,
> + u8 *sysfs_buf,
> + u8 param_size);
> #endif
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> c1c45c1..7d38e1b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2895,7 +2895,7 @@ static int __ufshcd_query_descriptor(struct
> ufs_hba *hba,
> * The buf_len parameter will contain, on return, the length parameter
> * received on the response.
> */
> -int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
> +static int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
> enum query_opcode opcode,
> enum desc_idn idn, u8 index, u8 selector,
> u8 *desc_buf, int *buf_len)
> @@ -3079,6 +3079,46 @@ static int ufshcd_read_desc_param(struct ufs_hba
> *hba,
> return ret;
> }
>
> +ssize_t ufshcd_sysfs_read_desc_param(struct device *dev,
> + enum desc_idn desc_id,
> + u8 desc_index,
> + u8 param_offset,
> + u8 *sysfs_buf,
> + u8 param_size)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + u8 desc_buf[UFS_PARAM_QWORD_SIZE] = {0};
> + int ret;
> +
> + if (param_size > UFS_PARAM_QWORD_SIZE)
> + return -EINVAL;
> +
> + ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
> + param_offset, desc_buf, param_size);
> + if (ret)
> + return ret;
> +
> + switch (param_size) {
> + case UFS_PARAM_BYTE_SIZE:
> + ret = sprintf(sysfs_buf, "0x%02X\n", *desc_buf);
> + break;
> + case UFS_PARAM_WORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%04X\n",
> + be16_to_cpu(*((u16 *)desc_buf)));
> + break;
> + case UFS_PARAM_DWORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%08X\n",
> + be32_to_cpu(*((u32 *)desc_buf)));
> + break;
> + case UFS_PARAM_QWORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%016llX\n",
> + be64_to_cpu(*((u64 *)desc_buf)));
> + break;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL(ufshcd_sysfs_read_desc_param);
> +
> static inline int ufshcd_read_desc(struct ufs_hba *hba,
> enum desc_idn desc_id,
> int desc_index,
> @@ -7701,12 +7741,12 @@ 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);
> - ufs_sysfs_add_device_management(hba);
> + ufs_sysfs_add_device_management(hba->dev);
> }
>
> static inline void ufshcd_remove_sysfs_nodes(struct ufs_hba *hba) {
> - ufs_sysfs_remove_device_management(hba);
> + ufs_sysfs_remove_device_management(hba->dev);
> device_remove_file(hba->dev, &hba->rpm_lvl_attr);
> device_remove_file(hba->dev, &hba->spm_lvl_attr); } diff --git
> a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> 6a0ec4b..1332e54 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -841,10 +841,6 @@ static inline bool ufshcd_is_hs_mode(struct
> ufs_pa_layer_attr *pwr_info) }
>
> /* Expose Query-Request API */
> -int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
> - enum query_opcode opcode,
> - enum desc_idn idn, u8 index, u8 selector,
> - u8 *desc_buf, int *buf_len);
> int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
> enum flag_idn idn, bool *flag_res);
> int ufshcd_hold(struct ufs_hba *hba, bool async);

Thank you a lot for your advice. I'll add the default group for existing sysfs entries
as first patch, update all patches as recommended and test it.