Re: [PATCH v4 5/5] 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 1 potential
issue to consider:
- [Medium] phy: rockchip: inno-usb2: late clk480m setup in
  rockchip_usb2phy_probe()
--

commit 6ec3b4b8f3c0ab3cafbd20b325e5d225b7df828c
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 and resume handling on single-port devices
into the 480MHz clock prepare and unprepare callbacks, and moves 480MHz
clock registration in rockchip_usb2phy_probe() after port initialization.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 642c996d23dec..9fbd9e25f2c19 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
[ ... ]
> @@ -1365,10 +1382,6 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>  	/* Clocks are optional */
>  	rphy->num_clks = ret < 0 ? 0 : ret;
>  
> -	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 phy clock\n");
> @@ -1421,6 +1434,10 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = rockchip_usb2phy_clk480m_register(rphy);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register 480m output clock\n");
> +
>  	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);

[Severity: Medium]
Does moving rockchip_usb2phy_clk480m_register() after the port
initialization loop allow rport->otg_sm_work and per-port IRQs to use
rphy->clk480m before it is registered and after it is unregistered?

Inside the loop, rockchip_usb2phy_otg_port_init() and
rockchip_usb2phy_host_port_init() request per-port threaded IRQs, and
rockchip_usb2phy_otg_port_init() also registers the extcon notifier and
synchronously triggers rockchip_otg_event() to schedule rport->otg_sm_work:

drivers/phy/rockchip/phy-rockchip-inno-usb2.c:rockchip_usb2phy_otg_port_init() {
	...
	ret = rockchip_usb2phy_port_irq_init(rphy, rport, child_np);
	...
	if (!IS_ERR(rphy->edev)) {
		rport->event_nb.notifier_call = rockchip_otg_event;

		ret = devm_extcon_register_notifier(rphy->dev, rphy->edev,
					EXTCON_USB_HOST, &rport->event_nb);
		...
		if (!of_property_present(rphy->dev->of_node, "extcon")) {
			/* do initial sync of usb state */
			id = property_enabled(rphy->grf, &rport->port_cfg->utmi_id);
			extcon_set_state_sync(rphy->edev, EXTCON_USB_HOST, !id);
		}
	}
}

Because rockchip_usb2phy_clk480m_register() now queues its devm action
after the devm-managed port IRQs and extcon notifier, devres teardown runs
rockchip_usb2phy_clk480m_unregister() first and frees rphy->clk480m while
port IRQ handlers and work items can still run:

CPU0                                  CPU1 (kworker / IRQ thread)
device_unbind_cleanup()
  devres_release_all()
    rockchip_usb2phy_clk480m_unregister()
      clk_unregister(rphy->clk480m)
                                      rockchip_usb2phy_otg_sm_work()
                                        rockchip_usb2phy_power_on()
                                          clk_prepare_enable(rphy->clk480m)
                                            <-- use-after-free
    devm_irq_release()

If rockchip_usb2phy_clk480m_register() fails after
rockchip_usb2phy_otg_port_init() has queued rport->otg_sm_work,
rphy->clk480m is left holding an ERR_PTR that crashes clk_prepare(), and
rport->otg_sm_work remains queued after devres frees rphy.

Also, if rport->otg_sm_work or a port IRQ reaches
rockchip_usb2phy_power_on() before rockchip_usb2phy_clk480m_register()
assigns rphy->clk480m, clk_prepare_enable(NULL) succeeds as a no-op while
rockchip_usb2phy_set_suspend() is skipped, leaving the PHY suspended with
rport->suspended set to false and causing a later
rockchip_usb2phy_power_off() to unprepare an unprepared clock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-phy-rockchip-inno-usb2-clock-fix-v4-0-51acc071634f@collabora.com?part=5

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