Re: [PATCH v13 20/35] phy: rockchip: usbdp: Factor out lane_mux_sel setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unclocked MMIO access in DP PHY power path causes synchronous external abort (system hang).
--

--- Patch [20]: [PATCH v13 20/35] phy: rockchip: usbdp: Factor out lane_mux_sel setup ---
commit 47e65509a216979a5784ba5eae3623e6393b5bcb
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Factor out lane_mux_sel setup

Avoid describing the usb and dp lane mux setup logic twice by introducing
a helper function to reduce code duplication.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 4b454798c4ed4..4e54020d17551 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -582,6 +582,42 @@ static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 mode)
>  	udphy->mode = mode;
>  }
>  
> +static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state)
> +{
> +	u8 mode;

[ ... ]

While reviewing this patch, I noticed a pre-existing issue in the same file:

>  static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  {
>  	int ret;
>  
>  	if (!(udphy->mode & mode)) {
>  		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but returning 0 here instead of an error code
can lead to unclocked mmio accesses later on.

If the display stack calls phy_power_on() for the dp phy while the type-c
port is disconnected or in a usb-only mode, rk_udphy_power_on() returns 0.

This false success propagates to rk_udphy_dp_phy_power_on():

rk_udphy_dp_phy_power_on() {
    ...
    ret = rk_udphy_power_on(udphy, UDPHY_MODE_DP);
    if (ret)
        goto unlock;

    rk_udphy_dp_lane_enable(udphy, udphy->dp_lanes);
    ...
}

Because ret is 0, does this code then call rk_udphy_dp_lane_enable() without
the clocks being enabled? Accessing unclocked device registers on arm
platforms can cause a fatal bus hang.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=20

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