Re: [PATCH v3 05/10] usb: typec: Implement automated mode selection

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Andrei,

On Mon, Aug 04, 2025 at 09:03:34AM +0000, Andrei Kuchynski wrote:
> This patch introduces new sysfs attributes to enable user control over
> Type-C automated mode selection and provide negotiation feedback.
> 
> `mode_selection` attribute shows a prioritized list of supported modes
> with the currently entered mode bracketed. Writing boolean 0 or 1 to
> this attribute starts or stops the mode selection process,
> respectively.
> 
> `entry_result`, `usb4_entry_result` read-only attributes show the
> result of the last mode selection attempt for a specific mode.
> 
> Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  39 ++
>  drivers/usb/typec/class.c                   |  95 ++++-
>  drivers/usb/typec/class.h                   |  12 +
>  drivers/usb/typec/mode_selection.c          | 445 ++++++++++++++++++++
>  drivers/usb/typec/mode_selection.h          |  31 ++
>  include/linux/usb/pd_vdo.h                  |   2 +
>  include/linux/usb/typec_altmode.h           |   5 +
>  7 files changed, 626 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index 575dd94f33ab..ed89b9880085 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -280,6 +280,45 @@ Description:	The USB Modes that the partner device supports. The active mode
>  		- usb3 (USB 3.2)
>  		- usb4 (USB4)
>  
> +What:		/sys/class/typec/<port>-partner/mode_selection
> +Date:		July 2025
> +Contact:	Andrei Kuchynski <akuchynski@chromium.org>
> +Description:	Displays a prioritized list of modes that both the port and the
> +		partner support with the currently entered mode bracketed. Parentheses
> +		indicates a mode currently in progress. Modes listed before the active
> +		or in-progress mode have failed.
> +		Automated mode selection is activated by writing boolean 1 to the
> +		file. Conversely, writing boolean 0 will cancel any ongoing selection
> +		process and exit the currently active mode, if any.
> +		This attribute is only present if the kernel supports AP driven mode
> +		entry, where the Application Processor manages USB Type-C alt-modes.
> +
> +		Example values:
> +		- "USB4 (TBT) DP": USB4 mode entry failed, Thunderbolt alt-mode is in
> +			progress, DisplayPort alt-mode is next.
> +		- "[USB4] TBT DP": USB4 mode is currently active.

There seems to be at least two or three different functions for this
one file (listing of the modes, showing the state and enabling the
"automated mode selection"), so it's probable not going to work like
that.

I'm actually not completely sure from that what do you mean by
"automated mode selection", but is the idea that the "automated mode
selection" is newer enabled by default?

Perhaps the "automated mode selection" enabling should be handled with
its own file that that you can write and that also returns 0 or 1.

> +What:		/sys/class/typec/<port>-partner/<alt-mode>/entry_result
> +Date:		July 2025
> +Contact:	Andrei Kuchynski <akuchynski@chromium.org>
> +Description:	This read-only file represents the status for a specific
> +		alt-mode after the last mode selection process.
> +		This attribute is visible only if the kernel supports mode selection.
> +
> +		Example values:
> +		- "none": No mode selection attempt has occurred for this alt-mode.
> +		- "in progress": The mode entry process is currently underway.
> +		- "active": The alt-mode is currently active.
> +		- "cable failed": The connected cable doesn't support the mode.
> +		- "timeout": Mode entry failed due to a timeout.
> +		- "failed": The attempt to activate the mode failed.

That looks like just debugging information. Where are those states
coming from - they are not defined in any public specification, or are
they?

I'm not sure if you can have a sysfs file for that, but maybe it would
still be okay to inform the user space about a state like that with an
uevent (or maybe not)? Somebody else needs to comment on this.

        char *envp[2] = { };
        ...
        envp[0] = kasprintf(GFP_KERNEL, "STATE=%s", entry_state);
        ...
        kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
        kfree(envp[0]);

I think this patch needs to be split into two or more patches in any
case, but I would suggest that you propose these sysfs files only
after we the mode priority order for the port figured out and
accepted.

thanks,

-- 
heikki