Re: [PATCH 3/5] usb: typec: mux: ps883x: refactor DP altmode handling and support TYPEC_DP_STATE_F

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dereferencing NULL pointer during Thunderbolt Alt Mode teardown.
- [High] Failure to tear down DisplayPort Alt Mode correctly during SAFE state transition.
--

--- Patch [3]: [PATCH 3/5] usb: typec: mux: ps883x: refactor DP altmode handling and support TYPEC_DP_STATE_F ---
commit 1182bfe184aec8b5b6e514d7c1a3d153c73073da
Author: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
usb: typec: mux: ps883x: refactor DP altmode handling and support TYPEC_DP_STATE_F

This commit refactors DisplayPort altmode handling into a helper function.
It also adds support for the legacy TYPEC_DP_STATE_F mode to ensure DP
altmode works correctly on certain docks during Type-C fallback mode.

> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index 3fa26ce01a9c9..3533d4f363286 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
[ ... ]
> @@ -188,6 +188,25 @@ static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
>  	return 0;
>  }
>  
> +static void ps883x_apply_dp_altmode(int *cfg0, int *cfg1, int dp_state)
> +{
> +	*cfg1 |= CONN_STATUS_1_DP_CONNECTED | CONN_STATUS_1_DP_HPD_LEVEL;
> +
> +	switch (dp_state) {

[Severity: High]
This is a pre-existing issue, but does this correctly handle tearing down
the DisplayPort Alt Mode?

When the system requests TYPEC_STATE_SAFE (mode 0) to tear down the alt
mode, dp_state will be 0. It seems the CONN_STATUS_1_DP_CONNECTED and
CONN_STATUS_1_DP_HPD_LEVEL bits are unconditionally applied here, and the
switch statement hits the default empty case. Because state->alt is still
present, the retimer remains inappropriately configured for DisplayPort
instead of resetting.

> +	case TYPEC_DP_STATE_D:
> +	case TYPEC_DP_STATE_F:
> +		*cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
> +		fallthrough;
> +	case TYPEC_DP_STATE_C:
> +	case TYPEC_DP_STATE_E:
> +		*cfg1 |= CONN_STATUS_1_DP_SINK_REQUESTED |
> +			 CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state *state)
>  {
>  	struct typec_thunderbolt_data *tb_data;
[ ... ]
>  		case USB_TYPEC_TBT_SID:
>  			tb_data = state->data;
> -
>  			/* Unconditional */
>  			cfg2 |= CONN_STATUS_2_TBT_CONNECTED;
>  

[Severity: High]
This isn't a bug introduced by this patch, but will this cause a NULL
pointer dereference when a Thunderbolt device is unplugged?

During an active Thunderbolt device unplug, the Type-C bus core calls
typec_altmode_remove() which sets the state to TYPEC_STATE_SAFE and passes
NULL for state->data. Here in ps883x_set(), tb_data is assigned to
state->data and then unconditionally dereferenced just below this block for
tb_data->cable_mode, which seems like it would cause a kernel panic during
teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718-ps883x-disable-usb4-v1-0-cec86d0b909e@oldschoolsolutions.biz?part=3