Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW_DMAC.

[Vinod Koul <vinod.koul@intel.com>]

On Mon, Jan 20, 2014 at 07:38:32PM +0530, Shevchenko, Andriy wrote:
> > > > > > > As mentioned by Andy, we are using *_noirq  verion of suspend/resume PM
> > > > > > > callback whereby the callbacks would be executed after IRQ handlers have been
> > > > > > > disabled. If using SET_SYSTEM_SLEEP_PM_OPS, it would be the normal
> > > > > > > suspend/resume PM callback. Looking at the Desginware DMAC platform code
> > > > > > > (drivers/dma/dw/platform.c), it is using the *_noirq suspend/resume PM
> > > > > > > callback. Is it advisable to use the normal suspend/resume PM callback instead
> > > > > > > of *_noirq suspend/PM callback? 
> > > > > > 
> > > > > > i dont see a reason why we need the noirq versions
> > > > > 
> > > > > Okay. I imagine the following use case.
> > > > > 
> > > > > For example we have compiled in DMA driver (dw_dmac) along with, for
> > > > > example, SPI driver.
> > > > > 
> > > > > System was scheduled to go sleep.
> > > > > 
> > > > > An order of calling IIUC might be DMA first, then SPI (since they are
> > > > > not in parent / child relations).
> > > > > 
> > > > > What was happened when SPI would like to do a DMA transfer and DMA is
> > > > > going to sleep? I'm trying to understand if this is a case.
> > > > In that case how does no irq version help us?
> > > 
> > > It guarantees that we have no user of DMA anymore, since there is no
> > > interrupt going on.
> > well how is that. It will gaurantee that there wont be interrupt. User can still
> > submit a transaction or another transaction will be in progress...
> 
> This is how system suspend callback tree is called.
> 
> First it calls .suspend() for all devices, then .suspend_late(),
> then .suspend_noirq().
> 
> There is set of assumptions per each callback round. After .suspend()
> the device must be quiescent.
> 
> But...
> 
> > > > For these cases, I have been using suspend_late. Since the dmaengine driver is
> > > > providing service to other clients (SPI), it needs to esnure that it suspends
> > > > after SPI using suspend_late and resume using resume_early. That way dma is
> > > > availble whenever the client is active
> > > 
> > > suspend_late is working in context that interrupt handler may be
> > > invoked. Thus, to have DMA driver be properly shut down we have to
> > > wait / terminate possible ongoing transfer.
> > Well client is already suspended via .suspend. So where is the transaction :)
> 
> ...as I already wrote before we have no parent-child relationship
> between DMA and, for example, SPI. That means we may possible have the
> case when SPI's .suspend() will be called later than DMA's one.
> 
> > > It seems for me all DMA drivers that are using
> > > system .suspend()/.resume() are potentially buggy.
> > Yup!
> 
> So, we have to decide what to do with them. .suspend_late() still seems
> for me not the best approach. *Or* we have to check for ongoing
> transaction and do something with it. *Or* just shut down the device and
> rely on DMA transaction initiator that it handles the terminated
> transaction properly.

As you clearly said, we dont have a parent-child relatation though we have big
dependency. I think this is true for DMA clients, i ran into similar situation
with i2c few days back!

So only think which can give us a good system behaviour would be clients getting
suspended first and then then service providing subsystems. (same reason why we
do dma driver loading and init much before others drivers)

So yes in the .suspend callback of the client, it needs to
1) abort any transactions it has
2) make the client quiscent

then the .late_suspend kicks in and suspend the core drivers like dma.

This is how it has worked reliably for me in production systems. I am all ears
if we have a better and cleaner apprach to this problem :)

-- 
~Vinod