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

[Elson Serrao]

On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> On 8/9/2022, Elson Serrao wrote:
> > On 8/4/2022 6:26 PM, Thinh Nguyen wrote:
> > > On 8/4/2022, Elson Serrao wrote:
> > > > On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
> > > > > On 8/2/2022, Elson Roy Serrao wrote:
> > > > > > An interface which is in function suspend state has to send a function
> > > > > > wakeup notification to the host in case it needs to initate any data
> > > > > > transfer. One notable difference between this and the existing remote
> > > > > > wakeup mechanism is that this can be called per-interface, and a UDC
> > > > > > would need to know the particular interface number to convey in its
> > > > > > Device Notification transaction packet.  Hence, we need to introduce
> > > > > > a new callback in the gadget_ops structure that UDC device drivers
> > > > > > can implement.  Similarly add a convenience function in the composite
> > > > > > driver which function drivers can call. Add support to handle such
> > > > > > requests in the composite layer and invoke the gadget op.
> > > > >
> > > > > Sending the function wake notification should be done in the controller
> > > > > driver. The controller driver knows when is the proper link state
> > > > > (U0/ON) the device is in and would notify the host then.
> > > > >
> > > > > What we need to add in the usb_gadget is whether the device is remote
> > > > > wakeup capable. Something like a flag usb_gadget->rw_capable.
> > > > >
> > > > > We would also need some functions like
> > > > > usb_gadget_enable_remote_wakeup()
> > > > > and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
> > > > > the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
> > > > > the bmAttributes configuration.
> > > > >
> > > > > BR,
> > > > > Thinh
> > > >
> > > >
> > > > If we handle this in controller driver, then it would fail to get the
> > > > right interface id when multiple functions have to send function wake
> > > > notification. As per USB3.0 spec (below snippets) a function can be
> > > > independently placed into function suspend state within a composite
> > > > device and each function in function suspend state has to send a
> > > > function wake notification to exit.
> > > >
> > > > USB 3.0 Spec Section 9.2.5.3
> > > > "A function may be placed into Function Suspend independently of other
> > > > functions within a composite device"
> > > >
> > > > USB 3.0 Spec Section 9.2.5.4
> > > > "A function may signal that it wants to exit from Function Suspend by
> > > > sending a Function Wake Notification to the host if it is enabled for
> > > > function remote wakeup. This applies to single function devices as
> > > > well as multiple function ( i.e., composite) devices. If the link is in
> > > > a non-U0 state, then the device must transition the link to U0 prior
> > > > to sending the remote wake message. If a remote wake event occurs in
> > > > multiple functions, each function shall send a Function Wake
> > > > Notification"
> > >
> > > Ok, so the issue here is adding the ability to pass the interface number
> > > to the controller driver when sending the device notification function
> > > wakeup right? Sounds like the callback should be
> > > send_wakeup_notification(gadget, func_id) instead.
> > >
> > > As for remote wakeup, the spec states that "If remote wake event occurs
> > > in multiple functions, each function shall send a Function Wake
> > > Notification."
> > >
> > > The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host
> > > will put the device in Suspend State for a remote wake event to occur.
> > > It only places the function in Function Suspend. However often the host
> > > will put the device in suspend after this. The dwc3 driver can track if
> > > the host puts the device in suspend state and what interfaces are armed
> > > for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can
> > > send Function Wake Notification for each function armed with remote
> > > wakeup.
> > >
> > > Please correct me if I'm wrong.
> > >
> > > Also, make sure that device remote wakeup will still work for highspeed
> > > (not function remote wakeup). I see this check which doesn't look right
> > > in one of your patches:
> > > +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
> > > +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> > > +        ret =  -EPERM;
> > > +        goto out;
> > > +    }
> > >
> > > Thanks,
> > > Thinh
> >
> > For superspeed capable devices, when a function is in suspend state and
> > wants to
> > initiate a resume, it has to send a function wake notification to the
> > host irrespective
> > of whether the device is in SUSPEND or not. Like you mentioned the
> > device need not be in
> > suspend state when a function is suspended. If the device is in suspend,
> > then first the
> > controller driver has to transition the link to U0 state before sending
> > function wake notification.
>
> Was I incorrect? I'm not clear on the point of reiteration above.
>
> > Note that the DEVICE_REMOTE_WAKEUP feature is ignored for super-speed
> > devices and they
>
> We're still talking about Enhanced Super Speed here.
>
> > are by default remote wakeup capable if any function within the device
> > is armed for
> > function remote wakeup.
>
> What you're saying is if the host arms the function for remote wakeup,
> then the device is remote capable.
>
> However, the important point here is that the host only arms for remote
> wakeup _if_ the device is remote wakeup capable. That needs to be checked.
>
> > So in my current implementation when the host sends a function suspend
> > SET_FEATURE(FUNCTION_SUSPEND),
> > the device delegates it to the respective function driver. There we
> > inspect if it is capable
> > of initiating a function remote wakeup. If it is, then when a remote
> > wakeup event
> > occurs (in my current implementation when TCP/IP layer wants to send
> > data to the host. patch#5) then
> > we trigger a function wakeup by calling usb_gadget_func_wakeup(gadget,
> > id) callback. Controller driver then
> > checks if the device is in suspend or not. If it is in suspend, it first
> > brings the device to U0 state
>
> "brings the device to U0 state" means the device initiates remote wakeup
> here.
>
> > and then sends a function wake notification (via
> > dwc3_send_gadget_generic_command() API) only after an
>
> So now the dwc3 tracks which interface(s) were armed for remote here?
>
> I don't recall seeing it in your patches. Did you handle and send device
> notification for all the functions armed with remote wakeup after device
> wakeup?
>
>
> > U0 event has occurred. If the device is not in suspend then it directly
> > sends function wake notification
> > to the host. Once the host receives the function wake notification it
> > sends a SET_FEATURE(FUNCTION_SUSPEND)
> > with suspend bit (BIT 0) reset to signal function resume. The controller
> > driver upon receiving this packet
> > delegates to the respective function driver. Note that at this point the
> > device is in U0 state but some other
>
> We can't assume that the device is in U0 state. There's also no
> mechanism in your change to know that either.
>
> > function within the device may still be in suspend state (if more than
> > one function was put to suspend state).
> > So the only way to exit from function suspend is via function resume
> > which is independent of device suspend/resume.
> >
> > Also the task of finding the interface id is done by composite driver
> > because most function drivers have
> > a transport layer and this layer is the one responsible for issuing a
> > function remote wakeup and
> > this has no direct reference to interface id. For example u_ether
> > transport layer can have either f_ecm or f_rndis
> > as its underlying channel and u_ether has no knowledge of the interface
> > id/function driver it is using.
>
> > For high speed devices there is no concept of function suspend and there
> > is only device suspend. The ability
> > of a device to send a remote wakeup to exit from suspend is dictated by
> > DEVICE_REMOTE_WAKEUP feature selector.
> > The below snippet controls this aspect and sends remote wakeup for high
> > speed devices only if they are remote wakeup capable.
> > dwc->is_remote_wakeup_enabled flag is set when DEVICE_REMOTE_WAKEUP is
> > received.
>
> The flag "is_remote_wakeup_enabled" implies that it applies for both
> device remote wakeup and function remote wakeup. If it only meant for
> function remote wakeup, then rename it. But I think you can use the same
> flag for both scenarios.
>
>
> > +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
> > +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> > +        ret =  -EPERM;
>
> Also, don't use -EPERM. Use -EINVAL.
>
> > +        goto out;
> > +    }
> >
> > Please let me know your thoughts on this approach. I will address your
> > other comments and rectify the patches accordingly.
>
>
> 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.

I am not clear on why device notification function wake should be sent 
for ALL interfaces armed with remote wakeup. Since function 
suspend/wakeup of an interface is independent of other functions in a 
composite device only the interface in which a remote wakeup event 
occurred should send the wake notification right? The other functions 
will continue to remain
in function suspend state.

> 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).
>
> To be able to do 3), you can teach the composite layer _when_ to send
> device notification function wake and for what functions. This can be
> retry sending the notification until send_wakeup_notification() succeed?
>
> I suggested to do that in dwc3 driver to avoid having to add the logic
> in composite layer as I think it is simpler in dwc3. However, the
> downside is that other UDCs have to handle it like dwc3 also.
>
> Now that I think about it again, it maybe better to do it in the
> composite driver for the long run. If you want to handle this in the
> composite layer, please document and design the mechanism to handle all
> the points above.
>
> Thanks,
> Thinh