Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
From: Elson Serrao
Date: Fri Mar 17 2023 - 20:24:14 EST
On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
On Fri, Mar 17, 2023, Thinh Nguyen wrote:
On Fri, Mar 17, 2023, Elson Serrao wrote:
On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
When host sends a suspend notification to the device, handle
the suspend callbacks in the function driver. Enhanced super
speed devices can support function suspend feature to put the
function in suspend state. Handle function suspend callback.
Depending on the remote wakeup capability the device can either
trigger a remote wakeup or wait for the host initiated resume to
start data transfer again.
Signed-off-by: Elson Roy Serrao <quic_eserrao@xxxxxxxxxxx>
---
drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.h | 4 ++
3 files changed, 138 insertions(+)
diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index a7ab30e..c43cd557 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
usb_ep_disable(ecm->notify);
ecm->notify->desc = NULL;
+ f->func_suspended = false;
+ f->func_wakeup_armed = false;
}
/*-------------------------------------------------------------------------*/
@@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
return &opts->func_inst;
}
+static void ecm_suspend(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ if (f->func_suspended) {
+ DBG(cdev, "Function already suspended\n");
+ return;
+ }
+
+ DBG(cdev, "ECM Suspend\n");
+
+ gether_suspend(&ecm->port);
+}
+
+static void ecm_resume(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ /*
+ * If the function is in USB3 Function Suspend state, resume is
+ * canceled. In this case resume is done by a Function Resume request.
+ */
+ if (f->func_suspended)
+ return;
+
+ DBG(cdev, "ECM Resume\n");
+
+ gether_resume(&ecm->port);
+}
+
+static int ecm_get_status(struct usb_function *f)
+{
+ struct usb_configuration *c = f->config;
+
+ /* D0 and D1 bit set to 0 if device is not wakeup capable */
+ if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
+ return 0;
+
+ return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
+ USB_INTRF_STAT_FUNC_RW_CAP;
+}
Why do we need to implement ecm_get_status if it's already handled in
composite.c now?
Yes this can be removed now. Will modify accordingly.
+
+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+ struct usb_composite_dev *cdev = f->config->cdev;
+
+ DBG(cdev, "func susp %u cmd\n", options);
+
+ if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
This feature selector doesn't indicate whether it's SetFeature or
ClearFeature request. ecm_func_suspend is supposed to be for
SetFeature(suspend) only. Perhaps we may have to define func_resume()
for ClearFeature(suspend)?
Host uses the same feature selector FUNCTION_SUSPEND for function suspend
and function resume and func_suspend() callback can be used to
handle both the cases ? The distinction comes whether it is a
How do you plan to handle that? Pass this info in some unused/reserved
bit of the "options" argument? Introduce a new parameter to the
func_suspend()?
If that's the case, then you need to update the document on
func_suspend() to also support ClearFeature(suspend). Right now it's
documented for SetFeature only. Also, make sure that other existing
function drivers will not break because of the change of the
func_suspend behavior.
SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
easily done in the func_suspend callback itself. We can add another callback
func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
redundant and more callback handling on function driver/composite side as
well? Please let me know your opinion.
We actually didn't properly define func_suspend and its counter part. It
seems cleaner to me to introduce func_resume as it seems more intuitive
and easier to read. Let me know how you plan to use func_suspend() for
both cases.
How about we handle function suspend resume in composite also? I mean
something like this:
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 36add1879ed2..79dc055eb5f7 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
- status = f->get_status ? f->get_status(f) : 0;
- if (status < 0)
- break;
+
+ if (f->get_status) {
+ status = f->get_status(f);
+ if (status < 0)
+ break;
+ } else {
+ if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
+ status |= USB_INTRF_STAT_FUNC_RW_CAP;
+ if (f->func_wakeup_armed)
+ status |= USB_INTRF_STAT_FUNC_RW;
+ }
+ }
put_unaligned_le16(status & 0x0000ffff, req->buf);
break;
/*
@@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
+ if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
+ if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
+ break;
+
+ f->func_wakeup_armed = (ctrl->bRequest == USB_REQ_SET_FEATURE);
+ }
+
value = 0;
- if (f->func_suspend)
+ if (f->func_suspend) {
value = f->func_suspend(f, w_index >> 8);
+ } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
+ if (f->suspend && && !f->func_suspended &&
+ ctrl->bRequest == USB_REQ_SET_FEATURE)) {
+ f->suspend(f);
+ f->func_suspended = true;
+ } else if (f->resume && f->func_suspended &&
+ ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ }
+
if (value < 0) {
ERROR(cdev,
"func_suspend() returned error %d\n",
At individual function driver level there is no need to differentiate
between suspend() and func_suspend() APIs, as both are intended to put
the function in suspend state. So your idea/implementation above makes
it much more clearer. Let composite also handle this and call either
f->suspend() or f->resume() callback based on the setup packet received.
Thank you for this suggestion.
Also, do we need the f->func_suspended flag? we'd need the remote wakeup
flag for the status, but when do we need f->func_suspended? It seems
like it can be handled within the function driver's scope.
f->func_suspended flag I had added for below purposes
1.) Function drivers should know the right wakeup() op to be called.
That is if they are in FUNC_SUSPEND then call usb_func_wakeup() and if
they are in device suspend then call usb_gadget_wakeup(). (we can use
f->func_wakeup_armed flag for this purpose as well)
2.) If a function is in USB3 FUNCTION_SUSPEND state then it shouldn't
allow f->resume() called through composite_resume() as the exit from
FUNCTION_SUSPEND state is via ClearFeature(FUNCTION_SUSPEND).
So we need a way to tell function drivers if they are in USB3
FUNCTION_SUSPEND state OR device suspend.
Please let me know if you see any alternative or better approach here.
Thanks
Elson