Re: [PATCH] ufs: sysfs: fix current_power_mode read without SSU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Exiting Hibern8 manually without restoring it leaves the UFS link in the ACTIVE state, leading to hardware state corruption and power leaks when the host controller auto-suspends.
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race exists between the sysfs read and WLUN runtime resume, potentially causing duplicate Hibern8 exits and forcing severe link recovery.
- [High] The sysfs read blindly sends a UPIU query over an OFF link if the UFS device was suspended to a DeepSleep/PowerDown state.
--

commit f4fe909880e2766ce73fa9889b03cdb85f6b474d
Author: Yiwei Lin <s921975628@gmail.com>

ufs: sysfs: fix current_power_mode read without SSU

This patch changes how current_power_mode is read from sysfs to avoid waking
the device to Active state. It directly fetches the PM sync reference, exits
Hibern8 if necessary, queries the attribute, and drops the PM reference.

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index cf49ad0258e14..86705432aa42d 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
[ ... ]
> +	ufshcd_hold(hba);
> +
> +	if (ufshcd_is_link_hibern8(hba)) {

[Severity: High]
Could this introduce a Time-Of-Check to Time-Of-Use (TOCTOU) race with a
concurrent WLUN runtime resume?

If a sysfs read of current_power_mode executes concurrently with an I/O
request that triggers ufshcd_wl_runtime_resume(), both threads might check
ufshcd_is_link_hibern8() at the same time without holding uic_cmd_mutex.

If both see the link in Hibern8, they will serialize on uic_cmd_mutex inside
ufshcd_uic_pwr_ctrl(). The first thread will set the link to ACTIVE, and the
second thread will send a duplicate UIC_CMD_DME_HIBER_EXIT command to the
already ACTIVE link, which can cause a hardware command failure and trigger
a severe link recovery.

> +		ret = ufshcd_uic_hibern8_exit(hba);
> +		if (!ret)
> +			ufshcd_set_link_active(hba);
> +		else
> +			dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
> +				__func__, ret);
> +	}
> +
> +	if (!ret)
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_POWER_MODE, 0, 0, &value);

[Severity: High]
Does this blindly send a UPIU query over an OFF link if the UFS device was
suspended to a DeepSleep/PowerDown state?

When the UFS WLUN is suspended to a level that puts the link into
UIC_LINK_OFF_STATE, ufshcd_is_link_hibern8() evaluates to false. This
function will skip link initialization entirely and leave it in the OFF
state, proceeding to call ufshcd_query_attr().

Sending a query to unpowered/unlinked hardware typically fails with a
hardware timeout or fatal controller error.

> +
> +	ufshcd_release(hba);
> +	pm_runtime_put(hba->dev);

[Severity: High]
Does dropping the host controller's runtime PM reference here leave the
hardware in an invalid state if we explicitly exited Hibern8?

Because the link was manually transitioned to ACTIVE in this function
without an explicit mechanism to restore it to Hibern8, the UFS link
remains in the ACTIVE state.

When pm_runtime_put() drops the usage count, ufshcd_runtime_suspend()
can execute and unconditionally turn off the UFS clocks. Turning off
clocks while the link is physically ACTIVE violates the UFS specification
and can lead to hardware lockup or fatal controller errors upon the
next access.

> +
> +	if (ret) {
> +		up(&hba->host_sem);
> +		return -EINVAL;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914170032.5551-1-s921975628@gmail.com?part=1