[PATCH 0/4] usb: gadget: serial: add supend resume support

[PATCH 0/4] usb: gadget: serial: add supend resume support

[Fabrice Gasnier]

This series handles an issue seen when doing basic testing using ACM or SERIAL
gadget functions like in [1], and using the DWC2 gadget driver. It occurs when
the HOST has suspended the bus before testing, in this order:
- On the device:
  cat /dev/ttyGS<Y>
  Dmesg error log is seen on device side:
  configfs-gadget gadget: acm ttyGS0 can't notify serial state, -11
- On the host:
  echo test > /dev/ttyACM<X>
  The bus resumes, but the serial link isn't functional.

So, implement suspend/resume callbacks in gadget serial drivers to handle this.
There is a precursor patch in DWC2 to enable submitting usb requests from the
gadget resume routine, once in L0 state.

[1] https://www.kernel.org/doc/html/latest/usb/gadget-testing.html#testing-the-acm-function

Fabrice Gasnier (4):
  usb: dwc2: gadget: move gadget resume after the core is in L0 state
  usb: gadget: u_serial: add suspend resume callbacks
  usb: gadget: f_serial: add suspend resume callbacks
  usb: gadget: f_acm: add suspend resume callbacks

 drivers/usb/dwc2/core_intr.c           | 10 ++++--
 drivers/usb/gadget/function/f_acm.c    | 16 ++++++++++
 drivers/usb/gadget/function/f_serial.c | 16 ++++++++++
 drivers/usb/gadget/function/u_serial.c | 57 +++++++++++++++++++++++++++++-----
 drivers/usb/gadget/function/u_serial.h |  2 ++
 5 files changed, 90 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Fabrice Gasnier]

When the remote wakeup interrupt is triggered, lx_state is resumed from L2
to L0 state. But when the gadget resume is called, lx_state is still L2.
This prevents the resume callback to queue any request. Any attempt
to queue a request from resume callback will result in:
- "submit request only in active state" debug message to be issued
- dwc2_hsotg_ep_queue() returns -EAGAIN

Move the call to resume gadget after the core is put in DWC2_L0 state.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/usb/dwc2/core_intr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 876ff31..b8ebda5 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 	}
 
 	if (dwc2_is_device_mode(hsotg)) {
+		enum dwc2_lx_state lx_state = hsotg->lx_state;
+
 		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
 			dwc2_readl(hsotg, DSTS));
