Re: [PATCH 2/2] ufs: core: Really fix the code for updating the "hid" attribute group

[Neil Armstrong <neil.armstrong@linaro.org>]

On 10/27/25 18:09, Bart Van Assche wrote:
> Recently a sysfs_update_group() call was added in ufs_get_device_desc().
> ufs_get_device_desc() may be called from the error handler and the error
> handler may be activated before ufs_sysfs_add_nodes() is called. This
> causes creation of the "hid" directory before ufs_sysfs_add_nodes() is
> called and hence causes this function to fail.
> 
> Fix this by tracking whether or not ufs_sysfs_add_nodes() has already
> been called. This patch fixes the following kernel warning:
> 
> sysfs: cannot create duplicate filename '/devices/platform/3c2d0000.ufs/hid'
> Workqueue: async async_run_entry_fn
> Call trace:
>   dump_backtrace+0xfc/0x17c
>   show_stack+0x18/0x28
>   dump_stack_lvl+0x40/0x104
>   dump_stack+0x18/0x3c
>   sysfs_warn_dup+0x6c/0xc8
>   internal_create_group+0x1c8/0x504
>   sysfs_create_groups+0x38/0x9c
>   ufs_sysfs_add_nodes+0x20/0x58
>   ufshcd_init+0x1114/0x134c
>   ufshcd_pltfrm_init+0x728/0x7d8
>   ufs_google_probe+0x30/0x84
>   platform_probe+0xa0/0xe0
>   really_probe+0x114/0x454
>   __driver_probe_device+0xa4/0x160
>   driver_probe_device+0x44/0x23c
>   __device_attach_driver+0x15c/0x1f4
>   bus_for_each_drv+0x10c/0x168
>   __device_attach_async_helper+0x80/0xf8
>   async_run_entry_fn+0x4c/0x17c
>   process_one_work+0x26c/0x65c
>   worker_thread+0x33c/0x498
>   kthread+0x110/0x134
>   ret_from_fork+0x10/0x20
> ufshcd 3c2d0000.ufs: ufs_sysfs_add_nodes: sysfs groups creation failed (err = -17)
> 
> Cc: Daniel Lee <chullee@google.com>
> Cc: Peter Wang <peter.wang@mediatek.com>
> Fixes: bb7663dec67b ("scsi: ufs: sysfs: Make HID attributes visible")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufs-sysfs.c | 7 ++++++-
>   drivers/ufs/core/ufshcd.c    | 3 ++-
>   include/ufs/ufshcd.h         | 3 +++
>   3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index 7150c15287d1..714c611b85b0 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -2095,13 +2095,18 @@ void ufs_sysfs_add_nodes(struct ufs_hba *hba)
>   	int ret;
>   
>   	ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
> -	if (ret)
> +	if (ret) {
>   		dev_err(dev,
>   			"%s: sysfs groups creation failed (err = %d)\n",
>   			__func__, ret);
> +		return;
> +	}
> +
> +	WRITE_ONCE(hba->sysfs_attrs_added, true);
>   }
>   
>   void ufs_sysfs_remove_nodes(struct ufs_hba *hba)
>   {
> +	WRITE_ONCE(hba->sysfs_attrs_added, false);
>   	sysfs_remove_groups(&hba->dev->kobj, ufs_sysfs_groups);
>   }
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 9521aa38211c..4a543a2a10a4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8499,7 +8499,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>   				DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
>   				UFS_DEV_HID_SUPPORT;
>   
> -	sysfs_update_group(&hba->dev->kobj, &ufs_sysfs_hid_group);
> +	if (READ_ONCE(hba->sysfs_attrs_added))
> +		sysfs_update_group(&hba->dev->kobj, &ufs_sysfs_hid_group);
>   
>   	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
>   
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 9425cfd9d00e..de7420ee127e 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -869,6 +869,8 @@ enum ufshcd_mcq_opr {
>    * @ee_ctrl_mutex: Used to serialize exception event information.
>    * @is_powered: flag to check if HBA is powered
>    * @shutting_down: flag to check if shutdown has been invoked
> + * @sysfs_attrs_added: Whether or not the sysfs attributes have been added to
> + *	hba->dev.
>    * @host_sem: semaphore used to serialize concurrent contexts
>    * @eh_wq: Workqueue that eh_work works on
>    * @eh_work: Worker to handle UFS errors that require s/w attention
> @@ -1021,6 +1023,7 @@ struct ufs_hba {
>   	struct mutex ee_ctrl_mutex;
>   	bool is_powered;
>   	bool shutting_down;
> +	bool sysfs_attrs_added;
>   	struct semaphore host_sem;
>   
>   	/* Work Queues */

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK