Re: [PATCH v2 6/7] usb: dwc3: qcom: Support broken suspend for SA8255P

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Qscratch programming is not restored during system resume if device wakeup is enabled, leaving the controller broken.
- [High] Flawed boolean logic enables device-mode VBUS override during host mode operations.
--

commit 62abc776630f8d66e2fc8e0f35ca6edc55d04a4d
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>

usb: dwc3: qcom: Support broken suspend for SA8255P

This commit addresses Qscratch programming loss on SA8255P during APPS Suspend
by restoring UTMI clock selection and VBUS valid signals after system resume.
It also ensures the controller is properly reset from the POR state.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6d25f81800a7f..e58a9ca23b009 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c

[ ... ]

> @@ -388,6 +416,28 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
>  	if (!qcom->is_suspended)
>  		return 0;
>  
> +	if (qcom->broken_suspend && !PMSG_IS_AUTO(msg)) {
> +		/*
> +		 * Only system suspend fully powers off the controller and
> +		 * puts it into POR state. Runtime suspend does not, so skip
> +		 * the reset on runtime resume to avoid needlessly clobbering
> +		 * state that was never lost.
> +		 */
> +		ret = reset_control_assert(qcom->resets);

[Severity: High]
Does this unconditionally reset the controller during system resume,
destroying Qscratch state regardless of whether device wakeup is enabled?

[ ... ]

> @@ -415,6 +465,14 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
>  				  PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
>  	}
>  
> +	if (qcom->broken_suspend) {
> +		if (!wakeup && qcom->ignore_pipe_clk)
> +			dwc3_qcom_select_utmi_clk(qcom);

[Severity: High]
Could this logic improperly skip required Qscratch reprogramming after the
controller reset if wakeup is enabled? Since the reset wiped the state above,
the clock configuration shouldn't be skipped when wakeup is true, as it
breaks USB operations on resume.

> +		/* Make sure vbus valid is set for PHYs after PM resume */
> +		if (!(dwc3_qcom_is_host(qcom) && wakeup))
> +			dwc3_qcom_vbus_override_enable(qcom, true);

[Severity: High]
Will this boolean logic accidentally enable device-mode VBUS override during
host mode operations? By De Morgan's laws, if dwc3_qcom_is_host() is true
and wakeup is false, this evaluates to true. Putting the PHY into device
mode VBUS override disrupts normal host mode operations.

> +	}
> +
>  	qcom->is_suspended = false;
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=6