Re: [PATCH v12 23/36] phy: rockchip: usbdp: Clear USB status on PHY exit

[sashiko-bot@kernel.org]

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

New issues:
- [High] Clearing the USB status flag without checking if the PHY should be completely disabled leads to a clock reference and PHY power leak.

Pre-existing issues:
- [High] The `rk_udphy_dp_phy_configure` function accesses and modifies shared PHY driver state without holding the proper subsystem lock (`udphy->mutex`).
--

--- Patch [23]: [PATCH v12 23/36] phy: rockchip: usbdp: Clear USB status on PHY exit ---
commit 87780d47b4da4078adb1684ca9ebb6fc0529d86c
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Clear USB status on PHY exit
    
Ensure the USB status flag is cleared when the USB3 PHY is
exited while the system is in DP-only mode.

Closes: https://sashiko.dev/#/message/20260625-rockchip-usbdp-cleanup-v7-24-38eb3cf654fd%40collabora.com

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 8c165bcab7968..a742bde7155be 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1273,8 +1273,10 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  	guard(mutex)(&udphy->mutex);
>  
>  	/* DP only or high-speed */
> -	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
> +	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
> +		udphy->status &= ~UDPHY_MODE_USB;
>  		return 0;
> +	}

[Severity: High]
Does this early return bypass rk_udphy_disable() if the PHY should be completely
disabled?

If the DP side is not active, udphy->status drops to UDPHY_MODE_NONE (0) here.
Because the function returns early, it bypasses rk_udphy_power_off() and fails
to call rk_udphy_disable(udphy).

Will this leave the PHY hardware permanently powered on and the clock counts
unbalanced?

The next time the PHY is powered on, status == 0 would cause another call to
rk_udphy_setup(), which calls clk_bulk_prepare_enable() again, causing a
cumulative clock reference leak.

>  
>  	rk_udphy_power_off(udphy, UDPHY_MODE_USB);

[ ... ]

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
    struct rk_udphy *udphy = phy_get_drvdata(phy);
    struct phy_configure_opts_dp *dp = &opts->dp;
    u32 i, val, lane;
    int ret;

    if (dp->set_rate) {
    ...
}

[Severity: High]
This is a pre-existing issue, but does rk_udphy_dp_phy_configure() access and
modify shared PHY driver state without holding the proper subsystem lock?

The PHY core framework calls .configure which reads udphy->dp_lanes and
udphy->dp_lane_sel, and writes udphy->lanes without acquiring udphy->mutex.

Concurrently, a Type-C event handled by rk_udphy_typec_mux_set() or
rk_udphy_orien_sw_set() might alter those shared fields while holding the mutex.

Could this race condition lead to the hardware being programmed with
inconsistent lane mappings during normal Type-C hotplug and DP modesetting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=23

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