| Message ID | 1679009888-8239-7-git-send-email-quic_eserrao@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add function suspend/resume and remote wakeup support | expand |
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@quicinc.com> > --- > 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? > + > +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)? Thanks, Thinh > + if (!f->func_suspended) { > + ecm_suspend(f); > + f->func_suspended = true; > + } > + } else { > + if (f->func_suspended) { > + f->func_suspended = false; > + ecm_resume(f); > + } > + } > + > + return 0; > +} > +
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@quicinc.com> >> --- >> 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)? > > Thanks, > Thinh > 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 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. Thanks Elson >> + if (!f->func_suspended) { >> + ecm_suspend(f); >> + f->func_suspended = true; >> + } >> + } else { >> + if (f->func_suspended) { >> + f->func_suspended = false; >> + ecm_resume(f); >> + } >> + } >> + >> + return 0; >> +} >> +
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@quicinc.com> > > > --- > > > 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. Thanks, Thinh
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@quicinc.com> > > > > --- > > > > 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", 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. Thanks, Thinh
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@quicinc.com> >>>>> --- >>>>> 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
On Fri, Mar 17, 2023, Elson Serrao wrote: > > > 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@quicinc.com> > > > > > > --- > > > > > > 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. Ok. So do you plan to update composite_resume to check that? Perhaps document this expected behavior also? > > Please let me know if you see any alternative or better approach here. > If we have a use for it in composite.c, then it should be made available. Otherwise it should only exist in the function driver (ie. in f_ecm structure). Thanks, Thinh
On 3/17/2023 7:26 PM, Thinh Nguyen wrote: > On Fri, Mar 17, 2023, Elson Serrao wrote: >> >> >> 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@quicinc.com> >>>>>>> --- >>>>>>> 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. > > Ok. So do you plan to update composite_resume to check that? Perhaps > document this expected behavior also? > Yes I will update composite resume to handle this scenario. I was handling it in f_ecm till now, but based on this new discussion I will move all handling to composite layer. >> >> Please let me know if you see any alternative or better approach here. >> > > If we have a use for it in composite.c, then it should be made > available. Otherwise it should only exist in the function driver (ie. in > f_ecm structure). > Yes I will keep it for now. Since we are moving all handling to composite layer better to have some kind of flag that function drivers/composite layer can use to check if a given function is in FUNCTION_SUSPEND state or composite_suspend() state. Thanks Elson
On 3/20/2023 10:42 AM, Elson Serrao wrote: > > > On 3/17/2023 7:26 PM, Thinh Nguyen wrote: >> On Fri, Mar 17, 2023, Elson Serrao wrote: >>> >>> >>> 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@quicinc.com> >>>>>>>> --- >>>>>>>> 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)) { Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish based on SET_FEATURE and CLEAR_FEATURE packets for function suspend and function resume. Instead we can generalize (w_index & USB_INTRF_FUNC_SUSPEND_LP) for function suspend and all other cases for function resume. 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) { f->suspend(f); f->func_suspended = true; } } else { if (f->resume && f->func_suspended) { f->resume(f); f->func_suspended = false; } } Thanks Elson >>>> + 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. >> >> Ok. So do you plan to update composite_resume to check that? Perhaps >> document this expected behavior also? >> > Yes I will update composite resume to handle this scenario. I was > handling it in f_ecm till now, but based on this new discussion I will > move all handling to composite layer. >>> >>> Please let me know if you see any alternative or better approach here. >>> >> >> If we have a use for it in composite.c, then it should be made >> available. Otherwise it should only exist in the function driver (ie. in >> f_ecm structure). >> > Yes I will keep it for now. Since we are moving all handling to > composite layer better to have some kind of flag that function > drivers/composite layer can use to check if a given function is in > FUNCTION_SUSPEND state or composite_suspend() state. > > Thanks > Elson >
On Thu, Mar 23, 2023, Elson Serrao wrote: > > > On 3/20/2023 10:42 AM, Elson Serrao wrote: > > > > > > On 3/17/2023 7:26 PM, Thinh Nguyen wrote: > > > On Fri, Mar 17, 2023, Elson Serrao wrote: > > > > > > > > > > > > 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@quicinc.com> > > > > > > > > > --- > > > > > > > > > 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)) { > > Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function > resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e > USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish based > on SET_FEATURE and CLEAR_FEATURE packets for function suspend and function > resume. Instead we can generalize (w_index & USB_INTRF_FUNC_SUSPEND_LP) for > function suspend and all other cases for function resume. > > 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) { > f->suspend(f); > f->func_suspended = true; > } > } else { > if (f->resume && f->func_suspended) { > f->resume(f); > f->func_suspended = false; > } > } > > Ah.. right. That's possible also. Then can we check for both cases? Something like this: @ -1994,8 +2003,34 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) if (!f) break; value = 0; - if (f->func_suspend) + if (f->func_suspend) { value = f->func_suspend(f, w_index >> 8); + } else if (ctrl->bRequest == USB_REQ_SET_FEATURE) { + if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) && + (w_index & USB_INTRF_FUNC_SUSPEND_RW)) + break; + + f->func_wakeup_armed = !!(w_index & USB_INTRF_FUNC_SUSPEND_RW); + + if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) && + f->suspend && !f->func_suspended) { + f->suspend(f); + f->func_suspended = true; + } else if (f->resume && f->func_suspended) { + f->resume(f); + f->func_suspended = false; + } + } else if (ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) { + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) + f->func_wakeup_armed = false; + + if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) && + f->resume && f->func_suspended) { + f->resume(f); + f->func_suspended = false; + } + } + if (value < 0) { ERROR(cdev, "func_suspend() returned error %d\n", Thanks, Thinh
On Fri, Mar 24, 2023, Thinh Nguyen wrote: > On Thu, Mar 23, 2023, Elson Serrao wrote: > > > > > > On 3/20/2023 10:42 AM, Elson Serrao wrote: > > > > > > > > > On 3/17/2023 7:26 PM, Thinh Nguyen wrote: > > > > On Fri, Mar 17, 2023, Elson Serrao wrote: > > > > > > > > > > > > > > > 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@quicinc.com> > > > > > > > > > > --- > > > > > > > > > > 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)) { > > > > Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function > > resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e > > USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish based > > on SET_FEATURE and CLEAR_FEATURE packets for function suspend and function > > resume. Instead we can generalize (w_index & USB_INTRF_FUNC_SUSPEND_LP) for > > function suspend and all other cases for function resume. > > > > 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) { > > f->suspend(f); > > f->func_suspended = true; > > } > > } else { > > if (f->resume && f->func_suspended) { > > f->resume(f); > > f->func_suspended = false; > > } > > } > > > > > > Ah.. right. That's possible also. Then can we check for both cases? > Something like this: > Correction, ClearFeature clears the entire suspend option (remote wake + suspend) @@ -1994,8 +2003,32 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) if (!f) break; value = 0; - if (f->func_suspend) + if (f->func_suspend) { value = f->func_suspend(f, w_index >> 8); + } else if (ctrl->bRequest == USB_REQ_SET_FEATURE) { + if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) && + (w_index & USB_INTRF_FUNC_SUSPEND_RW)) + break; + + f->func_wakeup_armed = !!(w_index & USB_INTRF_FUNC_SUSPEND_RW); + + if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) && + f->suspend && !f->func_suspended) { + f->suspend(f); + f->func_suspended = true; + } else if (f->resume && f->func_suspended) { + f->resume(f); + f->func_suspended = false; + } + } else if (ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) { + f->func_wakeup_armed = false; + + if (f->resume && f->func_suspended) { + f->resume(f); + f->func_suspended = false; + } + } + if (value < 0) { ERROR(cdev, "func_suspend() returned error %d\n", Something like that. Thanks, Thinh
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; +} + +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)) { + if (!f->func_suspended) { + ecm_suspend(f); + f->func_suspended = true; + } + } else { + if (f->func_suspended) { + f->func_suspended = false; + ecm_resume(f); + } + } + + return 0; +} + static void ecm_free(struct usb_function *f) { struct f_ecm *ecm; @@ -952,6 +1019,10 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi) ecm->port.func.setup = ecm_setup; ecm->port.func.disable = ecm_disable; ecm->port.func.free_func = ecm_free; + ecm->port.func.suspend = ecm_suspend; + ecm->port.func.get_status = ecm_get_status; + ecm->port.func.func_suspend = ecm_func_suspend; + ecm->port.func.resume = ecm_resume; return &ecm->port.func; } diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c index f259975..8eba018 100644 --- a/drivers/usb/gadget/function/u_ether.c +++ b/drivers/usb/gadget/function/u_ether.c @@ -437,6 +437,20 @@ static inline int is_promisc(u16 cdc_filter) return cdc_filter & USB_CDC_PACKET_TYPE_PROMISCUOUS; } +static int ether_wakeup_host(struct gether *port) +{ + int ret; + struct usb_function *func = &port->func; + struct usb_gadget *gadget = func->config->cdev->gadget; + + if (func->func_suspended) + ret = usb_func_wakeup(func); + else + ret = usb_gadget_wakeup(gadget); + + return ret; +} + static netdev_tx_t eth_start_xmit(struct sk_buff *skb, struct net_device *net) { @@ -456,6 +470,15 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb, in = NULL; cdc_filter = 0; } + + if (dev->port_usb->is_suspend) { + DBG(dev, "Port suspended. Triggering wakeup\n"); + netif_stop_queue(net); + spin_unlock_irqrestore(&dev->lock, flags); + ether_wakeup_host(dev->port_usb); + return NETDEV_TX_BUSY; + } + spin_unlock_irqrestore(&dev->lock, flags); if (!in) { @@ -1014,6 +1037,45 @@ int gether_set_ifname(struct net_device *net, const char *name, int len) } EXPORT_SYMBOL_GPL(gether_set_ifname); +void gether_suspend(struct gether *link) +{ + struct eth_dev *dev = link->ioport; + unsigned long flags; + + if (!dev) + return; + + if (atomic_read(&dev->tx_qlen)) { + /* + * There is a transfer in progress. So we trigger a remote + * wakeup to inform the host. + */ + ether_wakeup_host(dev->port_usb); + return; + } + spin_lock_irqsave(&dev->lock, flags); + link->is_suspend = true; + spin_unlock_irqrestore(&dev->lock, flags); +} +EXPORT_SYMBOL_GPL(gether_suspend); + +void gether_resume(struct gether *link) +{ + struct eth_dev *dev = link->ioport; + unsigned long flags; + + if (!dev) + return; + + if (netif_queue_stopped(dev->net)) + netif_start_queue(dev->net); + + spin_lock_irqsave(&dev->lock, flags); + link->is_suspend = false; + spin_unlock_irqrestore(&dev->lock, flags); +} +EXPORT_SYMBOL_GPL(gether_resume); + /* * gether_cleanup - remove Ethernet-over-USB device * Context: may sleep @@ -1176,6 +1238,7 @@ void gether_disconnect(struct gether *link) spin_lock(&dev->lock); dev->port_usb = NULL; + link->is_suspend = false; spin_unlock(&dev->lock); } EXPORT_SYMBOL_GPL(gether_disconnect); diff --git a/drivers/usb/gadget/function/u_ether.h b/drivers/usb/gadget/function/u_ether.h index 4014454..851ee10 100644 --- a/drivers/usb/gadget/function/u_ether.h +++ b/drivers/usb/gadget/function/u_ether.h @@ -79,6 +79,7 @@ struct gether { /* called on network open/close */ void (*open)(struct gether *); void (*close)(struct gether *); + bool is_suspend; }; #define DEFAULT_FILTER (USB_CDC_PACKET_TYPE_BROADCAST \ @@ -258,6 +259,9 @@ int gether_set_ifname(struct net_device *net, const char *name, int len); void gether_cleanup(struct eth_dev *dev); +void gether_suspend(struct gether *link); +void gether_resume(struct gether *link); + /* connect/disconnect is handled by individual functions */ struct net_device *gether_connect(struct gether *); void gether_disconnect(struct gether *);
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@quicinc.com> --- 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(+)