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

[Guenter Roeck <linux@roeck-us.net>]

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