Message-ID: <98966b47-0bc5-6ec0-ec80-5eff1d71d9fd@synopsys.com>
Date:   Fri, 12 Aug 2022 02:00:36 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Elson Serrao <quic_eserrao@...cinc.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        "quic_mrana@...cinc.com" <quic_mrana@...cinc.com>
Subject: Re: [PATCH 2/5] usb: gadget: Add function wakeup support

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.


> I have explained below, how the 4 points you mentioned are handled in my
> current implementation.
> 
> The function driver will send a wakeup notification only if it is armed
> for remote wakeup.
> 
> patch#5
> +        if (!port->func_wakeup_allowed) {
> +            DBG(port->ioport, "Function wakeup not allowed\n");
> +            return -EOPNOTSUPP;
> +        }
> +        ret = usb_func_wakeup(func);
> +        if (ret)
> +            port->is_wakeup_pending = true;
> 
> If the device is in suspend, we do a LFPS handshake first and return
> -EAGAIN to composite layer which will set the is_wakeup_pending flag.
> 

I don't see you're checking for -EAGAIN. Also what happens when wakeup()
fails, should we retry?

> Patch#3
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
> +{
> +    int    ret = 0;
> +    u32    reg;
> +    struct    dwc3 *dwc = gadget_to_dwc(g);
> +
> +    reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +
> +    /*
> +     * If the link is in LPM, first bring the link to U0
> +     * before triggering function wakeup. Ideally this
> +     * needs to be expanded to other LPMs as well in
> +     * addition to U3
> +     */
> +    if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
> +        dwc3_gadget_wakeup(g);
> +        return -EAGAIN;
> +    }
> 
> The above should take care of Point 2.
> 
> After triggering a remote wakeup in Point 2, if the link transitions to
> U0 then we will receive a U0 link state event for the same and that
> would trigger a gadget_resume callback to inform the composite layer
> that device has resumed. As soon as the function/composite layer gets
> this info it will re-send the wakeup notification to the controller
> driver based on the is_wakeup_pending flag

See comment above.

> 
> linksts_change_interrupt() in Patch#1
> +    case DWC3_LINK_STATE_U0:
> +        if (dwc->is_gadget_wakeup) {
> +            linksts_change_events_set(dwc, false);
> +            dwc3_resume_gadget(dwc);
> +            dwc->is_gadget_wakeup = false;
> +        }
> +        break;
> 
> 
> u_ether resume callback in Patch#5
> +    if (func_suspend) {
> +        if (link->is_wakeup_pending) {
> +            usb_func_wakeup(func);
> +            link->is_wakeup_pending = false;
> +        }
> 
> The above should take care of Point 3.

We should also check for other functions to send device notification.

> 
> For Point 4 like you mentioned I will add U0 check instead of U3 check.
> 
> Point 1 would have resolved in enumeration stage itself (bmAttributes in
> config descriptor) based on which the host sets the
> USB_INTRF_FUNC_SUSPEND_RW option in the SET_FEATURE(FUNCTION_SUSPEND)
> packet. Based on this option the function/composite driver will set
> func_wakeup_allowed flag arming it for remote_wakeup
> 
> +static int ecm_func_suspend(struct usb_function *f, u8 options)
> +{
> +    bool func_wakeup_allowed;
> +    struct f_ecm *ecm = func_to_ecm(f);
> +    struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> +
> +    DBG(cdev, "func susp %u cmd\n", options);
> +
> +    func_wakeup_allowed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
> 
> Do we need any additional checks for Point 1 ? Please let me know if my
> understanding is incorrect here.
> 
> 

If the device is not remote wake capable, even if the host tries to arm
the device, the device shouldn't allow it. We're missing that check.

See 3.2 spec 9.1.1.6: "If a device is capable of remote wakeup, the
device shall support the ability of the host to enable and disable this
capability."

> 
> 
>>
>> 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
>>
> 
> Please let me know if this implementation fails to cover the 4 points
> you mentioned or any other rectification needed to handle these points.
> 

BR,
Thinh

Powered by blists - more mailing lists