diff mbox

[4.4-rc1,regression] pxa27x_udc and suspend/resume

Message ID 87io4zgcc1.fsf@saruman.tx.rr.com
State New
Headers show

Commit Message

Felipe Balbi Nov. 18, 2015, 10:41 p.m. UTC 
Hi,

Robert Jarzmik <robert.jarzmik@free.fr> writes:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:

>

>> Felipe Balbi <balbi@ti.com> writes:

>>

>>> Hi,

>>>

>>> without any sort of logs it a bit difficult :-) Care to send some output

>>> of the failure ? Are there any oopses or what exactly happens ?

>> Ah well, the UDC is the only way to "speak" to the board (no UART), so I don't

>> have any written feedback available. All I have are the logs displayed on the

>> phone's screen in the framebuffer screen.

>

> I can have logs if I kinda ... revert the patch, applying the diff in [1].

> This enables the suspend/resume to work again, and I can gather the logs when

> [1] is applied :

>

> [   63.649528] Suspending console(s) (use no_console_suspend to debug)

> [   63.688100] PM: suspend of devices complete after 37.275 msecs

> [   63.690351] PM: late suspend of devices complete after 2.191 msecs

> [   63.692595] PM: noirq suspend of devices complete after 2.196 msecs

> [   63.694387] PM: noirq resume of devices complete after 1.482 msecs

> [   63.696711] PM: early resume of devices complete after 1.533 msecs

> [   63.802930] PM: resume of devices complete after 106.122 msecs

> [   63.804976] Restarting tasks ... 

> [   63.821371] pxa27x-udc pxa27x-udc: USB reset

> [   63.822988] done.

> [   63.933666] pxa27x-udc pxa27x-udc: USB reset

> [   64.064026] g_ether gadget: full-speed config #1: CDC Subset/SAFE

> [   64.069692] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk already enabled, doing nothing


this could be a bug in either g_ether or pxa27x... Seems like something
is enabling endpoints which were already enabled. Not sure if this is
pxa27x not _really_ disabling endpoints or g_ether being stupid.

> [1] Stupid diff to renable the suspend/resume to work on 4.4-rc1

> rj@belgarion:~/mio_linux/kernel$ git diff

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h

> index 3d583a10b926..2b8dcf546d03 100644

> --- a/include/linux/usb/gadget.h

> +++ b/include/linux/usb/gadget.h

> @@ -267,8 +267,8 @@ static inline int usb_ep_enable(struct usb_ep *ep)

>  {

>         int ret;

>  

> -       if (ep->enabled)

> -               return 0;

> +       /* if (ep->enabled) */

> +       /*      return 0; */

>  

>         ret = ep->ops->enable(ep, ep->desc);

>         if (ret)

> @@ -295,8 +295,8 @@ static inline int usb_ep_disable(struct usb_ep *ep)

>  {

>         int ret;

>  

> -       if (!ep->enabled)

> -               return 0;

> +       /* if (!ep->enabled) */

> +       /*      return 0; */


yeah, so something is not disabling endpoints when they should :-) So
this could be a bug with your suspend/resume callbacks. If you're going
to disconnect from the bus, you need to tell the gadget driver about it,
which means after disabling pullups, you should call
gadget_driver->disconnect().

Can you see if this *stupid* and *untested* diff helps :


I'm not 100% sure this is enough, as I'm not at all familiar with
pxa27x, but that driver looks hugely unmaintained. Which device are you
using for development/test ? Is it this Mio A701 ? I can't find any
sources of it :-s

-- 
balbi

Comments

Felipe Balbi Nov. 18, 2015, 11:05 p.m. UTC | #1 
Hi,

Robert Jarzmik <robert.jarzmik@free.fr> writes:
> Felipe Balbi <balbi@ti.com> writes:

>

>> Hi,

>>

>> this could be a bug in either g_ether or pxa27x... Seems like something

>> is enabling endpoints which were already enabled. Not sure if this is

>> pxa27x not _really_ disabling endpoints or g_ether being stupid.

> You're probably right.

> From what I remember, g_ether is enabling already enabled endpoints upon resume,

> but that memory is pretty thin and goes back to 2008 when I created the driver,

> so I'm not sure anymore.

>

>> yeah, so something is not disabling endpoints when they should :-) So

>> this could be a bug with your suspend/resume callbacks. If you're going

>> to disconnect from the bus, you need to tell the gadget driver about it,

>> which means after disabling pullups, you should call

>> gadget_driver->disconnect().

> Okay.

>

>> Can you see if this *stupid* and *untested* diff helps :

> ...zip...

> Yes it does. I don't understand why, but it does fix it, and the logs "already

> enabled" are still gone. I wasn't aware that a disconnect was required in the

> suspend, nor did I see it in another udc driver (in 2008 of course).


it's not required on suspend :-) It's required because you disconnect
data pullups and, essentially, disconnected from the bus. You won't get
an IRQ for it however :-)

I'll send this as a fix tomorrow, I'm pretty much done for today. How
far back should this be backported ? Does it cover your side if we
backport to v3.10+ ?

>> I'm not 100% sure this is enough, as I'm not at all familiar with

>> pxa27x, but that driver looks hugely unmaintained.

> That would be my fault, as I'm maintaining it. That's the reason I'm seeing the

> regression. If you have on mind improvements required just say so, I'll see what

> I can do.

>

>> Which device are you using for development/test ? Is it this Mio A701 ?

> Yes it is.


cool.

>> I can't find any sources of it :-s

> What about arch/arm/mach-pxa/mioa701.c ?


I mean a real-world source (ebay, amazon, etc) :-)

-- 
balbi
diff mbox

Patch

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
index 670ac0b12f00..a08ae19ca410 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2535,6 +2535,7 @@  static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
 	udc_disable(udc);
 	udc->pullup_resume = udc->pullup_on;
 	dplus_pullup(udc, 0);
+	udc->driver->disconnect(&udc->gadget);
 
 	return 0;
 }