[PATCH 0/2] Two additional UFS fixes for this rc cycle

[PATCH 0/2] Two additional UFS fixes for this rc cycle

[Bart Van Assche]

Hi Martin,

Commit c74dc8ab47c1 ("scsi: ufs: core: Fix a race condition related to the
"hid" attribute group") is an incomplete fix for the race condition mentioned
in the description of that commit. These two patches fix the remaining race
condition. Please consider these two patches for the current development cycle.

Thank you,

Bart.

Bart Van Assche (2):
  ufs: core: Change the ufs_sysfs_{add,remove}_nodes() argument type
  ufs: core: Really fix the code for updating the "hid" attribute group

 drivers/ufs/core/ufs-sysfs.c | 14 ++++++++++----
 drivers/ufs/core/ufs-sysfs.h |  5 +++--
 drivers/ufs/core/ufshcd.c    |  7 ++++---
 include/ufs/ufshcd.h         |  3 +++
 4 files changed, 20 insertions(+), 9 deletions(-)

[PATCH 1/2] ufs: core: Change the ufs_sysfs_{add,remove}_nodes() argument type

[Bart Van Assche]

Prepare for adding code that accesses a struct ufs_hba member. No
functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-sysfs.c | 7 ++++---
 drivers/ufs/core/ufs-sysfs.h | 5 +++--
 drivers/ufs/core/ufshcd.c    | 4 ++--
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index c040afc6668e..7150c15287d1 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -2089,8 +2089,9 @@ const struct attribute_group ufs_sysfs_lun_attributes_group = {
 	.attrs = ufs_sysfs_lun_attributes,
 };
 
-void ufs_sysfs_add_nodes(struct device *dev)
+void ufs_sysfs_add_nodes(struct ufs_hba *hba)
 {
+	struct device *dev = hba->dev;
 	int ret;
 
 	ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
@@ -2100,7 +2101,7 @@ void ufs_sysfs_add_nodes(struct device *dev)
 			__func__, ret);
 }
 
-void ufs_sysfs_remove_nodes(struct device *dev)
+void ufs_sysfs_remove_nodes(struct ufs_hba *hba)
 {
-	sysfs_remove_groups(&dev->kobj, ufs_sysfs_groups);
+	sysfs_remove_groups(&hba->dev->kobj, ufs_sysfs_groups);
 }
diff --git a/drivers/ufs/core/ufs-sysfs.h b/drivers/ufs/core/ufs-sysfs.h
index 6efb82a082fd..c9e3751c6793 100644
--- a/drivers/ufs/core/ufs-sysfs.h
+++ b/drivers/ufs/core/ufs-sysfs.h
@@ -8,9 +8,10 @@
 #include <linux/sysfs.h>
 
 struct device;
+struct ufs_hba;
 
-void ufs_sysfs_add_nodes(struct device *dev);
-void ufs_sysfs_remove_nodes(struct device *dev);
+void ufs_sysfs_add_nodes(struct ufs_hba *hba);
+void ufs_sysfs_remove_nodes(struct ufs_hba *hba);
 
 extern const struct attribute_group ufs_sysfs_unit_descriptor_group;
 extern const struct attribute_group ufs_sysfs_lun_attributes_group;
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5d6297aa5c28..9521aa38211c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10429,7 +10429,7 @@ void ufshcd_remove(struct ufs_hba *hba)
 		ufshcd_rpm_get_sync(hba);
 	ufs_hwmon_remove(hba);
 	ufs_bsg_remove(hba);
-	ufs_sysfs_remove_nodes(hba->dev);
+	ufs_sysfs_remove_nodes(hba);
 	cancel_delayed_work_sync(&hba->ufs_rtc_update_work);
 	blk_mq_destroy_queue(hba->tmf_queue);
 	blk_put_queue(hba->tmf_queue);
@@ -10887,7 +10887,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	if (err)
 		goto out_disable;
 
-	ufs_sysfs_add_nodes(hba->dev);
+	ufs_sysfs_add_nodes(hba);
 	async_schedule(ufshcd_async_scan, hba);
 
 	device_enable_async_suspend(dev);

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

[Bart Van Assche]

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 */

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

[Bjorn Andersson]

On Mon, Oct 27, 2025 at 10:09:30AM -0700, 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:
> 

I don't fancy the spirit of this fix. The device driver should be
designed such that initialization happens in a controlled manner, and we
should not have to rely on conditionals sprinkled across the driver to
seemingly get the result we expect (and as this isn't the first attempt,
are we sure you got whacked all the moles now?).


In the message above you're talking about "the error handler", but you
don't specify which error handler this would be.

Looking at the driver, there are a bunch of places where we might end up
calling:

ufshcd_host_reset_and_restore()
  ufshcd_device_init()
    ufshcd_device_params_init()
      ufs_get_device_desc()
  ufshcd_probe_hba()
    ufshcd_device_init()
      ufshcd_device_params_init()
        ufs_get_device_desc()

