mbox series

[v8,0/5] Add function suspend/resume and remote wakeup support

Message ID 1678731892-20503-1-git-send-email-quic_eserrao@quicinc.com
Headers show
Series Add function suspend/resume and remote wakeup support | expand

Message

Elson Serrao March 13, 2023, 6:24 p.m. UTC 
Changes in v8
 - Added else case to return error value while setting remote wakeup feature
   selector so that device will respond with a protocl stall

Changes in v7
 - Added a check to set device remote wakeup feature selector in ep0.c based on whether
   the device is configured for remote wakeup.
 - Commit message and usb_func_wakeup documentation changes.

Changes in v6
 - Combined usb_gadget_func_wakeup API with usb_func_wakeup API in composite layer
   so that there is only 1 API for triggering function remote wakeup for better error
   handling. Since function suspend is something specific to usb functions, better to
   keep the related APIs in composite layer and above. Also documented the usage and
   applicability of the usb_func_wakeup API.

Changes in v5
 - Add wakeup_armed check in patch2 in the link status change event handler
   so that resume gets triggeed only in the remote wakeup context.
 - Costmetic changes in patch3 and patch4

Changes in v4
 - Moved the wakeup bit check to bind function for warning the user at an early
   stage itself.
 - Added the remote wakeup configured check to gadget_wakeup() and func_wakeup()
   routines so that wakeup can be triggered only if user has configured it.
 - Cosmetic changes with respect to renaming the variables to reflect the operation
   better.

Changes in v3
 - Modified rw_capable flag to reflect the gadgets capability for wakeup
   signalling.
 - Added a check to configure wakeup bit in bmAttributes only if gadget
   is capable of triggering wakeup.
 - Implemented a gadget op for composite layer to inform UDC whether device
   is configured for remote wakeup.
 - Added a check in __usb_gadget_wakeup() API to trigger wakeup only if the
   device is configured for it.
 - Cosmetic changes in dwc3_gadget_func_wakeup() API.

Changes in v2
 - Added a flag to indicate whether the device is remote wakeup capable.
 - Added an async parameter to _dwc3_gadget_wakeup() API and few cosmetic
   changes.
 - Added flags to reflect the state of  function suspend and function remote
   wakeup to usb_function struct rather than function specific struct (f_ecm).
 - Changed the dwc3_gadget_func__wakeup() API to run synchronously by first
   checking the link state and then sending the device notification. Also
   added debug log for DEVICE_NOTIFICATION generic cmd.
 - Added changes to arm the device for remotewakeup/function remotewakeup
   only if device is capable.

An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
To achieve this an interface can invoke gadget_wakeup op and wait for the
device to come out of LPM. But the current polling based implementation
fails if the host takes a long time to drive the resume signaling specially
in high speed capable devices. Switching to an interrupt based approach is
more robust and efficient. This can be leveraged by enabling link status
change events and triggering a gadget resume when the link comes to active
state.

If the device is enhanced super-speed capable, individual interfaces can
also be put into suspend state. An interface can be in function suspend
state even when the device is not in suspend state. Function suspend state
is retained throughout the device suspend entry and exit process.
A function can be put to function suspend through FUNCTION_SUSPEND feature
selector sent by the host. This setup packet also decides whether that
function is capable of initiating a function remote wakeup. When the
function sends a wakeup notification to the host the link must be first
brought to a non-U0 state and then this notification is sent.

This change adds the infrastructure needed to support the above
functionalities.

