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