[PATCH v5] ufs: core: Add HID support

[PATCH v5] ufs: core: Add HID support

[Huan Tang]

Follow JESD220G, support HID(Host Initiated Defragmentation)
through sysfs, the relevant sysfs nodes are as follows:
	1.analysis_trigger
	2.defrag_trigger
	3.fragmented_size
	4.defrag_size
	5.progress_ratio
	6.state
The detailed definition	of the six nodes can be	found in the sysfs
documentation.

HID's execution policy is given to user-space.

Signed-off-by: Huan Tang <tanghuan@vivo.com>
Signed-off-by: Wenxing Cheng <wenxing.cheng@vivo.com>
---
Changelog
===
v4 - > v5:
	1.Fixed a typo in "indicates"
	2."DEFRAG_IS_NOT_REQUIRED" -> "DEFRAG_NOT_REQUIRED"
	3.Fix some coding style issues
v3 - > v4:
	1.Move the changelog description under "---"
v2 - > v3:
	1.Remove the "ufs_" prefix from directory name
	2.Remove the "hid_" prefix from node names
	3.Make "ufs" appear only once in the HID group name
	4.Add "is_visible" callback for "ufs_sysfs_hid_group"
v1 - > v2:
	1.Refactor the HID code according to Bart and Peter and
	Arvi's suggestions

v4
	https://lore.kernel.org/all/20250520063512.213-1-tanghuan@vivo.com/
v3
	https://lore.kernel.org/all/20250519022912.292-1-tanghuan@vivo.com/
v2
	https://lore.kernel.org/all/20250512131519.138-1-tanghuan@vivo.com/
v1
	https://lore.kernel.org/all/20250417125008.123-1-tanghuan@vivo.com/

 Documentation/ABI/testing/sysfs-driver-ufs |  83 +++++++++
 drivers/ufs/core/ufs-sysfs.c               | 192 +++++++++++++++++++++
 drivers/ufs/core/ufshcd.c                  |   4 +
 include/ufs/ufs.h                          |  25 +++
 4 files changed, 304 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index d4140dc6c5ba..f3de8c521bbd 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1685,3 +1685,86 @@ Description:
 		================  ========================================
 
 		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/analysis_trigger
