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

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

[Hans de Goede]

Hi All,

Here is a patch series which fixes an AB BA lock inversion between thr
tcpm code and alt-mode drivers (seen with the display-port alt-mode driver).

The actual fix is in patch 5. I could have just done an unlock before and
a re-lock after all the troublesome typec_altmode_foo(adev, ...) calls, but
I've done some reshuffling in patches 1-4 to make it clearer that when the
unlock happens we are really done with the port-state so it is ok to unlock
then.

Patch 6 is a bonus patch which adds an extra sanity check (WARN_ON) for a
potential issue which I noticed while working on this.

Regards,

Hans

[PATCH 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.

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

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

[Guenter Roeck]

On 7/15/20 6:22 AM, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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

[PATCH 2/6] usb: typec: tcpm: Move locking into tcpm_queue_vdm()

[Hans de Goede]

Various callers (all the typec_altmode_ops) take the port-lock just for
the tcpm_queue_vdm() call.

Rename the raw (unlocked) tcpm_queue_vdm() call to
tcpm_queue_vdm_unlocked() and add a new tcpm_queue_vdm() 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>
---
 drivers/usb/typec/tcpm/tcpm.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index fc6455a29c74..30e997d6ea1e 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -958,9 +958,11 @@ static void tcpm_queue_message(struct tcpm_port *port,
 /*
  * VDM/VDO handling functions
  */
-static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
-			   const u32 *data, int cnt)
+static void tcpm_queue_vdm_unlocked(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(struct tcpm_port *port, const u32 header,
+			   const u32 *data, int cnt)
+{
+	mutex_lock(&port->lock);
+	tcpm_queue_vdm_unlocked(port, header, data, cnt);
+	mutex_unlock(&port->lock);
+}
+
 static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
 				  int cnt)
 {
@@ -1249,7 +1259,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
 		rlen = tcpm_pd_svdm(port, payload, cnt, response);
 
 	if (rlen > 0)
-		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
+		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
 }
 
 static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
@@ -1263,7 +1273,7 @@ static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
 	/* set VDM header with VID & 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);
+	tcpm_queue_vdm_unlocked(port, header, data, count);
 }
 
 static unsigned int vdm_ready_timeout(u32 vdm_hdr)
@@ -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);
-
 	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);
-
 	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);
-
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH 2/6] usb: typec: tcpm: Move locking into tcpm_queue_vdm()

[Guenter Roeck]

On 7/15/20 6:22 AM, Hans de Goede wrote:
> Various callers (all the typec_altmode_ops) take the port-lock just for
> the tcpm_queue_vdm() call.
> 
> Rename the raw (unlocked) tcpm_queue_vdm() call to
> tcpm_queue_vdm_unlocked() and add a new tcpm_queue_vdm() 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>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc6455a29c74..30e997d6ea1e 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -958,9 +958,11 @@ static void tcpm_queue_message(struct tcpm_port *port,
>  /*
>   * VDM/VDO handling functions
>   */
> -static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
> -			   const u32 *data, int cnt)
> +static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
> +				    const u32 *data, int cnt)

The new function name is misleading. I think tcpm_queue_vdm_locked()
would be a much better name. Alternatively, consider keeping tcpm_queue_vdm()
as is and add tcpm_queue_vdm_unlocked().

>  {
> +	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(struct tcpm_port *port, const u32 header,
> +			   const u32 *data, int cnt)
> +{
> +	mutex_lock(&port->lock);
> +	tcpm_queue_vdm_unlocked(port, header, data, cnt);
> +	mutex_unlock(&port->lock);
> +}
> +
>  static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
>  				  int cnt)
>  {
> @@ -1249,7 +1259,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>  		rlen = tcpm_pd_svdm(port, payload, cnt, response);
>  
>  	if (rlen > 0)
> -		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> +		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
>  }
>  
>  static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
> @@ -1263,7 +1273,7 @@ static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
>  	/* set VDM header with VID & 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);
> +	tcpm_queue_vdm_unlocked(port, header, data, count);
>  }
>  
>  static unsigned int vdm_ready_timeout(u32 vdm_hdr)
> @@ -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);
> -
>  	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);
> -
>  	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);
> -
>  	return 0;
>  }
>  
> 

Re: [PATCH 2/6] usb: typec: tcpm: Move locking into tcpm_queue_vdm()

[Hans de Goede]

Hi,

Thank you for the reviews.

On 7/15/20 5:41 PM, Guenter Roeck wrote:
> On 7/15/20 6:22 AM, Hans de Goede wrote:
>> Various callers (all the typec_altmode_ops) take the port-lock just for
>> the tcpm_queue_vdm() call.
>>
>> Rename the raw (unlocked) tcpm_queue_vdm() call to
>> tcpm_queue_vdm_unlocked() and add a new tcpm_queue_vdm() 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>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index fc6455a29c74..30e997d6ea1e 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -958,9 +958,11 @@ static void tcpm_queue_message(struct tcpm_port *port,
>>   /*
>>    * VDM/VDO handling functions
>>    */
>> -static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
>> -			   const u32 *data, int cnt)
>> +static void tcpm_queue_vdm_unlocked(struct tcpm_port *port, const u32 header,
>> +				    const u32 *data, int cnt)
> 
> The new function name is misleading. I think tcpm_queue_vdm_locked()
> would be a much better name. Alternatively, consider keeping tcpm_queue_vdm()
> as is and add tcpm_queue_vdm_unlocked().

