Re: [PATCH] usb: musb: fix pm_runtime mismatch

[Felipe Balbi]

On Fri, Dec 16, 2011 at 01:31:02AM +0200, Felipe Contreras wrote:
> On Fri, Dec 16, 2011 at 1:01 AM, Felipe Balbi <balbi@xxxxxx> wrote:
> > On Fri, Dec 16, 2011 at 12:42:14AM +0200, Felipe Contreras wrote:
> >> In musb_init_controller() there's a pm_runtime_put(), but there's no
> >> pm_runtime_get(), which creates a mismatch that causes the driver to
> >> sleep when it shouldn't.
> >>
> >> This was introduced in 7acc619, but it wasn't triggered until 18a2689
> >> was merged to Linus' branch at point 6899608.
> >
> > you need to add the commit description (whatever was the mail's subject)
> > here too. And you should put in Cc the author or those commits too,
> > otherwise we can't poke into their brains to understand what they were
> > thinking when they originally wrote those patches.
> 
> True, but code is code, and even if you don't know what he was
> thinking, it's clear there's something wrong.
> 
> >> However, it seems most of the time this is used in a way that keeps the
> >> counter above 0, so nobody noticed. Also, it seems to depend on the
> >> configuration used.
> >>
> >> I found the problem by loading isp1704_charger before any usb gadgets:
> >> http://article.gmane.org/gmane.linux.kernel/1226122
> >>
> >> All versions after 2.6.39 are affected.
> >>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
> >> ---
> >>  drivers/usb/musb/musb_core.c |    2 --
> >>  1 files changed, 0 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> index b63ab15..920f04e 100644
> >> --- a/drivers/usb/musb/musb_core.c
> >> +++ b/drivers/usb/musb/musb_core.c
> >> @@ -2012,8 +2012,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
> >>       if (status < 0)
> >>               goto fail3;
> >>
> >> -     pm_runtime_put(musb->controller);
> >
> > To me the real fix would be add the missing pm_runtime_get_sync(). On
> > probe() we're actually accessing MUSB's address space which needs it's
> > clocks turned on. I guess it's only working now by chance, probably
> > because glue layer calls pm_runtime_get_sync() to access it's own
> > address space and that uses the same clocks.
> 
> Are you sure it's "musb-hdrc", and not "musb-omap2430" the one
> accessing the relevant address-space? From the runtime_pm
> documentation it looks like only the probe function should deal with
> this.
> 
> If "musb-hdrc" was truly accessing these registers, then I would get
> the same failure because the clocks are turned off, but I don't...

see musb_core_init(); You don't see any problems when accessing those
addresses because musb_platform_init() will fall into
omap2430_musb_init() which calls pm_runtime_get_sync(), and the same
clock actually enables both address spaces (musb-omap2430 and
musb-hdrc).

(now, this was all from memory, so it could be wrong ;-)

-- 
balbi

Attachment: signature.asc
Description: Digital signature