+What:		/sys/bus/platform/devices/*.ufs/hid/analysis_trigger
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		The host can enable or disable HID analysis operation.
+
+		=======  =========================================
+		disable   disable HID analysis operation
+		enable    enable HID analysis operation
+		=======  =========================================
+
+		The file is write only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/defrag_trigger
+What:		/sys/bus/platform/devices/*.ufs/hid/defrag_trigger
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		The host can enable or disable HID defragmentation operation.
+
+		=======  =========================================
+		disable   disable HID defragmentation operation
+		enable    enable HID defragmentation operation
+		=======  =========================================
+
+		The attribute is write only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/fragmented_size
+What:		/sys/bus/platform/devices/*.ufs/hid/fragmented_size
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		The total fragmented size in the device is reported through
+		this attribute.
+
+		The attribute is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/defrag_size
+What:		/sys/bus/platform/devices/*.ufs/hid/defrag_size
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		The host sets the size to be defragmented by an HID
+		defragmentation operation.
+
+		The attribute is read/write.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/progress_ratio
+What:		/sys/bus/platform/devices/*.ufs/hid/progress_ratio
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		Defragmentation progress is reported by this attribute,
+		indicates the ratio of the completed defragmentation size
+		over the requested defragmentation size.
+
+		====  ============================================
+		1     1%
+		...
+		100   100%
+		====  ============================================
+
+		The attribute is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/hid/state
+What:		/sys/bus/platform/devices/*.ufs/hid/state
+Date:		May 2025
+Contact:	Huan Tang <tanghuan@vivo.com>
+Description:
+		The HID state is reported by this attribute.
+
+		====================   ===========================
+		idle			Idle (analysis required)
+		analysis_in_progress    Analysis in progress
+		defrag_required      	Defrag required
+		defrag_in_progress      Defrag in progress
+		defrag_completed      	Defrag completed
+		defrag_not_required     Defrag is not required
+		====================   ===========================
+
+		The attribute is read only.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index de8b6acd4058..978374b12931 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -87,6 +87,26 @@ static const char *ufs_wb_resize_status_to_string(enum wb_resize_status status)
 	}
 }
 
+static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
+{
+	switch (state) {
+	case HID_IDLE:
+		return "idle";
+	case ANALYSIS_IN_PROGRESS:
+		return "analysis_in_progress";
+	case DEFRAG_REQUIRED:
+		return "defrag_required";
+	case DEFRAG_IN_PROGRESS:
+		return "defrag_in_progress";
+	case DEFRAG_COMPLETED:
+		return "defrag_completed";
+	case DEFRAG_NOT_REQUIRED:
+		return "defrag_not_required";
+	default:
+		return "unknown";
+	}
+}
+
 static const char *ufshcd_uic_link_state_to_string(
 			enum uic_link_state state)
 {
@@ -1763,6 +1783,177 @@ static const struct attribute_group ufs_sysfs_attributes_group = {
 	.attrs = ufs_sysfs_attributes,
 };
 
+static int hid_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
+			enum attr_idn idn, u32 *attr_val)
+{
+	int ret;
+
+	down(&hba->host_sem);
+	if (!ufshcd_is_user_access_allowed(hba)) {
+		up(&hba->host_sem);
+		return -EBUSY;
+	}
+
+	ufshcd_rpm_get_sync(hba);
+	ret = ufshcd_query_attr(hba, opcode, idn, 0, 0, attr_val);
+	ufshcd_rpm_put_sync(hba);
+
+	up(&hba->host_sem);
+	return ret;
+}
+
+static const char * const hid_trigger_mode[] = {"disable", "enable"};
+
+static ssize_t analysis_trigger_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int mode;
+	int ret;
+
+	mode = sysfs_match_string(hid_trigger_mode, buf);
+	if (mode < 0)
+		return -EINVAL;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
+
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_WO(analysis_trigger);
+
+static ssize_t defrag_trigger_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int mode;
+	int ret;
+
+	mode = sysfs_match_string(hid_trigger_mode, buf);
+	if (mode < 0)
+		return -EINVAL;
+
+	if (mode)
+		mode = HID_ANALYSIS_AND_DEFRAG_ENABLE;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
+
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_WO(defrag_trigger);
+
+static ssize_t fragmented_size_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_HID_AVAILABLE_SIZE, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", value);
+}
+
+static DEVICE_ATTR_RO(fragmented_size);
+
+static ssize_t defrag_size_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_HID_SIZE, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", value);
+}
+
+static ssize_t defrag_size_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_HID_SIZE, &value);
+
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_RW(defrag_size);
+
+static ssize_t progress_ratio_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_HID_PROGRESS_RATIO, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", value);
+}
+
+static DEVICE_ATTR_RO(progress_ratio);
+
+static ssize_t state_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_HID_STATE, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n", ufs_hid_state_to_string(value));
+}
+
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *ufs_sysfs_hid[] = {
+	&dev_attr_analysis_trigger.attr,
+	&dev_attr_defrag_trigger.attr,
+	&dev_attr_fragmented_size.attr,
+	&dev_attr_defrag_size.attr,
+	&dev_attr_progress_ratio.attr,
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static umode_t ufs_sysfs_hid_is_visible(struct kobject *kobj,
+		struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return	hba->dev_info.hid_sup ? attr->mode : 0;
+}
+
+static const struct attribute_group ufs_sysfs_hid_group = {
+	.name = "hid",
+	.attrs = ufs_sysfs_hid,
+	.is_visible = ufs_sysfs_hid_is_visible,
+};
+
 static const struct attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_default_group,
 	&ufs_sysfs_capabilities_group,
@@ -1777,6 +1968,7 @@ static const struct attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_string_descriptors_group,
 	&ufs_sysfs_flags_group,
 	&ufs_sysfs_attributes_group,
+	&ufs_sysfs_hid_group,
 	NULL,
 };
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3e2097e65964..8ccd923a5761 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 
 	dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
 
+	dev_info->hid_sup = get_unaligned_be32(desc_buf +
+				DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
+				UFS_DEV_HID_SUPPORT;
+
 	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
 
 	err = ufshcd_read_string_desc(hba, model_index,
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index c0c59a8f7256..63af47c4e9ed 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -182,6 +182,11 @@ enum attr_idn {
 	QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE        = 0x1F,
 	QUERY_ATTR_IDN_TIMESTAMP		= 0x30,
 	QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID     = 0x34,
+	QUERY_ATTR_IDN_HID_DEFRAG_OPERATION	= 0x35,
+	QUERY_ATTR_IDN_HID_AVAILABLE_SIZE	= 0x36,
+	QUERY_ATTR_IDN_HID_SIZE			= 0x37,
+	QUERY_ATTR_IDN_HID_PROGRESS_RATIO	= 0x38,
+	QUERY_ATTR_IDN_HID_STATE		= 0x39,
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_HINT	= 0x3C,
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_EN		= 0x3D,
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_STATUS	= 0x3E,
@@ -401,6 +406,7 @@ enum {
 	UFS_DEV_HPB_SUPPORT		= BIT(7),
 	UFS_DEV_WRITE_BOOSTER_SUP	= BIT(8),
 	UFS_DEV_LVL_EXCEPTION_SUP       = BIT(12),
+	UFS_DEV_HID_SUPPORT		= BIT(13),
 };
 #define UFS_DEV_HPB_SUPPORT_VERSION		0x310
 
@@ -466,6 +472,23 @@ enum ufs_ref_clk_freq {
 	REF_CLK_FREQ_INVAL	= -1,
 };
 
+/* bDefragOperation attribute values */
+enum ufs_hid_defrag_operation {
+	HID_ANALYSIS_AND_DEFRAG_DISABLE	= 0,
+	HID_ANALYSIS_ENABLE		= 1,
+	HID_ANALYSIS_AND_DEFRAG_ENABLE	= 2,
+};
+
+/* bHIDState attribute values */
+enum ufs_hid_state {
+	HID_IDLE		= 0,
+	ANALYSIS_IN_PROGRESS	= 1,
+	DEFRAG_REQUIRED		= 2,
+	DEFRAG_IN_PROGRESS	= 3,
+	DEFRAG_COMPLETED	= 4,
+	DEFRAG_NOT_REQUIRED	= 5,
+};
+
 /* bWriteBoosterBufferResizeEn attribute */
 enum wb_resize_en {
 	WB_RESIZE_EN_IDLE	= 0,
@@ -625,6 +648,8 @@ struct ufs_dev_info {
 	u32 rtc_update_period;
 
 	u8 rtt_cap; /* bDeviceRTTCap */
+
+	bool hid_sup;
 };
 
 /*
-- 
2.48.1

Re: [PATCH v5] ufs: core: Add HID support

[Peter Wang (王信友)]

On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
> 
> +static const char * const hid_trigger_mode[] = {"disable",
> "enable"};
> +
> +static ssize_t analysis_trigger_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       int mode;
> +       int ret;
> +
> +       mode = sysfs_match_string(hid_trigger_mode, buf);
> +       if (mode < 0)
> +               return -EINVAL;
> 

Hi Haun,

Consider use below coding style for readability.

if (sysfs_streq(buf, "enable"))
    mode = ...;
else if (sysfs_streq(buf, "disable"))
    mode = ...;
else
    return -EINVAL;



> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3e2097e65964..8ccd923a5761 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> 
>         dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
> 
> +       dev_info->hid_sup = get_unaligned_be32(desc_buf +
> +                              
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> +                               UFS_DEV_HID_SUPPORT;
> +
> 

Could add the double negation (!!) ensures the value is exactly 0 or 1.
dev_info->hid_sup = !!(get_unaligned_be32(desc_buf +
            DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
            UFS_DEV_HID_SUPPORT);

The rest of the parts looks good to me.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>


Thanks
Peter

Re: [PATCH v5] ufs: core: Add HID support

[Bean Huo]

On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
> @@ -87,6 +87,26 @@ static const char *ufs_wb_resize_status_to_string(enum wb_resize_status status)
>         }
>  }
>  
> +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
> +{
> +       switch (state) {
> +       case HID_IDLE:
> +               return "idle";
> +       case ANALYSIS_IN_PROGRESS:
> +               return "analysis_in_progress";
> +       case DEFRAG_REQUIRED:
> +               return "defrag_required";
> +       case DEFRAG_IN_PROGRESS:
> +               return "defrag_in_progress";
> +       case DEFRAG_COMPLETED:
> +               return "defrag_completed";
> +       case DEFRAG_NOT_REQUIRED:
> +               return "defrag_not_required";
> +       default:
> +               return "unknown";
> +       }
> +}

The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :

#define NUM_UFS_HID_STATES 6
static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
    "idle",
    "analysis_in_progress",
    "defrag_required",
    "defrag_in_progress",
    "defrag_completed",
    "defrag_not_required"
};

static const char *ufs_hid_state_to_string(enum ufs_hid_state state) {
    if ((unsigned int)state < NUM_UFS_HID_STATES)
        return ufs_hid_states[state];
    return "unknown";
}

Re: [PATCH v5] ufs: core: Add HID support

[Bart Van Assche]

On 5/20/25 6:14 AM, Peter Wang (王信友) wrote:
> On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 3e2097e65964..8ccd923a5761 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct ufs_hba
>> *hba)
>> 
>>         dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
>> 
>> +       dev_info->hid_sup = get_unaligned_be32(desc_buf +
>> +                              
>> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
>> +                               UFS_DEV_HID_SUPPORT;
>> +
>> 
> 
> Could add the double negation (!!) ensures the value is exactly 0 or 1.
> dev_info->hid_sup = !!(get_unaligned_be32(desc_buf +
>              DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
>              UFS_DEV_HID_SUPPORT);

Hi Peter,

I do not agree that this change is required. ufs_dev_info.hid_sup has 
type bool and hence the compiler will convert the result of the binary
and operation implicitly into a boolean value (0/1). From the C23
standard: "When any scalar value is converted to bool, the result is
false if the value is a zero (for arithmetic types), null (for pointer
types), or the scalar has type nullptr_t; otherwise, the result is
true."

A double negation does not occur elsewhere in the kernel if an integer
value is assigned to a boolean variable so I think that it shouldn't be
added here either.

Bart.

Re: [PATCH v5] ufs: core: Add HID support

[Bart Van Assche]

On 5/20/25 9:01 AM, Bean Huo wrote:
> On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
>> @@ -87,6 +87,26 @@ static const char *ufs_wb_resize_status_to_string(enum wb_resize_status status)
>>          }
>>   }
>>   
>> +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
>> +{
>> +       switch (state) {
>> +       case HID_IDLE:
>> +               return "idle";
>> +       case ANALYSIS_IN_PROGRESS:
>> +               return "analysis_in_progress";
>> +       case DEFRAG_REQUIRED:
>> +               return "defrag_required";
>> +       case DEFRAG_IN_PROGRESS:
>> +               return "defrag_in_progress";
>> +       case DEFRAG_COMPLETED:
>> +               return "defrag_completed";
>> +       case DEFRAG_NOT_REQUIRED:
>> +               return "defrag_not_required";
>> +       default:
>> +               return "unknown";
>> +       }
>> +}
> 
> The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :
> 
> #define NUM_UFS_HID_STATES 6
> static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
>      "idle",
>      "analysis_in_progress",
>      "defrag_required",
>      "defrag_in_progress",
>      "defrag_completed",
>      "defrag_not_required"
> };

If this change is made, please use the designated initializer syntax
([label] = "text"). This will make it easier to verify that the
enumeration labels and the strings match.

Thanks,

Bart.

Re: [PATCH v5] ufs: core: Add HID support

[Peter Wang (王信友)]

On Tue, 2025-05-20 at 13:13 -0700, Bart Van Assche wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 5/20/25 6:14 AM, Peter Wang (王信友) wrote:
> > On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
> > > diff --git a/drivers/ufs/core/ufshcd.c
> > > b/drivers/ufs/core/ufshcd.c
> > > index 3e2097e65964..8ccd923a5761 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct
> > > ufs_hba
> > > *hba)
> > > 
> > >         dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
> > > 
> > > +       dev_info->hid_sup = get_unaligned_be32(desc_buf +
> > > +
> > > DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> > > +                               UFS_DEV_HID_SUPPORT;
> > > +
> > > 
> > 
> > Could add the double negation (!!) ensures the value is exactly 0
> > or 1.
> > dev_info->hid_sup = !!(get_unaligned_be32(desc_buf +
> >              DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> >              UFS_DEV_HID_SUPPORT);
> 
> Hi Peter,
> 
> I do not agree that this change is required. ufs_dev_info.hid_sup has
> type bool and hence the compiler will convert the result of the
> binary
> and operation implicitly into a boolean value (0/1). From the C23
> standard: "When any scalar value is converted to bool, the result is
> false if the value is a zero (for arithmetic types), null (for
> pointer
> types), or the scalar has type nullptr_t; otherwise, the result is
> true."
> 
> A double negation does not occur elsewhere in the kernel if an
> integer
> value is assigned to a boolean variable so I think that it shouldn't
> be
> added here either.
> 
> Bart.

Hi Bart,

Yes, this is not strictly necessary.
For me, this just makes it immediately clear that 
hid_sup is a bool. But I understand that you might 
think it redundant and some people will feel confused 
when reading this code.

It doesn't affect the correctness of this patch, and it is 
totally fine even if no changes are made at all. 
Hence I've added my review tag.

Thanks
Peter

Re: [PATCH v5] ufs: core: Add HID support

[Huan Tang]

>>> +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
>>> +{
>>> +       switch (state) {
>>> +       case HID_IDLE:
>>> +               return "idle";
>>> +       case ANALYSIS_IN_PROGRESS:
>>> +               return "analysis_in_progress";
>>> +       case DEFRAG_REQUIRED:
>>> +               return "defrag_required";
>>> +       case DEFRAG_IN_PROGRESS:
>>> +               return "defrag_in_progress";
>>> +       case DEFRAG_COMPLETED:
>>> +               return "defrag_completed";
>>> +       case DEFRAG_NOT_REQUIRED:
>>> +               return "defrag_not_required";
>>> +       default:
>>> +               return "unknown";
>>> +       }
>>> +}
>>
>>The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :
>>
>>#define NUM_UFS_HID_STATES 6
>>static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
>>     "idle",
>>      "analysis_in_progress",
>>      "defrag_required",
>>      "defrag_in_progress",
>>      "defrag_completed",
>>      "defrag_not_required"
>> };

> If this change is made, please use the designated initializer syntax
> ([label] = "text"). This will make it easier to verify that the
> enumeration labels and the strings match.

Hi bart and bean sir,

Thank you for your comments and guidance！

What do you think of the following changes?

ufs.h
 +/* bHIDState attribute values */                                               
 +enum ufs_hid_state {                                                           
 +       HID_IDLE                = 0,                                            
 +       ANALYSIS_IN_PROGRESS    = 1,                                            
 +       DEFRAG_REQUIRED         = 2,                                            
 +       DEFRAG_IN_PROGRESS      = 3,                                            
 +       DEFRAG_COMPLETED        = 4,                                            
 +       DEFRAG_NOT_REQUIRED     = 5,                                            
 +       NUM_UFS_HID_STATES      = 6,                                            
 +}; 

ufs-sysfs.c
 +static const char * const ufs_hid_states[] = {                                 
 +       [HID_IDLE]              = "idle",                                       
 +       [ANALYSIS_IN_PROGRESS]  = "analysis_in_progress",                       
 +       [DEFRAG_REQUIRED]       = "defrag_required",                            
 +       [DEFRAG_IN_PROGRESS]    = "defrag_in_progress",                         
 +       [DEFRAG_COMPLETED]      = "defrag_completed",                           
 +       [DEFRAG_NOT_REQUIRED]   = "defrag_not_required",                        
 +};                                                                             
 +                                                                               
 +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)           
 +{                                                                              
 +       if (state < NUM_UFS_HID_STATES)                                         
 +               return ufs_hid_states[state];                                   
 +                                                                               
 +       return "unknown";                                                       
 +} 