At first I was a bit surprised by your comment, because I'm sure I have seen the
unlocked pattern used before, in the same way as I'm using it. But upon checking
it indeed seems that most of the time it is used in the way you suggest using it
including in the usb subsys. So I will make the requested changes for v2 of the
patchset.

Regards,

Hans



> 
>>   {
>> +	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(struct tcpm_port *port, const u32 header,
>> +			   const u32 *data, int cnt)
>> +{
>> +	mutex_lock(&port->lock);
>> +	tcpm_queue_vdm_unlocked(port, header, data, cnt);
>> +	mutex_unlock(&port->lock);
>> +}
>> +
>>   static void svdm_consume_identity(struct tcpm_port *port, const __le32 *payload,
>>   				  int cnt)
>>   {
>> @@ -1249,7 +1259,7 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>   		rlen = tcpm_pd_svdm(port, payload, cnt, response);
>>   
>>   	if (rlen > 0)
>> -		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
>> +		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
>>   }
>>   
>>   static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
>> @@ -1263,7 +1273,7 @@ static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
>>   	/* set VDM header with VID & 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);
>> +	tcpm_queue_vdm_unlocked(port, header, data, count);
>>   }
>>   
>>   static unsigned int vdm_ready_timeout(u32 vdm_hdr)
>> @@ -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);
>> -
>>   	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);
>> -
>>   	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);
>> -
>>   	return 0;
>>   }
>>   
>>
> 

[PATCH 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.

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 30e997d6ea1e..bf5a0322dbfe 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -981,16 +981,15 @@ static void tcpm_queue_vdm(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_unlocked(port, response[0], &response[1], rlen - 1);
-- 
2.26.2

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

[Guenter Roeck]

On 7/15/20 6:22 AM, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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 30e997d6ea1e..bf5a0322dbfe 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -981,16 +981,15 @@ static void tcpm_queue_vdm(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_unlocked(port, response[0], &response[1], rlen - 1);
> 

[PATCH 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>
---
 drivers/usb/typec/tcpm/tcpm.c | 80 +++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index bf5a0322dbfe..4745b4062000 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)
@@ -1079,9 +1087,8 @@ 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)
+			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 +1104,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 +1129,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;
@@ -1178,27 +1181,18 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
 			}
 			break;
 		case CMD_ENTER_MODE:
-			if (adev && pdev) {
+			if (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) {
+			if (pdev)
 				typec_altmode_update_active(pdev, false);
 
-				/* Back to USB Operation */
-				WARN_ON(typec_altmode_notify(adev,
-							     TYPEC_STATE_USB,
-							     NULL));
-			}
-			break;
+			/* Back to USB Operation */
+			*adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
+			return 0;
 		default:
 			break;
 		}
@@ -1207,11 +1201,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 +1212,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 +1228,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 +1245,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, 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_unlocked(port, response[0], &response[1], rlen - 1);
-- 
2.26.2

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

[Guenter Roeck]

On 7/15/20 6:22 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>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 80 +++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index bf5a0322dbfe..4745b4062000 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)
> @@ -1079,9 +1087,8 @@ 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)
> +			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 +1104,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 +1129,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;
> @@ -1178,27 +1181,18 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>  			}
>  			break;
>  		case CMD_ENTER_MODE:
> -			if (adev && pdev) {
> +			if (pdev)
>  				typec_altmode_update_active(pdev, true);
>  

There is a slight semantic difference: typec_altmode_update_active() is now
called even if adev is NULL. Is this intentional ?

> -				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) {
> +			if (pdev)
>  				typec_altmode_update_active(pdev, false);
>  
> -				/* Back to USB Operation */
> -				WARN_ON(typec_altmode_notify(adev,
> -							     TYPEC_STATE_USB,
> -							     NULL));
> -			}
> -			break;
> +			/* Back to USB Operation */
> +			*adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> +			return 0;
>  		default:
>  			break;
>  		}
> @@ -1207,11 +1201,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 +1212,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 +1228,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 +1245,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, 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_unlocked(port, response[0], &response[1], rlen - 1);
> 

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

[Hans de Goede]

Hi,

