linux-kernel - Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode DW

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: <1390226911.7619.41.camel@smile>
Date:	Mon, 20 Jan 2014 14:08:32 +0000
From:	"Shevchenko, Andriy" <andriy.shevchenko@...el.com>
To:	"Koul, Vinod" <vinod.koul@...el.com>
CC:	Andy Shevchenko <andriy.shevchenko@...ux.jf.intel.com>,
	"Chew, Chiau Ee" <chiau.ee.chew@...el.com>,
	Viresh Kumar <viresh.linux@...il.com>,
	"Williams, Dan J" <dan.j.williams@...el.com>,
	"dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] dma: dw: Add suspend and resume handling for PCI mode
 DW_DMAC.

On Mon, 2014-01-20 at 17:37 +0530, Vinod Koul wrote:
> On Mon, Jan 20, 2014 at 06:17:56PM +0530, Shevchenko, Andriy wrote:
> > On Mon, 2014-01-20 at 14:55 +0530, Vinod Koul wrote:
> > > On Thu, Dec 19, 2013 at 12:51:29PM +0200, Andy Shevchenko wrote:
> > > > On Wed, 2013-12-18 at 21:19 +0530, Vinod Koul wrote:
> > > > > On Mon, Dec 16, 2013 at 01:51:47PM +0530, Chew, Chiau Ee 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.

What is you opinion?


-- 
Andy Shevchenko <andriy.shevchenko@...el.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.