...

+static ssize_t state_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 value;
+	int ret;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_HID_STATE, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n", ufs_hid_state_to_string(value));
+}

Thanks
Huan

Re: [PATCH v5] ufs: core: Add HID support

[Huan Tang]

>> +static const char * const hid_trigger_mode[] = {"disable",
>> "enable"};
>> +
>> +static ssize_t analysis_trigger_store(struct device *dev,
>> +               struct device_attribute *attr, const char *buf,
>> size_t count)
>> +{
>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>> +       int mode;
>> +       int ret;
>> +
>> +       mode = sysfs_match_string(hid_trigger_mode, buf);
>> +       if (mode < 0)
>> +               return -EINVAL;
>> 
> 
> Hi Haun,
> 
> Consider use below coding style for readability.
> 
> if (sysfs_streq(buf, "enable"))
>     mode = ...;
> else if (sysfs_streq(buf, "disable"))
>     mode = ...;
> else
>    return -EINVAL;

Hi peter sir,

Thank you for your comments and guidance！
I think your modification will indeed improve the readability of the code.
What do you think of the following changes?

ufs-sysfs.c
+static ssize_t analysis_trigger_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int mode;
+	int ret;
+
+	if (sysfs_streq(buf, "enable"))
+		mode = HID_ANALYSIS_ENABLE;
+	else if (sysfs_streq(buf, "disable"))
+		mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
+	else
+		return -EINVAL;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
+
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_WO(analysis_trigger);
+
+static ssize_t defrag_trigger_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	int mode;
+	int ret;
+
+	if (sysfs_streq(buf, "enable"))
+		mode = HID_ANALYSIS_AND_DEFRAG_ENABLE;
+	else if (sysfs_streq(buf, "disable"))
+		mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
+	else
+		return -EINVAL;
+
+	ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
+
+	return ret < 0 ? ret : count;
+}


