Re: [PATCH v2 1/2] usb: chipidea: ci_hdrc_imx: Propagate errors from optional IRQ lookup

On Fri, Aug 07, 2026 at 07:03:52AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] The newly added error path for `platform_get_irq_optional()` bypasses `usb_phy_shutdown()`, potentially leaking the PHY initialization.
>
> Pre-existing issues:
> - [High] Pre-existing error handlers for IRQ allocation skip `usb_phy_shutdown()`, potentially leaking the initialized PHY.
> --
>
> --- Patch [1]: [PATCH v2 1/2] usb: chipidea: ci_hdrc_imx: Propagate errors from optional IRQ lookup ---
> commit 6c82c9cecd7d9e9567b9f2902520e1c741b42e44
> Author: bui duc phuc <[email protected]>
>
> usb: chipidea: ci_hdrc_imx: Propagate errors from optional IRQ lookup
>
> platform_get_irq_optional() returns a positive IRQ number on success or
> a negative error code on failure. For an optional IRQ, -ENXIO indicates
> that no IRQ is available, while other errors should be propagated.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 56d2ba824a0b2..7387c9903e800 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -525,6 +525,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> >  		data->supports_runtime_pm = true;
> >
> >  	data->wakeup_irq = platform_get_irq_optional(pdev, 1);
> > +	if (data->wakeup_irq < 0 && data->wakeup_irq != -ENXIO) {
> > +		ret = data->wakeup_irq;
> > +		goto err_clk;
> > +	}
>
> [Severity: High]
> Does this jump skip the necessary cleanup for the USB PHY?
>
> If usb_phy_init() succeeded earlier in ci_hdrc_imx_probe() and
> platform_get_irq_optional() returns an error like -EPROBE_DEFER,
> jumping directly to err_clk bypasses the phy_shutdown label:
>
> ci_hdrc_imx_probe()
> ...
> phy_shutdown:
> 	if (data->override_phy_control)
> 		usb_phy_shutdown(data->phy);
> err_clk:
>
> Can this leak the initialized PHY state, regulators, or clocks, especially
> since -EPROBE_DEFER can be hit multiple times? Should this be goto
> phy_shutdown instead?


Please check this, make sense.

Frank

>
> >  	if (data->wakeup_irq > 0) {
> >  		irq_name = devm_kasprintf(dev, GFP_KERNEL, "%s:wakeup", pdata.name);
> >  		if (!irq_name) {
>
> [Severity: High]
> This is a pre-existing issue, but do the error handlers for IRQ allocation
> also incorrectly skip the PHY shutdown?
>
> If devm_kasprintf() fails or if devm_request_threaded_irq() fails below:
>
> ci_hdrc_imx_probe()
> ...
> 		ret = devm_request_threaded_irq(dev, data->wakeup_irq,
> ...
> 		if (ret)
> 			goto err_clk;
>
> The error paths jump to err_clk, missing the phy_shutdown cleanup.
> Could this lead to similar resource leaks if the USB PHY was successfully
> initialized earlier?
>
> > 			ret = dev_err_probe(dev, -ENOMEM, "failed to create irq_name\n");
> > 			goto err_clk;
> > 		}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1