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