[PATCH] usb: chipidea: Fix missing resume call after suspend

[PATCH] usb: chipidea: Fix missing resume call after suspend

[Bernhard Walle]

We have a i.MX53-based hardware (quite similar to the i.MX53 QSB from
Freescale/NXP). I'm reading the ..../ci_hdrc.0/gadget/suspended sysfs
file to find out whether a PC is connected to the USB gadget. With old
kernel versions, this worked. However, with kernel 4.9 this didn't work.

When the host is suspended once, it never sets back the suspended status
to 0. The reason is that this seems to be done in the resume handler,
which should be executed in the interrupt handler:

udc_irq:

...
     if (USBi_PCI & intr) {
             ci->gadget.speed = hw_port_is_high_speed(ci) ?
                     USB_SPEED_HIGH : USB_SPEED_FULL;
             if (ci->suspended && ci->driver->resume) {
                     spin_unlock(&ci->lock);
                     ci->driver->resume(&ci->gadget);
                     spin_lock(&ci->lock);
                     ci->suspended = 0;
             }
     }
...

However, ci->suspended is already 0 here because _gadget_stop_activity
is called before. So the resume handler never gets called. The obvious
solution is to not touch ci->suspended in _gadget_stop_activity and
to trust the interrupt handler to set it back (and to modify it to set
ci->suspended to 0 even if ci->driver->resume is NULL).

This code works on my platform. However, since I didn't write the driver
and since I have no deep understanding of it, I cannot determine if
there are any negative side effects, so I hope to get some review here.

Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 drivers/usb/chipidea/udc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index f88e9157fad0..0c780ba39b37 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -712,7 +712,6 @@ static int _gadget_stop_activity(struct usb_gadget *gadget)
 	spin_lock_irqsave(&ci->lock, flags);
 	ci->gadget.speed = USB_SPEED_UNKNOWN;
 	ci->remote_wakeup = 0;
-	ci->suspended = 0;
 	spin_unlock_irqrestore(&ci->lock, flags);
 
 	/* flush all endpoints */