On 7/15/20 5:58 PM, Guenter Roeck wrote:
> On 7/15/20 6:22 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>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 80 +++++++++++++++++++++--------------
>>   1 file changed, 49 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index bf5a0322dbfe..4745b4062000 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)
>> @@ -1079,9 +1087,8 @@ 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)
>> +			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 +1104,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 +1129,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;
>> @@ -1178,27 +1181,18 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const u32 *p, int cnt,
>>   			}
>>   			break;
>>   		case CMD_ENTER_MODE:
>> -			if (adev && pdev) {
>> +			if (pdev)
>>   				typec_altmode_update_active(pdev, true);
>>   
> 
> There is a slight semantic difference: typec_altmode_update_active() is now
> called even if adev is NULL. Is this intentional ?

Sort of, I was / am aware of this and choose to accept the small behavior change,
since it seemed innocence and leads to some code cleanup, but thinking more about
it the behavior change indeed seems unwanted, so I will fix this for v2 of the
patch-set.

Regards,

Hans



> 
>> -				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) {
>> +			if (pdev)
>>   				typec_altmode_update_active(pdev, false);
>>   
>> -				/* Back to USB Operation */
>> -				WARN_ON(typec_altmode_notify(adev,
>> -							     TYPEC_STATE_USB,
>> -							     NULL));
>> -			}
>> -			break;
>> +			/* Back to USB Operation */
>> +			*adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>> +			return 0;
>>   		default:
>>   			break;
>>   		}
>> @@ -1207,11 +1201,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 +1212,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 +1228,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 +1245,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, 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_unlocked(port, response[0], &response[1], rlen - 1);
>>
> 

[PATCH 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>
---
 drivers/usb/typec/tcpm/tcpm.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 4745b4062000..ea14240423d1 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1247,6 +1247,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
 	if (PD_VDO_SVDM(p[0]))
 		rlen = tcpm_pd_svdm(port, 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:
@@ -1272,7 +1293,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
 	}
 
 	if (rlen > 0)
-		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
+		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
+
+	/*
+	 * We must re-take the lock here to balance the unlock in
+	 * tcpm_pd_rx_handler, note that no changes are made while the lock
+	 * is held again. All that is done is unwinding the call stack until
+	 * we return to tcpm_pd_rx_handler and do the unlock there.
+	 */
+	mutex_lock(&port->lock);
 }
 
 static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
-- 
2.26.2

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

[Guenter Roeck]

On 7/15/20 6:23 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>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 4745b4062000..ea14240423d1 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1247,6 +1247,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>  	if (PD_VDO_SVDM(p[0]))
>  		rlen = tcpm_pd_svdm(port, 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:
> @@ -1272,7 +1293,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>  	}
>  
>  	if (rlen > 0)
> -		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
> +		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
> +
> +	/*
> +	 * We must re-take the lock here to balance the unlock in
> +	 * tcpm_pd_rx_handler, note that no changes are made while the lock
> +	 * is held again. All that is done is unwinding the call stack until
> +	 * we return to tcpm_pd_rx_handler and do the unlock there.
> +	 */
> +	mutex_lock(&port->lock);

Unless I am missing something, tcpm_queue_vdm() now also acquires the lock
and releases it. Why not move this further up and keep tcpm_queue_vdm_unlocked() ?
This would avoid one set of lock/unlock calls.

Thanks,
Guenter

>  }
>  
>  static void tcpm_send_vdm(struct tcpm_port *port, u32 vid, int cmd,
> 

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

[Hans de Goede]

Hi,

On 7/15/20 6:05 PM, Guenter Roeck wrote:
> On 7/15/20 6:23 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>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 31 ++++++++++++++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 4745b4062000..ea14240423d1 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -1247,6 +1247,27 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>   	if (PD_VDO_SVDM(p[0]))
>>   		rlen = tcpm_pd_svdm(port, 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:
>> @@ -1272,7 +1293,15 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>>   	}
>>   
>>   	if (rlen > 0)
>> -		tcpm_queue_vdm_unlocked(port, response[0], &response[1], rlen - 1);
>> +		tcpm_queue_vdm(port, response[0], &response[1], rlen - 1);
>> +
>> +	/*
>> +	 * We must re-take the lock here to balance the unlock in
>> +	 * tcpm_pd_rx_handler, note that no changes are made while the lock
>> +	 * is held again. All that is done is unwinding the call stack until
>> +	 * we return to tcpm_pd_rx_handler and do the unlock there.
>> +	 */
>> +	mutex_lock(&port->lock);
> 
> Unless I am missing something, tcpm_queue_vdm() now also acquires the lock
> and releases it. Why not move this further up and keep tcpm_queue_vdm_unlocked() ?
> This would avoid one set of lock/unlock calls.

You're right, I've changed this for v2 of the patch-set.

Regards,

Hans

[PATCH 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not tryong 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>
---
 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 ea14240423d1..5adc30666566 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -971,6 +971,9 @@ static void tcpm_queue_vdm_unlocked(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 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not tryong to send 2 VDM packets at the same time

[Guenter Roeck]

On 7/15/20 6:23 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

subject: s/tryong/trying/

> ---
>  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 ea14240423d1..5adc30666566 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -971,6 +971,9 @@ static void tcpm_queue_vdm_unlocked(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 6/6] usb: typec: tcpm: Add WARN_ON ensure we are not tryong to send 2 VDM packets at the same time

[Hans de Goede]

Hi,

On 7/15/20 6:06 PM, Guenter Roeck wrote:
> On 7/15/20 6:23 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.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> subject: s/tryong/trying/

Ack, fixed for v2.

Regards,

Hans



> 
>> ---
>>   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 ea14240423d1..5adc30666566 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -971,6 +971,9 @@ static void tcpm_queue_vdm_unlocked(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);
>>
>