linux-kernel - Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160809073559.287d0a59@aktux>
Date:	Tue, 9 Aug 2016 07:35:59 +0200
From:	Andreas Kemnade <andreas@...nade.info>
To:	Tony Lindgren <tony@...mide.com>
Cc:	Discussions about the Letux Kernel <letux-kernel@...nphoenux.org>,
	Linux USB Mailing List <linux-usb@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-omap <linux-omap@...r.kernel.org>, Bin Liu <b-liu@...com>
Subject: Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume
 balanced enable()/disable()

On Fri, 5 Aug 2016 23:21:35 -0700
Tony Lindgren <tony@...mide.com> wrote:

> * Andreas Kemnade <andreas@...nade.info> [160805 08:35]:
> > I repeat the subject line of the patch:
> > [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()
> > It is *not* fix charging.
> > 
> > So you mean that the phy should magically know at which refcounter
> > it should power on and off if power on/off is not called in the
> > balanced way?
> 
> No, I mean we need to figure out why things can be called in
> unbalanced way. With your patch I end up with unbalanced calls
> leaving the phy on after all the modules have been removed :)
> 
Well, causing trouble when modules are removed was not the intention.
I was just happy to have things working a bit again.
The phy is powered on/off via
musb_platform_enable()/musb_platform_disable().

Calls to musb_platform_enable() occur at only 1 place.
musb_platform_disable() is called at 4 places.

about balancing:
There is musb_start() and musb_stop(). They are called from
musb_gadget_start/stop()
These call musb_platform_enable() and musb_platform_disable().
Looks ok.

There is musb_suspend() and musb_resume():

musb_suspend() calls musb_platform_disable()
musb_resume() calls musb_plaform_enable() via musb_start()
looks balanced but why don't we use musb_stop() in musb_suspend()?

Now the odd things:
musb_platform_disable() in musb_remove() called upon module removal
musb_platform_disable() in musb_init_controller() called from
musb_probe()

This looks clearly unbalanced.


> > Maybe the phy-twl4030 uses the phy layer wrong. 
> > Now the relevant part of power on/off in phy-twl4030 is done via
> > struct phy_ops.power_off/power_on (*not* via pm_runtime). Maybe
> > that is wrong and more parts should be moved to the pm_runtime
> > stuff (which is also present). 
> 
> We should use phy power_off/power_on for the USB related parts.
> The parts needed by other components, like VBUS detection, should
> be handled by PM runtime. We should get phy-twl4030-usb and the
> charger driver working also when no musb modules are loaded.
> 
> > Then the phy subsystem has its own power refcounter in struct
> > phy.power_count. It it handled via phy_power_off()/phy_power_on().
> > And that is called from musb/omap2430.c 
> > But that is another story. 
> 
> Yes that's what the USB driver is expected to do. But obviously
> there are issues remaining also in the phy-twl4030-usb.c driver.
> And it seems that we should have some OTG parts always enabled
> when VBUS is detected when twl4030-charger is configured?
> 
Seems so. I am writing a patch for it.

> > > If there are MUSB specific PM runtime issues then let's fix
> > > those separately.
> > > 
> > And that exactly tries my patch to do. For that task it does not
> > even use the PM runtime system. Again please read the subject line
> > of the patch. Maybe it unveils some other pm issues in musb
> > which should first be fixed in a complete patch series.
> 
> Certainly that needs to be fixed, but let's do it in a way where
> things work for other test cases also. Care to describe how to
> to reproduce the issue you're seeing? It seems that you are
> seeing devices not being enmerated leading to the charger not
> working? Is this with built in MUSB and phy modules?
> 
Both as modules. I added some debug output to the driver/phy/phy-core.c
and have seen the phy->power_count sticking at -1 or 0. 
g_ether is also loaded.
Gadget stops for me (device not showing up at the other side) at 4.7rc4.
But I remember Nikolaus having the situation on the same type of device
that it was important on which side you replug the usb cable
(probably causing some timing differences) with 4.7rc1.

Regards,
Andreas

Content of type "application/pgp-signature" skipped