Re: [PATCH v3 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] phy: rockchip: inno-usb2: NULL deref in rockchip_usb2phy_clk480m_unprepare()
- [High] phy: rockchip: inno-usb2: -EAGAIN in rockchip_usb2phy_clk480m_prepare()
--

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

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

This commit moves PHY suspend handling on single-port PHYs (RK3588 and
RK3576) from the PHY power_on and power_off callbacks into the 480MHz clock
prepare and unprepare callbacks so the PHY PLL is running whenever the clock
is prepared.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 2919c2e11968b..b2633b83f7ab3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -363,10 +363,15 @@ static int rockchip_usb2phy_set_suspend(struct rockchip_usb2phy *rphy,
>  
>  static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
>  {
> +	struct rockchip_usb2phy *rphy =
> +		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>  	const struct usb2phy_reg *clkout_ctl;
>  	struct regmap *base;
>  	int ret;
>  
> +	if (rphy->phy_cfg->num_ports == 1 && !rphy->ports[0].port_cfg)
> +		return -EAGAIN;

[Severity: High]
As reported by Sashiko on v2 [1], in rockchip_usb2phy_probe(), the 480MHz
clock provider is registered via rockchip_usb2phy_clk480m_register() and
of_clk_add_provider() before input clocks are enabled, before phy_tuning()
runs, and before the child port loop initializes rphy->ports[0].port_cfg:

drivers/phy/rockchip/phy-rockchip-inno-usb2.c:rockchip_usb2phy_probe() {
	...
	ret = rockchip_usb2phy_clk480m_register(rphy);
	if (ret)
		return dev_err_probe(dev, ret, "failed to register 480m output clock\n");

	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
	if (ret)
		return dev_err_probe(dev, ret, "failed to enable usb2phy clks\n");
	...
	for_each_available_child_of_node(np, child_np) {
		struct rockchip_usb2phy_port *rport = &rphy->ports[index];
		...
}

Clock consumers on RK3588 and RK3576 include ohci-platform and ehci-platform
(both of which set PROBE_PREFER_ASYNCHRONOUS and call pdata->power_on() ->
clk_prepare_enable() in their probe functions before usb_add_hcd() requests
the PHY) as well as phy-rockchip-usbdp (which prepares and enables the utmi
clock in rk_udphy_setup()).

If an asynchronous consumer calls clk_prepare_enable() after
of_clk_add_provider() publishes the clock but before rphy->ports[0].port_cfg
is initialized (or on boards where the parent usb2phy node is enabled to
supply the utmi clock to usbdp_phy while the child port subnode is disabled
in DT), rockchip_usb2phy_clk480m_prepare() will return -EAGAIN.

Neither clk_prepare_enable() nor the callers in ohci_platform_probe(),
ehci_platform_probe(), or rk_udphy_setup() retry on -EAGAIN, and the driver
core only defers probing on -EPROBE_DEFER. Can returning -EAGAIN here cause
consumer probing or PHY setup to fail permanently instead of waiting for port
initialization?

> +
>  	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
>  
>  	/* turn on 480m clk output if it is off */
> @@ -379,16 +384,31 @@ 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);
> +		if (ret)
> +			goto disable_clkout;
> +	}
> +
>  	return 0;
> +
> +disable_clkout:
> +	property_enable(base, clkout_ctl, false);
> +	return ret;
>  }
>  
>  static void rockchip_usb2phy_clk480m_unprepare(struct clk_hw *hw)
>  {
> +	struct rockchip_usb2phy *rphy =
> +		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>  	const struct usb2phy_reg *clkout_ctl;
>  	struct regmap *base;
>  
>  	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
>  
> +	if (rphy->phy_cfg->num_ports == 1)
> +		rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], true);

[Severity: High]
What happens in rockchip_usb2phy_clk480m_unprepare() if
rphy->ports[0].port_cfg is NULL?

In rk3588-base.dtsi, rk3588-extra.dtsi, and rk3576.dtsi, the parent usb2phy
nodes and their child otg-port or host-port nodes have independent status
properties. If a parent usb2phy node is enabled while its child port node is
disabled in DT, rockchip_usb2phy_probe() registers clk480m with
rphy->ports[0].port_cfg left as NULL.

Because rockchip_usb2phy_clkout_ops defines .is_prepared =
rockchip_usb2phy_clk480m_prepared(), which checks property_enabled(base,
clkout_ctl), and clkout_ctl defaults to enabled in hardware at reset on
RK3588 and RK3576, clk_unprepare_unused_subtree() will see the clock as
prepared and call rockchip_usb2phy_clk480m_unprepare() during
clk_disable_unused() even if rockchip_usb2phy_clk480m_prepare() was never
called or returned -EAGAIN.

Unlike rockchip_usb2phy_clk480m_prepare(), rockchip_usb2phy_clk480m_unprepare()
does not check rphy->ports[0].port_cfg before calling
rockchip_usb2phy_set_suspend(rphy, &rphy->ports[0], true), which then
dereferences &rport->port_cfg->phy_sus:

rockchip_usb2phy_clk480m_unprepare()
  -> rockchip_usb2phy_set_suspend()
       -> property_enable(rphy->grf, &rport->port_cfg->phy_sus, do_suspend)

Can this cause a NULL pointer dereference when unpreparing unused clocks at
boot?

[1] https://lore.kernel.org/r/20261001-phy-rockchip-inno-usb2-clock-fix-v2-4-e01bf509359e@collabora.com

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

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