Re: [PATCH v14 10/38] phy: rockchip: usbdp: Add missing mode_change update

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

Pre-existing issues:
- [High] Type-C state fallback to USB/Safe modes skips the PHY mode update and HPD reset, permanently breaking USB 3.0 and leaving a ghost DisplayPort connection.
- [High] The PHY driver indefinitely leaks active power states (clocks and resets) if the Type-C mux mode changes before the USB or DP controllers execute their teardown routines.
--

--- Patch [10]: [PATCH v14 10/38] phy: rockchip: usbdp: Add missing mode_change update ---
commit 9bed4fa08dd116716a39f228bdd961c66a8f8b64
Author: Sebastian Reichel <[email protected]>

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.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 982315535caf7..7540c897dde13 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -617,6 +617,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;
> +	udphy->mode = mode;
> +}

[Severity: High]
This isn't a bug introduced by this patch, but since we are updating how the
mode is set, can this asynchronous update to udphy->mode lead to a power state
leak?

If the Type-C subsystem asynchronously updates the requested mode via
rk_udphy_typec_mux_set() (e.g., changing from UDPHY_MODE_DP_USB to
UDPHY_MODE_DP), does this cause later teardown routines to fail?

For example, when the USB controller suspends and calls phy_exit() or
phy_power_off(), routines like rk_udphy_usb3_phy_exit() check the mode:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_usb3_phy_exit() {
    ...
	/* DP only or high-speed */
	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		goto unlock;
    ...
}

Similarly in rk_udphy_power_off():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_off() {
    ...
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return;
	}
    ...
}

Because udphy->mode has already changed, do these functions return early
without clearing the USB bit from udphy->status, leaving active hardware
clocks and resets leaked indefinitely?

[ ... ]

> @@ -1361,10 +1370,7 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  			usleep_range(750, 800);
>  			rk_udphy_dp_hpd_event_trigger(udphy, true);
>  		} else if (data->status & DP_STATUS_HPD_STATE) {
> -			if (udphy->mode != mode) {
> -				udphy->mode = mode;
> -				udphy->mode_change = true;
> -			}
> +			rk_udphy_mode_set(udphy, mode);
>  			rk_udphy_dp_hpd_event_trigger(udphy, true);
>  		} else {
>  			rk_udphy_dp_hpd_event_trigger(udphy, false);

[Severity: High]
This is a pre-existing issue, but I have a question about the broader logic
in rk_udphy_typec_mux_set() when the mode changes.

When the Type-C port exits Alt Mode to a safe or USB-only state, does this
skip the PHY mode update and HPD reset entirely?

It looks like the vital state updates (including the new rk_udphy_mode_set()
call) are gated behind an earlier check in rk_udphy_typec_mux_set():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
    ...
	if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
		struct typec_displayport_data *data = state->data;
    ...
}

Because state->alt is NULL when exiting DisplayPort Alt Mode, this block is
bypassed. Can this leave udphy->mode stuck in UDPHY_MODE_DP, potentially
breaking USB 3.0 functionality and leaving a ghost DisplayPort connection?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy