Re: [PATCH 2/2] usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Masking of legitimate errors (like -EPROBE_DEFER or -ENOMEM) from devm_clk_get_optional() by using IS_ERR_OR_NULL().
- [High] Passing ref_clk to the DWC3 core causes a reference count leak that leaves the clock physically enabled during runtime suspend.
- [Low] Spammy dev_info() log on every boot for platforms without the optional clock.
--

--- Patch [2]: [PATCH 2/2] usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core ---
commit 0c1a156683fd6c3e4e8f63adc608d4af8b20c95e
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

The dwc3-qcom glue layer sets probe_data.ignore_clocks_and_resets before
calling dwc3_core_probe(), preventing the DWC3 core from acquiring ref_clk.
This patch allows the Qualcomm glue driver to provide ref_clk to the core for
platforms that need it.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..2d6de6469c1ea 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -705,6 +705,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	probe_data.res = &res;
>  	probe_data.ignore_clocks_and_resets = true;
>  	probe_data.properties = DWC3_DEFAULT_PROPERTIES;
> +
> +	qcom->dwc.ref_clk = devm_clk_get_optional(dev, "ref_clk");

[Severity: High]
Does this code leak a clock reference count during runtime suspend?

The glue driver gets and enables all clocks via devm_clk_bulk_get_all(), which
includes ref_clk. By passing ref_clk to the DWC3 core here, dwc3_clk_enable()
will increment the clock's enable count a second time.

During HOST runtime suspend, dwc3_suspend_common() intentionally skips
disabling clocks:

drivers/usb/dwc3/core.c:dwc3_suspend_common() {
...
		if (!PMSG_IS_AUTO(msg) &&
		    (!device_may_wakeup(dwc->dev) || dwc->needs_full_reinit)) {
			dwc3_core_exit(dwc);
			break;
		}
...
}

Because of this, when the glue driver suspends and calls
clk_bulk_disable_unprepare() in dwc3_qcom_suspend(), the enable count for
ref_clk drops from 2 to 1 instead of reaching 0. Will this leave the clock
physically powered on and cause a permanent power management regression?

> +	if (IS_ERR_OR_NULL(qcom->dwc.ref_clk)) {

[Severity: High]
Does masking the error return from devm_clk_get_optional() with IS_ERR_OR_NULL
discard legitimate errors?

If the clock provider is not yet ready, devm_clk_get_optional() will return
-EPROBE_DEFER. Masking this error means the driver silently proceeds without the
required clock, leading to incorrect hardware initialization instead of deferring
the probe.

> +		qcom->dwc.ref_clk = NULL;
> +		dev_info(dev, "NOT setting dwc.ref_clk\n");

[Severity: Low]
Will this dev_info() cause unnecessary log spam on every boot?

When the optional clock is naturally absent from the device tree,
devm_clk_get_optional() returns NULL. This logs unconditionally on platforms
without the optional clock. Could this be removed or changed to dev_dbg to avoid
dmesg spam?

> +	}
> +
>  	ret = dwc3_core_probe(&probe_data);
>  	if (ret)  {
>  		ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-ctrl-v1-0-609e2291814d@oss.qualcomm.com?part=2