@@ -1845,10 +1844,12 @@ static irqreturn_t udc_irq(struct ci_hdrc *ci)
 		if (USBi_PCI & intr) {
 			ci->gadget.speed = hw_port_is_high_speed(ci) ?
 				USB_SPEED_HIGH : USB_SPEED_FULL;
-			if (ci->suspended && ci->driver->resume) {
-				spin_unlock(&ci->lock);
-				ci->driver->resume(&ci->gadget);
-				spin_lock(&ci->lock);
+			if (ci->suspended) {
+				if (ci->driver->resume) {
+					spin_unlock(&ci->lock);
+					ci->driver->resume(&ci->gadget);
+					spin_lock(&ci->lock);
+				}
 				ci->suspended = 0;
 			}
 		}
-- 
2.12.2

Re: [PATCH] usb: chipidea: Fix missing resume call after suspend

[Peter Chen]

On Fri, Apr 21, 2017 at 12:10:53PM +0200, Bernhard Walle wrote:
> We have a i.MX53-based hardware (quite similar to the i.MX53 QSB from
> Freescale/NXP). I'm reading the ..../ci_hdrc.0/gadget/suspended sysfs
> file to find out whether a PC is connected to the USB gadget. With old
> kernel versions, this worked. However, with kernel 4.9 this didn't work.
> 
> When the host is suspended once, it never sets back the suspended status
> to 0. The reason is that this seems to be done in the resume handler,
> which should be executed in the interrupt handler:
> 
> udc_irq:
> 
> ...
>      if (USBi_PCI & intr) {
>              ci->gadget.speed = hw_port_is_high_speed(ci) ?
>                      USB_SPEED_HIGH : USB_SPEED_FULL;
>              if (ci->suspended && ci->driver->resume) {
>                      spin_unlock(&ci->lock);
>                      ci->driver->resume(&ci->gadget);
>                      spin_lock(&ci->lock);
>                      ci->suspended = 0;
>              }
>      }
> ...
> 
> However, ci->suspended is already 0 here because _gadget_stop_activity
> is called before. So the resume handler never gets called. The obvious
> solution is to not touch ci->suspended in _gadget_stop_activity and
> to trust the interrupt handler to set it back (and to modify it to set
> ci->suspended to 0 even if ci->driver->resume is NULL).

The current code logic is:
- When the resume is received from host, the ci->dirver->resume is
called, and suspended is cleared.
- When the reset is received from host, the isr_reset_handler is called,
and suspended is cleared by _gadget_stop_activity. Since reset is
called, so ci->driver->resume doesn't need to be called.

There is a patch to fix clear suspended even the ci->driver->resume is
NULL at v4.12-rc1.

      usb: chipidea: udc: update gadget state after bus resume

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: chipidea: Fix missing resume call after suspend

[Bernhard Walle]

Hi,

> Peter Chen <peter.chen@nxp.com> hat am 24. April 2017 um 05:51 geschrieben:
> > 
> The current code logic is:
> - When the resume is received from host, the ci->dirver->resume is
> called, and suspended is cleared.
> - When the reset is received from host, the isr_reset_handler is called,
> and suspended is cleared by _gadget_stop_activity. Since reset is
> called, so ci->driver->resume doesn't need to be called.

My problem is that dump_stack() doesn't show the complete stackframe. What I see from debug messages is that when I plug a cable (after the host has been suspended) then _gadget_stop_activity is executed before udc_irq. 

My first attempt was to save ci->suspended at the beginning of udc_irq (i.e. before isr_reset_handler is called), but that doesn't work. Even in the beginning of udc_irq ci->suspended is 0. So isr_reset_handler is called before.

The result is that when I unplug a cable and attach it again, driver->suspended has been called but driver->resume doesn't get called.

> There is a patch to fix clear suspended even the ci->driver->resume is
> NULL at v4.12-rc1.
> 
>       usb: chipidea: udc: update gadget state after bus resume

Thanks for the hint. That makes the second part of my patch irrelevant, but the first part (removing the ci->suspended = 0) is needed in order to make my setup working.

Any idea to find the cause why reset is called before resume? According to your explanation, that shouldn't be the case?



Regards,
Bernhard

Re: [PATCH] usb: chipidea: Fix missing resume call after suspend

[Peter Chen]

On Mon, Apr 24, 2017 at 01:25:21PM +0000, Bernhard Walle wrote:
> Hi,
> 
> > Peter Chen <peter.chen@nxp.com> hat am 24. April 2017 um 05:51 geschrieben:
> > > 
> > The current code logic is:
> > - When the resume is received from host, the ci->dirver->resume is
> > called, and suspended is cleared.
> > - When the reset is received from host, the isr_reset_handler is called,
> > and suspended is cleared by _gadget_stop_activity. Since reset is
> > called, so ci->driver->resume doesn't need to be called.
> 
> My problem is that dump_stack() doesn't show the complete stackframe. What I see from debug messages is that when I plug a cable (after the host has been suspended) then _gadget_stop_activity is executed before udc_irq. 
> 
> My first attempt was to save ci->suspended at the beginning of udc_irq (i.e. before isr_reset_handler is called), but that doesn't work. Even in the beginning of udc_irq ci->suspended is 0. So isr_reset_handler is called before.
> 
> The result is that when I unplug a cable and attach it again, driver->suspended has been called but driver->resume doesn't get called.
> 
> > There is a patch to fix clear suspended even the ci->driver->resume is
> > NULL at v4.12-rc1.
> > 
> >       usb: chipidea: udc: update gadget state after bus resume
> 
> Thanks for the hint. That makes the second part of my patch irrelevant, but the first part (removing the ci->suspended = 0) is needed in order to make my setup working.
> 
> Any idea to find the cause why reset is called before resume? According to your explanation, that shouldn't be the case?
> 

Since you unplug the cable first, and plug in again. The driver will
treat it as connection but not resume event. You may use 
/sys/class/udc/ci_hdrc.0/state to get udc's connection, eg "not attached"
or other states to indicate connection.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: chipidea: Fix missing resume call after suspend

[Bernhard Walle]

Hi Peter,

> Peter Chen <peter.chen@nxp.com> hat am 25. April 2017 um 10:14 geschrieben:
> 
> Since you unplug the cable first, and plug in again. The driver will
> treat it as connection but not resume event. You may use
> /sys/class/udc/ci_hdrc.0/state to get udc's connection, eg "not attached"
> or other states to indicate connection.

Thanks for the hint. This works for me.

Regards,
Bernhard