mbox series

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

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

Message

Elson Serrao Feb. 16, 2023, 10:29 p.m. UTC 
Changes in v5
 - Add wakeup_armed check in patch2 in the link status change event handler
   so that resume gets triggeed only in the remote wakeup context.
 - Costmetic changes in patch3 and patch4 

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

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

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

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

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

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

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

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

Comments

Thinh Nguyen Feb. 16, 2023, 11:58 p.m. UTC | #1 
On Thu, Feb 16, 2023, Elson Roy Serrao wrote:
> A function which is in function suspend state has to send a
> function wake notification to the host in case it needs to
> exit from this state and resume data transfer. Add support to
> handle such requests by exposing a new gadget op.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
>  drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
>  include/linux/usb/composite.h  |  6 ++++++
>  include/linux/usb/gadget.h     |  4 ++++
>  4 files changed, 55 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index a37a8f4..f649f997 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
>  }
>  EXPORT_SYMBOL_GPL(usb_interface_id);
>  
> +int usb_func_wakeup(struct usb_function *func)
> +{
> +	int ret, id;
> +
> +	if (!func->func_wakeup_armed) {
> +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
> +		return -EINVAL;
> +	}
> +
> +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
> +		if (func->config->interface[id] == func)
> +			break;
> +
> +	if (id == MAX_CONFIG_INTERFACES) {
> +		ERROR(func->config->cdev, "Invalid function\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_func_wakeup);
> +
>  static u8 encode_bMaxPower(enum usb_device_speed speed,
>  		struct usb_configuration *c)
>  {
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 3dcbba7..59e7a7e 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -846,6 +846,27 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_activate);
>  
> +/**
> + * usb_gadget_func_wakeup - sends function wake notification to the host.
> + * @gadget: controller used to wake up the host
> + * @interface_id: interface on which function wake notification is sent.

Device notification is only applicable for eSS devices. What will happen
if the device is operating in lower speed and the driver calls this
function?

Thanks,
Thinh

> + *
> + * On completion, function wake notification is sent. If the device is in
> + * low power state it tries to bring the device to active state before sending
> + * the wake notification. Since it is a synchronous call, caller must take care
> + * of not calling it in interrupt context.
> + *
> + * Returns zero on success, else negative errno.
> + */
> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
> +{
> +	if (!gadget->ops->func_wakeup)
> +		return -EOPNOTSUPP;
> +
> +	return gadget->ops->func_wakeup(gadget, intf_id);
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);
Elson Serrao Feb. 17, 2023, 12:38 a.m. UTC | #2 
On 2/16/2023 3:58 PM, Thinh Nguyen wrote:
> On Thu, Feb 16, 2023, Elson Roy Serrao wrote:
>> A function which is in function suspend state has to send a
>> function wake notification to the host in case it needs to
>> exit from this state and resume data transfer. Add support to
>> handle such requests by exposing a new gadget op.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
>>   drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
>>   include/linux/usb/composite.h  |  6 ++++++
>>   include/linux/usb/gadget.h     |  4 ++++
>>   4 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index a37a8f4..f649f997 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
>>   }
>>   EXPORT_SYMBOL_GPL(usb_interface_id);
>>   
>> +int usb_func_wakeup(struct usb_function *func)
>> +{
>> +	int ret, id;
>> +
>> +	if (!func->func_wakeup_armed) {
>> +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
>> +		if (func->config->interface[id] == func)
>> +			break;
>> +
>> +	if (id == MAX_CONFIG_INTERFACES) {
>> +		ERROR(func->config->cdev, "Invalid function\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_func_wakeup);
>> +
>>   static u8 encode_bMaxPower(enum usb_device_speed speed,
>>   		struct usb_configuration *c)
>>   {
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index 3dcbba7..59e7a7e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -846,6 +846,27 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_gadget_activate);
>>   
>> +/**
>> + * usb_gadget_func_wakeup - sends function wake notification to the host.
>> + * @gadget: controller used to wake up the host
>> + * @interface_id: interface on which function wake notification is sent.
> 
> Device notification is only applicable for eSS devices. What will happen
> if the device is operating in lower speed and the driver calls this
> function?
> 
> Thanks,
> Thinh
> 

Since the non-eSS devices dont support function suspend, the function 
suspend feature selector is not sent by the host and the function is not 
armed for sending function remote wakeup. So the usb_func_wakeup() API 
that is called from the function drivers fails the attempt

int usb_func_wakeup(struct usb_function *func)
{
	int ret, id;

	if (!func->func_wakeup_armed) {
		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
		return -EINVAL;
	}

Let me know if its better to add an explicit speed check as well here.

Thanks
Elson

>> + *
>> + * On completion, function wake notification is sent. If the device is in
>> + * low power state it tries to bring the device to active state before sending
>> + * the wake notification. Since it is a synchronous call, caller must take care
>> + * of not calling it in interrupt context.
>> + *
>> + * Returns zero on success, else negative errno.
>> + */
>> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
>> +{
>> +	if (!gadget->ops->func_wakeup)
>> +		return -EOPNOTSUPP;
>> +
>> +	return gadget->ops->func_wakeup(gadget, intf_id);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);
Thinh Nguyen Feb. 17, 2023, 2:02 a.m. UTC | #3 
On Thu, Feb 16, 2023, Elson Serrao wrote:
> 
> 
> On 2/16/2023 3:58 PM, Thinh Nguyen wrote:
> > On Thu, Feb 16, 2023, Elson Roy Serrao wrote:
> > > A function which is in function suspend state has to send a
> > > function wake notification to the host in case it needs to
> > > exit from this state and resume data transfer. Add support to
> > > handle such requests by exposing a new gadget op.
> > > 
> > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > ---
> > >   drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
> > >   drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
> > >   include/linux/usb/composite.h  |  6 ++++++
> > >   include/linux/usb/gadget.h     |  4 ++++
> > >   4 files changed, 55 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index a37a8f4..f649f997 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
> > >   }
> > >   EXPORT_SYMBOL_GPL(usb_interface_id);
> > > +int usb_func_wakeup(struct usb_function *func)
> > > +{
> > > +	int ret, id;
> > > +
> > > +	if (!func->func_wakeup_armed) {
> > > +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
> > > +		if (func->config->interface[id] == func)
> > > +			break;
> > > +
> > > +	if (id == MAX_CONFIG_INTERFACES) {
> > > +		ERROR(func->config->cdev, "Invalid function\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_func_wakeup);
> > > +
> > >   static u8 encode_bMaxPower(enum usb_device_speed speed,
> > >   		struct usb_configuration *c)
> > >   {
> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > index 3dcbba7..59e7a7e 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -846,6 +846,27 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> > >   }
> > >   EXPORT_SYMBOL_GPL(usb_gadget_activate);
> > > +/**
> > > + * usb_gadget_func_wakeup - sends function wake notification to the host.
> > > + * @gadget: controller used to wake up the host
> > > + * @interface_id: interface on which function wake notification is sent.
> > 
> > Device notification is only applicable for eSS devices. What will happen
> > if the device is operating in lower speed and the driver calls this
> > function?
> > 
> > Thanks,
> > Thinh
> > 
> 
> Since the non-eSS devices dont support function suspend, the function
> suspend feature selector is not sent by the host and the function is not
> armed for sending function remote wakeup. So the usb_func_wakeup() API that
> is called from the function drivers fails the attempt

We may be able to say that for usb_func_wakeup() and document more on
the func_wakeup_armed flag. However, the driver can still call
usb_gadget_func_wakeup() directly right? Can you document the expected
behavior of usb_gadget_func_wakeup(). Would it fall back to behave like
usb_gadget_wakeup()? Or would it become no-op? Whichever you choose,
please document it.

Thanks,
Thinh

> 
> int usb_func_wakeup(struct usb_function *func)
> {
> 	int ret, id;
> 
> 	if (!func->func_wakeup_armed) {
> 		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
> 		return -EINVAL;
> 	}
> 
> Let me know if its better to add an explicit speed check as well here.
> 
> Thanks
> Elson
>