linux-kernel - Re: [PATCH v2] ufs: core: Add HID support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <88718b2f-0583-4444-8bf0-7ecf9a45329c@acm.org>
Date: Wed, 14 May 2025 14:04:56 -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, matthias.bgg@...il.com,
 angelogioacchino.delregno@...labora.com, peter.wang@...iatek.com,
 manivannan.sadhasivam@...aro.org, quic_nguyenb@...cinc.com,
 luhongfei@...o.com, linux-scsi@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Cc: opensource.kernel@...o.com, Wenxing Cheng <wenxing.cheng@...o.com>
Subject: Re: [PATCH v2] ufs: core: Add HID support

On 5/12/25 6:15 AM, Huan Tang wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/ufs_hid/hid_analysis_trigger
> +What:		/sys/bus/platform/devices/*.ufs/ufs_hid/hid_analysis_trigger
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@...o.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.

All HID sysfs attributes occur in the "ufs_hid" directory and have a
"hid_" prefix. That's two times "hid". Please remove the "hid_" prefix
from the sysfs attribute names since it is redundant.

> +static struct attribute *ufs_sysfs_ufs_hid[] = {
> +	&dev_attr_hid_analysis_trigger.attr,
> +	&dev_attr_hid_defrag_trigger.attr,
> +	&dev_attr_hid_fragmented_size.attr,
> +	&dev_attr_hid_defrag_size.attr,
> +	&dev_attr_hid_progress_ratio.attr,
> +	&dev_attr_hid_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ufs_sysfs_ufs_hid_group = {
> +	.name = "ufs_hid",
> +	.attrs = ufs_sysfs_ufs_hid,
> +};

Isn't the prefix "ufs_" in "ufs_hid" redundant since this sysfs group
occurs under a UFS host controller directory?

Regarding the name of this sysfs group, "ufs" occurs twice in that
name (ufs_sysfs_ufs_hid_group). Please make sure that "ufs" only occurs
once in that sysfs group name.

>   #define UFS_LUN_DESC_PARAM(_pname, _puname, _duname, _size)		\
>   static ssize_t _pname##_show(struct device *dev,			\
>   	struct device_attribute *attr, char *buf)			\
> @@ -1898,6 +2079,7 @@ const struct attribute_group ufs_sysfs_lun_attributes_group = {
>   
>   void ufs_sysfs_add_nodes(struct device *dev)
>   {
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
>   	int ret;
>   
>   	ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
> @@ -1905,9 +2087,22 @@ void ufs_sysfs_add_nodes(struct device *dev)
>   		dev_err(dev,
>   			"%s: sysfs groups creation failed (err = %d)\n",
>   			__func__, ret);
> +
> +	if (hba->dev_info.hid_sup) {
> +		ret = sysfs_create_group(&dev->kobj, &ufs_sysfs_ufs_hid_group);
> +		if (ret)
> +			dev_err(dev,
> +				"%s: sysfs ufs_hid group creation failed (err = %d)\n",
> +				__func__, ret);
> +	}
>   }

Please merge ufs_sysfs_ufs_hid_group into ufs_sysfs_groups, remove the
new sysfs_create_group() call and add a visibility callback in
ufs_sysfs_ufs_hid_group that only makes the attributes in this group
visible if hba->dev_info.hid_sup == true.

Otherwise this patch looks good to me.

Thanks,

Bart.