Elson Roy Serrao (5):
  usb: gadget: Properly configure the device for remote wakeup
  usb: dwc3: Add remote wakeup handling
  usb: gadget: Add function wakeup support
  usb: dwc3: Add function suspend and function wakeup support
  usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

 drivers/usb/dwc3/core.h               |   5 ++
 drivers/usb/dwc3/debug.h              |   2 +
 drivers/usb/dwc3/ep0.c                |  19 +++---
 drivers/usb/dwc3/gadget.c             | 118 ++++++++++++++++++++++++++++++++--
 drivers/usb/gadget/composite.c        |  58 +++++++++++++++++
 drivers/usb/gadget/configfs.c         |   3 +
 drivers/usb/gadget/function/f_ecm.c   |  68 ++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.c |  63 ++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |   4 ++
 drivers/usb/gadget/udc/core.c         |  27 ++++++++
 drivers/usb/gadget/udc/trace.h        |   5 ++
 include/linux/usb/composite.h         |   8 +++
 include/linux/usb/gadget.h            |   9 +++
 13 files changed, 375 insertions(+), 14 deletions(-)

Comments

Thinh Nguyen March 14, 2023, 8:16 p.m. UTC | #1 
On Tue, Mar 14, 2023, Elson Serrao wrote:
> 
> 
> On 3/13/2023 1:27 PM, Thinh Nguyen wrote:
> > On Mon, Mar 13, 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   | 68 +++++++++++++++++++++++++++++++++++
> > >   drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
> > >   drivers/usb/gadget/function/u_ether.h |  4 +++
> > >   3 files changed, 135 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > index a7ab30e..d50c1a4 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,68 @@ 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)
> > > +{
> > > +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > +		USB_INTRF_STAT_FUNC_RW_CAP;
> > 
> > Need to check the usb configuration is if it's wakeup_capable.
> > 
> > > +}
> > > +
> > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > +{
> > > +	struct f_ecm *ecm = func_to_ecm(f);
> > > +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > +
> > > +	DBG(cdev, "func susp %u cmd\n", options);
> > > +
> > > +	f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
> > 
> > Same here. Check config's bmAttributes if it's remote wakeup capable
> > before arming for remote wakeup.
> > 
> Done. I will add that check for above two cases.
> > > +
> > > +	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;
> > 
> > Need to return negative error if SetFeature fails. We should fix the
> > composite layer to allow for protocal STALL here. Host needs to know if
> > it should proceed to put the function in suspend.
> > 
> > Thanks,
> > Thinh
> > 
> 
> Could you please clarify what SetFeature fail here means? The host puts the
> function in function suspend state through this SetFeature request.
> If the device is not configured for remote wakeup (bmAtrributes wakeup bit),
> like you mentioned above we should not arm the function for remote wakeup.
> But the host is free to put the function in function suspend state and wake
> it up through host initiated function resume right?
> 

I mean if we want to tell the host that a feature cannot be set or that
it doesn't exist, we should respond with a protocol STALL. How the host
respond to the rejected SetFeature request is up to the host. But we
should at least let the host know that.

I'm suggesting to remove the setting of value = 0 in composite.c:

-- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
                                ERROR(cdev,
                                      "func_suspend() returned error %d\n",
                                      value);
-                               value = 0;
                        }
                        break;
                }


i.e. we should allow the return value to go through.