-		if (hsotg->lx_state == DWC2_L2) {
+		if (lx_state == DWC2_L2) {
 			u32 dctl = dwc2_readl(hsotg, DCTL);
 
 			/* Clear Remote Wakeup Signaling */
@@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			ret = dwc2_exit_partial_power_down(hsotg, true);
 			if (ret && (ret != -ENOTSUPP))
 				dev_err(hsotg->dev, "exit power_down failed\n");
-
-			call_gadget(hsotg, resume);
 		}
 		/* Change to L0 state */
 		hsotg->lx_state = DWC2_L0;
+
+		/* Gadget may queue new requests upon resume to L0 state */
+		if (lx_state == DWC2_L2)
+			call_gadget(hsotg, resume);
 	} else {
 		if (hsotg->params.power_down)
 			return;
-- 
2.7.4

[PATCH 2/4] usb: gadget: u_serial: add suspend resume callbacks

[Fabrice Gasnier]

Add suspend resume callbacks to handle the case seen when the bus is
suspended by the HOST, and the device opens the port (cat /dev/ttyGS0).

Gadget controller (like DWC2) doesn't accept usb requests to be queued in
this case (when in L2 state), from the gs_open() call. Error log is printed
- configfs-gadget gadget: acm ttyGS0 can't notify serial state, -11
If the HOST resumes (opens) the bus, the port still isn't functional.

Use suspend/resume callbacks to monitor the gadget suspended state by using
'suspended' flag. In case the port gets opened (cat /dev/ttyGS0), the I/O
stream will be delayed until the bus gets resumed by the HOST.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/usb/gadget/function/u_serial.c | 57 +++++++++++++++++++++++++++++-----
 drivers/usb/gadget/function/u_serial.h |  2 ++
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 8167d37..3cfc6e2 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -120,6 +120,8 @@ struct gs_port {
 	wait_queue_head_t	drain_wait;	/* wait while writes drain */
 	bool                    write_busy;
 	wait_queue_head_t	close_wait;
+	bool			suspended;	/* port suspended */
+	bool			start_delayed;	/* delay start when suspended */
 
 	/* REVISIT this state ... */
 	struct usb_cdc_line_coding port_line_coding;	/* 8-N-1 etc */
@@ -630,13 +632,19 @@ static int gs_open(struct tty_struct *tty, struct file *file)
 
 	/* if connected, start the I/O stream */
 	if (port->port_usb) {
-		struct gserial	*gser = port->port_usb;
-
-		pr_debug("gs_open: start ttyGS%d\n", port->port_num);
-		gs_start_io(port);
-
-		if (gser->connect)
-			gser->connect(gser);
+		/* if port is suspended, wait resume to start I/0 stream */
+		if (!port->suspended) {
+			struct gserial	*gser = port->port_usb;
+
+			pr_debug("gs_open: start ttyGS%d\n", port->port_num);
+			gs_start_io(port);
+
+			if (gser->connect)
+				gser->connect(gser);
+		} else {
+			pr_debug("delay start of ttyGS%d\n", port->port_num);
+			port->start_delayed = true;
+		}
 	}
 
 	pr_debug("gs_open: ttyGS%d (%p,%p)\n", port->port_num, tty, file);
@@ -680,7 +688,7 @@ static void gs_close(struct tty_struct *tty, struct file *file)
 	pr_debug("gs_close: ttyGS%d (%p,%p) ...\n", port->port_num, tty, file);
 
 	gser = port->port_usb;
-	if (gser && gser->disconnect)
+	if (gser && !port->suspended && gser->disconnect)
 		gser->disconnect(gser);
 
 	/* wait for circular write buffer to drain, disconnect, or at
@@ -708,6 +716,7 @@ static void gs_close(struct tty_struct *tty, struct file *file)
 	else
 		kfifo_reset(&port->port_write_buf);
 
+	port->start_delayed = false;
 	port->port.count = 0;
 	port->port.tty = NULL;
 
@@ -1403,6 +1412,38 @@ void gserial_disconnect(struct gserial *gser)
 }
 EXPORT_SYMBOL_GPL(gserial_disconnect);
 
+void gserial_suspend(struct gserial *gser)
+{
+	struct gs_port	*port = gser->ioport;
+	unsigned long	flags;
+
+	spin_lock_irqsave(&port->port_lock, flags);
+	port->suspended = true;
+	spin_unlock_irqrestore(&port->port_lock, flags);
+}
+EXPORT_SYMBOL_GPL(gserial_suspend);
+
+void gserial_resume(struct gserial *gser)
+{
+	struct gs_port *port = gser->ioport;
+	unsigned long	flags;
+
+	spin_lock_irqsave(&port->port_lock, flags);
+	port->suspended = false;
+	if (!port->start_delayed) {
+		spin_unlock_irqrestore(&port->port_lock, flags);
+		return;
+	}
+
+	pr_debug("delayed start ttyGS%d\n", port->port_num);
+	gs_start_io(port);
+	if (gser->connect)
+		gser->connect(gser);
+	port->start_delayed = false;
+	spin_unlock_irqrestore(&port->port_lock, flags);
+}
+EXPORT_SYMBOL_GPL(gserial_resume);
+
 static int userial_init(void)
 {
 	unsigned			i;
diff --git a/drivers/usb/gadget/function/u_serial.h b/drivers/usb/gadget/function/u_serial.h
index e5b08ab..009a959 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -68,6 +68,8 @@ ssize_t gserial_get_console(unsigned char port_num, char *page);
 /* connect/disconnect is handled by individual functions */
 int gserial_connect(struct gserial *, u8 port_num);
 void gserial_disconnect(struct gserial *);
+void gserial_suspend(struct gserial *p);
+void gserial_resume(struct gserial *p);
 
 /* functions are bound to configurations by a config or gadget driver */
 int gser_bind_config(struct usb_configuration *c, u8 port_num);
-- 
2.7.4

[PATCH 3/4] usb: gadget: f_serial: add suspend resume callbacks

[Fabrice Gasnier]

Add suspend resume callbacks to notify u_serial of the bus suspend/resume
state.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/usb/gadget/function/f_serial.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/function/f_serial.c b/drivers/usb/gadget/function/f_serial.c
index 1406255..e627138 100644
--- a/drivers/usb/gadget/function/f_serial.c
+++ b/drivers/usb/gadget/function/f_serial.c
@@ -348,6 +348,20 @@ static void gser_unbind(struct usb_configuration *c, struct usb_function *f)
 	usb_free_all_descriptors(f);
 }
 
+static void gser_resume(struct usb_function *f)
+{
+	struct f_gser *gser = func_to_gser(f);
+
+	gserial_resume(&gser->port);
+}
+
+static void gser_suspend(struct usb_function *f)
+{
+	struct f_gser *gser = func_to_gser(f);
+
+	gserial_suspend(&gser->port);
+}
+
 static struct usb_function *gser_alloc(struct usb_function_instance *fi)
 {
 	struct f_gser	*gser;
@@ -369,6 +383,8 @@ static struct usb_function *gser_alloc(struct usb_function_instance *fi)
 	gser->port.func.set_alt = gser_set_alt;
 	gser->port.func.disable = gser_disable;
 	gser->port.func.free_func = gser_free;
+	gser->port.func.resume = gser_resume;
+	gser->port.func.suspend = gser_suspend;
 
 	return &gser->port.func;
 }
-- 
2.7.4

[PATCH 4/4] usb: gadget: f_acm: add suspend resume callbacks

[Fabrice Gasnier]

Add suspend resume callbacks to notify u_serial of the bus suspend/resume
state.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/usb/gadget/function/f_acm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 7c152c2..200596e 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -723,6 +723,20 @@ static void acm_free_func(struct usb_function *f)
 	kfree(acm);
 }
 
