Re: [PATCH] phy: ti: omap-usb2: fix OF node leak at probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing return value check for `omap_usb2_enable_clocks()` in `omap_usb_init()` leads to bus faults and clock framework warnings.
--

--- Patch [1]: [PATCH] phy: ti: omap-usb2: fix OF node leak at probe ---
commit afaeba96304035f84f232912a2828b3d807db811
Author: Guangshuo Li <lgs201920130244@gmail.com>

phy: ti: omap-usb2: fix OF node leak at probe

omap_usb2_probe() parses the ctrl-module phandle and then looks up the
corresponding platform device.

of_parse_phandle() returns a device node with a reference that must be
dropped by the caller. The driver currently keeps the device reference
taken by of_find_device_by_node() until unbind, but never drops the
temporary OF node reference taken by of_parse_phandle().

Drop the OF node reference after of_find_device_by_node() has consumed
the node.

> diff --git a/drivers/phy/ti/phy-omap-usb2.c b/drivers/phy/ti/phy-omap-usb2.c
> index 1eb252604441b..660df3181e4f3 100644
> --- a/drivers/phy/ti/phy-omap-usb2.c
> +++ b/drivers/phy/ti/phy-omap-usb2.c
> @@ -426,6 +426,7 @@ static int omap_usb2_probe(struct platform_device *pdev)
>  		}
>  
>  		control_pdev = of_find_device_by_node(control_node);
> +		of_node_put(control_node);
>  		if (!control_pdev) {
>  			dev_err(&pdev->dev, "Failed to get control device\n");
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing
issue with how clock enablement failures are handled:

drivers/phy/ti/phy-omap-usb2.c:omap_usb_init() {
	...
	omap_usb2_enable_clocks(phy);

	if (phy->flags & OMAP_USB2_CALIBRATE_FALSE_DISCONNECT) {
		...
		val = omap_usb_readl(phy->phy_base, USB2PHY_ANA_CONFIG1);
	...
}

If omap_usb2_enable_clocks() fails, it returns a negative error code and
rolls back any enabled clocks. Because omap_usb_init() ignores this error,
it assumes the clocks are enabled and proceeds to access memory-mapped
hardware registers.

Can accessing unclocked hardware here trigger a synchronous external abort
(bus fault) on OMAP platforms?

Furthermore, because omap_usb_init() falsely returns success, the PHY
framework will eventually call omap_usb_exit():

drivers/phy/ti/phy-omap-usb2.c:omap_usb_exit() {
	struct omap_usb *phy = phy_get_drvdata(x);

	return omap_usb2_disable_clocks(phy);
}

Will this attempt to disable clocks that were never successfully enabled,
triggering a warning in the clock framework about unbalanced disables?

Should we check the return value of omap_usb2_enable_clocks() and propagate
the error to avoid these issues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708081254.734892-1-lgs201920130244@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy