1. Patchew
  2. linux
  3. View series

[PATCH v1] scsi: ufs: core: Hold a clock reference across the probe

Naomi Chu posted 1 patch 2 weeks, 2 days ago 
drivers/ufs/core/ufshcd.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
[PATCH v1] scsi: ufs: core: Hold a clock reference across the probe
Posted by Naomi Chu 2 weeks, 2 days ago 
Clock gating becomes possible as soon as ufshcd_init_clk_gating() has
run, and from that point on the probe keeps accessing host registers
without ever taking a clock reference. This has been safe only because
of the state check in __ufshcd_release(): gate_work is not queued
unless hba->ufshcd_state is UFSHCD_STATE_OPERATIONAL, and the promotion
to that state used to happen after the last register access of the
probe, at the end of ufshcd_probe_hba().

That is fragile: it only works while the promotion happens after the
register accesses. Commit a390e6677f41 ("scsi: ufs: core: Expand the
ufshcd_device_init(hba, true) call") changed that ordering by moving
the promotion into ufshcd_init(), which schedules ufshcd_async_scan()
afterwards. ufshcd_probe_hba() therefore now runs with the state
already promoted, and it accesses host registers without holding a
clock reference:

 - on hosts with UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH it calls
   ufshcd_hba_stop() and ufshcd_hba_enable() before
   ufshcd_device_init() sets the state back to UFSHCD_STATE_RESET, and
   both read REG_CONTROLLER_ENABLE, so gated clocks stall there instead
   of just losing a write.
 - it ends with an ufshcd_configure_auto_hibern8() write, which its
   other callers do take a clock reference for.

Take a clock reference as soon as clock gating has been initialised and
keep it until the probe is over. It then does not matter who drops a
clock reference while the probe is running, and the register accesses
of the probe no longer depend on hba->ufshcd_state. The reference is
dropped by ufshcd_async_scan() once the scan has finished, or by the
new out_release label if the probe fails after it was taken.

Fixes: a390e6677f41 ("scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call")
Signed-off-by: Naomi Chu <naomi.chu@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ee21388e74f5..7d63287b5ca9 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9550,6 +9550,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	ret = ufshcd_add_lus(hba);
 
 out:
+	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
 
 	if (ret)
@@ -11272,6 +11273,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	ufshcd_init_clk_gating(hba);
 
+	/* Released by ufshcd_async_scan(), or by out_release on failure. */
+	ufshcd_hold(hba);
+
 	ufshcd_init_clk_scaling(hba);
 
 	/*
@@ -11292,7 +11296,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
 	if (err) {
 		dev_err(hba->dev, "request irq failed\n");
-		goto out_disable;
+		goto out_release;
 	} else {
 		hba->is_irq_enabled = true;
 	}
@@ -11308,7 +11312,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		dev_err(hba->dev, "Host controller enable failed\n");
 		ufshcd_print_evt_hist(hba);
 		ufshcd_print_host_state(hba);
-		goto out_disable;
+		goto out_release;
 	}
 
 	INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work, ufshcd_rpm_dev_flush_recheck_work);
@@ -11322,7 +11326,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	err = ufshcd_add_scsi_host(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	/* Hold auto suspend until async scan completes */
 	pm_runtime_get_sync(dev);
@@ -11342,7 +11346,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	err = ufshcd_link_startup(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	if (hba->mcq_enabled)
 		ufshcd_config_mcq(hba);
@@ -11359,23 +11363,23 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	/* Verify device initialization by sending NOP OUT UPIU */
 	err = ufshcd_verify_dev_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	/* Initiate UFS initialization, and waiting until completion */
 	err = ufshcd_complete_dev_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	err = ufshcd_device_params_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	err = ufshcd_post_device_init(hba);
 
 initialized:
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	ufs_sysfs_add_nodes(hba->dev);
 	hba->dme_qos_sysfs_handle = sysfs_get_dirent(hba->dev->kobj.sd,
@@ -11386,6 +11390,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_pm_qos_init(hba);
 	return 0;
 
+out_release:
+	ufshcd_release(hba);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);
-- 
2.45.2
Re: [PATCH v1] scsi: ufs: core: Hold a clock reference across the probe
Posted by Peter Wang 4 days, 5 hours ago 
On Wed, 2026-09-09 at 17:10 +0800, Naomi Chu wrote:
> Clock gating becomes possible as soon as ufshcd_init_clk_gating() has
> run, and from that point on the probe keeps accessing host registers
> without ever taking a clock reference. This has been safe only
> because
> of the state check in __ufshcd_release(): gate_work is not queued
> unless hba->ufshcd_state is UFSHCD_STATE_OPERATIONAL, and the
> promotion
> to that state used to happen after the last register access of the
> probe, at the end of ufshcd_probe_hba().
> 
> That is fragile: it only works while the promotion happens after the
> register accesses. Commit a390e6677f41 ("scsi: ufs: core: Expand the
> ufshcd_device_init(hba, true) call") changed that ordering by moving
> the promotion into ufshcd_init(), which schedules ufshcd_async_scan()
> afterwards. ufshcd_probe_hba() therefore now runs with the state
> already promoted, and it accesses host registers without holding a
> clock reference:
> 
>  - on hosts with UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH it calls
>    ufshcd_hba_stop() and ufshcd_hba_enable() before
>    ufshcd_device_init() sets the state back to UFSHCD_STATE_RESET,
> and
>    both read REG_CONTROLLER_ENABLE, so gated clocks stall there
> instead
>    of just losing a write.
>  - it ends with an ufshcd_configure_auto_hibern8() write, which its
>    other callers do take a clock reference for.
> 
> Take a clock reference as soon as clock gating has been initialised
> and
> keep it until the probe is over. It then does not matter who drops a
> clock reference while the probe is running, and the register accesses
> of the probe no longer depend on hba->ufshcd_state. The reference is
> dropped by ufshcd_async_scan() once the scan has finished, or by the
> new out_release label if the probe fails after it was taken.
> 
> Fixes: a390e6677f41 ("scsi: ufs: core: Expand the
> ufshcd_device_init(hba, true) call")
> Signed-off-by: Naomi Chu <naomi.chu@mediatek.com>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.