* [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm()
@ 2020-07-24 17:46 Hans de Goede
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:46 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
All callers of tcpm_queue_vdm() immediately follow the tcpm_queue_vdm()
vdm call with a:
mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
Call, fold this into tcpm_queue_vdm() itself.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/usb/typec/tcpm/tcpm.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index b4a66e6bf68c..fc6455a29c74 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -967,6 +967,8 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
/* Set ready, vdm state machine will actually send */
port->vdm_retries = 0;
port->vdm_state = VDM_STATE_READY;
+
+ mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
}
static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
@@ -1246,10 +1248,8 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
if (PD_VDO_SVDM(p0))
rlen = tcpm_pd_svdm(port, payload, cnt, response);
- if (rlen > 0) {
+ if (rlen > 0)
tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
- mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
- }
}
static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
@@ -1264,8 +1264,6 @@ static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
header = VDO(vid, ((vid & USB_SID_PD) == USB_SID_PD) ?
1 : (PD_VDO_CMD(cmd) <= CMD_ATTENTION), cmd);
tcpm_queue_vdm(port, header, data, count);
-
- mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
}
static unsigned int vdm_ready_timeout(u32 vdm_hdr)
@@ -1513,7 +1511,6 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
- mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
mutex_unlock(&port->lock);
return 0;
@@ -1529,7 +1526,6 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode)
header |= VDO_OPOS(altmode->mode);
tcpm_queue_vdm(port, header, NULL, 0);
- mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
mutex_unlock(&port->lock);
return 0;
@@ -1542,7 +1538,6 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode,
mutex_lock(&port->lock);
tcpm_queue_vdm(port, header, data, count - 1);
- mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
mutex_unlock(&port->lock);
return 0;
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
@ 2020-07-24 17:46 ` Hans de Goede
2020-07-25 14:37 ` Guenter Roeck
2020-07-28 12:44 ` Heikki Krogerus
2020-07-24 17:46 ` [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling Hans de Goede
` (4 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:46 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
Various callers (all the typec_altmode_ops) take the port-lock just for
the tcpm_queue_vdm() call.
Add a new tcpm_queue_vdm_unlocked() helper which takes the lock, so that
its callers don't have to do this themselves.
This is a preparation patch for fixing an AB BA lock inversion between
the tcpm code and some altmode drivers.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Name the new helper tcpm_queue_vdm_unlocked() instead of renaming the
original function to this
---
drivers/usb/typec/tcpm/tcpm.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index fc6455a29c74..862c474b3ebd 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -961,6 +961,8 @@ static void tcpm_queue_message(struct tcpm_port *port,
static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
const u32 *data, int cnt)
{
+ WARN_ON(!mutex_is_locked(&port->lock));
+
port->vdo_count = cnt + 1;
port->vdo_data[0] = header;
memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
@@ -971,6 +973,14 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
}
+static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
+ const u32 *data, int cnt)
+{
+ mutex_lock(&port->lock);
+ tcpm_queue_vdm(port, header, data, cnt);
+ mutex_unlock(&port->lock);
+}
+
static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
int cnt)
{
@@ -1506,13 +1516,10 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
u32 header;
- mutex_lock(&port->lock);
header = VDO(altmode->svid, vdo ? 2 : 1, CMD_ENTER_MODE);
header |= VDO_OPOS(altmode->mode);
- tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
- mutex_unlock(&port->lock);
-
+ tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0);
return 0;
}
@@ -1521,13 +1528,10 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode)
struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
u32 header;
- mutex_lock(&port->lock);
header = VDO(altmode->svid, 1, CMD_EXIT_MODE);
header |= VDO_OPOS(altmode->mode);
- tcpm_queue_vdm(port, header, NULL, 0);
- mutex_unlock(&port->lock);
-
+ tcpm_queue_vdm_unlocked(port, header, NULL, 0);
return 0;
}
@@ -1536,10 +1540,7 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode,
{
struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
- mutex_lock(&port->lock);
- tcpm_queue_vdm(port, header, data, count - 1);
- mutex_unlock(&port->lock);
-
+ tcpm_queue_vdm_unlocked(port, header, data, count - 1);
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
@ 2020-07-24 17:46 ` Hans de Goede
2020-07-28 12:59 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request Hans de Goede
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:46 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
Refactor the tcpm_handle_vdm_request payload handling by doing the
endianness conversion only once directly inside tcpm_handle_vdm_request
itself instead of doing it multiple times inside various helper functions
called by tcpm_handle_vdm_request.
This is a preparation patch for some further refactoring to fix an AB BA
lock inversion between the tcpm code and some altmode drivers.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/usb/typec/tcpm/tcpm.c | 49 ++++++++++++++++-------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 862c474b3ebd..ee239b54bcd8 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -981,16 +981,15 @@ static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
mutex_unlock(&port->lock);
}
-static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
- int cnt)
+static void svdm_consume_identity(struct tcpm_port *port, const u32 *p, int cnt)
{
- u32 vdo = le32_to_cpu(payload[VDO_INDEX_IDH]);
- u32 product = le32_to_cpu(payload[VDO_INDEX_PRODUCT]);
+ u32 vdo = p[VDO_INDEX_IDH];
+ u32 product = p[VDO_INDEX_PRODUCT];
memset(&port->mode_data, 0, sizeof(port->mode_data));
port->partner_ident.id_header = vdo;
- port->partner_ident.cert_stat = le32_to_cpu(payload[VDO_INDEX_CSTAT]);
+ port->partner_ident.cert_stat = p[VDO_INDEX_CSTAT];
port->partner_ident.product = product;
typec_partner_set_identity(port->partner);
@@ -1000,17 +999,15 @@ static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
PD_PRODUCT_PID(product), product & 0xffff);
}
-static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
- int cnt)
+static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
{
struct pd_mode_data *pmdata = &port->mode_data;
int i;
for (i = 1; i < cnt; i++) {
- u32 p = le32_to_cpu(payload[i]);
u16 svid;
- svid = (p >> 16) & 0xffff;
+ svid = (p[i] >> 16) & 0xffff;
if (!svid)
return false;
@@ -1020,7 +1017,7 @@ static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
pmdata->svids[pmdata->nsvids++] = svid;
tcpm_log(port, "SVID %d: 0x%x", pmdata->nsvids, svid);
- svid = p & 0xffff;
+ svid = p[i] & 0xffff;
if (!svid)
return false;
@@ -1036,8 +1033,7 @@ static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
return false;
}
-static void svdm_consume_modes(struct tcpm_port *port, const __le32 *payload,
- int cnt)
+static void svdm_consume_modes(struct tcpm_port *port, const u32 *p, int cnt)
{
struct pd_mode_data *pmdata = &port->mode_data;
struct typec_altmode_desc *paltmode;
@@ -1054,7 +1050,7 @@ static void svdm_consume_modes(struct tcpm_port *port, const __le32 *payload,
paltmode->svid = pmdata->svids[pmdata->svid_index];
paltmode->mode = i;
- paltmode->vdo = le32_to_cpu(payload[i]);
+ paltmode->vdo = p[i];
tcpm_log(port, " Alternate mode %d: SVID 0x%04x, VDO %d: 0x%08x",
pmdata->altmodes, paltmode->svid,
@@ -1082,21 +1078,17 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
#define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
-static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
+static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
u32 *response)
{
struct typec_altmode *adev;
struct typec_altmode *pdev;
struct pd_mode_data *modep;
- u32 p[PD_MAX_PAYLOAD];
int rlen = 0;
int cmd_type;
int cmd;
int i;
- for (i = 0; i < cnt; i++)
- p[i] = le32_to_cpu(payload[i]);
-
cmd_type = PD_VDO_CMDT(p[0]);
cmd = PD_VDO_CMD(p[0]);
@@ -1157,13 +1149,13 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
switch (cmd) {
case CMD_DISCOVER_IDENT:
/* 6.4.4.3.1 */
- svdm_consume_identity(port, payload, cnt);
+ svdm_consume_identity(port, p, cnt);
response[0] = VDO(USB_SID_PD, 1, CMD_DISCOVER_SVID);
rlen = 1;
break;
case CMD_DISCOVER_SVID:
/* 6.4.4.3.2 */
- if (svdm_consume_svids(port, payload, cnt)) {
+ if (svdm_consume_svids(port, p, cnt)) {
response[0] = VDO(USB_SID_PD, 1,
CMD_DISCOVER_SVID);
rlen = 1;
@@ -1175,7 +1167,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
break;
case CMD_DISCOVER_MODES:
/* 6.4.4.3.3 */
- svdm_consume_modes(port, payload, cnt);
+ svdm_consume_modes(port, p, cnt);
modep->svid_index++;
if (modep->svid_index < modep->nsvids) {
u16 svid = modep->svids[modep->svid_index];
@@ -1238,15 +1230,18 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
static void tcpm_handle_vdm_request(struct tcpm_port *port,
const __le32 *payload, int cnt)
{
- int rlen = 0;
+ u32 p[PD_MAX_PAYLOAD];
u32 response[8] = { };
- u32 p0 = le32_to_cpu(payload[0]);
+ int i, rlen = 0;
+
+ for (i = 0; i < cnt; i++)
+ p[i] = le32_to_cpu(payload[i]);
if (port->vdm_state == VDM_STATE_BUSY) {
/* If UFP responded busy retry after timeout */
- if (PD_VDO_CMDT(p0) == CMDT_RSP_BUSY) {
+ if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
port->vdm_state = VDM_STATE_WAIT_RSP_BUSY;
- port->vdo_retry = (p0 & ~VDO_CMDT_MASK) |
+ port->vdo_retry = (p[0] & ~VDO_CMDT_MASK) |
CMDT_INIT;
mod_delayed_work(port->wq, &port->vdm_state_machine,
msecs_to_jiffies(PD_T_VDM_BUSY));
@@ -1255,8 +1250,8 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
port->vdm_state = VDM_STATE_DONE;
}
- if (PD_VDO_SVDM(p0))
- rlen = tcpm_pd_svdm(port, payload, cnt, response);
+ if (PD_VDO_SVDM(p[0]))
+ rlen = tcpm_pd_svdm(port, p, cnt, response);
if (rlen > 0)
tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
2020-07-24 17:46 ` [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling Hans de Goede
@ 2020-07-24 17:47 ` Hans de Goede
2020-07-25 14:45 ` Guenter Roeck
2020-07-28 13:06 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers Hans de Goede
` (2 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
Refactor tcpm_handle_vdm_request and its tcpm_pd_svdm helper function so
that reporting the results of the vdm to the altmode-driver is separated
out into a clear separate step inside tcpm_handle_vdm_request, instead
of being scattered over various places inside the tcpm_pd_svdm helper.
This is a preparation patch for fixing an AB BA lock inversion between the
tcpm code and some altmode drivers.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Keep "if (adev && pdev)" checks as is instead of modifying them
---
drivers/usb/typec/tcpm/tcpm.c | 76 ++++++++++++++++++++++-------------
1 file changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index ee239b54bcd8..03a0c083ee9a 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -159,6 +159,14 @@ enum pd_msg_request {
PD_MSG_DATA_SOURCE_CAP,
};
+enum adev_actions {
+ ADEV_NONE = 0,
+ ADEV_NOTIFY_USB_AND_QUEUE_VDM,
+ ADEV_QUEUE_VDM,
+ ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL,
+ ADEV_ATTENTION,
+};
+
/* Events from low level driver */
#define TCPM_CC_EVENT BIT(0)
@@ -1078,10 +1086,10 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
#define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
-static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
- u32 *response)
+static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
+ const u32 *p, int cnt, u32 *response,
+ enum adev_actions *adev_action)
{
- struct typec_altmode *adev;
struct typec_altmode *pdev;
struct pd_mode_data *modep;
int rlen = 0;
@@ -1097,9 +1105,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
modep = &port->mode_data;
- adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
- PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
-
pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
@@ -1125,8 +1130,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
break;
case CMD_ATTENTION:
/* Attention command does not have response */
- if (adev)
- typec_altmode_attention(adev, p[1]);
+ *adev_action = ADEV_ATTENTION;
return 0;
default:
break;
@@ -1180,23 +1184,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
case CMD_ENTER_MODE:
if (adev && pdev) {
typec_altmode_update_active(pdev, true);
-
- if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
- response[0] = VDO(adev->svid, 1,
- CMD_EXIT_MODE);
- response[0] |= VDO_OPOS(adev->mode);
- return 1;
- }
+ *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
}
return 0;
case CMD_EXIT_MODE:
if (adev && pdev) {
typec_altmode_update_active(pdev, false);
-
/* Back to USB Operation */
- WARN_ON(typec_altmode_notify(adev,
- TYPEC_STATE_USB,
- NULL));
+ *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
+ return 0;
}
break;
default:
@@ -1207,11 +1203,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
switch (cmd) {
case CMD_ENTER_MODE:
/* Back to USB Operation */
- if (adev)
- WARN_ON(typec_altmode_notify(adev,
- TYPEC_STATE_USB,
- NULL));
- break;
+ *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
+ return 0;
default:
break;
}
@@ -1221,15 +1214,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
}
/* Informing the alternate mode drivers about everything */
- if (adev)
- typec_altmode_vdm(adev, p[0], &p[1], cnt);
-
+ *adev_action = ADEV_QUEUE_VDM;
return rlen;
}
static void tcpm_handle_vdm_request(struct tcpm_port *port,
const __le32 *payload, int cnt)
{
+ enum adev_actions adev_action = ADEV_NONE;
+ struct typec_altmode *adev;
u32 p[PD_MAX_PAYLOAD];
u32 response[8] = { };
int i, rlen = 0;
@@ -1237,6 +1230,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
for (i = 0; i < cnt; i++)
p[i] = le32_to_cpu(payload[i]);
+ adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
+ PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
+
if (port->vdm_state == VDM_STATE_BUSY) {
/* If UFP responded busy retry after timeout */
if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
@@ -1251,7 +1247,31 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
}
if (PD_VDO_SVDM(p[0]))
- rlen = tcpm_pd_svdm(port, p, cnt, response);
+ rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
+
+ if (adev) {
+ switch (adev_action) {
+ case ADEV_NONE:
+ break;
+ case ADEV_NOTIFY_USB_AND_QUEUE_VDM:
+ WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, NULL));
+ typec_altmode_vdm(adev, p[0], &p[1], cnt);
+ break;
+ case ADEV_QUEUE_VDM:
+ typec_altmode_vdm(adev, p[0], &p[1], cnt);
+ break;
+ case ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL:
+ if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
+ response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
+ response[0] |= VDO_OPOS(adev->mode);
+ rlen = 1;
+ }
+ break;
+ case ADEV_ATTENTION:
+ typec_altmode_attention(adev, p[1]);
+ break;
+ }
+ }
if (rlen > 0)
tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
` (2 preceding siblings ...)
2020-07-24 17:47 ` [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request Hans de Goede
@ 2020-07-24 17:47 ` Hans de Goede
2020-07-25 14:46 ` Guenter Roeck
2020-07-28 13:08 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
2020-07-28 12:43 ` [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Heikki Krogerus
5 siblings, 2 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
When we receive a PD data packet which ends up being for the alt-mode
driver we have the following lock order:
1. tcpm_pd_rx_handler take the tcpm-port lock
2. We call into the alt-mode driver which takes the alt-mode's lock
And when the alt-mode driver initiates communication we have the following
lock order:
3. alt-mode driver takes the alt-mode's lock
4. alt-mode driver calls tcpm_altmode_enter which takes the tcpm-port lock
This is a classic AB BA lock inversion issue.
With the refactoring of tcpm_handle_vdm_request() done before this patch,
we don't rely on, or need to make changes to the tcpm-port data by the
time we make call 2. from above. All data to be passed to the alt-mode
driver sits on our stack at this point, and thus does not need locking.
So after the refactoring we can simply fix this by releasing the
tcpm-port lock before calling into the alt-mode driver.
This fixes the following lockdep warning:
[ 191.454238] ======================================================
[ 191.454240] WARNING: possible circular locking dependency detected
[ 191.454244] 5.8.0-rc5+ #1 Not tainted
[ 191.454246] ------------------------------------------------------
[ 191.454248] kworker/u8:5/794 is trying to acquire lock:
[ 191.454251] ffff9bac8e30d4a8 (&dp->lock){+.+.}-{3:3}, at: dp_altmode_vdm+0x30/0xf0 [typec_displayport]
[ 191.454263]
but task is already holding lock:
[ 191.454264] ffff9bac9dc240a0 (&port->lock#2){+.+.}-{3:3}, at: tcpm_pd_rx_handler+0x43/0x12c0 [tcpm]
[ 191.454273]
which lock already depends on the new lock.
[ 191.454275]
the existing dependency chain (in reverse order) is:
[ 191.454277]
-> #1 (&port->lock#2){+.+.}-{3:3}:
[ 191.454286] __mutex_lock+0x7b/0x820
[ 191.454290] tcpm_altmode_enter+0x23/0x90 [tcpm]
[ 191.454293] dp_altmode_work+0xca/0xe0 [typec_displayport]
[ 191.454299] process_one_work+0x23f/0x570
[ 191.454302] worker_thread+0x55/0x3c0
[ 191.454305] kthread+0x138/0x160
[ 191.454309] ret_from_fork+0x22/0x30
[ 191.454311]
-> #0 (&dp->lock){+.+.}-{3:3}:
[ 191.454317] __lock_acquire+0x1241/0x2090
[ 191.454320] lock_acquire+0xa4/0x3d0
[ 191.454323] __mutex_lock+0x7b/0x820
[ 191.454326] dp_altmode_vdm+0x30/0xf0 [typec_displayport]
[ 191.454330] tcpm_pd_rx_handler+0x11ae/0x12c0 [tcpm]
[ 191.454333] process_one_work+0x23f/0x570
[ 191.454336] worker_thread+0x55/0x3c0
[ 191.454338] kthread+0x138/0x160
[ 191.454341] ret_from_fork+0x22/0x30
[ 191.454343]
other info that might help us debug this:
[ 191.454345] Possible unsafe locking scenario:
[ 191.454347] CPU0 CPU1
[ 191.454348] ---- ----
[ 191.454350] lock(&port->lock#2);
[ 191.454353] lock(&dp->lock);
[ 191.454355] lock(&port->lock#2);
[ 191.454357] lock(&dp->lock);
[ 191.454360]
*** DEADLOCK ***
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Move the mutex_lock call to above the tcpm_queue_vdm() call, so that
we can use the regular tcpm_queue_vdm() instead of having to call
tcpm_queue_vdm_unlocked()
---
drivers/usb/typec/tcpm/tcpm.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 03a0c083ee9a..9b26b57a0172 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1249,6 +1249,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
if (PD_VDO_SVDM(p[0]))
rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
+ /*
+ * We are done with any state stored in the port struct now, except
+ * for any port struct changes done by the tcpm_queue_vdm() call
+ * below, which is a separate operation.
+ *
+ * So we can safely release the lock here; and we MUST release the
+ * lock here to avoid an AB BA lock inversion:
+ *
+ * If we keep the lock here then the lock ordering in this path is:
+ * 1. tcpm_pd_rx_handler take the tcpm port lock
+ * 2. One of the typec_altmode_* calls below takes the alt-mode's lock
+ *
+ * And we also have this ordering:
+ * 1. alt-mode driver takes the alt-mode's lock
+ * 2. alt-mode driver calls tcpm_altmode_enter which takes the
+ * tcpm port lock
+ *
+ * Dropping our lock here avoids this.
+ */
+ mutex_unlock(&port->lock);
+
if (adev) {
switch (adev_action) {
case ADEV_NONE:
@@ -1273,6 +1294,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
}
}
+ /*
+ * We must re-take the lock here to balance the unlock in
+ * tcpm_pd_rx_handler, note that no changes, other then the
+ * tcpm_queue_vdm call, are made while the lock is held again.
+ * All that is done after the call is unwinding the call stack until
+ * we return to tcpm_pd_rx_handler and do the unlock there.
+ */
+ mutex_lock(&port->lock);
+
if (rlen > 0)
tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
` (3 preceding siblings ...)
2020-07-24 17:47 ` [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers Hans de Goede
@ 2020-07-24 17:47 ` Hans de Goede
2020-07-24 17:57 ` Sergei Shtylyov
` (2 more replies)
2020-07-28 12:43 ` [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Heikki Krogerus
5 siblings, 3 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 17:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: Hans de Goede, linux-usb
The tcpm.c code for sending VDMs assumes that there will only be one VDM
in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry
deep.
This assumes that the higher layers (tcpm state-machine and alt-mode
drivers) ensure that queuing a new VDM before the old one has been
completely send (or it timed out) add a WARN_ON to check for this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Fix typo in commit-msg subject
---
drivers/usb/typec/tcpm/tcpm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 9b26b57a0172..cc786d558f14 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
{
WARN_ON(!mutex_is_locked(&port->lock));
+ /* Make sure we are not still processing a previous VDM packet */
+ WARN_ON(port->vdm_state > VDM_STATE_DONE);
+
port->vdo_count = cnt + 1;
port->vdo_data[0] = header;
memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
@ 2020-07-24 17:57 ` Sergei Shtylyov
2020-07-24 19:09 ` Hans de Goede
2020-07-25 14:53 ` Guenter Roeck
2020-07-28 13:10 ` Heikki Krogerus
2 siblings, 1 reply; 20+ messages in thread
From: Sergei Shtylyov @ 2020-07-24 17:57 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: linux-usb
Hello.
On 7/24/20 8:47 PM, Hans de Goede wrote:
> The tcpm.c code for sending VDMs assumes that there will only be one VDM
> in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry
> deep.
>
> This assumes that the higher layers (tcpm state-machine and alt-mode
> drivers) ensure that queuing a new VDM before the old one has been
> completely send (or it timed out) add a WARN_ON to check for this.
Sent?
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Fix typo in commit-msg subject
Another typo? :-)
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time
2020-07-24 17:57 ` Sergei Shtylyov
@ 2020-07-24 19:09 ` Hans de Goede
0 siblings, 0 replies; 20+ messages in thread
From: Hans de Goede @ 2020-07-24 19:09 UTC (permalink / raw)
To: Sergei Shtylyov, Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus
Cc: linux-usb
Hi,
On 7/24/20 7:57 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 7/24/20 8:47 PM, Hans de Goede wrote:
>
>> The tcpm.c code for sending VDMs assumes that there will only be one VDM
>> in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry
>> deep.
>>
>> This assumes that the higher layers (tcpm state-machine and alt-mode
>> drivers) ensure that queuing a new VDM before the old one has been
>> completely send (or it timed out) add a WARN_ON to check for this.
>
> Sent?
The dictionary says "has been sending" and gives no "has been sen[d|t]"
answer. I guess you might be right. Anyways not worth respinning
the series for IMHO.
Regards,
Hans
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Fix typo in commit-msg subject
>
> Another typo? :-)
>
> [...]
>
> MBR, Sergei
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
@ 2020-07-25 14:37 ` Guenter Roeck
2020-07-28 12:44 ` Heikki Krogerus
1 sibling, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2020-07-25 14:37 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
On 7/24/20 10:46 AM, Hans de Goede wrote:
> Various callers (all the typec_altmode_ops) take the port-lock just for
> the tcpm_queue_vdm() call.
>
> Add a new tcpm_queue_vdm_unlocked() helper which takes the lock, so that
> its callers don't have to do this themselves.
>
> This is a preparation patch for fixing an AB BA lock inversion between
> the tcpm code and some altmode drivers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes in v2:
> - Name the new helper tcpm_queue_vdm_unlocked() instead of renaming the
> original function to this
> ---
> drivers/usb/typec/tcpm/tcpm.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc6455a29c74..862c474b3ebd 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -961,6 +961,8 @@ static void tcpm_queue_message(struct tcpm_port *port,
> static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> const u32 *data, int cnt)
> {
> + WARN_ON(!mutex_is_locked(&port->lock));
> +
> port->vdo_count = cnt + 1;
> port->vdo_data[0] = header;
> memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
> @@ -971,6 +973,14 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> }
>
> +static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
> + const u32 *data, int cnt)
> +{
> + mutex_lock(&port->lock);
> + tcpm_queue_vdm(port, header, data, cnt);
> + mutex_unlock(&port->lock);
> +}
> +
> static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
> int cnt)
> {
> @@ -1506,13 +1516,10 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> u32 header;
>
> - mutex_lock(&port->lock);
> header = VDO(altmode->svid, vdo ? 2 : 1, CMD_ENTER_MODE);
> header |= VDO_OPOS(altmode->mode);
>
> - tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0);
> return 0;
> }
>
> @@ -1521,13 +1528,10 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode)
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> u32 header;
>
> - mutex_lock(&port->lock);
> header = VDO(altmode->svid, 1, CMD_EXIT_MODE);
> header |= VDO_OPOS(altmode->mode);
>
> - tcpm_queue_vdm(port, header, NULL, 0);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, NULL, 0);
> return 0;
> }
>
> @@ -1536,10 +1540,7 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode,
> {
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
>
> - mutex_lock(&port->lock);
> - tcpm_queue_vdm(port, header, data, count - 1);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, data, count - 1);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request
2020-07-24 17:47 ` [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request Hans de Goede
@ 2020-07-25 14:45 ` Guenter Roeck
2020-07-26 10:58 ` Hans de Goede
2020-07-28 13:06 ` Heikki Krogerus
1 sibling, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2020-07-25 14:45 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
On 7/24/20 10:47 AM, Hans de Goede wrote:
> Refactor tcpm_handle_vdm_request and its tcpm_pd_svdm helper function so
> that reporting the results of the vdm to the altmode-driver is separated
> out into a clear separate step inside tcpm_handle_vdm_request, instead
> of being scattered over various places inside the tcpm_pd_svdm helper.
>
> This is a preparation patch for fixing an AB BA lock inversion between the
> tcpm code and some altmode drivers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Keep "if (adev && pdev)" checks as is instead of modifying them
> ---
> drivers/usb/typec/tcpm/tcpm.c | 76 ++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index ee239b54bcd8..03a0c083ee9a 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -159,6 +159,14 @@ enum pd_msg_request {
> PD_MSG_DATA_SOURCE_CAP,
> };
>
> +enum adev_actions {
> + ADEV_NONE = 0,
> + ADEV_NOTIFY_USB_AND_QUEUE_VDM,
> + ADEV_QUEUE_VDM,
> + ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL,
> + ADEV_ATTENTION,
> +};
> +
> /* Events from low level driver */
>
> #define TCPM_CC_EVENT BIT(0)
> @@ -1078,10 +1086,10 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
>
> #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
>
> -static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> - u32 *response)
> +static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
> + const u32 *p, int cnt, u32 *response,
> + enum adev_actions *adev_action)
> {
> - struct typec_altmode *adev;
> struct typec_altmode *pdev;
> struct pd_mode_data *modep;
> int rlen = 0;
> @@ -1097,9 +1105,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>
> modep = &port->mode_data;
>
> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> -
> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>
> @@ -1125,8 +1130,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> break;
> case CMD_ATTENTION:
> /* Attention command does not have response */
> - if (adev)
> - typec_altmode_attention(adev, p[1]);
> + *adev_action = ADEV_ATTENTION;
> return 0;
> default:
> break;
> @@ -1180,23 +1184,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> case CMD_ENTER_MODE:
> if (adev && pdev) {
I must be missing something. Does this compile ? The 'adev' variable was removed above.
Maybe move the call to typec_altmode_update_active() into tcpm_handle_vdm_request()
instead ?
Thanks,
Guenter
> typec_altmode_update_active(pdev, true);
> -
> - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> - response[0] = VDO(adev->svid, 1,
> - CMD_EXIT_MODE);
> - response[0] |= VDO_OPOS(adev->mode);
> - return 1;
> - }
> + *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
> }
> return 0;
> case CMD_EXIT_MODE:
> if (adev && pdev) {
> typec_altmode_update_active(pdev, false);
> -
> /* Back to USB Operation */
> - WARN_ON(typec_altmode_notify(adev,
> - TYPEC_STATE_USB,
> - NULL));
> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> + return 0;
> }
> break;
> default:
> @@ -1207,11 +1203,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> switch (cmd) {
> case CMD_ENTER_MODE:
> /* Back to USB Operation */
> - if (adev)
> - WARN_ON(typec_altmode_notify(adev,
> - TYPEC_STATE_USB,
> - NULL));
> - break;
> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> + return 0;
> default:
> break;
> }
> @@ -1221,15 +1214,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> }
>
> /* Informing the alternate mode drivers about everything */
> - if (adev)
> - typec_altmode_vdm(adev, p[0], &p[1], cnt);
> -
> + *adev_action = ADEV_QUEUE_VDM;
> return rlen;
> }
>
> static void tcpm_handle_vdm_request(struct tcpm_port *port,
> const __le32 *payload, int cnt)
> {
> + enum adev_actions adev_action = ADEV_NONE;
> + struct typec_altmode *adev;
> u32 p[PD_MAX_PAYLOAD];
> u32 response[8] = { };
> int i, rlen = 0;
> @@ -1237,6 +1230,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> for (i = 0; i < cnt; i++)
> p[i] = le32_to_cpu(payload[i]);
>
> + adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> + PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> +
> if (port->vdm_state == VDM_STATE_BUSY) {
> /* If UFP responded busy retry after timeout */
> if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
> @@ -1251,7 +1247,31 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> }
>
> if (PD_VDO_SVDM(p[0]))
> - rlen = tcpm_pd_svdm(port, p, cnt, response);
> + rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
> +
> + if (adev) {
> + switch (adev_action) {
> + case ADEV_NONE:
> + break;
> + case ADEV_NOTIFY_USB_AND_QUEUE_VDM:
> + WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, NULL));
> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
> + break;
> + case ADEV_QUEUE_VDM:
> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
> + break;
> + case ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL:
> + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> + response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
> + response[0] |= VDO_OPOS(adev->mode);
> + rlen = 1;
> + }
> + break;
> + case ADEV_ATTENTION:
> + typec_altmode_attention(adev, p[1]);
> + break;
> + }
> + }
>
> if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers
2020-07-24 17:47 ` [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers Hans de Goede
@ 2020-07-25 14:46 ` Guenter Roeck
2020-07-28 13:08 ` Heikki Krogerus
1 sibling, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2020-07-25 14:46 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
On 7/24/20 10:47 AM, Hans de Goede wrote:
> When we receive a PD data packet which ends up being for the alt-mode
> driver we have the following lock order:
>
> 1. tcpm_pd_rx_handler take the tcpm-port lock
> 2. We call into the alt-mode driver which takes the alt-mode's lock
>
> And when the alt-mode driver initiates communication we have the following
> lock order:
>
> 3. alt-mode driver takes the alt-mode's lock
> 4. alt-mode driver calls tcpm_altmode_enter which takes the tcpm-port lock
>
> This is a classic AB BA lock inversion issue.
>
> With the refactoring of tcpm_handle_vdm_request() done before this patch,
> we don't rely on, or need to make changes to the tcpm-port data by the
> time we make call 2. from above. All data to be passed to the alt-mode
> driver sits on our stack at this point, and thus does not need locking.
>
> So after the refactoring we can simply fix this by releasing the
> tcpm-port lock before calling into the alt-mode driver.
>
> This fixes the following lockdep warning:
>
> [ 191.454238] ======================================================
> [ 191.454240] WARNING: possible circular locking dependency detected
> [ 191.454244] 5.8.0-rc5+ #1 Not tainted
> [ 191.454246] ------------------------------------------------------
> [ 191.454248] kworker/u8:5/794 is trying to acquire lock:
> [ 191.454251] ffff9bac8e30d4a8 (&dp->lock){+.+.}-{3:3}, at: dp_altmode_vdm+0x30/0xf0 [typec_displayport]
> [ 191.454263]
> but task is already holding lock:
> [ 191.454264] ffff9bac9dc240a0 (&port->lock#2){+.+.}-{3:3}, at: tcpm_pd_rx_handler+0x43/0x12c0 [tcpm]
> [ 191.454273]
> which lock already depends on the new lock.
>
> [ 191.454275]
> the existing dependency chain (in reverse order) is:
> [ 191.454277]
> -> #1 (&port->lock#2){+.+.}-{3:3}:
> [ 191.454286] __mutex_lock+0x7b/0x820
> [ 191.454290] tcpm_altmode_enter+0x23/0x90 [tcpm]
> [ 191.454293] dp_altmode_work+0xca/0xe0 [typec_displayport]
> [ 191.454299] process_one_work+0x23f/0x570
> [ 191.454302] worker_thread+0x55/0x3c0
> [ 191.454305] kthread+0x138/0x160
> [ 191.454309] ret_from_fork+0x22/0x30
> [ 191.454311]
> -> #0 (&dp->lock){+.+.}-{3:3}:
> [ 191.454317] __lock_acquire+0x1241/0x2090
> [ 191.454320] lock_acquire+0xa4/0x3d0
> [ 191.454323] __mutex_lock+0x7b/0x820
> [ 191.454326] dp_altmode_vdm+0x30/0xf0 [typec_displayport]
> [ 191.454330] tcpm_pd_rx_handler+0x11ae/0x12c0 [tcpm]
> [ 191.454333] process_one_work+0x23f/0x570
> [ 191.454336] worker_thread+0x55/0x3c0
> [ 191.454338] kthread+0x138/0x160
> [ 191.454341] ret_from_fork+0x22/0x30
> [ 191.454343]
> other info that might help us debug this:
>
> [ 191.454345] Possible unsafe locking scenario:
>
> [ 191.454347] CPU0 CPU1
> [ 191.454348] ---- ----
> [ 191.454350] lock(&port->lock#2);
> [ 191.454353] lock(&dp->lock);
> [ 191.454355] lock(&port->lock#2);
> [ 191.454357] lock(&dp->lock);
> [ 191.454360]
> *** DEADLOCK ***
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes in v2:
> -Move the mutex_lock call to above the tcpm_queue_vdm() call, so that
> we can use the regular tcpm_queue_vdm() instead of having to call
> tcpm_queue_vdm_unlocked()
> ---
> drivers/usb/typec/tcpm/tcpm.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03a0c083ee9a..9b26b57a0172 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1249,6 +1249,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> if (PD_VDO_SVDM(p[0]))
> rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
>
> + /*
> + * We are done with any state stored in the port struct now, except
> + * for any port struct changes done by the tcpm_queue_vdm() call
> + * below, which is a separate operation.
> + *
> + * So we can safely release the lock here; and we MUST release the
> + * lock here to avoid an AB BA lock inversion:
> + *
> + * If we keep the lock here then the lock ordering in this path is:
> + * 1. tcpm_pd_rx_handler take the tcpm port lock
> + * 2. One of the typec_altmode_* calls below takes the alt-mode's lock
> + *
> + * And we also have this ordering:
> + * 1. alt-mode driver takes the alt-mode's lock
> + * 2. alt-mode driver calls tcpm_altmode_enter which takes the
> + * tcpm port lock
> + *
> + * Dropping our lock here avoids this.
> + */
> + mutex_unlock(&port->lock);
> +
> if (adev) {
> switch (adev_action) {
> case ADEV_NONE:
> @@ -1273,6 +1294,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> }
> }
>
> + /*
> + * We must re-take the lock here to balance the unlock in
> + * tcpm_pd_rx_handler, note that no changes, other then the
> + * tcpm_queue_vdm call, are made while the lock is held again.
> + * All that is done after the call is unwinding the call stack until
> + * we return to tcpm_pd_rx_handler and do the unlock there.
> + */
> + mutex_lock(&port->lock);
> +
> if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
2020-07-24 17:57 ` Sergei Shtylyov
@ 2020-07-25 14:53 ` Guenter Roeck
2020-07-28 13:10 ` Heikki Krogerus
2 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2020-07-25 14:53 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
On 7/24/20 10:47 AM, Hans de Goede wrote:
> The tcpm.c code for sending VDMs assumes that there will only be one VDM
> in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry
> deep.
>
> This assumes that the higher layers (tcpm state-machine and alt-mode
> drivers) ensure that queuing a new VDM before the old one has been
> completely send (or it timed out) add a WARN_ON to check for this.
Just in case you resend ... "has been sent" is grammatically correct.
The feedback on
https://www.quora.com/What-is-the-difference-between-send-and-sent
explains it in detail.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Thanks,
Guenter
> ---
> Changes in v2:
> - Fix typo in commit-msg subject
> ---
> drivers/usb/typec/tcpm/tcpm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 9b26b57a0172..cc786d558f14 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> {
> WARN_ON(!mutex_is_locked(&port->lock));
>
> + /* Make sure we are not still processing a previous VDM packet */
> + WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +
> port->vdo_count = cnt + 1;
> port->vdo_data[0] = header;
> memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request
2020-07-25 14:45 ` Guenter Roeck
@ 2020-07-26 10:58 ` Hans de Goede
2020-07-26 13:22 ` Guenter Roeck
0 siblings, 1 reply; 20+ messages in thread
From: Hans de Goede @ 2020-07-26 10:58 UTC (permalink / raw)
To: Guenter Roeck, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
Hi,
On 7/25/20 4:45 PM, Guenter Roeck wrote:
> On 7/24/20 10:47 AM, Hans de Goede wrote:
>> Refactor tcpm_handle_vdm_request and its tcpm_pd_svdm helper function so
>> that reporting the results of the vdm to the altmode-driver is separated
>> out into a clear separate step inside tcpm_handle_vdm_request, instead
>> of being scattered over various places inside the tcpm_pd_svdm helper.
>>
>> This is a preparation patch for fixing an AB BA lock inversion between the
>> tcpm code and some altmode drivers.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Keep "if (adev && pdev)" checks as is instead of modifying them
>> ---
>> drivers/usb/typec/tcpm/tcpm.c | 76 ++++++++++++++++++++++-------------
>> 1 file changed, 48 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index ee239b54bcd8..03a0c083ee9a 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -159,6 +159,14 @@ enum pd_msg_request {
>> PD_MSG_DATA_SOURCE_CAP,
>> };
>>
>> +enum adev_actions {
>> + ADEV_NONE = 0,
>> + ADEV_NOTIFY_USB_AND_QUEUE_VDM,
>> + ADEV_QUEUE_VDM,
>> + ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL,
>> + ADEV_ATTENTION,
>> +};
>> +
>> /* Events from low level driver */
>>
>> #define TCPM_CC_EVENT BIT(0)
>> @@ -1078,10 +1086,10 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
>>
>> #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
>>
>> -static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>> - u32 *response)
>> +static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>> + const u32 *p, int cnt, u32 *response,
>> + enum adev_actions *adev_action)
>> {
>> - struct typec_altmode *adev;
>> struct typec_altmode *pdev;
>> struct pd_mode_data *modep;
>> int rlen = 0;
>> @@ -1097,9 +1105,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>
>> modep = &port->mode_data;
>>
>> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>> -
>> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
>> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>
>> @@ -1125,8 +1130,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>> break;
>> case CMD_ATTENTION:
>> /* Attention command does not have response */
>> - if (adev)
>> - typec_altmode_attention(adev, p[1]);
>> + *adev_action = ADEV_ATTENTION;
>> return 0;
>> default:
>> break;
>> @@ -1180,23 +1184,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>> case CMD_ENTER_MODE:
>> if (adev && pdev) {
>
> I must be missing something. Does this compile ? The 'adev' variable was removed above.
> Maybe move the call to typec_altmode_update_active() into tcpm_handle_vdm_request()
> instead ?
Yes it compiles. The adev variable is now a function parameter, since
we already to the lookup for it in tcpm_handle_vdm_request() now,
I'm simply passing it along here.
Regards,
Hans
>> typec_altmode_update_active(pdev, true);
>> -
>> - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
>> - response[0] = VDO(adev->svid, 1,
>> - CMD_EXIT_MODE);
>> - response[0] |= VDO_OPOS(adev->mode);
>> - return 1;
>> - }
>> + *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
>> }
>> return 0;
>> case CMD_EXIT_MODE:
>> if (adev && pdev) {
>> typec_altmode_update_active(pdev, false);
>> -
>> /* Back to USB Operation */
>> - WARN_ON(typec_altmode_notify(adev,
>> - TYPEC_STATE_USB,
>> - NULL));
>> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>> + return 0;
>> }
>> break;
>> default:
>> @@ -1207,11 +1203,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>> switch (cmd) {
>> case CMD_ENTER_MODE:
>> /* Back to USB Operation */
>> - if (adev)
>> - WARN_ON(typec_altmode_notify(adev,
>> - TYPEC_STATE_USB,
>> - NULL));
>> - break;
>> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>> + return 0;
>> default:
>> break;
>> }
>> @@ -1221,15 +1214,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>> }
>>
>> /* Informing the alternate mode drivers about everything */
>> - if (adev)
>> - typec_altmode_vdm(adev, p[0], &p[1], cnt);
>> -
>> + *adev_action = ADEV_QUEUE_VDM;
>> return rlen;
>> }
>>
>> static void tcpm_handle_vdm_request(struct tcpm_port *port,
>> const __le32 *payload, int cnt)
>> {
>> + enum adev_actions adev_action = ADEV_NONE;
>> + struct typec_altmode *adev;
>> u32 p[PD_MAX_PAYLOAD];
>> u32 response[8] = { };
>> int i, rlen = 0;
>> @@ -1237,6 +1230,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>> for (i = 0; i < cnt; i++)
>> p[i] = le32_to_cpu(payload[i]);
>>
>> + adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>> + PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>> +
>> if (port->vdm_state == VDM_STATE_BUSY) {
>> /* If UFP responded busy retry after timeout */
>> if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
>> @@ -1251,7 +1247,31 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>> }
>>
>> if (PD_VDO_SVDM(p[0]))
>> - rlen = tcpm_pd_svdm(port, p, cnt, response);
>> + rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
>> +
>> + if (adev) {
>> + switch (adev_action) {
>> + case ADEV_NONE:
>> + break;
>> + case ADEV_NOTIFY_USB_AND_QUEUE_VDM:
>> + WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, NULL));
>> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
>> + break;
>> + case ADEV_QUEUE_VDM:
>> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
>> + break;
>> + case ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL:
>> + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
>> + response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
>> + response[0] |= VDO_OPOS(adev->mode);
>> + rlen = 1;
>> + }
>> + break;
>> + case ADEV_ATTENTION:
>> + typec_altmode_attention(adev, p[1]);
>> + break;
>> + }
>> + }
>>
>> if (rlen > 0)
>> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request
2020-07-26 10:58 ` Hans de Goede
@ 2020-07-26 13:22 ` Guenter Roeck
0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2020-07-26 13:22 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus; +Cc: linux-usb
On 7/26/20 3:58 AM, Hans de Goede wrote:
> Hi,
>
> On 7/25/20 4:45 PM, Guenter Roeck wrote:
>> On 7/24/20 10:47 AM, Hans de Goede wrote:
>>> Refactor tcpm_handle_vdm_request and its tcpm_pd_svdm helper function so
>>> that reporting the results of the vdm to the altmode-driver is separated
>>> out into a clear separate step inside tcpm_handle_vdm_request, instead
>>> of being scattered over various places inside the tcpm_pd_svdm helper.
>>>
>>> This is a preparation patch for fixing an AB BA lock inversion between the
>>> tcpm code and some altmode drivers.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> - Keep "if (adev && pdev)" checks as is instead of modifying them
>>> ---
>>> drivers/usb/typec/tcpm/tcpm.c | 76 ++++++++++++++++++++++-------------
>>> 1 file changed, 48 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index ee239b54bcd8..03a0c083ee9a 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -159,6 +159,14 @@ enum pd_msg_request {
>>> PD_MSG_DATA_SOURCE_CAP,
>>> };
>>> +enum adev_actions {
>>> + ADEV_NONE = 0,
>>> + ADEV_NOTIFY_USB_AND_QUEUE_VDM,
>>> + ADEV_QUEUE_VDM,
>>> + ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL,
>>> + ADEV_ATTENTION,
>>> +};
>>> +
>>> /* Events from low level driver */
>>> #define TCPM_CC_EVENT BIT(0)
>>> @@ -1078,10 +1086,10 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
>>> #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
>>> -static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> - u32 *response)
>>> +static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>>> + const u32 *p, int cnt, u32 *response,
>>> + enum adev_actions *adev_action)
>>> {
>>> - struct typec_altmode *adev;
>>> struct typec_altmode *pdev;
>>> struct pd_mode_data *modep;
>>> int rlen = 0;
>>> @@ -1097,9 +1105,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> modep = &port->mode_data;
>>> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>>> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>> -
>>> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
>>> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>> @@ -1125,8 +1130,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> break;
>>> case CMD_ATTENTION:
>>> /* Attention command does not have response */
>>> - if (adev)
>>> - typec_altmode_attention(adev, p[1]);
>>> + *adev_action = ADEV_ATTENTION;
>>> return 0;
>>> default:
>>> break;
>>> @@ -1180,23 +1184,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> case CMD_ENTER_MODE:
>>> if (adev && pdev) {
>>
>> I must be missing something. Does this compile ? The 'adev' variable was removed above.
>> Maybe move the call to typec_altmode_update_active() into tcpm_handle_vdm_request()
>> instead ?
>
> Yes it compiles. The adev variable is now a function parameter, since
> we already to the lookup for it in tcpm_handle_vdm_request() now,
> I'm simply passing it along here.
>
Thanks for the clarification, and sorry I missed that.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Regards,
>
> Hans
>
>
>
>>> typec_altmode_update_active(pdev, true);
>>> -
>>> - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
>>> - response[0] = VDO(adev->svid, 1,
>>> - CMD_EXIT_MODE);
>>> - response[0] |= VDO_OPOS(adev->mode);
>>> - return 1;
>>> - }
>>> + *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
>>> }
>>> return 0;
>>> case CMD_EXIT_MODE:
>>> if (adev && pdev) {
>>> typec_altmode_update_active(pdev, false);
>>> -
>>> /* Back to USB Operation */
>>> - WARN_ON(typec_altmode_notify(adev,
>>> - TYPEC_STATE_USB,
>>> - NULL));
>>> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>>> + return 0;
>>> }
>>> break;
>>> default:
>>> @@ -1207,11 +1203,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> switch (cmd) {
>>> case CMD_ENTER_MODE:
>>> /* Back to USB Operation */
>>> - if (adev)
>>> - WARN_ON(typec_altmode_notify(adev,
>>> - TYPEC_STATE_USB,
>>> - NULL));
>>> - break;
>>> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>>> + return 0;
>>> default:
>>> break;
>>> }
>>> @@ -1221,15 +1214,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>> }
>>> /* Informing the alternate mode drivers about everything */
>>> - if (adev)
>>> - typec_altmode_vdm(adev, p[0], &p[1], cnt);
>>> -
>>> + *adev_action = ADEV_QUEUE_VDM;
>>> return rlen;
>>> }
>>> static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>> const __le32 *payload, int cnt)
>>> {
>>> + enum adev_actions adev_action = ADEV_NONE;
>>> + struct typec_altmode *adev;
>>> u32 p[PD_MAX_PAYLOAD];
>>> u32 response[8] = { };
>>> int i, rlen = 0;
>>> @@ -1237,6 +1230,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>> for (i = 0; i < cnt; i++)
>>> p[i] = le32_to_cpu(payload[i]);
>>> + adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
>>> + PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>>> +
>>> if (port->vdm_state == VDM_STATE_BUSY) {
>>> /* If UFP responded busy retry after timeout */
>>> if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
>>> @@ -1251,7 +1247,31 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>> }
>>> if (PD_VDO_SVDM(p[0]))
>>> - rlen = tcpm_pd_svdm(port, p, cnt, response);
>>> + rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
>>> +
>>> + if (adev) {
>>> + switch (adev_action) {
>>> + case ADEV_NONE:
>>> + break;
>>> + case ADEV_NOTIFY_USB_AND_QUEUE_VDM:
>>> + WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, NULL));
>>> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
>>> + break;
>>> + case ADEV_QUEUE_VDM:
>>> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
>>> + break;
>>> + case ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL:
>>> + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
>>> + response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
>>> + response[0] |= VDO_OPOS(adev->mode);
>>> + rlen = 1;
>>> + }
>>> + break;
>>> + case ADEV_ATTENTION:
>>> + typec_altmode_attention(adev, p[1]);
>>> + break;
>>> + }
>>> + }
>>> if (rlen > 0)
>>> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
>>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm()
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
` (4 preceding siblings ...)
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
@ 2020-07-28 12:43 ` Heikki Krogerus
5 siblings, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 12:43 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:46:57PM +0200, Hans de Goede wrote:
> All callers of tcpm_queue_vdm() immediately follow the tcpm_queue_vdm()
> vdm call with a:
>
> mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
>
> Call, fold this into tcpm_queue_vdm() itself.
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
FWIW:
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index b4a66e6bf68c..fc6455a29c74 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -967,6 +967,8 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> /* Set ready, vdm state machine will actually send */
> port->vdm_retries = 0;
> port->vdm_state = VDM_STATE_READY;
> +
> + mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> }
>
> static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
> @@ -1246,10 +1248,8 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> if (PD_VDO_SVDM(p0))
> rlen = tcpm_pd_svdm(port, payload, cnt, response);
>
> - if (rlen > 0) {
> + if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> - mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> - }
> }
>
> static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
> @@ -1264,8 +1264,6 @@ static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
> header = VDO(vid, ((vid & USB_SID_PD) == USB_SID_PD) ?
> 1 : (PD_VDO_CMD(cmd) <= CMD_ATTENTION), cmd);
> tcpm_queue_vdm(port, header, data, count);
> -
> - mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> }
>
> static unsigned int vdm_ready_timeout(u32 vdm_hdr)
> @@ -1513,7 +1511,6 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
> header |= VDO_OPOS(altmode->mode);
>
> tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
> - mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> mutex_unlock(&port->lock);
>
> return 0;
> @@ -1529,7 +1526,6 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode)
> header |= VDO_OPOS(altmode->mode);
>
> tcpm_queue_vdm(port, header, NULL, 0);
> - mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> mutex_unlock(&port->lock);
>
> return 0;
> @@ -1542,7 +1538,6 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode,
>
> mutex_lock(&port->lock);
> tcpm_queue_vdm(port, header, data, count - 1);
> - mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> mutex_unlock(&port->lock);
>
> return 0;
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
2020-07-25 14:37 ` Guenter Roeck
@ 2020-07-28 12:44 ` Heikki Krogerus
1 sibling, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 12:44 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:46:58PM +0200, Hans de Goede wrote:
> Various callers (all the typec_altmode_ops) take the port-lock just for
> the tcpm_queue_vdm() call.
>
> Add a new tcpm_queue_vdm_unlocked() helper which takes the lock, so that
> its callers don't have to do this themselves.
>
> This is a preparation patch for fixing an AB BA lock inversion between
> the tcpm code and some altmode drivers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v2:
> - Name the new helper tcpm_queue_vdm_unlocked() instead of renaming the
> original function to this
> ---
> drivers/usb/typec/tcpm/tcpm.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc6455a29c74..862c474b3ebd 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -961,6 +961,8 @@ static void tcpm_queue_message(struct tcpm_port *port,
> static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> const u32 *data, int cnt)
> {
> + WARN_ON(!mutex_is_locked(&port->lock));
> +
> port->vdo_count = cnt + 1;
> port->vdo_data[0] = header;
> memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
> @@ -971,6 +973,14 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> }
>
> +static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
> + const u32 *data, int cnt)
> +{
> + mutex_lock(&port->lock);
> + tcpm_queue_vdm(port, header, data, cnt);
> + mutex_unlock(&port->lock);
> +}
> +
> static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
> int cnt)
> {
> @@ -1506,13 +1516,10 @@ static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> u32 header;
>
> - mutex_lock(&port->lock);
> header = VDO(altmode->svid, vdo ? 2 : 1, CMD_ENTER_MODE);
> header |= VDO_OPOS(altmode->mode);
>
> - tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, vdo, vdo ? 1 : 0);
> return 0;
> }
>
> @@ -1521,13 +1528,10 @@ static int tcpm_altmode_exit(struct typec_altmode *altmode)
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> u32 header;
>
> - mutex_lock(&port->lock);
> header = VDO(altmode->svid, 1, CMD_EXIT_MODE);
> header |= VDO_OPOS(altmode->mode);
>
> - tcpm_queue_vdm(port, header, NULL, 0);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, NULL, 0);
> return 0;
> }
>
> @@ -1536,10 +1540,7 @@ static int tcpm_altmode_vdm(struct typec_altmode *altmode,
> {
> struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
>
> - mutex_lock(&port->lock);
> - tcpm_queue_vdm(port, header, data, count - 1);
> - mutex_unlock(&port->lock);
> -
> + tcpm_queue_vdm_unlocked(port, header, data, count - 1);
> return 0;
> }
>
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling
2020-07-24 17:46 ` [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling Hans de Goede
@ 2020-07-28 12:59 ` Heikki Krogerus
0 siblings, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 12:59 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:46:59PM +0200, Hans de Goede wrote:
> Refactor the tcpm_handle_vdm_request payload handling by doing the
> endianness conversion only once directly inside tcpm_handle_vdm_request
> itself instead of doing it multiple times inside various helper functions
> called by tcpm_handle_vdm_request.
>
> This is a preparation patch for some further refactoring to fix an AB BA
> lock inversion between the tcpm code and some altmode drivers.
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
One nit below, but nothing that would require a new version, so FWIW:
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 49 ++++++++++++++++-------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 862c474b3ebd..ee239b54bcd8 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -981,16 +981,15 @@ static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
> mutex_unlock(&port->lock);
> }
>
> -static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
> - int cnt)
> +static void svdm_consume_identity(struct tcpm_port *port, const u32 *p, int cnt)
> {
> - u32 vdo = le32_to_cpu(payload[VDO_INDEX_IDH]);
> - u32 product = le32_to_cpu(payload[VDO_INDEX_PRODUCT]);
> + u32 vdo = p[VDO_INDEX_IDH];
> + u32 product = p[VDO_INDEX_PRODUCT];
>
> memset(&port->mode_data, 0, sizeof(port->mode_data));
>
> port->partner_ident.id_header = vdo;
> - port->partner_ident.cert_stat = le32_to_cpu(payload[VDO_INDEX_CSTAT]);
> + port->partner_ident.cert_stat = p[VDO_INDEX_CSTAT];
> port->partner_ident.product = product;
>
> typec_partner_set_identity(port->partner);
> @@ -1000,17 +999,15 @@ static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
> PD_PRODUCT_PID(product), product & 0xffff);
> }
>
> -static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
> - int cnt)
> +static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
> {
> struct pd_mode_data *pmdata = &port->mode_data;
> int i;
>
> for (i = 1; i < cnt; i++) {
> - u32 p = le32_to_cpu(payload[i]);
> u16 svid;
>
> - svid = (p >> 16) & 0xffff;
> + svid = (p[i] >> 16) & 0xffff;
> if (!svid)
> return false;
>
> @@ -1020,7 +1017,7 @@ static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
> pmdata->svids[pmdata->nsvids++] = svid;
> tcpm_log(port, "SVID %d: 0x%x", pmdata->nsvids, svid);
>
> - svid = p & 0xffff;
> + svid = p[i] & 0xffff;
> if (!svid)
> return false;
>
> @@ -1036,8 +1033,7 @@ static bool svdm_consume_svids(struct tcpm_port *port, const __le32 *payload,
> return false;
> }
>
> -static void svdm_consume_modes(struct tcpm_port *port, const __le32 *payload,
> - int cnt)
> +static void svdm_consume_modes(struct tcpm_port *port, const u32 *p, int cnt)
> {
> struct pd_mode_data *pmdata = &port->mode_data;
> struct typec_altmode_desc *paltmode;
> @@ -1054,7 +1050,7 @@ static void svdm_consume_modes(struct tcpm_port *port, const __le32 *payload,
>
> paltmode->svid = pmdata->svids[pmdata->svid_index];
> paltmode->mode = i;
> - paltmode->vdo = le32_to_cpu(payload[i]);
> + paltmode->vdo = p[i];
>
> tcpm_log(port, " Alternate mode %d: SVID 0x%04x, VDO %d: 0x%08x",
> pmdata->altmodes, paltmode->svid,
> @@ -1082,21 +1078,17 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
>
> #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
>
> -static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> +static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> u32 *response)
> {
> struct typec_altmode *adev;
> struct typec_altmode *pdev;
> struct pd_mode_data *modep;
> - u32 p[PD_MAX_PAYLOAD];
> int rlen = 0;
> int cmd_type;
> int cmd;
> int i;
>
> - for (i = 0; i < cnt; i++)
> - p[i] = le32_to_cpu(payload[i]);
> -
> cmd_type = PD_VDO_CMDT(p[0]);
> cmd = PD_VDO_CMD(p[0]);
>
> @@ -1157,13 +1149,13 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> switch (cmd) {
> case CMD_DISCOVER_IDENT:
> /* 6.4.4.3.1 */
> - svdm_consume_identity(port, payload, cnt);
> + svdm_consume_identity(port, p, cnt);
> response[0] = VDO(USB_SID_PD, 1, CMD_DISCOVER_SVID);
> rlen = 1;
> break;
> case CMD_DISCOVER_SVID:
> /* 6.4.4.3.2 */
> - if (svdm_consume_svids(port, payload, cnt)) {
> + if (svdm_consume_svids(port, p, cnt)) {
> response[0] = VDO(USB_SID_PD, 1,
> CMD_DISCOVER_SVID);
> rlen = 1;
> @@ -1175,7 +1167,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> break;
> case CMD_DISCOVER_MODES:
> /* 6.4.4.3.3 */
> - svdm_consume_modes(port, payload, cnt);
> + svdm_consume_modes(port, p, cnt);
> modep->svid_index++;
> if (modep->svid_index < modep->nsvids) {
> u16 svid = modep->svids[modep->svid_index];
> @@ -1238,15 +1230,18 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> static void tcpm_handle_vdm_request(struct tcpm_port *port,
> const __le32 *payload, int cnt)
> {
> - int rlen = 0;
> + u32 p[PD_MAX_PAYLOAD];
> u32 response[8] = { };
> - u32 p0 = le32_to_cpu(payload[0]);
> + int i, rlen = 0;
> +
> + for (i = 0; i < cnt; i++)
> + p[i] = le32_to_cpu(payload[i]);
>
> if (port->vdm_state == VDM_STATE_BUSY) {
> /* If UFP responded busy retry after timeout */
> - if (PD_VDO_CMDT(p0) == CMDT_RSP_BUSY) {
> + if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
> port->vdm_state = VDM_STATE_WAIT_RSP_BUSY;
> - port->vdo_retry = (p0 & ~VDO_CMDT_MASK) |
> + port->vdo_retry = (p[0] & ~VDO_CMDT_MASK) |
> CMDT_INIT;
You could have changed that to:
port->vdo_retry = (p[0] & ~VDO_CMDT_MASK) | CMDT_INIT;
while at it.
> mod_delayed_work(port->wq, &port->vdm_state_machine,
> msecs_to_jiffies(PD_T_VDM_BUSY));
> @@ -1255,8 +1250,8 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> port->vdm_state = VDM_STATE_DONE;
> }
>
> - if (PD_VDO_SVDM(p0))
> - rlen = tcpm_pd_svdm(port, payload, cnt, response);
> + if (PD_VDO_SVDM(p[0]))
> + rlen = tcpm_pd_svdm(port, p, cnt, response);
>
> if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request
2020-07-24 17:47 ` [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request Hans de Goede
2020-07-25 14:45 ` Guenter Roeck
@ 2020-07-28 13:06 ` Heikki Krogerus
1 sibling, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 13:06 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:47:00PM +0200, Hans de Goede wrote:
> Refactor tcpm_handle_vdm_request and its tcpm_pd_svdm helper function so
> that reporting the results of the vdm to the altmode-driver is separated
> out into a clear separate step inside tcpm_handle_vdm_request, instead
> of being scattered over various places inside the tcpm_pd_svdm helper.
>
> This is a preparation patch for fixing an AB BA lock inversion between the
> tcpm code and some altmode drivers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v2:
> - Keep "if (adev && pdev)" checks as is instead of modifying them
> ---
> drivers/usb/typec/tcpm/tcpm.c | 76 ++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index ee239b54bcd8..03a0c083ee9a 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -159,6 +159,14 @@ enum pd_msg_request {
> PD_MSG_DATA_SOURCE_CAP,
> };
>
> +enum adev_actions {
> + ADEV_NONE = 0,
> + ADEV_NOTIFY_USB_AND_QUEUE_VDM,
> + ADEV_QUEUE_VDM,
> + ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL,
> + ADEV_ATTENTION,
> +};
> +
> /* Events from low level driver */
>
> #define TCPM_CC_EVENT BIT(0)
> @@ -1078,10 +1086,10 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
>
> #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
>
> -static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> - u32 *response)
> +static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
> + const u32 *p, int cnt, u32 *response,
> + enum adev_actions *adev_action)
> {
> - struct typec_altmode *adev;
> struct typec_altmode *pdev;
> struct pd_mode_data *modep;
> int rlen = 0;
> @@ -1097,9 +1105,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>
> modep = &port->mode_data;
>
> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> -
> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
>
> @@ -1125,8 +1130,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> break;
> case CMD_ATTENTION:
> /* Attention command does not have response */
> - if (adev)
> - typec_altmode_attention(adev, p[1]);
> + *adev_action = ADEV_ATTENTION;
> return 0;
> default:
> break;
> @@ -1180,23 +1184,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> case CMD_ENTER_MODE:
> if (adev && pdev) {
> typec_altmode_update_active(pdev, true);
> -
> - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> - response[0] = VDO(adev->svid, 1,
> - CMD_EXIT_MODE);
> - response[0] |= VDO_OPOS(adev->mode);
> - return 1;
> - }
> + *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
> }
> return 0;
> case CMD_EXIT_MODE:
> if (adev && pdev) {
> typec_altmode_update_active(pdev, false);
> -
> /* Back to USB Operation */
> - WARN_ON(typec_altmode_notify(adev,
> - TYPEC_STATE_USB,
> - NULL));
> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> + return 0;
> }
> break;
> default:
> @@ -1207,11 +1203,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> switch (cmd) {
> case CMD_ENTER_MODE:
> /* Back to USB Operation */
> - if (adev)
> - WARN_ON(typec_altmode_notify(adev,
> - TYPEC_STATE_USB,
> - NULL));
> - break;
> + *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> + return 0;
> default:
> break;
> }
> @@ -1221,15 +1214,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
> }
>
> /* Informing the alternate mode drivers about everything */
> - if (adev)
> - typec_altmode_vdm(adev, p[0], &p[1], cnt);
> -
> + *adev_action = ADEV_QUEUE_VDM;
> return rlen;
> }
>
> static void tcpm_handle_vdm_request(struct tcpm_port *port,
> const __le32 *payload, int cnt)
> {
> + enum adev_actions adev_action = ADEV_NONE;
> + struct typec_altmode *adev;
> u32 p[PD_MAX_PAYLOAD];
> u32 response[8] = { };
> int i, rlen = 0;
> @@ -1237,6 +1230,9 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> for (i = 0; i < cnt; i++)
> p[i] = le32_to_cpu(payload[i]);
>
> + adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> + PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> +
> if (port->vdm_state == VDM_STATE_BUSY) {
> /* If UFP responded busy retry after timeout */
> if (PD_VDO_CMDT(p[0]) == CMDT_RSP_BUSY) {
> @@ -1251,7 +1247,31 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> }
>
> if (PD_VDO_SVDM(p[0]))
> - rlen = tcpm_pd_svdm(port, p, cnt, response);
> + rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
> +
> + if (adev) {
> + switch (adev_action) {
> + case ADEV_NONE:
> + break;
> + case ADEV_NOTIFY_USB_AND_QUEUE_VDM:
> + WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, NULL));
> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
> + break;
> + case ADEV_QUEUE_VDM:
> + typec_altmode_vdm(adev, p[0], &p[1], cnt);
> + break;
> + case ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL:
> + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) {
> + response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE);
> + response[0] |= VDO_OPOS(adev->mode);
> + rlen = 1;
> + }
> + break;
> + case ADEV_ATTENTION:
> + typec_altmode_attention(adev, p[1]);
> + break;
> + }
> + }
>
> if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers
2020-07-24 17:47 ` [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers Hans de Goede
2020-07-25 14:46 ` Guenter Roeck
@ 2020-07-28 13:08 ` Heikki Krogerus
1 sibling, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 13:08 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:47:01PM +0200, Hans de Goede wrote:
> When we receive a PD data packet which ends up being for the alt-mode
> driver we have the following lock order:
>
> 1. tcpm_pd_rx_handler take the tcpm-port lock
> 2. We call into the alt-mode driver which takes the alt-mode's lock
>
> And when the alt-mode driver initiates communication we have the following
> lock order:
>
> 3. alt-mode driver takes the alt-mode's lock
> 4. alt-mode driver calls tcpm_altmode_enter which takes the tcpm-port lock
>
> This is a classic AB BA lock inversion issue.
>
> With the refactoring of tcpm_handle_vdm_request() done before this patch,
> we don't rely on, or need to make changes to the tcpm-port data by the
> time we make call 2. from above. All data to be passed to the alt-mode
> driver sits on our stack at this point, and thus does not need locking.
>
> So after the refactoring we can simply fix this by releasing the
> tcpm-port lock before calling into the alt-mode driver.
>
> This fixes the following lockdep warning:
>
> [ 191.454238] ======================================================
> [ 191.454240] WARNING: possible circular locking dependency detected
> [ 191.454244] 5.8.0-rc5+ #1 Not tainted
> [ 191.454246] ------------------------------------------------------
> [ 191.454248] kworker/u8:5/794 is trying to acquire lock:
> [ 191.454251] ffff9bac8e30d4a8 (&dp->lock){+.+.}-{3:3}, at: dp_altmode_vdm+0x30/0xf0 [typec_displayport]
> [ 191.454263]
> but task is already holding lock:
> [ 191.454264] ffff9bac9dc240a0 (&port->lock#2){+.+.}-{3:3}, at: tcpm_pd_rx_handler+0x43/0x12c0 [tcpm]
> [ 191.454273]
> which lock already depends on the new lock.
>
> [ 191.454275]
> the existing dependency chain (in reverse order) is:
> [ 191.454277]
> -> #1 (&port->lock#2){+.+.}-{3:3}:
> [ 191.454286] __mutex_lock+0x7b/0x820
> [ 191.454290] tcpm_altmode_enter+0x23/0x90 [tcpm]
> [ 191.454293] dp_altmode_work+0xca/0xe0 [typec_displayport]
> [ 191.454299] process_one_work+0x23f/0x570
> [ 191.454302] worker_thread+0x55/0x3c0
> [ 191.454305] kthread+0x138/0x160
> [ 191.454309] ret_from_fork+0x22/0x30
> [ 191.454311]
> -> #0 (&dp->lock){+.+.}-{3:3}:
> [ 191.454317] __lock_acquire+0x1241/0x2090
> [ 191.454320] lock_acquire+0xa4/0x3d0
> [ 191.454323] __mutex_lock+0x7b/0x820
> [ 191.454326] dp_altmode_vdm+0x30/0xf0 [typec_displayport]
> [ 191.454330] tcpm_pd_rx_handler+0x11ae/0x12c0 [tcpm]
> [ 191.454333] process_one_work+0x23f/0x570
> [ 191.454336] worker_thread+0x55/0x3c0
> [ 191.454338] kthread+0x138/0x160
> [ 191.454341] ret_from_fork+0x22/0x30
> [ 191.454343]
> other info that might help us debug this:
>
> [ 191.454345] Possible unsafe locking scenario:
>
> [ 191.454347] CPU0 CPU1
> [ 191.454348] ---- ----
> [ 191.454350] lock(&port->lock#2);
> [ 191.454353] lock(&dp->lock);
> [ 191.454355] lock(&port->lock#2);
> [ 191.454357] lock(&dp->lock);
> [ 191.454360]
> *** DEADLOCK ***
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v2:
> -Move the mutex_lock call to above the tcpm_queue_vdm() call, so that
> we can use the regular tcpm_queue_vdm() instead of having to call
> tcpm_queue_vdm_unlocked()
> ---
> drivers/usb/typec/tcpm/tcpm.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 03a0c083ee9a..9b26b57a0172 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1249,6 +1249,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> if (PD_VDO_SVDM(p[0]))
> rlen = tcpm_pd_svdm(port, adev, p, cnt, response, &adev_action);
>
> + /*
> + * We are done with any state stored in the port struct now, except
> + * for any port struct changes done by the tcpm_queue_vdm() call
> + * below, which is a separate operation.
> + *
> + * So we can safely release the lock here; and we MUST release the
> + * lock here to avoid an AB BA lock inversion:
> + *
> + * If we keep the lock here then the lock ordering in this path is:
> + * 1. tcpm_pd_rx_handler take the tcpm port lock
> + * 2. One of the typec_altmode_* calls below takes the alt-mode's lock
> + *
> + * And we also have this ordering:
> + * 1. alt-mode driver takes the alt-mode's lock
> + * 2. alt-mode driver calls tcpm_altmode_enter which takes the
> + * tcpm port lock
> + *
> + * Dropping our lock here avoids this.
> + */
> + mutex_unlock(&port->lock);
> +
> if (adev) {
> switch (adev_action) {
> case ADEV_NONE:
> @@ -1273,6 +1294,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
> }
> }
>
> + /*
> + * We must re-take the lock here to balance the unlock in
> + * tcpm_pd_rx_handler, note that no changes, other then the
> + * tcpm_queue_vdm call, are made while the lock is held again.
> + * All that is done after the call is unwinding the call stack until
> + * we return to tcpm_pd_rx_handler and do the unlock there.
> + */
> + mutex_lock(&port->lock);
> +
> if (rlen > 0)
> tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> }
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
2020-07-24 17:57 ` Sergei Shtylyov
2020-07-25 14:53 ` Guenter Roeck
@ 2020-07-28 13:10 ` Heikki Krogerus
2 siblings, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-07-28 13:10 UTC (permalink / raw)
To: Hans de Goede; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb
On Fri, Jul 24, 2020 at 07:47:02PM +0200, Hans de Goede wrote:
> The tcpm.c code for sending VDMs assumes that there will only be one VDM
> in flight at the time. The "queue" used by tcpm_queue_vdm is only 1 entry
> deep.
>
> This assumes that the higher layers (tcpm state-machine and alt-mode
> drivers) ensure that queuing a new VDM before the old one has been
> completely send (or it timed out) add a WARN_ON to check for this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v2:
> - Fix typo in commit-msg subject
> ---
> drivers/usb/typec/tcpm/tcpm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 9b26b57a0172..cc786d558f14 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -971,6 +971,9 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> {
> WARN_ON(!mutex_is_locked(&port->lock));
>
> + /* Make sure we are not still processing a previous VDM packet */
> + WARN_ON(port->vdm_state > VDM_STATE_DONE);
> +
> port->vdo_count = cnt + 1;
> port->vdo_data[0] = header;
> memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
> --
> 2.26.2
thanks,
--
heikki
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-07-28 13:10 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 17:46 [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Hans de Goede
2020-07-24 17:46 ` [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper Hans de Goede
2020-07-25 14:37 ` Guenter Roeck
2020-07-28 12:44 ` Heikki Krogerus
2020-07-24 17:46 ` [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling Hans de Goede
2020-07-28 12:59 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request Hans de Goede
2020-07-25 14:45 ` Guenter Roeck
2020-07-26 10:58 ` Hans de Goede
2020-07-26 13:22 ` Guenter Roeck
2020-07-28 13:06 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers Hans de Goede
2020-07-25 14:46 ` Guenter Roeck
2020-07-28 13:08 ` Heikki Krogerus
2020-07-24 17:47 ` [PATCH v2 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not trying to send 2 VDM packets at the same time Hans de Goede
2020-07-24 17:57 ` Sergei Shtylyov
2020-07-24 19:09 ` Hans de Goede
2020-07-25 14:53 ` Guenter Roeck
2020-07-28 13:10 ` Heikki Krogerus
2020-07-28 12:43 ` [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() Heikki Krogerus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).