Re: [PATCH] usb: dwc3: meson-g12a: fix refcount leak in dwc3_meson_g12a_resume()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect error goto labels in dwc3_meson_g12a_probe() skip necessary cleanup steps, leaking partially initialized PHY resources.
- [High] Missing regulator_disable() in dwc3_meson_g12a_remove() leaves the vbus regulator enabled and leaks its reference.
- [High] IRQ handler can execute after remove() shuts down clocks, leading to unclocked register access.
--

commit 8a232207e4471191a727afd0c18f2ad5d61a38d9
Author: WenTao Liang <vulab@iscas.ac.cn>
usb: dwc3: meson-g12a: fix refcount leak in dwc3_meson_g12a_resume()

This commit adds an error path to dwc3_meson_g12a_resume() to call
reset_control_rearm() before returning. This fixes a leaked reset control
reference when initialization steps fail.

> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index 55e144ba8cfc6..4d611c08e8a4c 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -907,35 +907,39 @@ static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev)
>  
>  	ret = priv->drvdata->usb_init(priv);
>  	if (ret)
> -		return ret;
> +		goto err_rearm;
>  
>  	/* Init PHYs */
>  	for (i = 0 ; i < PHY_COUNT ; ++i) {
>  		ret = phy_init(priv->phys[i]);
>  		if (ret)
> -			return ret;
> +			goto err_rearm;
>  	}
>  
>  	/* Set PHY Power */
>  	for (i = 0 ; i < PHY_COUNT ; ++i) {
>  		ret = phy_power_on(priv->phys[i]);
>  		if (ret)
> -			return ret;
> +			goto err_rearm;
>  	}
>  
>  	if (priv->vbus && priv->otg_phy_mode == PHY_MODE_USB_HOST) {
>  		ret = regulator_enable(priv->vbus);
>  		if (ret)
> -			return ret;
> +			goto err_rearm;
>  	}
>  
>  	if (priv->drvdata->usb_post_init) {
>  		ret = priv->drvdata->usb_post_init(priv);
>  		if (ret)
> -			return ret;
> +			goto err_rearm;
>  	}
>  
>  	return 0;
> +
> +err_rearm:
> +	reset_control_rearm(priv->reset);
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing the error handling in this
file, does dwc3_meson_g12a_probe() skip necessary cleanup steps if
initialization fails?

If phy_init() fails within its loop, the code jumps directly to
err_disable_regulator:

drivers/usb/dwc3/dwc3-meson-g12a.c:dwc3_meson_g12a_probe() {
        /* Init PHYs */
        for (i = 0 ; i < PHY_COUNT ; ++i) {
                ret = phy_init(priv->phys[i]);
                if (ret)
                        goto err_disable_regulator;
        }
        ...
}

This skips the err_phys_exit label, which seems to fail to call phy_exit()
for previously initialized PHYs.

Similarly, if phy_power_on() fails, does jumping to err_phys_exit skip the
err_phys_power cleanup and leak powered-on PHY resources?

drivers/usb/dwc3/dwc3-meson-g12a.c:dwc3_meson_g12a_probe() {
        /* Set PHY Power */
        for (i = 0 ; i < PHY_COUNT ; ++i) {
                ret = phy_power_on(priv->phys[i]);
                if (ret)
                        goto err_phys_exit;
        }
        ...
}

[Severity: High]
This is a pre-existing issue, but does dwc3_meson_g12a_remove() leave the
vbus regulator enabled?

During dwc3_meson_g12a_probe(), the vbus regulator is unconditionally
enabled via regulator_enable() if present. However, dwc3_meson_g12a_remove()
completely lacks a corresponding regulator_disable(priv->vbus) call.

When the driver is unbound, devres releases the regulator reference but
does not automatically disable it. Since the enable count is never balanced,
will the power domain permanently stay active and trigger an unbalanced use
count warning?

[Severity: High]
This is a pre-existing issue, but can the IRQ handler execute after the
clocks are shut down during driver removal?

The dwc3_meson_g12a_remove() function manually disables the device clocks
while the devm-managed threaded IRQ might still be active:

drivers/usb/dwc3/dwc3-meson-g12a.c:dwc3_meson_g12a_remove() {
        ...
        clk_bulk_disable_unprepare(priv->drvdata->num_clks,
                                   priv->drvdata->clks);
        ...
}

Because devm-managed resources are freed only after the removal function
completely returns, the IRQ remains active. If
dwc3_meson_g12a_irq_thread() executes after the clocks are disabled, it
will call dwc3_meson_g12a_get_id(), which reads from the MMIO register
via regmap_read().

Will accessing the hardware without clocks cause a synchronous external
abort on ARM SoCs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611131121.81784-1-vulab@iscas.ac.cn?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic