Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/16/2022, Elson Serrao wrote:
> 
> 
> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>> On 8/11/2022, Thinh Nguyen wrote:
>>> On 8/11/2022, Thinh Nguyen wrote:
>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>
>>>>>
>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>
>>>> <snip>
>>>>
>>>>
>>>>>> To summarize the points:
>>>>>>
>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>> capable of
>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>> hardware
>>>>>> capability)
>>>>>>
>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>> (through
>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>
>>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>>> wakeup, the device will also send device notification function
>>>>>> wake for
>>>>>> all the interfaces armed with remote wakeup.
>>>>>>
>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>> notification function wake if it's in U0.
>>>>>>
>>>>>>
>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>> separate
>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>>> suggested to maybe add
>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>> intf_id) for the device notification. What you did was combining both
>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>
>>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>>> send_wakeup_notification to better align with the approach. The
>>>>> reason I
>>>>> have combined remote_wakeup and function wake notification in
>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>> is at
>>>>> function/composite level it has no knowledge on the link state. So I
>>>>> have delegated that task to controller driver to handle the
>>>>> notification
>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>> suspended and then send notification (explained below). But we can
>>>>> definitely separate this by adding an additional flag in the composite
>>>>> layer to set the link state based on the gadget suspend callback
>>>>> called
>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>> separating the two is a better approach.
>>>>>
>>>>
>>>> The reason I think we need to separate it is because of point 3. As I
>>>> note earlier, the spec states that "If remote wake event occurs in
>>>> multiple functions, each function shall send a Function Wake
>>>> Notification."
>>>>
>>>> But if there's no remote wake event, and the host brought the device up
>>>> instead, then the function suspend state is retained.
>>>>
>>>> If we separate these 2 operations, the caller can check whether the
>>>> operation went through properly. For example, if the wakeup() is
>>>> initiated properly, but the function wake device notification didn't go
>>>> through. We would only need to resend the device notification rather
>>>> than initiate remote wakeup again.
>>>
>>> If we don't have to send device notification for other interfaces, we
>>> can combine the operations here as you did.
>>>
>>
>> I still think it's better to split up the operations. The way you're
>> handling it now is not clear.
>>
>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
>> not go through and expect user to retry again. But here it does initiate
>> remote wake, but it just does not send device notification yet. This is
>> confusing.
>>
>> Also, instead of all the function wake handling coming from the function
>> driver, now we depend on the controller driver to call function resume()
>> on state change to U0, which will trigger device notification. What
>> happen if it doesn't call resume(). There's too many dependencies and it
>> seems fragile.
>>
>> I think all this can be handled in the function driver. You can fix the
>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
>> is what it's supposed to poll.
> 
> For transitioning from U3 to U0, the current upstream implementation is
> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
> blocking call. (this is a common API for both HS and SS)
> 
>     /* poll until Link State changes to ON */
>     retries = 20000;
> 
>     while (retries--) {
>         reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> 
>         /* in HS, means ON */
>         if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
>             break;
>     }
> 
> In my experiments I found that certain hosts take longer time to drive
> HS resume signalling between the remote wakeup and the resume K and this
> time varies across hosts. Hence the above polling timer is not generic
> across hosts. So how do we converge on a polling timer value to work
> across HS/SS and without blocking the system for a long time?

Can't we take the upper limit of both base on experiment? And it
shouldn't be blocking the whole system.

> 
> Some data layers like TCP/IP hold a TX lock while sending data (that
> causes a remote wakeup event) and hence this wakeup() may run in atomic
> context.

Why hold the lock while waiting for the host to wakeup? The host is
still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
it may run in atomic context.

> 
> To make this generic across hosts, I had switched to interrupt based
> approach, enabling link state events and return error value immediately
> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
> yeah, then we have to rely on the resume callback as an indication that
> link is transitioned to ON state.
> 

BR,
Thinh