Thanks,
Thinh
Thinh Nguyen March 14, 2023, 8:45 p.m. UTC | #2 
On Tue, Mar 14, 2023, Thinh Nguyen wrote:
> On Tue, Mar 14, 2023, Elson Serrao wrote:
> > 
> > 
> > On 3/13/2023 1:27 PM, Thinh Nguyen wrote:
> > > On Mon, Mar 13, 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   | 68 +++++++++++++++++++++++++++++++++++
> > > >   drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
> > > >   drivers/usb/gadget/function/u_ether.h |  4 +++
> > > >   3 files changed, 135 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > > index a7ab30e..d50c1a4 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,68 @@ 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)
> > > > +{
> > > > +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > +		USB_INTRF_STAT_FUNC_RW_CAP;
> > > 
> > > Need to check the usb configuration is if it's wakeup_capable.
> > > 
> > > > +}
> > > > +
> > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > +{
> > > > +	struct f_ecm *ecm = func_to_ecm(f);
> > > > +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > +
> > > > +	DBG(cdev, "func susp %u cmd\n", options);
> > > > +
> > > > +	f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
> > > 
> > > Same here. Check config's bmAttributes if it's remote wakeup capable
> > > before arming for remote wakeup.
> > > 
> > Done. I will add that check for above two cases.
> > > > +
> > > > +	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;
> > > 
> > > Need to return negative error if SetFeature fails. We should fix the
> > > composite layer to allow for protocal STALL here. Host needs to know if
> > > it should proceed to put the function in suspend.
> > > 
> > > Thanks,
> > > Thinh
> > > 
> > 
> > Could you please clarify what SetFeature fail here means? The host puts the
> > function in function suspend state through this SetFeature request.
> > If the device is not configured for remote wakeup (bmAtrributes wakeup bit),
> > like you mentioned above we should not arm the function for remote wakeup.
> > But the host is free to put the function in function suspend state and wake
> > it up through host initiated function resume right?
> > 
> 
> I mean if we want to tell the host that a feature cannot be set or that
> it doesn't exist, we should respond with a protocol STALL. How the host
> respond to the rejected SetFeature request is up to the host. But we
> should at least let the host know that.
> 
> I'm suggesting to remove the setting of value = 0 in composite.c:
> 
> -- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>                                 ERROR(cdev,
>                                       "func_suspend() returned error %d\n",
>                                       value);
> -                               value = 0;
>                         }
>                         break;
>                 }
> 
> 
> i.e. we should allow the return value to go through.
> 

Also, I imagine there are cases where we don't want the host to put the
device in suspend because it lacks remote wakeup. e.g. a HID device such
as a keyboard (though it's a bit odd to see one without remote wake
capability)

Thanks,
Thinh
Elson Serrao March 14, 2023, 9:03 p.m. UTC | #3 
On 3/14/2023 1:45 PM, Thinh Nguyen wrote:
> On Tue, Mar 14, 2023, Thinh Nguyen wrote:
>> On Tue, Mar 14, 2023, Elson Serrao wrote:
>>>
>>>
>>> On 3/13/2023 1:27 PM, Thinh Nguyen wrote:
>>>> On Mon, Mar 13, 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   | 68 +++++++++++++++++++++++++++++++++++
>>>>>    drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
>>>>>    drivers/usb/gadget/function/u_ether.h |  4 +++
>>>>>    3 files changed, 135 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>>>>> index a7ab30e..d50c1a4 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,68 @@ 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)
>>>>> +{
>>>>> +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>>>>> +		USB_INTRF_STAT_FUNC_RW_CAP;
>>>>
>>>> Need to check the usb configuration is if it's wakeup_capable.
>>>>
>>>>> +}
>>>>> +
>>>>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>>>>> +{
>>>>> +	struct f_ecm *ecm = func_to_ecm(f);
>>>>> +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>> +
>>>>> +	DBG(cdev, "func susp %u cmd\n", options);
>>>>> +
>>>>> +	f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
>>>>
>>>> Same here. Check config's bmAttributes if it's remote wakeup capable
>>>> before arming for remote wakeup.
>>>>
>>> Done. I will add that check for above two cases.
>>>>> +
>>>>> +	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;
>>>>
>>>> Need to return negative error if SetFeature fails. We should fix the
>>>> composite layer to allow for protocal STALL here. Host needs to know if
>>>> it should proceed to put the function in suspend.
>>>>
>>>> Thanks,
>>>> Thinh
>>>>
>>>
>>> Could you please clarify what SetFeature fail here means? The host puts the
>>> function in function suspend state through this SetFeature request.
>>> If the device is not configured for remote wakeup (bmAtrributes wakeup bit),
>>> like you mentioned above we should not arm the function for remote wakeup.
>>> But the host is free to put the function in function suspend state and wake
>>> it up through host initiated function resume right?
>>>
>>
>> I mean if we want to tell the host that a feature cannot be set or that
>> it doesn't exist, we should respond with a protocol STALL. How the host
>> respond to the rejected SetFeature request is up to the host. But we
>> should at least let the host know that.
>>
>> I'm suggesting to remove the setting of value = 0 in composite.c:
>>
>> -- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>                                  ERROR(cdev,
>>                                        "func_suspend() returned error %d\n",
>>                                        value);
>> -                               value = 0;
>>                          }
>>                          break;
>>                  }
>>
>>
>> i.e. we should allow the return value to go through.
>>
> 
> Also, I imagine there are cases where we don't want the host to put the
> device in suspend because it lacks remote wakeup. e.g. a HID device such
> as a keyboard (though it's a bit odd to see one without remote wake
> capability)
> 
> Thanks,
> Thinh