But we seem to only end up here in the event of suspend/resume or if
ufshcd_schedule_eh_work() has been invoked. I'm guessing that this is
the error handler you're referring to, but I don't see why we would end
up in either suspend or the error handler while we're still half-way
through the initialization of the controller.


Going back to ufshcd_init() on the other hand, we have the following
flow:

ufshcd_init()
  if not UFSHCD_QUIRK_SKIP_PH_CONFIGURATION
    ufshcd_device_params_init()
      ufs_get_device_desc()   <- the cause, at least on my target

  ufs_sysfs_add_nodes()   <- your callstack

  initialized:
  ufshcd_async_scan() <- also the cause, prior to c74dc8ab47c1 ("scsi: ufs: core: Fix a race condition...
    ufshcd_probe_hba()
      ufshcd_device_init()
        ufshcd_device_params_init()
          ufs_get_device_desc()


Which would imply that unless you're on Exynos, you will always
ufs_get_device_desc() and then ufs_sysfs_add_nodes().


So, unless I'm missing something with regards to the error handler that
you refer to, I think you should solve this problem in ufshcd_init(), by
making sure that syfs attributes are created before the
ufshcd_device_params_init() call.


While it does complicate error handling a little bit, I think it makes
total sense to ensure that all such initialization happens before you
start operating the link.


PS. How come these are attributes on the host device and not on the SCSI
host device (i.e. ufshcd_driver_template::shost_groups)? It seems like
more structured place to have such properties, and would avoid having to
dynamically create/destroy them from the ufshcd driver itself.

Regards,
Bjorn

> 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 */

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

[Neil Armstrong]

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

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

[Peter Wang (王信友)]

On Mon, 2025-10-27 at 22:03 -0500, Bjorn Andersson wrote:
> 
> 
> I don't fancy the spirit of this fix. The device driver should be
> designed such that initialization happens in a controlled manner, and
> we
> should not have to rely on conditionals sprinkled across the driver
> to
> seemingly get the result we expect (and as this isn't the first
> attempt,
> are we sure you got whacked all the moles now?).
> 

Agreed, ensuring the correct flow is better than using some
tricky protection to solve racing issues.
Previously, I asked about fixing the flow:
"Would it be better to ensure that ufshcd_async_scan completes before
invoking ufs_sysfs_add_nodes?"
The answer I got was:
"That would make the sysfs attributes unavailable if the LUN scan
hangs."
But if the LUN scan hangs, shouldn’t we just address the LUN scan hang
issue directly?
Otherwise, the system wouldn’t be able to boot anyway.

Anyway, this patch should be able to fix the warning, although it is a
bit tricky.

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

[Bart Van Assche]

On 10/28/25 5:47 AM, Peter Wang (王信友) wrote:
> Agreed, ensuring the correct flow is better than using some
> tricky protection to solve racing issues.
> Previously, I asked about fixing the flow:
> "Would it be better to ensure that ufshcd_async_scan completes before
> invoking ufs_sysfs_add_nodes?"
There is another reason why delaying the ufs_sysfs_add_nodes() call 
further is wrong: ideally sysfs attributes should be added before the
uevent is emitted for the associated device. This is why Greg KH keeps
asking to use the groups member of struct device instead of calling
sysfs_create_groups() directly.

Bart.

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

[Bart Van Assche]

On 10/27/25 8:03 PM, Bjorn Andersson wrote:
> So, unless I'm missing something with regards to the error handler that
> you refer to, I think you should solve this problem in ufshcd_init(), by
> making sure that syfs attributes are created before the
> ufshcd_device_params_init() call.

Something like this? (needs more work to make sure all state information 
that can be modified through sysfs has been initialized before the sysfs 
attributes are added)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5d6297aa5c28..3ad258922036 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10800,6 +10800,8 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)
          */
         ufshcd_readl(hba, REG_INTERRUPT_ENABLE);

+       ufs_sysfs_add_nodes(hba->dev);
+
         /* IRQ registration */
         err = devm_request_threaded_irq(dev, irq, ufshcd_intr, 
ufshcd_threaded_intr,
                                         IRQF_ONESHOT | IRQF_SHARED, 
UFSHCD, hba);
@@ -10887,7 +10889,6 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)
         if (err)
                 goto out_disable;

-       ufs_sysfs_add_nodes(hba->dev);
         async_schedule(ufshcd_async_scan, hba);

         device_enable_async_suspend(dev);

> PS. How come these are attributes on the host device and not on the SCSI
> host device (i.e. ufshcd_driver_template::shost_groups)? It seems like
> more structured place to have such properties, and would avoid having to
> dynamically create/destroy them from the ufshcd driver itself.

This choice was made before I started contributing to the UFSHCI driver.
I'm not sure why this choice has been made. If we would start over from
scratch, I think the UFSHCI attributes should be added to the UFSHCI
device instance and the UFS attributes should be added to the SCSI host.
Today the sysfs attributes created by the UFSHCI driver are a mix of
UFSHCI and UFS attributes.

