| 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: <20250911092606.GE89417@robin.jannau.net>
Date: Thu, 11 Sep 2025 11:26:06 +0200
From: Janne Grunau <j@...nau.net>
To: Sven Peter <sven@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Felipe Balbi <balbi@...nel.org>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Neal Gompa <neal@...pa.dev>, Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Philipp Zabel <p.zabel@...gutronix.de>, Frank Li <Frank.Li@....com>,
Ran Wang <ran.wang_1@....com>, Peter Chen <peter.chen@....com>,
linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, asahi@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-phy@...ts.infradead.org,
Hector Martin <marcan@...can.st>
Subject: Re: [PATCH v2 15/22] usb: typec: tipd: Handle mode transitions for
CD321x
On Sat, Sep 06, 2025 at 03:43:28PM +0000, Sven Peter wrote:
> From: Hector Martin <marcan@...can.st>
>
> On Apple Silicon machines there is no control over which alt mode is
> chosen. The CD321x' firmware negotiates the target mode on its own and
> only lets the main CPU know after the mode has already been chosen.
> Especially after plugging a new cable in this can result to quick mode
> changes from e.g. power only -> USB3 only -> USB3+DisplayPort in a short
> time. It is not possile to influence this in any way and we also do not
> get direct access to the PDOs or VDOs exchanged via USB PD.
>
> Additionally, mode changes must be tightly synchronized between DWC3 and
> the Type C PHY and most mode changes require a full reset of DWC3 to
> make the port work correctly.
> On the machines the usb role change is used to reset the controller.
> The role change is additionally done synchronously from the callback
> instead of relying on a workqueue as usual in order to avoid any races
> which can, in the worst case, result in resetting the entire SoC if
> Type-C PHY and DWC3 are out of sync.
This paragraph needs to be update considering the dwc3 apple glue
driver.
> To be able to control all this we trigger the entire process in the
> correct order directly from the TIPD driver and de-bounce any mode
> changes to avoid tearing down and re-setting DWC3 back up multiple times
> any time a new connection is made.
>
> Signed-off-by: Hector Martin <marcan@...can.st>
> Co-developed-by: Sven Peter <sven@...nel.org>
> Signed-off-by: Sven Peter <sven@...nel.org>
> ---
> drivers/usb/typec/tipd/core.c | 297 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 293 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index b558fc5ecbc35a9dabbf33c444f38173740af7c3..95218e8be65dbbb594465961b1fda76eed1e266c 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -17,6 +17,7 @@
> #include <linux/usb/typec.h>
> #include <linux/usb/typec_altmode.h>
> #include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>
> #include <linux/usb/typec_tbt.h>
> #include <linux/usb/role.h>
> #include <linux/workqueue.h>
> @@ -120,6 +121,9 @@ struct tps6598x_intel_vid_status_reg {
> #define TPS_TASK_TIMEOUT 1
> #define TPS_TASK_REJECTED 3
>
> +/* Debounce delay for mode changes, in milliseconds */
> +#define CD321X_DEBOUNCE_DELAY_MS 500
> +
> enum {
> TPS_MODE_APP,
> TPS_MODE_BOOT,
> @@ -145,6 +149,7 @@ struct tipd_data {
> irq_handler_t irq_handler;
> u64 irq_mask1;
> size_t tps_struct_size;
> + void (*remove)(struct tps6598x *tps);
> int (*register_port)(struct tps6598x *tps, struct fwnode_handle *node);
> void (*unregister_port)(struct tps6598x *tps);
> void (*trace_data_status)(u32 status);
> @@ -155,6 +160,7 @@ struct tipd_data {
> int (*switch_power_state)(struct tps6598x *tps, u8 target_state);
> bool (*read_data_status)(struct tps6598x *tps);
> int (*reset)(struct tps6598x *tps);
> + int (*connect)(struct tps6598x *tps, u32 status);
> };
>
> struct tps6598x {
> @@ -183,6 +189,17 @@ struct tps6598x {
> const struct tipd_data *data;
> };
>
> +struct cd321x_status {
> + u32 status;
> + u32 pwr_status;
> + u32 data_status;
> + u32 status_changed;
> + struct usb_pd_identity partner_identity;
> + struct tps6598x_dp_sid_status_reg dp_sid_status;
> + struct tps6598x_intel_vid_status_reg intel_vid_status;
> + struct tps6598x_usb4_status_reg usb4_status;
> +};
> +
> struct cd321x {
> struct tps6598x tps;
>
> @@ -192,6 +209,13 @@ struct cd321x {
>
> struct typec_altmode *port_altmode_dp;
> struct typec_altmode *port_altmode_tbt;
> +
> + struct typec_mux *mux;
> + struct typec_mux_state state;
> +
> + struct cd321x_status update_status;
> + struct delayed_work update_work;
> + struct usb_pd_identity cur_partner_identity;
> };
>
> static enum power_supply_property tps6598x_psy_props[] = {
> @@ -613,9 +637,229 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
> }
> }
>
> +static void cd321x_typec_update_mode(struct tps6598x *tps, struct cd321x_status *st)
> +{
> + struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> +
> + if (!(st->data_status & TPS_DATA_STATUS_DATA_CONNECTION)) {
> + if (cd321x->state.mode == TYPEC_STATE_SAFE)
> + return;
> + cd321x->state.alt = NULL;
> + cd321x->state.mode = TYPEC_STATE_SAFE;
> + cd321x->state.data = NULL;
> + typec_mux_set(cd321x->mux, &cd321x->state);
> + } else if (st->data_status & TPS_DATA_STATUS_DP_CONNECTION) {
> + struct typec_displayport_data dp_data;
> + unsigned long mode;
> +
> + switch (TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT(st->data_status)) {
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A:
> + mode = TYPEC_DP_STATE_A;
> + break;
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B:
> + mode = TYPEC_DP_STATE_B;
> + break;
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_C:
> + mode = TYPEC_DP_STATE_C;
> + break;
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_D:
> + mode = TYPEC_DP_STATE_D;
> + break;
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_E:
> + mode = TYPEC_DP_STATE_E;
> + break;
> + case TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_F:
> + mode = TYPEC_DP_STATE_F;
> + break;
> + default:
> + dev_err(tps->dev, "Invalid DP pin assignment\n");
> + return;
> + }
> +
> + if (cd321x->state.alt == cd321x->port_altmode_dp &&
> + cd321x->state.mode == mode) {
> + return;
> + }
> +
> + dp_data.status = le32_to_cpu(st->dp_sid_status.status_rx);
> + dp_data.conf = le32_to_cpu(st->dp_sid_status.configure);
> + cd321x->state.alt = cd321x->port_altmode_dp;
> + cd321x->state.data = &dp_data;
&dp_data points to the stack. As it's no longer used after the
typec_mux_set call set cd321x->state.data to NULL to clear the dangling
pointer.
> + cd321x->state.mode = mode;
> + typec_mux_set(cd321x->mux, &cd321x->state);
> + } else if (st->data_status & TPS_DATA_STATUS_TBT_CONNECTION) {
> + struct typec_thunderbolt_data tbt_data;
> +
> + if (cd321x->state.alt == cd321x->port_altmode_tbt &&
> + cd321x->state.mode == TYPEC_TBT_MODE)
> + return;
> +
> + tbt_data.cable_mode = le16_to_cpu(st->intel_vid_status.cable_mode);
> + tbt_data.device_mode = le16_to_cpu(st->intel_vid_status.device_mode);
> + tbt_data.enter_vdo = le16_to_cpu(st->intel_vid_status.enter_vdo);
> + cd321x->state.alt = cd321x->port_altmode_tbt;
> + cd321x->state.mode = TYPEC_TBT_MODE;
> + cd321x->state.data = &tbt_data;
same issue as with dp_data
> + typec_mux_set(cd321x->mux, &cd321x->state);
> + } else if (st->data_status & CD321X_DATA_STATUS_USB4_CONNECTION) {
> + struct enter_usb_data eusb_data;
> +
> + if (cd321x->state.alt == NULL && cd321x->state.mode == TYPEC_MODE_USB4)
> + return;
> +
> + eusb_data.eudo = le32_to_cpu(st->usb4_status.eudo);
> + eusb_data.active_link_training =
> + !!(st->data_status & TPS_DATA_STATUS_ACTIVE_LINK_TRAIN);
> +
> + cd321x->state.alt = NULL;
> + cd321x->state.data = &eusb_data;
and again
> + cd321x->state.mode = TYPEC_MODE_USB4;
> + typec_mux_set(cd321x->mux, &cd321x->state);
> + } else {
> + if (cd321x->state.alt == NULL && cd321x->state.mode == TYPEC_STATE_USB)
> + return;
> + cd321x->state.alt = NULL;
> + cd321x->state.mode = TYPEC_STATE_USB;
> + cd321x->state.data = NULL;
> + typec_mux_set(cd321x->mux, &cd321x->state);
> + }
> +}
> +
> +static void cd321x_update_work(struct work_struct *work)
> +{
> + struct cd321x *cd321x = container_of(to_delayed_work(work),
> + struct cd321x, update_work);
> + struct tps6598x *tps = &cd321x->tps;
> + struct cd321x_status st;
> +
> + guard(mutex)(&tps->lock);
> +
> + st = cd321x->update_status;
> + cd321x->update_status.status_changed = 0;
> +
> + bool old_connected = !!tps->partner;
> + bool new_connected = st.status & TPS_STATUS_PLUG_PRESENT;
> + bool was_disconnected = st.status_changed & TPS_STATUS_PLUG_PRESENT;
> +
> + bool usb_connection = st.data_status &
> + (TPS_DATA_STATUS_USB2_CONNECTION | TPS_DATA_STATUS_USB3_CONNECTION);
> +
> + enum usb_role old_role = usb_role_switch_get_role(tps->role_sw);
> + enum usb_role new_role = USB_ROLE_NONE;
> + enum typec_pwr_opmode pwr_opmode = TYPEC_PWR_MODE_USB;
> + enum typec_orientation orientation = TYPEC_ORIENTATION_NONE;
> +
> + if (usb_connection) {
> + if (tps->data_status & TPS_DATA_STATUS_USB_DATA_ROLE)
> + new_role = USB_ROLE_DEVICE;
> + else
> + new_role = USB_ROLE_HOST;
> + }
> +
> + if (new_connected) {
> + pwr_opmode = TPS_POWER_STATUS_PWROPMODE(st.pwr_status);
> + orientation = TPS_STATUS_TO_UPSIDE_DOWN(st.status) ?
> + TYPEC_ORIENTATION_REVERSE : TYPEC_ORIENTATION_NORMAL;
> + }
> +
> + bool is_pd = pwr_opmode == TYPEC_PWR_MODE_PD;
> + bool partner_changed = old_connected && new_connected &&
> + (was_disconnected ||
> + (is_pd && memcmp(&st.partner_identity,
> + &cd321x->cur_partner_identity, sizeof(struct usb_pd_identity))));
> +
> + /* If we are switching from an active role, transition to USB_ROLE_NONE first */
> + if (old_role != USB_ROLE_NONE && (new_role != old_role || was_disconnected))
> + usb_role_switch_set_role(tps->role_sw, USB_ROLE_NONE);
> +
> + /* Process partner disconnection or change */
> + if (!new_connected || partner_changed) {
> + if (!IS_ERR(tps->partner))
> + typec_unregister_partner(tps->partner);
> + tps->partner = NULL;
> + }
> +
> + /* If there was a disconnection, set PHY to off */
> + if (!new_connected || was_disconnected) {
> + cd321x->state.alt = NULL;
> + cd321x->state.mode = TYPEC_STATE_SAFE;
> + cd321x->state.data = NULL;
> + typec_set_mode(tps->port, TYPEC_STATE_SAFE);
> + }
> +
> + /* Update Type-C properties */
> + typec_set_pwr_opmode(tps->port, pwr_opmode);
> + typec_set_pwr_role(tps->port, TPS_STATUS_TO_TYPEC_PORTROLE(st.status));
> + typec_set_vconn_role(tps->port, TPS_STATUS_TO_TYPEC_VCONN(st.status));
> + typec_set_orientation(tps->port, orientation);
> + typec_set_data_role(tps->port, TPS_STATUS_TO_TYPEC_DATAROLE(st.status));
> + power_supply_changed(tps->psy);
> +
> + /* If the plug is disconnected, we are done */
> + if (!new_connected)
> + return;
> +
> + /* Set up partner if we were previously disconnected (or changed). */
> + if (!tps->partner) {
> + struct typec_partner_desc desc;
> +
> + desc.usb_pd = is_pd;
> + desc.accessory = TYPEC_ACCESSORY_NONE; /* XXX: handle accessories */
> + desc.identity = NULL;
> +
> + if (desc.usb_pd)
> + desc.identity = &st.partner_identity;
> +
> + tps->partner = typec_register_partner(tps->port, &desc);
> + if (IS_ERR(tps->partner))
> + dev_warn(tps->dev, "%s: failed to register partnet\n", __func__);
> +
> + if (desc.identity) {
> + typec_partner_set_identity(tps->partner);
> + cd321x->cur_partner_identity = st.partner_identity;
> + }
> + }
> +
> + /* Update the TypeC MUX/PHY state */
> + cd321x_typec_update_mode(tps, &st);
> +
> + /* Launch the USB role switch */
> + usb_role_switch_set_role(tps->role_sw, new_role);
> +
> + power_supply_changed(tps->psy);
> +}
> +
> +static void cd321x_queue_status(struct tps6598x *tps)
> +{
> + struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> +
> + cd321x->update_status.status_changed |= cd321x->update_status.status ^ tps->status;
> +
> + cd321x->update_status.status = tps->status;
> + cd321x->update_status.pwr_status = tps->pwr_status;
> + cd321x->update_status.data_status = tps->data_status;
> +
> + cd321x->update_status.partner_identity = tps->partner_identity;
> + cd321x->update_status.dp_sid_status = cd321x->dp_sid_status;
> + cd321x->update_status.intel_vid_status = cd321x->intel_vid_status;
> + cd321x->update_status.usb4_status = cd321x->usb4_status;
> +}
> +
> +static int cd321x_connect(struct tps6598x *tps, u32 status)
> +{
> + struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> +
> + tps->status = status;
> + cd321x_queue_status(tps);
> + schedule_delayed_work(&cd321x->update_work, msecs_to_jiffies(CD321X_DEBOUNCE_DELAY_MS));
> +
> + return 0;
> +}
> +
> static irqreturn_t cd321x_interrupt(int irq, void *data)
> {
> struct tps6598x *tps = data;
> + struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> u64 event = 0;
> u32 status;
> int ret;
> @@ -652,9 +896,15 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
> if (!tps->data->read_data_status(tps))
> goto err_unlock;
>
> - /* Handle plug insert or removal */
> - if (event & APPLE_CD_REG_INT_PLUG_EVENT)
> - tps6598x_handle_plug_event(tps, status);
> + tps->status = status;
> + cd321x_queue_status(tps);
> +
> + /*
> + * Cancel pending work if not already running.
> + * We will requeue the work after CD321X_DEBOUNCE_DELAY_MS regardless.
> + */
> + cancel_delayed_work(&cd321x->update_work);
Should the work be canceled before updating tps->status and calling
cd321x_queue_status()? If so the status updates and rescheduling the
work can be handled by calling cd321x_connect().
> + schedule_delayed_work(&cd321x->update_work, msecs_to_jiffies(CD321X_DEBOUNCE_DELAY_MS));
>
> err_unlock:
> mutex_unlock(&tps->lock);
> @@ -1014,6 +1264,13 @@ cd321x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
> int ret;
>
> + /*
> + * This is only called from _probe such that update_work can be
> + * initialized and then scheduled for the first time to handle
> + * plugs already connected at boot time.
> + */
this comment seems to be outdated and at the wrong place
> + INIT_DELAYED_WORK(&cd321x->update_work, cd321x_update_work);
> +
> ret = tps6598x_register_port(tps, fwnode);
> if (ret)
> return ret;
> @@ -1022,10 +1279,26 @@ cd321x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> if (ret)
> goto err_unregister_port;
>
> + cd321x->mux = fwnode_typec_mux_get(fwnode);
> + if (IS_ERR(cd321x->mux)) {
> + ret = PTR_ERR(cd321x->mux);
> + goto err_unregister_altmodes;
> + }
> +
> + cd321x->state.alt = NULL;
> + cd321x->state.mode = TYPEC_STATE_SAFE;
> + cd321x->state.data = NULL;
> typec_set_mode(tps->port, TYPEC_STATE_SAFE);
>
> return 0;
>
> +err_unregister_altmodes:
> + if (cd321x->port_altmode_dp)
> + typec_unregister_altmode(cd321x->port_altmode_dp);
typec_unregister_altmode() is ERR / NULL safe so the if is not needed
> + if (cd321x->port_altmode_tbt)
> + typec_unregister_altmode(cd321x->port_altmode_tbt);
> + cd321x->port_altmode_dp = NULL;
> + cd321x->port_altmode_tbt = NULL;
> err_unregister_port:
> typec_unregister_port(tps->port);
> return ret;
> @@ -1042,6 +1315,8 @@ cd321x_unregister_port(struct tps6598x *tps)
> {
> struct cd321x *cd321x = container_of(tps, struct cd321x, tps);
>
> + if (cd321x->mux)
> + typec_mux_put(cd321x->mux);
typec_mux_put is ERR / NULL safe so the if is not needed
for consistency with port_altmode_* the dangling pointer should be
cleared to NULL
Janne
Powered by blists - more mailing lists