Messages in this thread | | | Date | Fri, 29 Apr 2011 10:15:57 +0100 | From | Russell King - ARM Linux <> | Subject | Re: [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.
| |