Re: [PATCH v2 4/4] phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: phy: rockchip: NULL ptr deref in clk480m_prepare due to early clock registration
- [Medium] usb: phy: rockchip: missing hardware rollback on clk prepare error
--

commit eaaff48582a4fba9285d43555bb9babaac779141
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: inno-usb2: move suspend handling into clock prepare/unprepare for RK3588/RK3576

This commit modifies the RK3588/RK3576 USB2 PHY driver to fully power the PHY
during clock prepare operations. It addresses an issue during OHCI system
resume where the PHY must be running to access controller registers, moving the
suspend handling directly into the clock prepare/unprepare hooks.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 925a03fee6bc5..cc923d12ef7b4 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c

[ ... ]

> @@ -379,16 +381,27 @@ static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
>  		usleep_range(1200, 1300);
>  	}
>  
> +	if (rphy->phy_cfg->num_ports == 1) {
> +		ret = rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], false);

[Severity: High]
Could this lead to a null pointer dereference during probe?

In rockchip_usb2phy_probe(), the clock is registered via
rockchip_usb2phy_clk480m_register() before iterating over child nodes to
initialize the ports array and configurations.

If a concurrent or deferred probe from a clock consumer (such as the OHCI
controller) requests and prepares the clock immediately, this is called before
port initialization finishes.

At this point, rport->port_cfg will be NULL, which leads to a null pointer
dereference when property_enable() attempts to access
&rport->port_cfg->phy_sus.

> +		if (ret)
> +			return ret;

[Severity: Medium]
If rockchip_usb2phy_set_suspend() fails here, does this leave the clock
output permanently enabled?

Earlier in rockchip_usb2phy_clk480m_prepare(), the clock output is turned on
using property_enable().

Since the Common Clock Framework aborts preparation on error, .unprepare
will never be called to clean it up.

Returning immediately without rolling back the earlier hardware state change
appears to leave the clock output enabled.

> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-phy-rockchip-inno-usb2-clock-fix-v2-0-e01bf509359e@collabora.com?part=4

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