Re: [PATCH v4 1/3] usb: typec: ucsi: Detect and skip duplicate altmodes from buggy firmware

[Heikki Krogerus]

On Mon, Apr 13, 2026 at 03:35:49PM +0800, Chia-Lin Kao (AceLan) wrote:
> Some firmware implementations incorrectly return the same altmode
> multiple times at different offsets when queried via UCSI_GET_ALTERNATE_MODES.
> This causes sysfs duplicate filename errors and kernel call traces when
> the driver attempts to register the same altmode twice:
> 
>   sysfs: cannot create duplicate filename '/devices/.../typec/port0/port0.0/partner'
>   typec-thunderbolt port0-partner.1: failed to create symlinks
>   typec-thunderbolt port0-partner.1: probe with driver typec-thunderbolt failed with error -17
> 
> Detect duplicate altmodes by comparing SVID and VDO before registration.
> If a duplicate is detected, skip it and print a single clean warning
> message instead of generating a kernel call trace:
> 
>   ucsi_acpi USBC000:00: con2: Firmware bug: duplicate partner altmode SVID 0x8087 (VDO 0x8087a043 vs 0x00000001) at offset 1, ignoring. Please update your system firmware.
> 
> This makes the error handling more user-friendly while still alerting
> users to the firmware bug.
> 
> The duplicate detection logic is implemented in a reusable helper
> function ucsi_altmode_is_duplicate() and used in ucsi_register_altmodes().
> The fix applies to all three recipient types: partner (SOP), port (CON),
> and plug (SOP_P) altmodes.
> 
> Fixes: a79f16efcd00 ("usb: typec: ucsi: Add support for the partner USB Modes")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@xxxxxxxxxxxxx>
> ---
> v4. rebase
> v3. 1. move ucsi_altmode_is_duplicate() before ucsi_register_altmodes_nvidia()
>        for later modification on ucsi_register_altmodes_nvidia()
>     2. use struct typec_altmode **altmodes to simplify the logic
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 76 +++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index f181afca2bb28..eebbb80fae566 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -498,6 +498,73 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
>  	return ret;
>  }
>  
> +/*
> + * Check if an altmode is a duplicate. Some firmware implementations
> + * incorrectly return the same altmode multiple times, causing sysfs errors.
> + * Returns true if the altmode should be skipped.
> + */
> +static bool ucsi_altmode_is_duplicate(struct ucsi_connector *con, u8 recipient,
> +				      const struct ucsi_altmode *alt_batch, int batch_idx,
> +				      u16 svid, u32 vdo, int offset)
> +{
> +	struct typec_altmode **altmodes;
> +	const char *recipient_name;
> +	int k;
> +
> +	/* Check for duplicates within the current batch first */
> +	for (k = 0; k < batch_idx; k++) {
> +		if (alt_batch[k].svid == svid && alt_batch[k].mid == vdo) {
> +			dev_warn_once(con->ucsi->dev,
> +				      "con%d: Firmware bug: duplicate altmode SVID 0x%04x in same response at offset %d, ignoring. Please update your system firmware.\n",
> +				      con->num, svid, offset);
> +			return true;
> +		}
> +	}

What is this loop meant to do? It will now always return true because
the svid is always from one of the altmodes in the alt_batch, no?

The "batch" here means what the PPM returns to the GET_ALTERNATE_MODES
command (right?), so you can have maximum of two entries in it. So
wouldn't it be simpler to just check if there is two (instead of only
the one that was requested) altmodes returned, and then just directly
compare the two (alt[0].* == alt[1].*)?

> +	/* Check for duplicates in already registered altmodes */
> +
> +	switch (recipient) {
> +	case UCSI_RECIPIENT_CON:
> +		altmodes = con->port_altmode;
> +		recipient_name = "port";
> +		break;
> +	case UCSI_RECIPIENT_SOP:
> +		altmodes = con->partner_altmode;
> +		recipient_name = "partner";
> +		break;
> +	case UCSI_RECIPIENT_SOP_P:
> +		altmodes = con->plug_altmode;
> +		recipient_name = "plug";
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	for (k = 0; k < UCSI_MAX_ALTMODES; k++) {
> +		if (!altmodes[k])
> +			break;
> +
> +		/* Check SVID for all, VDO only for non-SOP */
> +		if (altmodes[k]->svid != svid)
> +			continue;
> +		if (recipient != UCSI_RECIPIENT_SOP && altmodes[k]->vdo != vdo)
> +			continue;
> +
> +		if (recipient == UCSI_RECIPIENT_SOP) {
> +			dev_warn(con->ucsi->dev,
> +				 "con%d: Firmware bug: duplicate %s altmode SVID 0x%04x (VDO 0x%08x vs 0x%08x) at offset %d, ignoring. Please update your system firmware.\n",
> +				 con->num, recipient_name, svid, altmodes[k]->vdo, vdo, offset);
> +		} else {
> +			dev_warn_once(con->ucsi->dev,
> +				      "con%d: Firmware bug: duplicate %s altmode SVID 0x%04x at offset %d, ignoring. Please update your system firmware.\n",
> +				      con->num, recipient_name, svid, offset);

You have to clean up these prints. Provide a helper function for them.
You can split the print in two. A generic part, and separate for
details if necessary.

With the "recipient_name" I would use a lookup table.

> +		}
> +		return true;
> +	}
> +
> +	return false;
> +}

thanks,

-- 
heikki