[v12,6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

Commit Message

Elson Roy Serrao March 16, 2023, 11:38 p.m. UTC

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 <[e-mail]>
---
 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(+)

Comments

Thinh Nguyen March 17, 2023, 12:11 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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;
> +}
> +

Elson Roy Serrao March 17, 2023, 5:59 p.m. UTC | #2

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 <[e-mail]>
>> ---
>>   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;
>> +}
>> +

Thinh Nguyen March 17, 2023, 9:28 p.m. UTC | #3

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 <[e-mail]>
> > > ---
> > >   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

Thinh Nguyen March 17, 2023, 11:20 p.m. UTC | #4

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 <[e-mail]>
> > > > ---
> > > >   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

Elson Roy Serrao March 18, 2023, 12:23 a.m. UTC | #5

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 <[e-mail]>
>>>>> ---
>>>>>    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

Thinh Nguyen March 18, 2023, 2:26 a.m. UTC | #6

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 <[e-mail]>
> > > > > > ---
> > > > > >    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

Elson Roy Serrao March 20, 2023, 5:42 p.m. UTC | #7

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 <[e-mail]>
>>>>>>> ---
>>>>>>>     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

Dan Carpenter March 21, 2023, 8:12 a.m. UTC | #8

Hi Elson,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230317-074030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/1679009888-8239-7-git-send-email-quic_eserrao%40quicinc.com
patch subject: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
config: riscv-randconfig-m031-20230319 (https://download.01.org/0day-ci/archive/20230321/[e-mail]/config)
compiler: riscv32-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>
| Reported-by: Dan Carpenter <[e-mail]>
| Link: https://lore.kernel.org/r/[e-mail]/

New smatch warnings:
drivers/usb/gadget/function/u_ether.c:474 eth_start_xmit() error: we previously assumed 'dev->port_usb' could be null (see line 466)
drivers/usb/gadget/function/u_ether.c:539 eth_start_xmit() warn: variable dereferenced before check 'dev->port_usb' (see line 474)

Old smatch warnings:
drivers/usb/gadget/function/u_ether.c:553 eth_start_xmit() error: we previously assumed 'skb' could be null (see line 491)

vim +474 drivers/usb/gadget/function/u_ether.c

25a79c41ce0ce8 drivers/usb/gadget/u_ether.c          Stephen Hemminger   2009-08-31  454  static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
25a79c41ce0ce8 drivers/usb/gadget/u_ether.c          Stephen Hemminger   2009-08-31  455  					struct net_device *net)
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  456  {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  457  	struct eth_dev		*dev = netdev_priv(net);
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  458  	int			length = 0;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  459  	int			retval;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  460  	struct usb_request	*req = NULL;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  461  	unsigned long		flags;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  462  	struct usb_ep		*in;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  463  	u16			cdc_filter;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  464  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  465  	spin_lock_irqsave(&dev->lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19 @466  	if (dev->port_usb) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  467  		in = dev->port_usb->in_ep;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  468  		cdc_filter = dev->port_usb->cdc_filter;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  469  	} else {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  470  		in = NULL;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  471  		cdc_filter = 0;

NULL on this path.

2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  472  	}
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  473  
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16 @474  	if (dev->port_usb->is_suspend) {
                                                                                                    ^^^^^^^^^^^^^^^
Dead.  (Both ->port_usb warnings are caused by this dereference).

11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  475  		DBG(dev, "Port suspended. Triggering wakeup\n");
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  476  		netif_stop_queue(net);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  477  		spin_unlock_irqrestore(&dev->lock, flags);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  478  		ether_wakeup_host(dev->port_usb);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  479  		return NETDEV_TX_BUSY;
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  480  	}
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao    2023-03-16  481  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  482  	spin_unlock_irqrestore(&dev->lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  483  
8ae01239609b29 drivers/usb/gadget/function/u_ether.c Maciej Żenczykowski 2021-07-01  484  	if (!in) {
8ae01239609b29 drivers/usb/gadget/function/u_ether.c Maciej Żenczykowski 2021-07-01  485  		if (skb)
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  486  			dev_kfree_skb_any(skb);
6ed106549d1747 drivers/usb/gadget/u_ether.c          Patrick McHardy     2009-06-23  487  		return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  488  	}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  489  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  490  	/* apply outgoing CDC or RNDIS filters */
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  491  	if (skb && !is_promisc(cdc_filter)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  492  		u8		*dest = skb->data;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  493  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  494  		if (is_multicast_ether_addr(dest)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  495  			u16	type;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  496  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  497  			/* ignores USB_CDC_PACKET_TYPE_MULTICAST and host
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  498  			 * SET_ETHERNET_MULTICAST_FILTERS requests
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  499  			 */
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  500  			if (is_broadcast_ether_addr(dest))
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  501  				type = USB_CDC_PACKET_TYPE_BROADCAST;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  502  			else
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  503  				type = USB_CDC_PACKET_TYPE_ALL_MULTICAST;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  504  			if (!(cdc_filter & type)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  505  				dev_kfree_skb_any(skb);
6ed106549d1747 drivers/usb/gadget/u_ether.c          Patrick McHardy     2009-06-23  506  				return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  507  			}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  508  		}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  509  		/* ignores USB_CDC_PACKET_TYPE_DIRECTED */
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  510  	}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  511  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  512  	spin_lock_irqsave(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  513  	/*
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  514  	 * this freelist can be empty if an interrupt triggered disconnect()
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  515  	 * and reconfigured the gadget (shutting down this queue) after the
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  516  	 * network stack decided to xmit but before we got the spinlock.
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  517  	 */
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  518  	if (list_empty(&dev->tx_reqs)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  519  		spin_unlock_irqrestore(&dev->req_lock, flags);
5b548140225c6b drivers/usb/gadget/u_ether.c          Patrick McHardy     2009-06-12  520  		return NETDEV_TX_BUSY;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  521  	}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  522  
fea14e68ff5e11 drivers/usb/gadget/function/u_ether.c Felipe Balbi        2017-03-22  523  	req = list_first_entry(&dev->tx_reqs, struct usb_request, list);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  524  	list_del(&req->list);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  525  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  526  	/* temporarily stop TX queue when the freelist empties */
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  527  	if (list_empty(&dev->tx_reqs))
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  528  		netif_stop_queue(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  529  	spin_unlock_irqrestore(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  530  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  531  	/* no buffer copies needed, unless the network stack did it
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  532  	 * or the hardware can't use skb buffers.
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  533  	 * or there's not enough space for extra headers we need
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  534  	 */
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  535  	if (dev->wrap) {
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c          Brian Niebuhr       2009-08-14  536  		unsigned long	flags;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  537  
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c          Brian Niebuhr       2009-08-14  538  		spin_lock_irqsave(&dev->lock, flags);
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c          Brian Niebuhr       2009-08-14 @539  		if (dev->port_usb)
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c          Brian Niebuhr       2009-08-14  540  			skb = dev->wrap(dev->port_usb, skb);
3a383cc0b8cc33 drivers/usb/gadget/function/u_ether.c Greg Kroah-Hartman  2016-09-19  541  		spin_unlock_irqrestore(&dev->lock, flags);
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  542  		if (!skb) {
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  543  			/* Multi frame CDC protocols may store the frame for
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  544  			 * later which is not a dropped frame.
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  545  			 */
88c09eacf560c3 drivers/usb/gadget/function/u_ether.c Peter Chen          2016-07-01  546  			if (dev->port_usb &&
3a383cc0b8cc33 drivers/usb/gadget/function/u_ether.c Greg Kroah-Hartman  2016-09-19  547  					dev->port_usb->supports_multi_frame)
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  548  				goto multiframe;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  549  			goto drop;
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  550  		}
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  551  	}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  552  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  553  	length = skb->len;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  554  	req->buf = skb->data;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  555  	req->context = skb;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  556  	req->complete = tx_complete;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  557  
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  558  	/* NCM requires no zlp if transfer is dwNtbInMaxSize */
79775f44183840 drivers/usb/gadget/function/u_ether.c Harish Jenny K N    2016-09-09  559  	if (dev->port_usb &&
79775f44183840 drivers/usb/gadget/function/u_ether.c Harish Jenny K N    2016-09-09  560  	    dev->port_usb->is_fixed &&
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  561  	    length == dev->port_usb->fixed_in_len &&
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  562  	    (length % in->maxpacket) == 0)
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  563  		req->zero = 0;
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  564  	else
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  565  		req->zero = 1;
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  566  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  567  	/* use zlp framing on tx for strict CDC-Ether conformance,
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  568  	 * though any robust network rx path ignores extra padding.
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  569  	 * and some hardware doesn't like to write zlps.
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  570  	 */
5c1168dbc50828 drivers/usb/gadget/u_ether.c          Yauheni Kaliuta     2010-12-08  571  	if (req->zero && !dev->zlp && (length % in->maxpacket) == 0)
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  572  		length++;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  573  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  574  	req->length = length;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  575  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  576  	retval = usb_ep_queue(in, req, GFP_ATOMIC);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  577  	switch (retval) {
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  578  	default:
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  579  		DBG(dev, "tx queue err %d\n", retval);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  580  		break;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  581  	case 0:
860e9538a9482b drivers/usb/gadget/function/u_ether.c Florian Westphal    2016-05-03  582  		netif_trans_update(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  583  		atomic_inc(&dev->tx_qlen);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  584  	}
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  585  
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  586  	if (retval) {
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c          Brian Niebuhr       2009-08-14  587  		dev_kfree_skb_any(skb);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  588  drop:
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  589  		dev->net->stats.tx_dropped++;
6d3865f9d41f15 drivers/usb/gadget/u_ether.c          Jim Baxter          2014-07-07  590  multiframe:
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  591  		spin_lock_irqsave(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  592  		if (list_empty(&dev->tx_reqs))
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  593  			netif_start_queue(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  594  		list_add(&req->list, &dev->tx_reqs);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  595  		spin_unlock_irqrestore(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  596  	}
6ed106549d1747 drivers/usb/gadget/u_ether.c          Patrick McHardy     2009-06-23  597  	return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c          David Brownell      2008-06-19  598  }

Elson Roy Serrao March 24, 2023, 12:12 a.m. UTC | #9

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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>     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
>

Thinh Nguyen March 24, 2023, 12:59 a.m. UTC | #10

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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >     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

Thinh Nguyen March 24, 2023, 1:14 a.m. UTC | #11

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 <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >     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

Patch

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 *);