Re: [PATCH 2/2] phy: qcom: qmp-usb: Add support for SA8255P

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Wed, Jul 08, 2026 at 13:11, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The driver leaks runtime PM usage counters for its manually managed power domains ('usb_transfer' and 'usb_core') on multiple error paths, leaving hardware permanently active and risking usage counter overflow.
> - [High] Teardown operations disable PM domains before disabling leaf consumer clocks and asserting resets, violating strict reverse-initialization ordering and risking hardware access faults.
> --
>
> --- Patch [2]: [PATCH 2/2] phy: qcom: qmp-usb: Add support for SA8255P ---
> commit 487667fe3c076bbe0851ee1739a5d0f2311634d9
> Author: Sriram Dash <quic_sriramd@quicinc.com>
> Subject: phy: qcom: qmp-usb: Add support for SA8255P
>
> This commit adds support for the SA8255P SoC to the QMP USB PHY driver.
> It introduces firmware-managed power domains that are controlled via
> runtime PM APIs during the driver's power state transitions.
>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> index c5507168e1354..2ac6a23868bf9 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> [ ... ]
>> @@ -1890,6 +1890,77 @@ static const struct qmp_phy_cfg glymur_usb3_uniphy_cfg = {
> [ ... ]
>> +/* d3_to_d0 transition by turning on all the suppliers */
>> +static int qmp_usb_d3_to_d0(struct qmp_usb *qmp)
>> +{
>> +	int ret;
>> +
>> +	if (!qmp->pd_list)
>> +		return 0;
>> +
>> +	ret = pm_runtime_resume_and_get(qmp->pd_list->pd_devs[0]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(qmp->pd_list->pd_devs[1]);
>> +
>> +	return ret;
>
> [Severity: High]
> If the resume of pd_devs[1] fails here, will this leak the runtime PM usage
> counter for pd_devs[0]? Should there be a pm_runtime_put_sync() for
> pd_devs[0] before returning the error?

Indeed. Will fix in v2.

>
>> +}
>> +
>> +/* d0_to_d3 transition by turning off all the suppliers */
>> +static void qmp_usb_d0_to_d3(struct qmp_usb *qmp)
>> +{
>> +	if (!qmp->pd_list)
>> +		return;
>> +
>> +	pm_runtime_put_sync(qmp->pd_list->pd_devs[0]);
>> +	pm_runtime_put_sync(qmp->pd_list->pd_devs[1]);
>
> [Severity: High]
> Since qmp_usb_d3_to_d0() enables these domains in the order 0 then 1,
> should this disable them in the reverse order (1 then 0)?

Indeed. Will fix in v2.

>
>> +}
> [ ... ]
>> @@ -1980,6 +1980,12 @@ static int qmp_usb_init(struct phy *phy)
>>  	void __iomem *pcs = qmp->pcs;
>>  	int ret;
>>  
>> +	ret = qmp_usb_d3_to_d0(qmp);
>> +	if (ret < 0) {
>> +		dev_err(qmp->dev, "Failed to transition to d0 state\n");
>> +		return ret;
>> +	}
>> +
>>  	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>  	if (ret) {
>>  		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
>>  		return ret;
>
> [Severity: High]
> If regulator_bulk_enable() fails, this returns directly. Does this leave
> the PM domains enabled, leaking their usage counters since qmp_usb_d0_to_d3()
> is not called to balance the earlier qmp_usb_d3_to_d0() call?

Indeed. Will fix in v2.

>
>> @@ -2025,6 +2025,8 @@ static int qmp_usb_exit(struct phy *phy)
>>  	struct qmp_usb *qmp = phy_get_drvdata(phy);
>>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>>  
>> +	qmp_usb_d0_to_d3(qmp);
>> +
>>  	reset_control_bulk_assert(qmp->num_resets, qmp->resets);
>>  
>>  	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>
> [Severity: High]
> This disables the power domains via qmp_usb_d0_to_d3() before disabling
> the leaf clocks and asserting the resets. Can this sequence cause
> unclocked or unpowered register accesses and trigger hardware faults?
> Should the domains be disabled at the end of the teardown instead?

No because resets and clocks are not used in the firmware-managed mode.
But for consistency, I will move qmp_usb_d0_to_d3() at the end of
qmp_usb_exit() in v2.

>
>> @@ -2212,6 +2212,8 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
>>  
>>  	qmp_usb_enable_autonomous_mode(qmp);
>>  
>> +	qmp_usb_d0_to_d1(qmp);
>> +
>>  	clk_disable_unprepare(qmp->pipe_clk);
>>  	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>
> [Severity: High]
> Similar to the exit path, this powers off the transfer domain before
> the clocks are disabled. Could this cause bus errors when the clocks are
> manipulated?

Will move after clk_bulk_disable() in v2.

>
>> @@ -2232,6 +2232,12 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>>  		return 0;
>>  	}
>>  
>> +	ret = qmp_usb_d1_to_d0(qmp);
>> +	if (ret < 0) {
>> +		dev_err(qmp->dev, "Failed to transition to d0 state\n");
>> +		return ret;
>> +	}
>> +
>>  	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>>  	if (ret)
>>  		return ret;
>
> [Severity: High]
> If clk_bulk_prepare_enable() fails, the function returns ret without
> undoing qmp_usb_d1_to_d0(). Will this leak the transfer domain's PM usage
> counter?

Indeed, will fix in v2.

>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260708-phy-qcom-qmp-usb-8255-v1-0-8d852a9535db@kernel.org?part=2