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

["Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>]

On Thu, Apr 16, 2026 at 12:22:26PM +0300, Heikki Krogerus wrote:
> 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@vger.kernel.org
> > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> > ---
> > 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;
> > +		}
> > +	}
Hi Heikki,

Thanks for the review.

>
> 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 loop iterates k from 0 to batch_idx - 1, comparing previously seen
entries against the current entry at batch_idx. It never compares an
entry against itself — svid and vdo come from alt_batch[batch_idx],
while k only reaches batch_idx - 1.

> 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].*)?
You're right that the standard path has at most 2 entries per
GET_ALTERNATE_MODES response. However, the helper is also used in
ucsi_register_altmodes_nvidia() (patch 2/3), where it iterates over
the full orig[]/updated[] arrays which are UCSI_MAX_ALTMODES in size.
A direct alt[0] vs alt[1] comparison wouldn't work there.

Keeping the loop in the helper means both call sites get correct
duplicate detection without special-casing each one.

>
> > +	/* 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.
Good idea, will do this in v5.

> > +		}
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
>
> thanks,
>
> --
> heikki