| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4eef6172-3ba2-43a9-b6af-3750c39bb344@acm.org>
Date: Thu, 17 Apr 2025 15:05:01 -0700
From: Bart Van Assche <bvanassche@....org>
To: Huan Tang <tanghuan@...o.com>, alim.akhtar@...sung.com,
avri.altman@....com, James.Bottomley@...senPartnership.com,
martin.petersen@...cle.com, beanhuo@...ron.com, keosung.park@...sung.com,
quic_ziqichen@...cinc.com, viro@...iv.linux.org.uk, gwendal@...omium.org,
peter.wang@...iatek.com, manivannan.sadhasivam@...aro.org,
quic_cang@...cinc.com, quic_nguyenb@...cinc.com, ebiggers@...gle.com,
minwoo.im@...sung.com, linux-kernel@...r.kernel.org,
linux-scsi@...r.kernel.org
Cc: opensource.kernel@...o.com, luhongfei@...o.com,
Wenxing Cheng <wenxing.cheng@...o.com>
Subject: Re: [PATCH] ufs: core: Add HID support
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@...o.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@...o.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@...o.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@...o.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@...o.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@...o.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.
Powered by blists - more mailing lists