| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aKbwby7OYdUpLvhA@kuha.fi.intel.com>
Date: Thu, 21 Aug 2025 13:09:51 +0300
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Andrei Kuchynski <akuchynski@...omium.org>
Cc: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
Benson Leung <bleung@...omium.org>,
Jameson Thies <jthies@...gle.com>,
Tzung-Bi Shih <tzungbi@...nel.org>, linux-usb@...r.kernel.org,
chrome-platform@...ts.linux.dev,
Guenter Roeck <groeck@...omium.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
"Christian A. Ehrhardt" <lk@...e.de>,
Venkat Jayaraman <venkat.jayaraman@...el.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 4/5] usb: typec: Implement alternate mode priority
handling
Hi Andrei,
On Thu, Aug 14, 2025 at 06:44:54PM +0000, Andrei Kuchynski wrote:
> This patch introduces APIs to manage the priority of USB Type-C alternate
> modes. These APIs allow for setting and retrieving a priority number for
> each mode. If a new priority value conflicts with an existing mode's
> priority, the priorities of the conflicting mode and all subsequent modes
> are automatically incremented to ensure uniqueness.
I think this needs to be simplified. You don't need this elaborate
implementation in the beginning.
I'm going to do some suggestions. I don't know if all of them work,
but hopefully you get the idea how I would like to see the initial
support to be implemented.
> Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> ---
> drivers/usb/typec/Makefile | 2 +-
> drivers/usb/typec/class.h | 1 +
> drivers/usb/typec/mode_selection.c | 127 +++++++++++++++++++++++++++++
> drivers/usb/typec/mode_selection.h | 8 ++
> include/linux/usb/typec_altmode.h | 9 ++
> 5 files changed, 146 insertions(+), 1 deletion(-)
> create mode 100644 drivers/usb/typec/mode_selection.c
> create mode 100644 drivers/usb/typec/mode_selection.h
>
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index 7a368fea61bc..8a6a1c663eb6 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 f05d9201c233..c6467e576569 100644
> --- a/drivers/usb/typec/class.h
> +++ b/drivers/usb/typec/class.h
> @@ -82,6 +82,7 @@ struct typec_port {
> struct device *usb3_dev;
>
> bool alt_mode_override;
> + struct list_head mode_list;
I'm not sure we need this.
> };
>
> #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> diff --git a/drivers/usb/typec/mode_selection.c b/drivers/usb/typec/mode_selection.c
> new file mode 100644
> index 000000000000..8a54639b86bf
> --- /dev/null
> +++ b/drivers/usb/typec/mode_selection.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2025 Google LLC.
> + */
> +
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include "mode_selection.h"
> +#include "class.h"
> +
> +static const char * const mode_names[TYPEC_ALTMODE_MAX] = {
> + [TYPEC_ALTMODE_DP] = "DisplayPort",
> + [TYPEC_ALTMODE_TBT] = "Thunderbolt3",
> + [TYPEC_ALTMODE_USB4] = "USB4",
> +};
You only need string for USB4. The altmode names come from the drivers.
> +static const int default_priorities[TYPEC_ALTMODE_MAX] = {
> + [TYPEC_ALTMODE_DP] = 2,
> + [TYPEC_ALTMODE_TBT] = 1,
> + [TYPEC_ALTMODE_USB4] = 0,
> +};
The default priorities is an array of svids. And I really think that
the highest priority should be 1 not 0.
> +static inline enum typec_mode_type typec_svid_to_altmode(const u16 svid)
> +{
> + switch (svid) {
> + case USB_TYPEC_DP_SID:
> + return TYPEC_ALTMODE_DP;
> + case USB_TYPEC_TBT_SID:
> + return TYPEC_ALTMODE_TBT;
> + case USB_TYPEC_USB4_SID:
> + return TYPEC_ALTMODE_USB4;
> + }
> + return TYPEC_ALTMODE_MAX;
> +}
Get rid of this.
> +/**
> + * struct mode_selection_state - State tracking for a specific Type-C mode
> + * @mode: The type of mode this instance represents
> + * @priority: The mode priority. Lower values indicate a more preferred mode.
> + * @list: List head to link this mode state into a prioritized list.
> + */
> +struct mode_selection_state {
> + enum typec_mode_type mode;
> + int priority;
> + struct list_head list;
> +};
Get rid of this. I don't see why you would need to separate the
priority handling at this point. Keep it as simply as possible first.
Just place the priority member to struct typec_altmode. 0 should
indicate that there is no specific priority set, or default order
should be used IMO.
> +/* -------------------------------------------------------------------------- */
> +/* port 'mode_priorities' attribute */
> +
> +int typec_mode_set_priority(struct typec_altmode *adev, const int priority)
> +{
> + struct typec_port *port = to_typec_port(adev->dev.parent);
> + const enum typec_mode_type mode = typec_svid_to_altmode(adev->svid);
> + struct mode_selection_state *ms_target = NULL;
> + struct mode_selection_state *ms, *tmp;
> +
> + if (mode >= TYPEC_ALTMODE_MAX || !mode_names[mode])
> + return -EOPNOTSUPP;
Just support every altmode bind to a driver and USB4. USB4 you
identify with a specific usb4 device type.
> + list_for_each_entry_safe(ms, tmp, &port->mode_list, list) {
> + if (ms->mode == mode) {
> + ms_target = ms;
> + list_del(&ms->list);
> + break;
> + }
> + }
> +
> + if (!ms_target) {
> + ms_target = kzalloc(sizeof(struct mode_selection_state), GFP_KERNEL);
> + if (!ms_target)
> + return -ENOMEM;
> + ms_target->mode = mode;
> + INIT_LIST_HEAD(&ms_target->list);
> + }
> +
> + if (priority >= 0)
> + ms_target->priority = priority;
> + else
> + ms_target->priority = default_priorities[mode];
> +
> + while (ms_target) {
> + struct mode_selection_state *ms_peer = NULL;
> +
> + list_for_each_entry(ms, &port->mode_list, list)
> + if (ms->priority >= ms_target->priority) {
> + if (ms->priority == ms_target->priority)
> + ms_peer = ms;
> + break;
> + }
> +
> + list_add_tail(&ms_target->list, &ms->list);
> + ms_target = ms_peer;
> + if (ms_target) {
> + ms_target->priority++;
> + list_del(&ms_target->list);
> + }
> + }
> +
> + return 0;
> +}
> +
> +int typec_mode_get_priority(struct typec_altmode *adev, int *priority)
> +{
> + struct typec_port *port = to_typec_port(adev->dev.parent);
> + const enum typec_mode_type mode = typec_svid_to_altmode(adev->svid);
> + struct mode_selection_state *ms;
> +
> + list_for_each_entry(ms, &port->mode_list, list)
> + if (ms->mode == mode) {
> + *priority = ms->priority;
> + return 0;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
> +void typec_mode_selection_destroy(struct typec_port *port)
> +{
> + struct mode_selection_state *ms, *tmp;
> +
> + list_for_each_entry_safe(ms, tmp, &port->mode_list, list) {
> + list_del(&ms->list);
> + kfree(ms);
> + }
> +}
> diff --git a/drivers/usb/typec/mode_selection.h b/drivers/usb/typec/mode_selection.h
> new file mode 100644
> index 000000000000..69adfcf39d7c
> --- /dev/null
> +++ b/drivers/usb/typec/mode_selection.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_tbt.h>
> +
> +int typec_mode_set_priority(struct typec_altmode *adev, const int priority);
> +int typec_mode_get_priority(struct typec_altmode *adev, int *priority);
> +void typec_mode_selection_destroy(struct typec_port *port);
> diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
> index b3c0866ea70f..318858fc7bec 100644
> --- a/include/linux/usb/typec_altmode.h
> +++ b/include/linux/usb/typec_altmode.h
> @@ -145,6 +145,15 @@ enum {
>
> #define TYPEC_MODAL_STATE(_state_) ((_state_) + TYPEC_STATE_MODAL)
>
> +#define USB_TYPEC_USB4_SID 0xff00
I would suggest that you create a separate patch or patches for the
USB4 mode registration.
> +enum typec_mode_type {
> + TYPEC_ALTMODE_DP = 0,
> + TYPEC_ALTMODE_TBT,
> + TYPEC_ALTMODE_USB4,
> + TYPEC_ALTMODE_MAX,
> +};
Drop this completely.
thanks,
--
heikki
Powered by blists - more mailing lists