+static void acm_resume(struct usb_function *f)
+{
+	struct f_acm *acm = func_to_acm(f);
+
+	gserial_resume(&acm->port);
+}
+
+static void acm_suspend(struct usb_function *f)
+{
+	struct f_acm *acm = func_to_acm(f);
+
+	gserial_suspend(&acm->port);
+}
+
 static struct usb_function *acm_alloc_func(struct usb_function_instance *fi)
 {
 	struct f_serial_opts *opts;
@@ -750,6 +764,8 @@ static struct usb_function *acm_alloc_func(struct usb_function_instance *fi)
 	acm->port_num = opts->port_num;
 	acm->port.func.unbind = acm_unbind;
 	acm->port.func.free_func = acm_free_func;
+	acm->port.func.resume = acm_resume;
+	acm->port.func.suspend = acm_suspend;
 
 	return &acm->port.func;
 }
-- 
2.7.4

Re: [PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Minas Harutyunyan]

Hi Fabrice,

On 4/21/2020 4:32 PM, Fabrice Gasnier wrote:
> When the remote wakeup interrupt is triggered, lx_state is resumed from L2
> to L0 state. But when the gadget resume is called, lx_state is still L2.
> This prevents the resume callback to queue any request. Any attempt
> to queue a request from resume callback will result in:
> - "submit request only in active state" debug message to be issued
> - dwc2_hsotg_ep_queue() returns -EAGAIN
> 
> Move the call to resume gadget after the core is put in DWC2_L0 state.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>   drivers/usb/dwc2/core_intr.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 876ff31..b8ebda5 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>   	}
>   
>   	if (dwc2_is_device_mode(hsotg)) {
> +		enum dwc2_lx_state lx_state = hsotg->lx_state;
> +
>   		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
>   			dwc2_readl(hsotg, DSTS));
> -		if (hsotg->lx_state == DWC2_L2) {
> +		if (lx_state == DWC2_L2) {
>   			u32 dctl = dwc2_readl(hsotg, DCTL);
>   
>   			/* Clear Remote Wakeup Signaling */
> @@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>   			ret = dwc2_exit_partial_power_down(hsotg, true);
>   			if (ret && (ret != -ENOTSUPP))
>   				dev_err(hsotg->dev, "exit power_down failed\n");
> -
> -			call_gadget(hsotg, resume);
>   		}
>   		/* Change to L0 state */
>   		hsotg->lx_state = DWC2_L0;
> +
> +		/* Gadget may queue new requests upon resume to L0 state */
> +		if (lx_state == DWC2_L2)
> +			call_gadget(hsotg, resume);
>   	} else {
>   		if (hsotg->params.power_down)
>   			return;
> 

What about below patch without introducing additional variable.

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 876ff31261d5..543865e31c72 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -416,6 +416,8 @@ static void dwc2_handle_wakeup_detected_intr(struct 
dwc2_hsotg *hsotg)
                         if (ret && (ret != -ENOTSUPP))
                                 dev_err(hsotg->dev, "exit power_down 
failed\n");

+                       /* Change to L0 state */
+                       hsotg->lx_state = DWC2_L0;
                         call_gadget(hsotg, resume);
                 }
                 /* Change to L0 state */


Thanks,
Minas

Re: [PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Fabrice Gasnier]

On 4/22/20 4:48 PM, Minas Harutyunyan wrote:
> Hi Fabrice,
> 
> On 4/21/2020 4:32 PM, Fabrice Gasnier wrote:
>> When the remote wakeup interrupt is triggered, lx_state is resumed from L2
>> to L0 state. But when the gadget resume is called, lx_state is still L2.
>> This prevents the resume callback to queue any request. Any attempt
>> to queue a request from resume callback will result in:
>> - "submit request only in active state" debug message to be issued
>> - dwc2_hsotg_ep_queue() returns -EAGAIN
>>
>> Move the call to resume gadget after the core is put in DWC2_L0 state.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>   drivers/usb/dwc2/core_intr.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 876ff31..b8ebda5 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>   	}
>>   
>>   	if (dwc2_is_device_mode(hsotg)) {
>> +		enum dwc2_lx_state lx_state = hsotg->lx_state;
>> +
>>   		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
>>   			dwc2_readl(hsotg, DSTS));
>> -		if (hsotg->lx_state == DWC2_L2) {
>> +		if (lx_state == DWC2_L2) {
>>   			u32 dctl = dwc2_readl(hsotg, DCTL);
>>   
>>   			/* Clear Remote Wakeup Signaling */
>> @@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>   			ret = dwc2_exit_partial_power_down(hsotg, true);
>>   			if (ret && (ret != -ENOTSUPP))
>>   				dev_err(hsotg->dev, "exit power_down failed\n");
>> -
>> -			call_gadget(hsotg, resume);
>>   		}
>>   		/* Change to L0 state */
>>   		hsotg->lx_state = DWC2_L0;
>> +
>> +		/* Gadget may queue new requests upon resume to L0 state */
>> +		if (lx_state == DWC2_L2)
>> +			call_gadget(hsotg, resume);
>>   	} else {
>>   		if (hsotg->params.power_down)
>>   			return;
>>
> 
> What about below patch without introducing additional variable.
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 876ff31261d5..543865e31c72 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -416,6 +416,8 @@ static void dwc2_handle_wakeup_detected_intr(struct 
> dwc2_hsotg *hsotg)
>                          if (ret && (ret != -ENOTSUPP))
>                                  dev_err(hsotg->dev, "exit power_down 
> failed\n");
> 
> +                       /* Change to L0 state */
> +                       hsotg->lx_state = DWC2_L0;

Hi Minas,

That was my first approach locally, but I added a variable to avoid do
it twice... few lines after.

But if you prefer, I can change in V2 ?

Please let me know.

Thanks,
Fabrice

>                          call_gadget(hsotg, resume);
>                  }
>                  /* Change to L0 state */
> 
> 
> Thanks,
> Minas
> 

Re: [PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Minas Harutyunyan]

Hi Fabrice,

On 4/22/2020 7:57 PM, Fabrice Gasnier wrote:
> On 4/22/20 4:48 PM, Minas Harutyunyan wrote:
>> Hi Fabrice,
>>
>> On 4/21/2020 4:32 PM, Fabrice Gasnier wrote:
>>> When the remote wakeup interrupt is triggered, lx_state is resumed from L2
>>> to L0 state. But when the gadget resume is called, lx_state is still L2.
>>> This prevents the resume callback to queue any request. Any attempt
>>> to queue a request from resume callback will result in:
>>> - "submit request only in active state" debug message to be issued
>>> - dwc2_hsotg_ep_queue() returns -EAGAIN
>>>
>>> Move the call to resume gadget after the core is put in DWC2_L0 state.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> ---
>>>    drivers/usb/dwc2/core_intr.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 876ff31..b8ebda5 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>    	}
>>>    
>>>    	if (dwc2_is_device_mode(hsotg)) {
>>> +		enum dwc2_lx_state lx_state = hsotg->lx_state;
>>> +
>>>    		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
>>>    			dwc2_readl(hsotg, DSTS));
>>> -		if (hsotg->lx_state == DWC2_L2) {
>>> +		if (lx_state == DWC2_L2) {
>>>    			u32 dctl = dwc2_readl(hsotg, DCTL);
>>>    
>>>    			/* Clear Remote Wakeup Signaling */
>>> @@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>    			ret = dwc2_exit_partial_power_down(hsotg, true);
>>>    			if (ret && (ret != -ENOTSUPP))
>>>    				dev_err(hsotg->dev, "exit power_down failed\n");
>>> -
>>> -			call_gadget(hsotg, resume);
>>>    		}
>>>    		/* Change to L0 state */
>>>    		hsotg->lx_state = DWC2_L0;
>>> +
>>> +		/* Gadget may queue new requests upon resume to L0 state */
>>> +		if (lx_state == DWC2_L2)
>>> +			call_gadget(hsotg, resume);
>>>    	} else {
>>>    		if (hsotg->params.power_down)
>>>    			return;
>>>
>>
>> What about below patch without introducing additional variable.
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 876ff31261d5..543865e31c72 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -416,6 +416,8 @@ static void dwc2_handle_wakeup_detected_intr(struct
>> dwc2_hsotg *hsotg)
>>                           if (ret && (ret != -ENOTSUPP))
>>                                   dev_err(hsotg->dev, "exit power_down
>> failed\n");
>>
>> +                       /* Change to L0 state */
>> +                       hsotg->lx_state = DWC2_L0;
> 
> Hi Minas,
> 
> That was my first approach locally, but I added a variable to avoid do
> it twice... few lines after.
> 
> But if you prefer, I can change in V2 ?
> 
> Please let me know.
> 
> Thanks,
> Fabrice
> 
>>                           call_gadget(hsotg, resume);
>>                   }
>>                   /* Change to L0 state */
>>
>>
>> Thanks,
>> Minas
>>
To avoid twice setting lx_state you can add 'else' before second setting.

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 876ff31261d5..f59dabd46e60 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -416,10 +416,14 @@ static void 
dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
                         if (ret && (ret != -ENOTSUPP))
                                 dev_err(hsotg->dev, "exit power_down 
failed\n");

+                       /* Change to L0 state */
+                       hsotg->lx_state = DWC2_L0;
                         call_gadget(hsotg, resume);
                 }
-               /* Change to L0 state */
-               hsotg->lx_state = DWC2_L0;
+               else {
+                       /* Change to L0 state */
+                       hsotg->lx_state = DWC2_L0;
+               }
         } else {
                 if (hsotg->params.power_down)
                         return;



Am I missed something?

Thanks,
Minas

Re: [PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Fabrice Gasnier]

On 4/22/20 6:16 PM, Minas Harutyunyan wrote:
> Hi Fabrice,
> 
> On 4/22/2020 7:57 PM, Fabrice Gasnier wrote:
>> On 4/22/20 4:48 PM, Minas Harutyunyan wrote:
>>> Hi Fabrice,
>>>
>>> On 4/21/2020 4:32 PM, Fabrice Gasnier wrote:
>>>> When the remote wakeup interrupt is triggered, lx_state is resumed from L2
>>>> to L0 state. But when the gadget resume is called, lx_state is still L2.
>>>> This prevents the resume callback to queue any request. Any attempt
>>>> to queue a request from resume callback will result in:
>>>> - "submit request only in active state" debug message to be issued
>>>> - dwc2_hsotg_ep_queue() returns -EAGAIN
>>>>
>>>> Move the call to resume gadget after the core is put in DWC2_L0 state.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> ---
>>>>    drivers/usb/dwc2/core_intr.c | 10 +++++++---
>>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index 876ff31..b8ebda5 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>    	}
>>>>    
>>>>    	if (dwc2_is_device_mode(hsotg)) {
>>>> +		enum dwc2_lx_state lx_state = hsotg->lx_state;
>>>> +
>>>>    		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
>>>>    			dwc2_readl(hsotg, DSTS));
>>>> -		if (hsotg->lx_state == DWC2_L2) {
>>>> +		if (lx_state == DWC2_L2) {
>>>>    			u32 dctl = dwc2_readl(hsotg, DCTL);
>>>>    
>>>>    			/* Clear Remote Wakeup Signaling */
>>>> @@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>    			ret = dwc2_exit_partial_power_down(hsotg, true);
>>>>    			if (ret && (ret != -ENOTSUPP))
>>>>    				dev_err(hsotg->dev, "exit power_down failed\n");
>>>> -
>>>> -			call_gadget(hsotg, resume);
>>>>    		}
>>>>    		/* Change to L0 state */
>>>>    		hsotg->lx_state = DWC2_L0;
>>>> +
>>>> +		/* Gadget may queue new requests upon resume to L0 state */
>>>> +		if (lx_state == DWC2_L2)
>>>> +			call_gadget(hsotg, resume);
>>>>    	} else {
>>>>    		if (hsotg->params.power_down)
>>>>    			return;
>>>>
>>>
>>> What about below patch without introducing additional variable.
>>>
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 876ff31261d5..543865e31c72 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -416,6 +416,8 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>> dwc2_hsotg *hsotg)
>>>                           if (ret && (ret != -ENOTSUPP))
>>>                                   dev_err(hsotg->dev, "exit power_down
>>> failed\n");
>>>
>>> +                       /* Change to L0 state */
>>> +                       hsotg->lx_state = DWC2_L0;
>>
>> Hi Minas,
>>
>> That was my first approach locally, but I added a variable to avoid do
>> it twice... few lines after.
>>
>> But if you prefer, I can change in V2 ?
>>
>> Please let me know.
>>
>> Thanks,
>> Fabrice
>>
>>>                           call_gadget(hsotg, resume);
>>>                   }
>>>                   /* Change to L0 state */
>>>
>>>
>>> Thanks,
>>> Minas
>>>
> To avoid twice setting lx_state you can add 'else' before second setting.

