Re: MUSB peripheral DMA regression caused by driver core runtime PM change

[Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>]

* Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> [151023 11:27]:
> On Fri, 23 Oct 2015, Grygorii Strashko wrote:
> 
> > Reviewed-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> > 
> > It always fun when DD/PM core is updated to fix some driver/subsystem's
> > specific PM issue :(
> > 
> > > 
> > >> --- a/drivers/usb/musb/omap2430.c
> > >> +++ b/drivers/usb/musb/omap2430.c
> > >> @@ -391,9 +391,20 @@ static int omap2430_musb_init(struct musb *musb)
> > >>   	}
> > >>   	musb->isr = omap2430_musb_interrupt;
> > >>   
> > >> +	/*
> > >> +	 * Enable runtime PM for musb parent (this driver). We can't
> > >> +	 * do it earlier as struct musb is not yet allocated and we
> > >> +	 * need to touch the musb registers for runtime PM.
> > >> +	 */
> > >> +	pm_runtime_enable(glue->dev);
> > >> +	status = pm_runtime_get_sync(glue->dev);
> > >> +	if (status < 0)
> > >> +		goto err1;
> > >> +
> > >>   	status = pm_runtime_get_sync(dev);
> > 
> > Hm. My assumption was that *Parent* device (omap2430) will be enabled
> > here :( But as I can see this will not happen:

Yes the parent child stuff here is very confusing right now :)

> > static int rpm_resume(struct device *dev, int rpmflags)
> > {...
> > 	if (!parent && dev->parent) {
> > 		/*
> > 		 * Increment the parent's usage counter and resume it if
> > 		 * necessary.  Not needed if dev is irq-safe; then the
> > 		 * parent is permanently resumed.
> > 		 */
> > 		parent = dev->parent;
> > 		if (dev->power.irq_safe)
> > 			goto skip_parent;
> > 
> > ^^^ and musb device is irq_safe :( 
> 
> This way of doing things looks very strange.
> 
> If the omap2430 is the parent of the musb device, and
> pm_runtime_irq_safe() has been called for the musb device, then after
> that the omap2430 will never be runtime suspended.  Therefore it
> doesn't matter whether you enable it for runtime PM or not.
> 
> It seems to me that the real problem must be that the musb device gets
> runtime-enabled and marked irq_safe too early.  These things should 
> happen before the musb device gets registered and exposed to userspace, 
> but not before the omap2430 parent is runtime-enabled.
> 
> Thus the sequence of events should be:
> 
> 	Allocate the musb device;
> 	Runtime-enable the omap2430 (since it is now safe to do so);
> 	Runtime-enable the musb and declare it irq_safe (this will
> 		automatically runtime-resume the omap2430);
> 	Register the musb.
> 
> If things are done this way, no special action needs to be taken.

Yes good point, that requires changing the init for the whole
drivers/musb though. Also, we should reorganize the whole musb and make
the platform glue and musb core drivers completely separate using a
shared interrupt where needed.

For the regression for the -rc series? Do you see any better
alternatives to what I posted?

Regards,

Tony
--
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