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.
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 --- 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
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.
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
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
linux-stable-mirror@lists.linaro.org
-
Badhri Jagan Sridharan -
Heikki Krogerus -
RD Babiera