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

[Heikki Krogerus]

Hi,

On Thu, Apr 23, 2026 at 11:09:45AM +0800, Chia-Lin Kao (AceLan) wrote:
> 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@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;
> > > +		}
> > > +	}
> 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.

I think this could be handled in ucsi_register_altmode() to cover both
cases. You probable just need to pass a bit more information to that
function, but that should be okay.

thanks,

-- 
heikki