Re: [PATCH 1/8] ufs: core: Fix a race condition related to the "hid" attribute group

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

On 10/14/25 1:00 PM, Bart Van Assche wrote:
> ufs_sysfs_add_nodes() is called concurrently with ufs_get_device_desc().
> This may cause the following code to be called before
> ufs_sysfs_add_nodes():
(replying to my own email)

I'm considering to replace this patch with the patch below:

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 875aab4a9cce..05f700f9cfa7 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -2086,18 +2086,23 @@ 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);
-	if (ret)
+	if (ret) {
  		dev_err(dev,
  			"%s: sysfs groups creation failed (err = %d)\n",
  			__func__, ret);
+	} else {
+		WRITE_ONCE(hba->sysfs_attrs_added, true);
+	}
  }

-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);
+	WRITE_ONCE(hba->sysfs_attrs_added, false);
+	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 0013d21e2169..2887c5623992 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8604,7 +8604,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];

@@ -10539,7 +10540,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);
@@ -11003,8 +11004,8 @@ int ufshcd_init(struct ufs_hba *hba, void 
__iomem *mmio_base, unsigned int irq)
  	if (err)
  		goto out_disable;

+	ufs_sysfs_add_nodes(hba);
  	async_schedule(ufshcd_async_scan, hba);
-	ufs_sysfs_add_nodes(dev);
  	trace_android_vh_ufs_update_sysfs(hba);

  	device_enable_async_suspend(dev);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index fba73d28ad87..9d7a88e73ba5 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -905,6 +905,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
@@ -1054,6 +1056,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 */