Thanks
Huan

Re: [PATCH v5] ufs: core: Add HID support

[Bean Huo]

On Wed, 2025-05-21 at 22:32 +0800, Huan Tang wrote:
> > > > +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
> > > > +{
> > > > +       switch (state) {
> > > > +       case HID_IDLE:
> > > > +               return "idle";
> > > > +       case ANALYSIS_IN_PROGRESS:
> > > > +               return "analysis_in_progress";
> > > > +       case DEFRAG_REQUIRED:
> > > > +               return "defrag_required";
> > > > +       case DEFRAG_IN_PROGRESS:
> > > > +               return "defrag_in_progress";
> > > > +       case DEFRAG_COMPLETED:
> > > > +               return "defrag_completed";
> > > > +       case DEFRAG_NOT_REQUIRED:
> > > > +               return "defrag_not_required";
> > > > +       default:
> > > > +               return "unknown";
> > > > +       }
> > > > +}
> > > 
> > > The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :
> > > 
> > > #define NUM_UFS_HID_STATES 6
> > > static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
> > >     "idle",
> > >      "analysis_in_progress",
> > >      "defrag_required",
> > >      "defrag_in_progress",
> > >      "defrag_completed",
> > >      "defrag_not_required"
> > > };
> 
> > If this change is made, please use the designated initializer syntax
> > ([label] = "text"). This will make it easier to verify that the
> > enumeration labels and the strings match.
> 
> Hi bart and bean sir,
> 
> Thank you for your comments and guidance！
> 
> What do you think of the following changes?
> 
> ufs.h
>  +/* bHIDState attribute values */                                               
>  +enum ufs_hid_state {                                                           
>  +       HID_IDLE                = 0,                                            
>  +       ANALYSIS_IN_PROGRESS    = 1,                                            
>  +       DEFRAG_REQUIRED         = 2,                                            
>  +       DEFRAG_IN_PROGRESS      = 3,                                            
>  +       DEFRAG_COMPLETED        = 4,                                            
>  +       DEFRAG_NOT_REQUIRED     = 5,                                            
>  +       NUM_UFS_HID_STATES      = 6,                                            
>  +}; 
> 
> ufs-sysfs.c
>  +static const char * const ufs_hid_states[] = {                                 
>  +       [HID_IDLE]              = "idle",                                       
>  +       [ANALYSIS_IN_PROGRESS]  = "analysis_in_progress",                       
>  +       [DEFRAG_REQUIRED]       = "defrag_required",                            
>  +       [DEFRAG_IN_PROGRESS]    = "defrag_in_progress",                         
>  +       [DEFRAG_COMPLETED]      = "defrag_completed",                           
>  +       [DEFRAG_NOT_REQUIRED]   = "defrag_not_required",                        
>  +};                                                                             
>  +                                                                               
>  +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)           
>  +{                                                                              
>  +       if (state < NUM_UFS_HID_STATES)                                         
>  +               return ufs_hid_states[state];                                   
>  +                                                                               
>  +       return "unknown";                                                       
>  +} 
> 
> ...
> 
> +static ssize_t state_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       u32 value;
> +       int ret;
> +
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +                       QUERY_ATTR_IDN_HID_STATE, &value);
> +       if (ret)
> +               return ret;
> +
> +       return sysfs_emit(buf, "%s\n", ufs_hid_state_to_string(value));
> +}
> 
> Thanks
> Huan
> 
> 
> 


