[PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm()

[PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm()

[Hans de Goede]

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

[PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper

[Hans de Goede]

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

Re: [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper

[Guenter Roeck]

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;
>  }
>  
> 

Re: [PATCH v2 2/6] usb: typec: tcpm: Add tcpm_queue_vdm_unlocked() helper

[Heikki Krogerus]

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

[PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling

[Hans de Goede]

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

Re: [PATCH v2 3/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request payload handling

[Heikki Krogerus]

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

[PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request

[Hans de Goede]

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

Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request

[Guenter Roeck]

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);
> 

Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request

[Hans de Goede]

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);
>>
> 

Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request

[Guenter Roeck]

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);
>>>
>>
> 

Re: [PATCH v2 4/6] usb: typec: tcpm: Refactor tcpm_handle_vdm_request

[Heikki Krogerus]

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

[PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers

[Hans de Goede]

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

Re: [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers

[Guenter Roeck]

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);
>  }
> 

Re: [PATCH v2 5/6] usb: typec: tcpm: Fix AB BA lock inversion between tcpm code and the alt-mode drivers

[Heikki Krogerus]

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

[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]

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

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

[Sergei Shtylyov]

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

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

[Hans de Goede]

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
> 

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

[Guenter Roeck]

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);
> 

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

[Heikki Krogerus]

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

Re: [PATCH v2 1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm()

[Heikki Krogerus]

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