Hi Minas,

Thanks for your quick answer. Sure, I'll update it in v2.

Also, do you think this can/should be marked as a fix ?
- e.g. add a Fixes tag?

Fixes: f81f46e1f530 ("usb: dwc2: implement hibernation during bus
suspend/resume")

Please advise,
Best Regards,
Fabrice

> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 876ff31261d5..f59dabd46e60 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -416,10 +416,14 @@ static void 
> dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>                          if (ret && (ret != -ENOTSUPP))
>                                  dev_err(hsotg->dev, "exit power_down 
> failed\n");
> 
> +                       /* Change to L0 state */
> +                       hsotg->lx_state = DWC2_L0;
>                          call_gadget(hsotg, resume);
>                  }
> -               /* Change to L0 state */
> -               hsotg->lx_state = DWC2_L0;
> +               else {
> +                       /* Change to L0 state */
> +                       hsotg->lx_state = DWC2_L0;
> +               }
>          } else {
>                  if (hsotg->params.power_down)
>                          return;
> 
> 
> 
> Am I missed something?
> 
> Thanks,
> Minas
> 

Re: [PATCH 1/4] usb: dwc2: gadget: move gadget resume after the core is in L0 state

[Minas Harutyunyan]

Hi Fabrice,

On 4/23/2020 1:27 PM, Fabrice Gasnier wrote:
> On 4/22/20 6:16 PM, Minas Harutyunyan wrote:
>> Hi Fabrice,
>>
>> On 4/22/2020 7:57 PM, Fabrice Gasnier wrote:
>>> On 4/22/20 4:48 PM, Minas Harutyunyan wrote:
>>>> Hi Fabrice,
>>>>
>>>> On 4/21/2020 4:32 PM, Fabrice Gasnier wrote:
>>>>> When the remote wakeup interrupt is triggered, lx_state is resumed from L2
>>>>> to L0 state. But when the gadget resume is called, lx_state is still L2.
>>>>> This prevents the resume callback to queue any request. Any attempt
>>>>> to queue a request from resume callback will result in:
>>>>> - "submit request only in active state" debug message to be issued
>>>>> - dwc2_hsotg_ep_queue() returns -EAGAIN
>>>>>
>>>>> Move the call to resume gadget after the core is put in DWC2_L0 state.
>>>>>
>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>> ---
>>>>>     drivers/usb/dwc2/core_intr.c | 10 +++++++---
>>>>>     1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>>> index 876ff31..b8ebda5 100644
>>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>>> @@ -404,9 +404,11 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>>     	}
>>>>>     
>>>>>     	if (dwc2_is_device_mode(hsotg)) {
>>>>> +		enum dwc2_lx_state lx_state = hsotg->lx_state;
>>>>> +
>>>>>     		dev_dbg(hsotg->dev, "DSTS=0x%0x\n",
>>>>>     			dwc2_readl(hsotg, DSTS));
>>>>> -		if (hsotg->lx_state == DWC2_L2) {
>>>>> +		if (lx_state == DWC2_L2) {
>>>>>     			u32 dctl = dwc2_readl(hsotg, DCTL);
>>>>>     
>>>>>     			/* Clear Remote Wakeup Signaling */
>>>>> @@ -415,11 +417,13 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>>     			ret = dwc2_exit_partial_power_down(hsotg, true);
>>>>>     			if (ret && (ret != -ENOTSUPP))
>>>>>     				dev_err(hsotg->dev, "exit power_down failed\n");
>>>>> -
>>>>> -			call_gadget(hsotg, resume);
>>>>>     		}
>>>>>     		/* Change to L0 state */
>>>>>     		hsotg->lx_state = DWC2_L0;
>>>>> +
>>>>> +		/* Gadget may queue new requests upon resume to L0 state */
>>>>> +		if (lx_state == DWC2_L2)
>>>>> +			call_gadget(hsotg, resume);
>>>>>     	} else {
>>>>>     		if (hsotg->params.power_down)
>>>>>     			return;
>>>>>
>>>>
>>>> What about below patch without introducing additional variable.
>>>>
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index 876ff31261d5..543865e31c72 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -416,6 +416,8 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>>> dwc2_hsotg *hsotg)
>>>>                            if (ret && (ret != -ENOTSUPP))
>>>>                                    dev_err(hsotg->dev, "exit power_down
>>>> failed\n");
>>>>
>>>> +                       /* Change to L0 state */
>>>> +                       hsotg->lx_state = DWC2_L0;
>>>
>>> Hi Minas,
>>>
>>> That was my first approach locally, but I added a variable to avoid do
>>> it twice... few lines after.
>>>
>>> But if you prefer, I can change in V2 ?
>>>
>>> Please let me know.
>>>
>>> Thanks,
>>> Fabrice
>>>
>>>>                            call_gadget(hsotg, resume);
>>>>                    }
>>>>                    /* Change to L0 state */
>>>>
>>>>
>>>> Thanks,
>>>> Minas
>>>>
>> To avoid twice setting lx_state you can add 'else' before second setting.
> 
> Hi Minas,
> 
> Thanks for your quick answer. Sure, I'll update it in v2.
> 
> Also, do you think this can/should be marked as a fix ?
> - e.g. add a Fixes tag?
> 
> Fixes: f81f46e1f530 ("usb: dwc2: implement hibernation during bus
> suspend/resume")
> 

Yes, go ahead to submit V2 with mentioned "Fixes:" tag.

Thanks,
Minas


> Please advise,
> Best Regards,
> Fabrice
> 
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 876ff31261d5..f59dabd46e60 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -416,10 +416,14 @@ static void
>> dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>                           if (ret && (ret != -ENOTSUPP))
>>                                   dev_err(hsotg->dev, "exit power_down
>> failed\n");
>>
>> +                       /* Change to L0 state */
>> +                       hsotg->lx_state = DWC2_L0;
>>                           call_gadget(hsotg, resume);
>>                   }
>> -               /* Change to L0 state */
>> -               hsotg->lx_state = DWC2_L0;
>> +               else {
>> +                       /* Change to L0 state */
>> +                       hsotg->lx_state = DWC2_L0;
>> +               }
>>           } else {
>>                   if (hsotg->params.power_down)
>>                           return;
>>
>>
>>
>> Am I missed something?
>>
>> Thanks,
>> Minas
>>