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

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

[Naomi Chu]

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing scsi_remove_host() on the ufshcd_init() error path leaves dangling sysfs references leading to use-after-free.
--

commit a971113b5d6d58a777d3955df4f8947756e7eb86
Author: Naomi Chu <naomi.chu@mediatek.com>

scsi: ufs: core: Hold a clock reference across the probe

This patch modifies the UFS core to take a clock reference as soon as clock
gating is initialized and holds it until the probe finishes or fails. This
ensures register accesses during the probe do not depend on the host state
and prevents them from occurring without a held clock reference.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ee21388e74f5f..7d63287b5ca94 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -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);

[ ... ]

> @@ -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);

[Severity: Critical]
This is a pre-existing issue, but does this error path leave the SCSI host
registered in sysfs if a failure occurs later in the initialization?

If ufshcd_init() fails after successfully calling ufshcd_add_scsi_host()
(for example, if ufshcd_link_startup() or ufshcd_verify_dev_init() fails
and jumps to out_release), the function returns an error without calling
scsi_remove_host().

Since the driver probe fails, devres will free managed resources such as
the MMIO region mappings and IRQs. However, the SCSI host would remain
registered in sysfs.

Could a subsequent interaction with these orphaned sysfs nodes invoke SCSI
callbacks that attempt to access the unmapped MMIO base and freed memory,
potentially leading to a use-after-free and system crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909091045.1134956-1-naomi.chu@mediatek.com?part=1