Re: [PATCHv2] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>]

Tony,

On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170116 15:36]:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170113 14:00]:
> > > * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170113 13:37]:
> > > > > Simplified diff with fix on top of your patch:
> > > > > 
> > > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> > > > > index ce37a1a..9e9403a 100644
> > > > > --- a/drivers/dma/cppi41.c
> > > > > +++ b/drivers/dma/cppi41.c
> > > > > @@ -319,12 +319,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
> > > > >  
> > > > >  		while (val) {
> > > > >  			u32 desc, len;
> > > > > -			int error;
> > > > > -
> > > > > -			error = pm_runtime_get(cdd->ddev.dev);
> > > > > -			if ((error != -EINPROGRESS) && (error < 0))
> > > > > -				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
> > > > > -					__func__, error);
> > > > >  
> > > > >  			/* This warning should never trigger */
> > > > >  			WARN_ON(cdd->is_suspended);
> > > > > @@ -500,7 +494,6 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
> > > > >  	spin_unlock_irqrestore(&cdd->lock, flags);
> > > > >  
> > > > >  	pm_runtime_mark_last_busy(cdd->ddev.dev);
> > > > > -	pm_runtime_put_autosuspend(cdd->ddev.dev);
> > > > >  }
> > > > >  
> > > > >  static u32 get_host_pd0(u32 length)
> > > > > 
> > > > 
> > > > Ok. this is incorrect in case of USB hub and will just hide the problem
> > > > by incrementing PM runtime usage counter every time USB host is connected :(
> > > 
> > > Yeah if anything changes in those two nested loops, the pm_runtime counts
> > > get unbalanced.
> > > 
> > > > Once USB hub is connected the PM runtime usage counter will became 1 and stay
> > > > 1 after disconnection. Which means that some descriptor was pushed in Q, but IRQ
> > > > was not triggered.
> > > > 
> > > > Don't know how to proceed as I'm not so familiar with musb :(
> > > 
> > > I'm now playing with saving the queue manager value and kicking the
> > > PM runtime if we have transfers in progress. Looks like the dma
> > > registers are zero while there are transfers in progress, or at least
> > > I have not yet found any hardware registers that would tell that.
> > 
> > Looks like drivers/usb/musb/musb_cppi41.c is not properly terminating
> > dma transactions.. The following additional patch makes things behave
> > without warnings for me.
> > 
> > I'll fold the drivers/dma/cppi41.c changes to $subject patch and repost,
> > then will post a separate musb fix for drivers/usb/musb/musb_cppi41.c
> > that avoids the warning after some more investigating.
> > 
> > Luckily the warning is harmless in this case as musb and cppi41 are
> > in the same device so the common parent is powered and cppi41 is
> > getting clocked.
> 
> Sorry actually it's after these fixes when we still need to implement
> something for cppi41 PM runtime usecount that makes sense as the
> calls will finally be paired. For testing, reverting 098de42ad670
> ("dmaengine: cppi41: Fix unpaired pm runtime when only a USB hub
> is connected") can be done. But I don't like that as it's still
> unpaired pm_runtime_calls potentially if something goes wrong.
> 
> Anyways, for the -rc series oops, we can just leave out the WARN_ON
> parts for now until drivers/usb/musb/musb_cppi41.c is fixed too.

Giving that cppi is a submodule inside the usb subsysytem and it does't
have separate power rail or clock, what is the benefit to adding runtime
PM in the cppi driver?

Thanks,
-Bin.
--
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