Message-ID: <20150811182929.0c778016@home.neil.brown.name>
Date:	Tue, 11 Aug 2015 18:29:29 +1000
From:	NeilBrown <neil@...wn.name>
To:	Alexander Holler <holler@...oftware.de>,
	Felipe Balbi <balbi@...com>
Cc:	Kishon Vijay Abraham I <kishon@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 4.1 099/267] phy: twl4030-usb: remove incorrect
 pm_runtime_get_sync() in probe function.

On Sun, 9 Aug 2015 12:45:20 +0200 Alexander Holler
<holler@...oftware.de> wrote:

> Am 09.08.2015 um 11:00 schrieb NeilBrown:
> > On Sat, 8 Aug 2015 12:18:55 +0530 Kishon Vijay Abraham I
> > <kishon@...com> wrote:
> >
> >>
> >>
> >> On Saturday 08 August 2015 11:23 AM, Alexander Holler wrote:
> >>> Hello,
> >>>
> >>> this patch killed the musb-host functionality on my classic Beagleboard (rev
> >>> c4). Symptom was that it there was a message I don't remember and the attached
> >>> device didn't enumerate anymore (likely because of missing power, but I'm not
> >>> sure).
> >>>
> >>> A simple revert has fixed it, I haven't looked further into the problem.
> >>
> >> Neil Brown, how was this tested?
> >
> >
> > Well, I have a board with an OMAP3 connected to a twl4030 for USB and I
> > noted that it wasn't power-managed properly and when I made that change,
> > it was.  I don't recall the exact details
> >
> > This is probably related to
> >
> > Commit: 56301df6bcaa ("phy: twl4030-usb: make runtime pm more reliable.")
> >
> > I certainly only tested with that patch in place.
> 
> Cherry-Picking 56301df6bcaa instead of reverting  d1221a608bd did the 
> trick too. So it looks like 56301df6bcaa is indeed a prerequisit for 
> d1221a608bd.
> 
> Therefor I suggest to feed 56301df6bcaa to the stable series (e.g. 
> 4.1.6) too.
> 

The reality is ... more complicated.

I had a close look at how refcounts are inc/dec for the twl4030 phy.

With the current mainline code (plus my twl4030 charger enhancements,
which are not deeply relevant), the refcount does go to zero when
nothing is plugged in, and goes to 2 when a regular USB cable is
plugged in.
The two counts come from twl4030_usb_irq and twl4030_charger_enable_usb,
which is what I would expect.

However at the end of twl4030_usb_probe, the count goes to -1 !!!
because of the pm_runtime_put_autosuspend, which no longer has a
balancing pm_runtime_get() -  which I really shouldn't have removed.

The extra refcount that I saw before and blamed on that
pm_runtime_get() actually comes from a phy_power_on() call in
omap2430_musb_init.

omap2430_musb_init() calls phy_power_on(), and doesn't call
phy_power_off() until omap2430_musb_exit().
So it tries to keep the phy on the entire time that the module is
loaded.

Do we want to just remove the phy_power_on() call from
omap2430_musb_init()?
That seems to work for me, but may well break on other boards.

I think the best thing to do for -stable it to leave 56301df6bcaa out
and revert the backport of d1221a608bd.
That will return to a state which, while not perfect, at least is not a
regression.

With that (older) code, the extra phy_power_on() call still increases
the usage_count, but the irq_handler in the twl4030 phy driver will
drop it down to zero without first increasing.  So things work for the
wrong reasons.

Felipe: you added the phy_power_on() call in 

 Commit: 3063a12be2b0 ("usb: musb: fix PHY power on/off")

Do we really want the phy to be on the whole time the modules is loaded?
If not, how/when should the phy be powered down?

Thanks,
NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists