From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCHv3] usb: musb: Fix unbalanced platform_disable Date: Thu, 15 Sep 2016 18:59:15 +0200 Message-ID: <288e07e4-5dc3-3906-1aec-a24c0769972a@redhat.com> References: <20160914181019.22937-1-tony@atomide.com> <79edbb45-cf85-1aba-1d42-1897021e0904@redhat.com> <20160915144343.jpeoizgbw2o23iuc@atomide.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160915144343.jpeoizgbw2o23iuc-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Tony Lindgren Cc: Bin Liu , Greg Kroah-Hartman , Andreas Kemnade , Felipe Balbi , George Cherian , Kishon Vijay Abraham I , Ivaylo Dimitrov , Ladislav Michl , Sergei Shtylyov , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-omap@vger.kernel.org Hi, On 15-09-16 16:43, Tony Lindgren wrote: > * Hans de Goede [160915 07:00]: >> Hi, >> >> On 14-09-16 20:10, Tony Lindgren wrote: >>> Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling >>> for 2430 glue layer") moved PHY enable/disable calls to happen from >>> omap2430_musb_enable/disable(). That broke enumeration for several >>> devices as PM runtime in the PHY will never enable it. >>> >>> The root cause of the problem is unpaired calls from musb_core.c to >>> musb_platform_enable/disable in musb_core.c as reported by >>> Andreas Kemnade . >>> >>> As musb_platform_enable/disable are being called from various functions, >>> let's not attempt to make them paiered immediately. This would require >>> fixing all the callers like musb_remove. >> >> Yeah, the sunxi glue actually has: >> >> static void sunxi_musb_enable(struct musb *musb) >> { >> struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent); >> >> glue->musb = musb; >> >> /* musb_core does not call us in a balanced manner */ >> if (test_and_set_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags)) >> return; > > Heh we're trying to get to the "balanced manner" point hopefully soon :) > >> schedule_work(&glue->work); >> } >> >> static void sunxi_musb_disable(struct musb *musb) >> { >> struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent); >> >> clear_bit(SUNXI_MUSB_FL_ENABLED, &glue->flags); >> } >> >> Note that since the glue struct is kzalloc-ed, this makes calling >> sunxi_musb_disable() from sunxi_musb_init() a nop, so if this >> needs a respin please drop the sunxi changes, they are not >> necessary. > > Hmm we have sunxi_musb_probe() do platform_device_register_full(), > then musb_probe() happens, and calls musb_init_controller(). > > In musb_init_controller() we do musb_platform_init() that finally > calls sunxi_musb_init(). > > So the sunxi glue is kzalloc'ed and initialized already in > sunxi_musb_probe(), and musb parts are already initialized in > musb_init_controller(). Not really, musb_init_controller does little of interest before calling musb_platform_init(), which is a function pointer to sunxi_musb_init(). > I don't quite follow what you mean how it's a nop.. Care to > clarify that a bit? Maybe you're thinking we're calling it from > sunxi_musb_probe() instead? Nope, I no the call chain, but nothing interesting happens between sunxi_musb_probe() and sunxi_musb_init(). > Anyways, calling sunxi_musb_disable() seems unnecessary from > sunxi_musb_init() because it does reset_control_deassert(). > But that can be cleaned up later on if no other reasons for > changes for v4.8-rc cycle. Ah is this patch for 4.8-rc, yeah then definitely keep it as is, as I tried to explain in my previous mail, my remark was only made in case you need to respin the patch for other reasons, if not then it is fine as is. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html