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(+)
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.1On 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";
}
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.
>>> +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
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
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
>> +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
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
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.
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
© 2016 - 2025 Red Hat, Inc.