Re: [PATCH v5 06/18] phy: rockchip: usbdp: Add missing mode_change update

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State loss on transient PHY initialization failure leaves hardware permanently in reset.
- [High] Data race in `rk_udphy_dp_phy_configure()` reading shared lane mappings without holding the driver mutex.
--

commit 22525e2688f8ca90810542bfa2ad9f261c949f0b
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Add missing mode_change update
    
rk_udphy_set_typec_default_mapping() updates the available modes,
but does not set the mode_change as required. This results in
missing re-initialization and thus non-working DisplayPort.
    
Fix this issue by introducing a new helper to update the available
modes.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 97e53b933225f..febc148a754e0 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -619,6 +619,15 @@ static void rk_udphy_dp_hpd_event_trigger(struct rk_udphy *udphy, bool hpd)
>  	rk_udphy_grfreg_write(udphy->vogrf, &cfg->vogrfcfg[udphy->id].hpd_trigger, hpd);
>  }
>  
> +static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 mode)
> +{
> +	if (udphy->mode == mode)
> +		return;
> +
> +	udphy->mode_change = true;

[Severity: High]
This isn't a bug introduced by this patch, but it seems there is a
pre-existing issue with how this flag is handled in rk_udphy_power_on().

When the PHY is partially active and a new mode is being powered on, it
checks the re-initialization flag (also known as phy_needs_reinit in the
broader context). It unconditionally clears the flag to false before calling
rk_udphy_init():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
    ...
    } else if (udphy->phy_needs_reinit) {
        udphy->phy_needs_reinit = false;
        ...
        ret = rk_udphy_init(udphy);
        if (ret)
            return ret;
    ...
}

If rk_udphy_init() fails and returns an error (like -EPROBE_DEFER from a PLL
lock timeout), the flag is left as false. Would the next retry by the
consumer skip initialization and falsely return success, leaving the PHY
hardware permanently held in reset?

> +	udphy->mode = mode;
> +}
> +
>  static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)
>  {
>  	if (udphy->flip) {

[Severity: High]
This is a pre-existing issue, but I have a question about concurrent access
to the lane mappings.

This function mutates the lane selection arrays concurrently with
rk_udphy_dp_phy_configure() reading them:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
    for (i = 0; i < udphy->lanes; i++) {
        lane = udphy->dp_lane_sel[i];
        ...
        regmap_update_bits(..., LN_ANA_TX_SER_TXCLK_INV,
            FIELD_PREP(LN_ANA_TX_SER_TXCLK_INV, udphy->lane_mux_sel[lane]));
        ...
        rk_udphy_dp_set_voltage(udphy, udphy->bw, dp->voltage[i], dp->pre[i], lane);
    }
    ...
}

Since rk_udphy_dp_phy_configure() reads these arrays without holding
udphy->mutex, could a race condition result in reading partially updated
lane assignments if a Type-C switch event occurs at the same time?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-rockchip-usbdp-cleanup-v5-0-efc83069869f@collabora.com?part=6

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