This is a change we can't make today because it would break user space.

Thanks,

Bart.

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

[Bjorn Andersson]

On Tue, Oct 28, 2025 at 06:57:07AM -0700, Bart Van Assche wrote:
> 
> On 10/27/25 8:03 PM, Bjorn Andersson wrote:
> > So, unless I'm missing something with regards to the error handler that
> > you refer to, I think you should solve this problem in ufshcd_init(), by
> > making sure that syfs attributes are created before the
> > ufshcd_device_params_init() call.
> 
> Something like this? (needs more work to make sure all state information
> that can be modified through sysfs has been initialized before the sysfs
> attributes are added)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5d6297aa5c28..3ad258922036 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10800,6 +10800,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>          */
>         ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> 
> +       ufs_sysfs_add_nodes(hba->dev);
> +
>         /* IRQ registration */
>         err = devm_request_threaded_irq(dev, irq, ufshcd_intr,
> ufshcd_threaded_intr,
>                                         IRQF_ONESHOT | IRQF_SHARED, UFSHCD,
> hba);
> @@ -10887,7 +10889,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>         if (err)
>                 goto out_disable;
> 
> -       ufs_sysfs_add_nodes(hba->dev);
>         async_schedule(ufshcd_async_scan, hba);
> 
>         device_enable_async_suspend(dev);
> 

Yes, to me that looks like it would solve the issue at hand in a clean
fashion. But you'd need cleanup in out_disable as well.

> > PS. How come these are attributes on the host device and not on the SCSI
> > host device (i.e. ufshcd_driver_template::shost_groups)? It seems like
> > more structured place to have such properties, and would avoid having to
> > dynamically create/destroy them from the ufshcd driver itself.
> 
> This choice was made before I started contributing to the UFSHCI driver.
> I'm not sure why this choice has been made. If we would start over from
> scratch, I think the UFSHCI attributes should be added to the UFSHCI
> device instance and the UFS attributes should be added to the SCSI host.
> Today the sysfs attributes created by the UFSHCI driver are a mix of
> UFSHCI and UFS attributes.
> 
> This is a change we can't make today because it would break user space.
> 

That's too bad, it would have been nice to get that cleaned up.

Regards,
Bjorn

> Thanks,
> 
> Bart.

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

[Bart Van Assche]

On 10/28/25 7:50 AM, Bjorn Andersson wrote:
> Yes, to me that looks like it would solve the issue at hand in a clean
> fashion. But you'd need cleanup in out_disable as well.
I have started testing the patch below:

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index d0a2c963a27d..aba27d60b53d 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -8,7 +8,7 @@

  static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
  {
-	return !hba->shutting_down;
+	return hba->sysfs_allowed && !hba->shutting_down;
  }

  void ufshcd_schedule_eh_work(struct ufs_hba *hba);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5d6297aa5c28..38fb75a89724 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10786,6 +10786,9 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)

  	ufshcd_init_clk_scaling(hba);

+	/* Add the sysfs nodes before the UFS error handler can be activated. */
+	ufs_sysfs_add_nodes(hba->dev);
+
  	/*
  	 * In order to avoid any spurious interrupt immediately after
  	 * registering UFS controller interrupt handler, clear any pending UFS
@@ -10883,11 +10886,15 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)
  	if (err)
  		goto out_disable;

+	/* Allow access through sysfs. */
+	down(&hba->host_sem);
+	hba->sysfs_allowed = true;
+	up(&hba->host_sem);
+
  	err = ufshcd_add_scsi_host(hba);
  	if (err)
  		goto out_disable;

-	ufs_sysfs_add_nodes(hba->dev);
  	async_schedule(ufshcd_async_scan, hba);

  	device_enable_async_suspend(dev);
@@ -10895,6 +10902,7 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)
  	return 0;

  out_disable:
+	ufs_sysfs_remove_nodes(hba->dev);
  	hba->is_irq_enabled = false;
  	ufshcd_hba_exit(hba);
  out_error:
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9425cfd9d00e..01c2db4473ee 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -868,6 +868,7 @@ enum ufshcd_mcq_opr {
   * @ee_usr_mask: Exception event mask for user (set via debugfs)
   * @ee_ctrl_mutex: Used to serialize exception event information.
   * @is_powered: flag to check if HBA is powered
+ * @sysfs_allowed: whether or not access from sysfs is allowed
   * @shutting_down: flag to check if shutdown has been invoked
   * @host_sem: semaphore used to serialize concurrent contexts
   * @eh_wq: Workqueue that eh_work works on
@@ -1020,6 +1021,7 @@ struct ufs_hba {
  	u16 ee_usr_mask;
  	struct mutex ee_ctrl_mutex;
  	bool is_powered;
+	bool sysfs_allowed;
  	bool shutting_down;
  	struct semaphore host_sem;