lkml.org 
[lkml]   [2011]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
On Thu, Apr 28, 2011 at 06:10:20PM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 27, 2011 at 03:06:44PM +0530, Viresh Kumar wrote:
> > @@ -407,6 +410,8 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
> > static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
> > u32 status_block, u32 status_err, u32 status_xfer)
> > {
> > + unsigned long flags;
> > +
> > if (status_block & dwc->mask) {
> > void (*callback)(void *param);
> > void *callback_param;
> > @@ -418,9 +423,9 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
> > callback = dwc->cdesc->period_callback;
> > callback_param = dwc->cdesc->period_callback_param;
> > if (callback) {
> > - spin_unlock(&dwc->lock);
> > + spin_unlock_irqrestore(&dwc->lock, flags);
> > callback(callback_param);
> > - spin_lock(&dwc->lock);
> > + spin_lock_irqsave(&dwc->lock, flags);
>
> I'm really not convinced that this is anywhere near correct. I'm
> surprised this doesn't spit out a compiler warning.
>
> spin_unlock_irqrestore() reads the flags argument and puts it into
> the PSR. spin_lock_irqsave() reads the PSR, puts it into the flags
> argument, sets the interrupt mask bit and writes back to the PSR.
>
> So, if you do:
>
> unsigned long flags;
>
> spin_unlock_irqrestore(&dwc->lock, flags);
> ...
> spin_lock_irqsave(&dwc->lock, flags);
>
> you're going to end up corrupting the PSR.
>
> In any case, releasing a spinlock temporarily within a called function
> is _really_ not a nice thing to do. It makes code review rather
> difficult as called functions become non-atomic when called within
> an atomic region.

BTW, how this gets handled in other drivers is basically as follows in
the tasklet:

tasklet()
{
LIST_HEAD(completed);

spin_lock_irqsave(lock, flags);
for each txd(txd) {
if (completed(txd))
list_move_tail(&txd->node, &completed);
}
try to start new txd();
spin_unlock_irqrestore(lock, flags);

for each list entry safe(txd, &completed) {
void (*callback)(void *) = txd->callback;
void *param = txd->callback_param;

free_txd(txd);

if (callback)
callback(param);
}
}

I'm not sure how easy it is to move dw_dmac to that kind of structure,
but I think this is what is required rather than dropping locks within
functions which they haven't themselves taken.


\
 
 \ /
  Last update: 2011-04-29 11:19    [W:0.058 / U:0.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site