On Tue, May 6, 2025 at 4:27 AM Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
On Tue, Apr 29, 2025 at 11:49:08PM +0000, RD Babiera wrote:
A state check was previously added to tcpm_queue_vdm_unlocked to prevent a deadlock where the DisplayPort Alt Mode driver would be executing work and attempting to grab the tcpm_lock while the TCPM was holding the lock and attempting to unregister the altmode, blocking on the altmode driver's cancel_work_sync call.
Because the state check isn't protected, there is a small window where the Alt Mode driver could determine that the TCPM is in a ready state and attempt to grab the lock while the TCPM grabs the lock and changes the TCPM state to one that causes the deadlock.
Change tcpm_queue_vdm_unlocked to queue for tcpm_queue_vdm_work, which can perform the state check while holding the TCPM lock while the Alt Mode lock is no longer held. This requires a new struct to hold the vdm data, altmode_vdm_event.
Hi RD,
The patch looks good to me. Lets also add the callstack of the different threads for reference.
Thanks, Badhri
Fixes: cdc9946ea637 ("usb: typec: tcpm: enforce ready state when queueing alt mode vdm") Cc: stable@vger.kernel.org Signed-off-by: RD Babiera rdbabiera@google.com
Reviewed-by: Heikki Krogerus heikki.krogerus@linux.intel.com
drivers/usb/typec/tcpm/tcpm.c | 91 +++++++++++++++++++++++++++-------- 1 file changed, 71 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 784fa23102f9..9b8d98328ddb 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -597,6 +597,15 @@ struct pd_rx_event { enum tcpm_transmit_type rx_sop_type; };
+struct altmode_vdm_event {
struct kthread_work work;struct tcpm_port *port;u32 header;u32 *data;int cnt;enum tcpm_transmit_type tx_sop_type;+};
static const char * const pd_rev[] = { [PD_REV10] = "rev1", [PD_REV20] = "rev2", @@ -1610,18 +1619,68 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, mod_vdm_delayed_work(port, 0); }
-static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
const u32 *data, int cnt, enum tcpm_transmit_type tx_sop_type)+static void tcpm_queue_vdm_work(struct kthread_work *work) {
if (port->state != SRC_READY && port->state != SNK_READY &&port->state != SRC_VDM_IDENTITY_REQUEST)return;
struct altmode_vdm_event *event = container_of(work,struct altmode_vdm_event,work);struct tcpm_port *port = event->port; mutex_lock(&port->lock);
tcpm_queue_vdm(port, header, data, cnt, tx_sop_type);
if (port->state != SRC_READY && port->state != SNK_READY &&port->state != SRC_VDM_IDENTITY_REQUEST) {tcpm_log_force(port, "dropping altmode_vdm_event");goto port_unlock;}tcpm_queue_vdm(port, event->header, event->data, event->cnt, event->tx_sop_type);+port_unlock:
kfree(event->data);kfree(event); mutex_unlock(&port->lock);}
+static int tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
const u32 *data, int cnt, enum tcpm_transmit_type tx_sop_type)+{
struct altmode_vdm_event *event;u32 *data_cpy;int ret = -ENOMEM;event = kzalloc(sizeof(*event), GFP_KERNEL);if (!event)goto err_event;data_cpy = kcalloc(cnt, sizeof(u32), GFP_KERNEL);if (!data_cpy)goto err_data;kthread_init_work(&event->work, tcpm_queue_vdm_work);event->port = port;event->header = header;memcpy(data_cpy, data, sizeof(u32) * cnt);event->data = data_cpy;event->cnt = cnt;event->tx_sop_type = tx_sop_type;ret = kthread_queue_work(port->wq, &event->work);if (!ret) {ret = -EBUSY;goto err_queue;}return 0;+err_queue:
kfree(data_cpy);+err_data:
kfree(event);+err_event:
tcpm_log_force(port, "failed to queue altmode vdm, err:%d", ret);return ret;+}
static void svdm_consume_identity(struct tcpm_port *port, const u32 *p, int cnt) { u32 vdo = p[VDO_INDEX_IDH]; @@ -2832,8 +2891,7 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo) header = VDO(altmode->svid, vdo ? 2 : 1, svdm_version, CMD_ENTER_MODE); header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0, TCPC_TX_SOP);return 0;
return tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0, TCPC_TX_SOP);}
static int tcpm_altmode_exit(struct typec_altmode *altmode) @@ -2849,8 +2907,7 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode) header = VDO(altmode->svid, 1, svdm_version, CMD_EXIT_MODE); header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm_unlocked(port, header, NULL, 0, TCPC_TX_SOP);return 0;
return tcpm_queue_vdm_unlocked(port, header, NULL, 0, TCPC_TX_SOP);}
static int tcpm_altmode_vdm(struct typec_altmode *altmode, @@ -2858,9 +2915,7 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode, { struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
tcpm_queue_vdm_unlocked(port, header, data, count - 1, TCPC_TX_SOP);return 0;
return tcpm_queue_vdm_unlocked(port, header, data, count - 1, TCPC_TX_SOP);}
static const struct typec_altmode_ops tcpm_altmode_ops = { @@ -2884,8 +2939,7 @@ static int tcpm_cable_altmode_enter(struct typec_altmode *altmode, enum typec_pl header = VDO(altmode->svid, vdo ? 2 : 1, svdm_version, CMD_ENTER_MODE); header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0, TCPC_TX_SOP_PRIME);return 0;
return tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0, TCPC_TX_SOP_PRIME);}
static int tcpm_cable_altmode_exit(struct typec_altmode *altmode, enum typec_plug_index sop) @@ -2901,8 +2955,7 @@ static int tcpm_cable_altmode_exit(struct typec_altmode *altmode, enum typec_plu header = VDO(altmode->svid, 1, svdm_version, CMD_EXIT_MODE); header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm_unlocked(port, header, NULL, 0, TCPC_TX_SOP_PRIME);return 0;
return tcpm_queue_vdm_unlocked(port, header, NULL, 0, TCPC_TX_SOP_PRIME);}
static int tcpm_cable_altmode_vdm(struct typec_altmode *altmode, enum typec_plug_index sop, @@ -2910,9 +2963,7 @@ static int tcpm_cable_altmode_vdm(struct typec_altmode *altmode, enum typec_plug { struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
tcpm_queue_vdm_unlocked(port, header, data, count - 1, TCPC_TX_SOP_PRIME);return 0;
return tcpm_queue_vdm_unlocked(port, header, data, count - 1, TCPC_TX_SOP_PRIME);}
static const struct typec_cable_ops tcpm_cable_ops = {
base-commit: 615dca38c2eae55aff80050275931c87a812b48c
2.49.0.967.g6a0df3ecc3-goog
-- heikki