Re: [PATCH v12 20/36] phy: rockchip: usbdp: Factor out lane_mux_sel setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Type-C states TYPEC_STATE_SAFE and TYPEC_STATE_USB incorrectly fall through to TYPEC_DP_STATE_D configuration, leaving the PHY in UDPHY_MODE_DP_USB mode.
- [High] The `rk_udphy_dp_phy_configure()` callback accesses and modifies shared PHY state without holding the `udphy->mutex`, leading to a race condition with concurrent Type-C events.
--

--- Patch [20]: [PATCH v12 20/36] phy: rockchip: usbdp: Factor out lane_mux_sel setup ---
commit 3727c9684937e2c6c25ea6548ada704fecefa99a
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Factor out lane_mux_sel setup
    
Avoid describing the USB+DP lane_mux_sel 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;
> +
> +	switch (state) {
> +	case TYPEC_DP_STATE_C:
> +	case TYPEC_DP_STATE_E:
> +		udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
> +		udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
> +		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
> +		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
> +		mode = UDPHY_MODE_DP;
> +		udphy->dp_lanes = 4;
> +		break;
> +
> +	case TYPEC_DP_STATE_D:
> +	default:

[Severity: High]
This is a pre-existing issue, but does this default case correctly handle
TYPEC_STATE_SAFE and TYPEC_STATE_USB?

When the Type-C subsystem exits DP AltMode or transitions to a safe state,
the mux callback rk_udphy_typec_mux_set() receives state->mode set to
TYPEC_STATE_SAFE (0).

Because rk_udphy_set_typec_state() does not explicitly handle 0, it falls
into the default case here, incorrectly setting udphy->dp_lanes = 2 and
mode = UDPHY_MODE_DP_USB.

Should TYPEC_STATE_SAFE and TYPEC_STATE_USB explicitly disable the DP lanes
instead of defaulting to DP_USB mode?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing lock in
rk_udphy_dp_phy_configure() that could race with Type-C events?

Looking at rk_udphy_dp_phy_configure() in
drivers/phy/rockchip/phy-rockchip-usbdp.c:

    static int rk_udphy_dp_phy_configure(struct phy *phy,
                                         union phy_configure_opts *opts)
    {
        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) {

It appears to access and modify shared PHY state without holding udphy->mutex.

Concurrently, a Type-C event such as rk_udphy_typec_mux_set() running in a
workqueue modifies udphy->dp_lane_sel and udphy->dp_lanes while holding
udphy->mutex.

Could rk_udphy_dp_phy_configure() read inconsistent values and cause
out-of-bounds register writes if it gets an invalid index from dp_lane_sel?

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