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