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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8705d52e-2181-aebd-43b8-2c8d021339c7@synopsys.com>
Date:   Fri, 5 Aug 2022 01:26:32 +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/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