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

[Hans de Goede <hdegoede@redhat.com>]

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