looks good to me, with this change, please add my reviewed-by tag in the next version.

Kind regards,
Bean

Re: [PATCH v5] ufs: core: Add HID support

[Peter Wang (王信友)]

On Wed, 2025-05-21 at 22:33 +0800, Huan Tang wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > > +static const char * const hid_trigger_mode[] = {"disable",
> 
> > > "enable"};
> 
> > > +
> 
> > > +static ssize_t analysis_trigger_store(struct device *dev,
> 
> > > +               struct device_attribute *attr, const char *buf,
> 
> > > size_t count)
> 
> > > +{
> 
> > > +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> > > +       int mode;
> 
> > > +       int ret;
> 
> > > +
> 
> > > +       mode = sysfs_match_string(hid_trigger_mode, buf);
> 
> > > +       if (mode < 0)
> 
> > > +               return -EINVAL;
> 
> > > 
> 
> > 
> 
> > Hi Haun,
> 
> > 
> 
> > Consider use below coding style for readability.
> 
> > 
> 
> > if (sysfs_streq(buf, "enable"))
> 
> >     mode = ...;
> 
> > else if (sysfs_streq(buf, "disable"))
> 
> >     mode = ...;
> 
> > else
> 
> >    return -EINVAL;
> 
> 
> 
> Hi peter sir,
> 
> 
> 
> Thank you for your comments and guidance��?
> I think your modification will indeed improve the readability of the
> code.
> 
> What do you think of the following changes?
> 
> 
> 
> ufs-sysfs.c
> 
> +static ssize_t analysis_trigger_store(struct device *dev,
> 
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> 
> +{
> 
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> +       int mode;
> 
> +       int ret;
> 
> +
> 
> +       if (sysfs_streq(buf, "enable"))
> 
> +               mode = HID_ANALYSIS_ENABLE;
> 
> +       else if (sysfs_streq(buf, "disable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> 
> +       else
> 
> +               return -EINVAL;
> 
> +
> 
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> 
> +                       QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
> 
> +
> 
> +       return ret < 0 ? ret : count;
> 
> +}
> 
> +
> 
> +static DEVICE_ATTR_WO(analysis_trigger);
> 
> +
> 
> +static ssize_t defrag_trigger_store(struct device *dev,
> 
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> 
> +{
> 
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> +       int mode;
> 
> +       int ret;
> 
> +
> 
> +       if (sysfs_streq(buf, "enable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_ENABLE;
> 
> +       else if (sysfs_streq(buf, "disable"))
> 
> +               mode = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> 
> +       else
> 
> +               return -EINVAL;
> 
> +
> 
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> 
> +                       QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, &mode);
> 
> +
> 
> +       return ret < 0 ? ret : count;
> 
> +}
> 
> 
> 
> 
> 
> Thanks
> 
> Huan
> 

Hi Huan,

looks good to me.

Thanks
Peter