| Message ID | 20200724174702.61754-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 95b4d51c96a87cd760c2a4f27fb28a59a27b6368 |
| Headers | show |
| Series | [v2,1/6] usb: typec: tcpm: Move mod_delayed_work(&port->vdm_state_machine) call into tcpm_queue_vdm() | expand |
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); >
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); >> >
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); >>> >> >
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,
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);
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(-)