Re: [PATCH] soc: qcom: pmic_glink_altmode: Fix SVID=DP && unconnected edge case

[Neil Armstrong <neil.armstrong@linaro.org>]

On 3/6/26 12:20, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The commit referenced in Fixes started evaluating the value of
> alt_port->mux_ctrl before checking the active SVID. This led to
> drm_aux_hpd_bridge_notify() no longer being called for the 'DP unplug'
> case.
> 
> Perhaps somewhat interestingly, the firmware sends a notification with
> SVID=DP, mux_ctrl=MUX_CTRL_STATE_NO_CONN and pin_assignment=0 on
> unplug. 'pin_assignment' was previously interpreted as a bitfield
> excerpt from the second byte of the DP pg_altmode payload (and stored
> as an u8).
> 
> That value is used in pmic_glink_altmode_sc8280xp_notify(), decremented
> by 1 (DPAM_HPD_A). Previously, this would result in an u8 underflow
> that would rollover to 0xff (which prior to the Fixes patch would have
> caused a pmic_glink_altmode_safe() and 'disconnected' bridge
> notification). That check was removed, without a replacement.
> 
> Resolve this issue by making sure the SID=DP && mux_ctrl=NO_CONN combo
> once again results in a HPD bridge notification.
> 
> Fixes: 0539c5a6fdef ("soc: qcom: pmic_glink_altmode: Consume TBT3/USB4 mode notifications")
> Reported-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> Tested-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>   drivers/soc/qcom/pmic_glink_altmode.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index d0afdcb96ee1..b496b88842a2 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -350,15 +350,17 @@ static void pmic_glink_altmode_worker(struct work_struct *work)
>   
>   	typec_switch_set(alt_port->typec_switch, alt_port->orientation);
>   
> -	if (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN) {
> -		pmic_glink_altmode_safe(altmode, alt_port);
> -	} else if (alt_port->svid == USB_TYPEC_TBT_SID) {
> +	if (alt_port->svid == USB_TYPEC_TBT_SID) {
>   		pmic_glink_altmode_enable_tbt(altmode, alt_port);
>   	} else if (alt_port->svid == USB_TYPEC_DP_SID) {
> -		pmic_glink_altmode_enable_dp(altmode, alt_port,
> -					     alt_port->mode,
> -					     alt_port->hpd_state,
> -					     alt_port->hpd_irq);
> +		if (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN) {
> +			pmic_glink_altmode_safe(altmode, alt_port);
> +		} else {
> +			pmic_glink_altmode_enable_dp(altmode, alt_port,
> +						     alt_port->mode,
> +						     alt_port->hpd_state,
> +						     alt_port->hpd_irq);
> +		}
>   
>   		if (alt_port->hpd_state)
>   			conn_status = connector_status_connected;
> @@ -368,6 +370,8 @@ static void pmic_glink_altmode_worker(struct work_struct *work)
>   		drm_aux_hpd_bridge_notify(&alt_port->bridge->dev, conn_status);
>   	} else if (alt_port->mux_ctrl == MUX_CTRL_STATE_TUNNELING) {
>   		pmic_glink_altmode_enable_usb4(altmode, alt_port);
> +	} else if (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN) {
> +		pmic_glink_altmode_safe(altmode, alt_port);
>   	} else {
>   		pmic_glink_altmode_enable_usb(altmode, alt_port);
>   	}

This looks fishy, because you had:

if (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN)
	pmic_glink_altmode_safe()
else (blah)
	...
else (alt_port->svid == USB_TYPEC_DP_SID)
	pmic_glink_altmode_enable_dp
else (blah)
	...
else (blah)
	...

So what's the difference with :


if (blah)
	...
else (alt_port->svid == USB_TYPEC_DP_SID) {
	if (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN)
		pmic_glink_altmode_safe()
	else
		pmic_glink_altmode_enable_dp
}
else (blah)
	...
else (blah)
	...
else (alt_port->mux_ctrl == MUX_CTRL_STATE_NO_CONN)
	pmic_glink_altmode_safe()


Before, if mux_ctrl was MUX_CTRL_STATE_NO_CONN, it would set to safe mode whatever the svid,
now it's either in the DP case or at the end.

I don't see the difference here, except if your desire is to ignore
the MUX_CTRL_STATE_NO_CONN for USB_TYPEC_TBT_SID and MUX_CTRL_STATE_TUNNELING.

But it doesn't match the commit message at all.

Neil

> 
> ---
> base-commit: fc7b1a72c6cd5cbbd989c6c32a6486e3e4e3594d
> change-id: 20260306-topic-pgaltmode_fixup-6b488960e355
> 
> Best regards,