Message-ID: <CAMMMRMepwLPExp4i3M-d3mD_55rcLPGRsB-wVjB0Z-+yzn75-Q@mail.gmail.com>
Date: Sat, 17 Jan 2026 20:28:15 +0100
From: Andrei Kuchynski <akuchynski@...omium.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>, Benson Leung <bleung@...omium.org>, 
	Jameson Thies <jthies@...gle.com>, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
	chrome-platform@...ts.linux.dev, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Guenter Roeck <groeck@...omium.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, 
	Łukasz Bartosik <ukaszb@...omium.org>, 
	Abel Vesa <abel.vesa@...aro.org>, Pooja Katiyar <pooja.katiyar@...el.com>, 
	Johan Hovold <johan@...nel.org>, Hsin-Te Yuan <yuanhsinte@...omium.org>, Madhu M <madhu.m@...el.com>, 
	Venkat Jayaraman <venkat.jayaraman@...el.com>
Subject: Re: [PATCH v4 5/8] usb: typec: Implement mode selection

On Thu, Jan 15, 2026 at 3:53 PM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
>
> Tue, Jan 13, 2026 at 01:05:33PM +0000, Andrei Kuchynski kirjoitti:
> > The mode selection process is controlled by the following API functions,
> > which allow to initiate and complete mode entry based on the priority of
> > each mode:
> >
> > `typec_mode_selection_start` function compiles a priority list of supported
> > Alternate Modes.
> > `typec_altmode_state_update` function is invoked by the port driver to
> > communicate the current mode of the Type-C connector.
> > `typec_mode_selection_delete` function stops the currently running mode
> > selection process and releases all associated system resources.
> >
> > `mode_selection_work_fn` task attempts to activate modes. The process stops
> > on success; otherwise, it proceeds to the next mode after a timeout or
> > error.
> >
> > Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> > ---
> >  drivers/usb/typec/Makefile         |   2 +-
> >  drivers/usb/typec/class.h          |   2 +
> >  drivers/usb/typec/mode_selection.c | 288 +++++++++++++++++++++++++++++
> >  include/linux/usb/typec_altmode.h  |  40 ++++
> >  4 files changed, 331 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/usb/typec/mode_selection.c
> >
> > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> > index 7a368fea61bc9..8a6a1c663eb69 100644
> > --- a/drivers/usb/typec/Makefile
> > +++ b/drivers/usb/typec/Makefile
> > @@ -1,6 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_TYPEC)          += typec.o
> > -typec-y                              := class.o mux.o bus.o pd.o retimer.o
> > +typec-y                              := class.o mux.o bus.o pd.o retimer.o mode_selection.o
> >  typec-$(CONFIG_ACPI)         += port-mapper.o
> >  obj-$(CONFIG_TYPEC)          += altmodes/
> >  obj-$(CONFIG_TYPEC_TCPM)     += tcpm/
> > diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> > index 2e89a83c2eb70..d3435936ee7c8 100644
> > --- a/drivers/usb/typec/class.h
> > +++ b/drivers/usb/typec/class.h
> > @@ -9,6 +9,7 @@
> >  struct typec_mux;
> >  struct typec_switch;
> >  struct usb_device;
> > +struct mode_selection;
> >
> >  struct typec_plug {
> >       struct device                   dev;
> > @@ -39,6 +40,7 @@ struct typec_partner {
> >       u8                              usb_capability;
> >
> >       struct usb_power_delivery       *pd;
> > +     struct mode_selection   *sel;
> >
> >       void (*attach)(struct typec_partner *partner, struct device *dev);
> >       void (*deattach)(struct typec_partner *partner, struct device *dev);
> > diff --git a/drivers/usb/typec/mode_selection.c b/drivers/usb/typec/mode_selection.c
> > new file mode 100644
> > index 0000000000000..63a1d251c72b4
> > --- /dev/null
> > +++ b/drivers/usb/typec/mode_selection.c
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2025 Google LLC.
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/list_sort.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/usb/typec_altmode.h>
> > +
> > +#include "class.h"
> > +
> > +/**
> > + * struct mode_state - State tracking for a specific Type-C alternate mode
> > + * @svid: Standard or Vendor ID of the Alternate Mode
> > + * @priority: Mode priority
> > + * @error: Outcome of the last attempt to enter the mode
> > + * @list: List head to link this mode state into a prioritized list
> > + */
> > +struct mode_state {
> > +     u16 svid;
> > +     u8 priority;
> > +     int error;
> > +     struct list_head list;
> > +};
> > +
> > +/**
> > + * struct mode_selection - Manages the selection and state of Alternate Modes
> > + * @mode_list: Prioritized list of available Alternate Modes
> > + * @lock: Mutex to protect mode_list
> > + * @work: Work structure
> > + * @partner: Handle to the Type-C partner device
> > + * @active_svid: svid of currently active mode
> > + * @timeout: Timeout for a mode entry attempt, ms
> > + * @delay: Delay between mode entry/exit attempts, ms
> > + */
> > +struct mode_selection {
> > +     struct list_head mode_list;
> > +     struct mutex lock;
> > +     struct delayed_work work;
> > +     struct typec_partner *partner;
> > +     u16 active_svid;
> > +     unsigned int timeout;
> > +     unsigned int delay;
> > +};
> > +
> > +/**
> > + * struct mode_order - Mode activation tracking
> > + * @svid: Standard or Vendor ID of the Alternate Mode
> > + * @enter: Flag indicating if the driver is currently attempting to enter or
> > + * exit the mode
> > + * @result: Outcome of the attempt to activate the mode
> > + */
> > +struct mode_order {
> > +     u16 svid;
> > +     int enter;
> > +     int result;
> > +};
> > +
> > +static int activate_altmode(struct device *dev, void *data)
> > +{
> > +     if (is_typec_partner_altmode(dev)) {
> > +             struct typec_altmode *alt = to_typec_altmode(dev);
> > +             struct mode_order *order = (struct mode_order *)data;
> > +
> > +             if (order->svid == alt->svid) {
> > +                     if (alt->ops && alt->ops->activate)
> > +                             order->result = alt->ops->activate(alt, order->enter);
> > +                     else
> > +                             order->result = -EOPNOTSUPP;
> > +                     return 1;
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int mode_selection_activate(struct mode_selection *sel,
> > +             const u16 svid, const int enter)
>
> You need to run these through "scripts/checkpatch.pl --strict".
> Please fix all the checks that it gives you.
>

Looks like I ran it without the "--strict" parameter.
My mistake, I'll fix it.

> > +
> > +     __must_hold(&sel->lock)
> > +{
> > +     struct mode_order order = {.svid = svid, .enter = enter, .result = -ENODEV};
> > +
> > +     /*
> > +      * The port driver may acquire its internal mutex during alternate mode
> > +      * activation. Since this is the same mutex that may be held during the
> > +      * execution of typec_altmode_state_update(), it is crucial to release
> > +      * sel->mutex before activation to avoid potential deadlock.
> > +      * Note that sel->mode_list must remain invariant throughout this unlocked
> > +      * interval.
> > +      */
> > +     mutex_unlock(&sel->lock);
> > +     device_for_each_child(&sel->partner->dev, &order, activate_altmode);
> > +     mutex_lock(&sel->lock);
> > +
> > +     return order.result;
> > +}
> > +
> > +static void mode_list_clean(struct mode_selection *sel)
> > +{
> > +     struct mode_state *ms, *tmp;
> > +
> > +     list_for_each_entry_safe(ms, tmp, &sel->mode_list, list) {
> > +             list_del(&ms->list);
> > +             kfree(ms);
> > +     }
> > +}
> > +
> > +/**
> > + * mode_selection_work_fn() - Alternate mode activation task
> > + * @work: work structure
> > + *
> > + * - If the Alternate Mode currently prioritized at the top of the list is already
> > + * active, the entire selection process is considered finished.
> > + * - If a different Alternate Mode is currently active, the system must exit that
> > + * active mode first before attempting any new entry.
> > + *
> > + * The function then checks the result of the attempt to entre the current mode,
> > + * stored in the `ms->error` field:
> > + * - if the attempt FAILED, the mode is deactivated and removed from the list.
> > + * - `ms->error` value of 0 signifies that the mode has not yet been activated.
> > + *
> > + * Once successfully activated, the task is scheduled for subsequent entry after
> > + * a timeout period. The alternate mode driver is expected to call back with the
> > + * actual mode entry result via `typec_altmode_state_update()`.
> > + */
> > +static void mode_selection_work_fn(struct work_struct *work)
> > +{
> > +     struct mode_selection *sel = container_of(work,
> > +                             struct mode_selection, work.work);
> > +     struct mode_state *ms;
> > +     unsigned int delay = sel->delay;
> > +     int result;
> > +
> > +     mutex_lock(&sel->lock);
>
> guard(mutex)(&sel->lock); ?
>

Sounds great. Thanks

> > +     ms = list_first_entry_or_null(&sel->mode_list, struct mode_state, list);
> > +     if (!ms) {
> > +             mutex_unlock(&sel->lock);
> > +             return;
> > +     }
> > +
> > +     if (sel->active_svid == ms->svid) {
> > +             dev_dbg(&sel->partner->dev, "%x altmode is active\n", ms->svid);
> > +             mode_list_clean(sel);
> > +     } else if (sel->active_svid != 0) {
> > +             result = mode_selection_activate(sel, sel->active_svid, 0);
> > +             if (result) {
> > +                     dev_dbg(&sel->partner->dev, "enable to exit %x altmode\n",
> > +                                     sel->active_svid);
>
> "enable to exit" ?
>
> Just drop that dev_dbg.
>
> > +                     mode_list_clean(sel);
> > +             } else {
> > +                     sel->active_svid = 0;
> > +             }
> > +     } else if (ms->error) {
> > +             dev_dbg(&sel->partner->dev, "%x: entry error %pe\n",
> > +                             ms->svid, ERR_PTR(ms->error));
>
> dev_err (or dev_warn)?
>

I will change dev_dbg to dev_err and remove the message when exiting
the mode.

Thanks,
Andrei

Powered by blists - more mailing lists