Sound good. I will make that change. Would you prefer this change (i.e 
removing value = 0  in composite.c) to be part of this series OR should 
I upload a separate change for this?

Thanks
Elson
Thinh Nguyen March 14, 2023, 9:34 p.m. UTC | #4 
On Tue, Mar 14, 2023, Elson Serrao wrote:
> 
> 
> On 3/14/2023 1:45 PM, Thinh Nguyen wrote:
> > On Tue, Mar 14, 2023, Thinh Nguyen wrote:
> > > On Tue, Mar 14, 2023, Elson Serrao wrote:
> > > > 
> > > > 
> > > > On 3/13/2023 1:27 PM, Thinh Nguyen wrote:
> > > > > On Mon, Mar 13, 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   | 68 +++++++++++++++++++++++++++++++++++
> > > > > >    drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
> > > > > >    drivers/usb/gadget/function/u_ether.h |  4 +++
> > > > > >    3 files changed, 135 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > > > > index a7ab30e..d50c1a4 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,68 @@ 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)
> > > > > > +{
> > > > > > +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > > > +		USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > 
> > > > > Need to check the usb configuration is if it's wakeup_capable.
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > > > +{
> > > > > > +	struct f_ecm *ecm = func_to_ecm(f);
> > > > > > +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > +
> > > > > > +	DBG(cdev, "func susp %u cmd\n", options);
> > > > > > +
> > > > > > +	f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
> > > > > 
> > > > > Same here. Check config's bmAttributes if it's remote wakeup capable
> > > > > before arming for remote wakeup.
> > > > > 
> > > > Done. I will add that check for above two cases.
> > > > > > +
> > > > > > +	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;
> > > > > 
> > > > > Need to return negative error if SetFeature fails. We should fix the
> > > > > composite layer to allow for protocal STALL here. Host needs to know if
> > > > > it should proceed to put the function in suspend.
> > > > > 
> > > > > Thanks,
> > > > > Thinh
> > > > > 
> > > > 
> > > > Could you please clarify what SetFeature fail here means? The host puts the
> > > > function in function suspend state through this SetFeature request.
> > > > If the device is not configured for remote wakeup (bmAtrributes wakeup bit),
> > > > like you mentioned above we should not arm the function for remote wakeup.
> > > > But the host is free to put the function in function suspend state and wake
> > > > it up through host initiated function resume right?
> > > > 
> > > 
> > > I mean if we want to tell the host that a feature cannot be set or that
> > > it doesn't exist, we should respond with a protocol STALL. How the host
> > > respond to the rejected SetFeature request is up to the host. But we
> > > should at least let the host know that.
> > > 
> > > I'm suggesting to remove the setting of value = 0 in composite.c:
> > > 
> > > -- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> > >                                  ERROR(cdev,
> > >                                        "func_suspend() returned error %d\n",
> > >                                        value);
> > > -                               value = 0;
> > >                          }
> > >                          break;
> > >                  }
> > > 
> > > 
> > > i.e. we should allow the return value to go through.
> > > 
> > 
> > Also, I imagine there are cases where we don't want the host to put the
> > device in suspend because it lacks remote wakeup. e.g. a HID device such
> > as a keyboard (though it's a bit odd to see one without remote wake
> > capability)
> > 
> > Thanks,
> > Thinh
> 
> Sound good. I will make that change. Would you prefer this change (i.e
> removing value = 0  in composite.c) to be part of this series OR should I
> upload a separate change for this?
> 

That change can be separated from this series. Let's try to get this
series in merged.

Thanks,
Thinh