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

[Bart Van Assche <bvanassche@acm.org>]

On 4/17/25 5:50 AM, Huan Tang wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/hid_trigger
> +What:		/sys/bus/platform/devices/*.ufs/hid_trigger
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		The host can enable or disable complete HID.
> +
> +		========  ============
> +		enable    Let kworker(ufs_hid_enable_work) execute the complete HID
> +		disable   Cancel kworker(ufs_hid_enable_work) and disable HID
> +		========  ============
> +		The file is write only.
> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/defrag_operation
> +What:		/sys/bus/platform/devices/*.ufs/attributes/defrag_operation
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		The host can enable or disable HID analysis and HID defrag operations.
> +
> +		===============  ==================================================
> +		all_disable      HID analysis and HID defrag operation are disabled
> +		analysis_enable  HID analysis is enabled
> +		all_enable       HID analysis and HID defrag operation are enabled
> +		===============  ==================================================
> +
> +		The attribute is read/write.

Combining HID analysis and HID defragmentation controls into a single
sysfs attribute seems weird to me. Please replace the above two
attributes with two different attributes: one for controlling HID
analysis and another one for controlling HID defragmentation.

> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/hid_available_size
> +What:		/sys/bus/platform/devices/*.ufs/attributes/hid_available_size
> +Date:		April 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.

Please change the name of this attribute into "hid_fragmented_size". I
think that this alternative name is much more clear.

> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/hid_size
> +What:		/sys/bus/platform/devices/*.ufs/attributes/hid_size
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		The host sets the size to be defragmented by an HID defrag operation.
> +
> +		The attribute is read/write.

Please make the name of this attribute more clear, e.g. by renaming it
into "hid_defrag_size". Additionally, please change "defrag" into
"defragmentation".

> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/hid_progress_ratio
> +What:		/sys/bus/platform/devices/*.ufs/attributes/hid_progress_ratio
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		Defrag progress is reported by this attribute,indicates the ratio of
> +		the completed defrag size over the requested defrag size.
> +
> +		====  ======================================
> +		01h   1%
> +		...
> +		64h   100%
> +		====  ======================================
> +
> +		The attribute is read only.

Please change the format of this attribute from hexadecimal into
decimal (64h -> 100).

> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/hid_state
> +What:		/sys/bus/platform/devices/*.ufs/attributes/hid_state
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		The HID state is reported by this attribute.
> +
> +		====   ======================================
> +		00h    Idle(analysis required)
> +		01h    Analysis in progress
> +		02h    Defrag required
> +		03h    Defrag in progress
> +		04h    Defrag completed
> +		05h    Defrag is not required
> +		====  ======================================
> +
> +		The attribute is read only.

Please change the format of this attribute from hexadecimal into
decimal. Please add an additional sysfs attribute that provides the
textual meaning of the HID state such that users don't have to look up
the documentation of these codes.

> +config SCSI_UFS_HID
> +	bool "Support UFS Host Initiated Defrag"
> +	help
> +	  The UFS HID feature allows the host to check the status of whether
> +	  defragmentation is needed or not and enable the device's internal
> +	  defrag operation explicitly.
> +
> +	  If unsure, say N.

Here and everywhere else in documentation that is intended for humans,
please change "defrag" into "defragmentation". Please also make the
Kconfig description more clear, e.g. by changing it into the following:

	help
	  In NAND-based storage devices garbage collection is
	  inevitable. Garbage collection may cause latency spikes.
	  The UFS Host-Initiated Defragmentation (HID) functionality
	  gives the host control over when defragmentation (garbage
	  collection) happens and hence can be used to avoid latency
	  spikes.

> +#define HID_SCHED_COUNT_LIMIT	300
> +static int hid_sched_cnt;
> +static void ufs_hid_enable_work_fn(struct work_struct *work)

A blank line is required after the macro definition and also between the
declaration of the static variable and the function definition.

> +{
> +	struct ufs_hba *hba;
> +	int ret = 0;
> +	enum ufs_hid_defrag_operation defrag_op;
> +	u32 hid_ahit = 0;
> +	bool hid_flag = false;

In new code, please order declarations such that the longest declaration
occurs first ("reverse Christmas tree").

> +	hba = container_of(work, struct ufs_hba, ufs_hid_enable_work.work);
> +
> +	if (!hba->dev_info.hid_sup)
> +		return;
> +
> +	down(&hba->host_sem);
> +
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return;
> +	}
> +
> +	ufshcd_rpm_get_sync(hba);
> +	hid_ahit = hba->ahit;
> +	ufshcd_auto_hibern8_update(hba, 0);

Why is auto-hibernation disabled here? Please add a comment.

> +int ufs_hid_disable(struct ufs_hba *hba)
> +{
> +	enum ufs_hid_defrag_operation defrag_op = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> +	u32 hid_ahit;
> +	int ret;
> +
> +	down(&hba->host_sem);
> +
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +
> +	ufshcd_rpm_get_sync(hba);
> +	hid_ahit = hba->ahit;
> +	ufshcd_auto_hibern8_update(hba, 0);
> +
> +	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +				QUERY_ATTR_IDN_HID_DEFRAG_OPERATION, 0, 0, &defrag_op);
> +
> +	ufshcd_auto_hibern8_update(hba, hid_ahit);
> +	ufshcd_rpm_put_sync(hba);
> +	up(&hba->host_sem);
> +
> +	return ret;
> +}

Please add a comment that explains why the ufshcd_auto_hibern8_update()
call is present.

> +static ssize_t hid_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, "0x%08X\n", value);
> +}

Why the hexadecimal format? All other sysfs size attributes I know of
use the decimal format.

Thanks,

Bart.