| Message ID | 1422379516-1633-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello. On 01/27/2015 08:25 PM, Laurent Pinchart wrote: [...] > Furthermore, as the driver can be used on SMP systems, the IRQ handlers > must use spin_lock_irqsave() instead of spin_lock(). Hmm, care to explain why? It's the first time I'm hearing that... This looks especially strange in the context of the current genirq code calling IRQ handlers with interrupts always disabled. > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Tuesday 27 January 2015 21:19:04 Sergei Shtylyov wrote: > Hello. > > On 01/27/2015 08:25 PM, Laurent Pinchart wrote: > > [...] > > > Furthermore, as the driver can be used on SMP systems, the IRQ handlers > > must use spin_lock_irqsave() instead of spin_lock(). > > Hmm, care to explain why? It's the first time I'm hearing that... > This looks especially strange in the context of the current genirq code > calling IRQ handlers with interrupts always disabled. I should have been more precise here. The DMAC has several interrupt sources than can occur simultaneously. While genirq should guarantee that an interrupt will not be preempted by another interrupt (including itself) on the same CPU, does it offer that guarantee across CPUs in an SMP system ? > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com>
On 01/27/2015 09:55 PM, Laurent Pinchart wrote: >> On 01/27/2015 08:25 PM, Laurent Pinchart wrote: >> [...] >>> Furthermore, as the driver can be used on SMP systems, the IRQ handlers >>> must use spin_lock_irqsave() instead of spin_lock(). >> Hmm, care to explain why? It's the first time I'm hearing that... >> This looks especially strange in the context of the current genirq code >> calling IRQ handlers with interrupts always disabled. > I should have been more precise here. > The DMAC has several interrupt sources than can occur simultaneously. While > genirq should guarantee that an interrupt will not be preempted by another > interrupt (including itself) on the same CPU, does it offer that guarantee > across CPUs in an SMP system ? No. But spin_lock_irqsave() wouldn't help with that -- it only disables interrupts on the local CPU... >>> Signed-off-by: Laurent Pinchart >>> <laurent.pinchart+renesas@ideasonboard.com> WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Tuesday 27 January 2015 22:01:18 Sergei Shtylyov wrote: > On 01/27/2015 09:55 PM, Laurent Pinchart wrote: > >> On 01/27/2015 08:25 PM, Laurent Pinchart wrote: > >> > >> [...] > >> > >>> Furthermore, as the driver can be used on SMP systems, the IRQ handlers > >>> must use spin_lock_irqsave() instead of spin_lock(). > >> > >> Hmm, care to explain why? It's the first time I'm hearing that... > >> This looks especially strange in the context of the current genirq code > >> calling IRQ handlers with interrupts always disabled. > > > > I should have been more precise here. > > > > The DMAC has several interrupt sources than can occur simultaneously. > > While genirq should guarantee that an interrupt will not be preempted by > > another interrupt (including itself) on the same CPU, does it offer that > > guarantee across CPUs in an SMP system ? > > No. But spin_lock_irqsave() wouldn't help with that -- it only disables > interrupts on the local CPU... You're of course right. I'm not sure what I was thinking. I'll send a v2 after waiting for review of the other patches. > >>> Signed-off-by: Laurent Pinchart > >>> <laurent.pinchart+renesas@ideasonboard.com>
Hello. On 01/27/2015 10:08 PM, Laurent Pinchart wrote: >>>> On 01/27/2015 08:25 PM, Laurent Pinchart wrote: >>>> >>>> [...] >>>>> Furthermore, as the driver can be used on SMP systems, the IRQ handlers >>>>> must use spin_lock_irqsave() instead of spin_lock(). >>>> Hmm, care to explain why? It's the first time I'm hearing that... >>>> This looks especially strange in the context of the current genirq code >>>> calling IRQ handlers with interrupts always disabled. >>> I should have been more precise here. >>> The DMAC has several interrupt sources than can occur simultaneously. >>> While genirq should guarantee that an interrupt will not be preempted by >>> another interrupt (including itself) on the same CPU, does it offer that >>> guarantee across CPUs in an SMP system ? >> No. But spin_lock_irqsave() wouldn't help with that -- it only disables >> interrupts on the local CPU... > You're of course right. I'm not sure what I was thinking. I'll send a v2 after > waiting for review of the other patches. BTW, the interrupt of the same # is still guaranteed to not occur, IIUC. >>>>> Signed-off-by: Laurent Pinchart >>>>> <laurent.pinchart+renesas@ideasonboard.com> WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 8367578bac63..b7ff64557d6d 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -487,16 +487,16 @@ static int rcar_dmac_desc_alloc(struct rcar_dmac_chan *chan, gfp_t gfp) * * The descriptor must have been removed from the channel's lists before calling * this function. - * - * Locking: Must be called in non-atomic context. */ static void rcar_dmac_desc_put(struct rcar_dmac_chan *chan, struct rcar_dmac_desc *desc) { - spin_lock_irq(&chan->lock); + unsigned long flags; + + spin_lock_irqsave(&chan->lock, flags); list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free); list_add_tail(&desc->node, &chan->desc.free); - spin_unlock_irq(&chan->lock); + spin_unlock_irqrestore(&chan->lock, flags); } static void rcar_dmac_desc_recycle_acked(struct rcar_dmac_chan *chan) @@ -738,6 +738,7 @@ static void rcar_dmac_stop(struct rcar_dmac *dmac) static void rcar_dmac_abort(struct rcar_dmac *dmac) { + unsigned long flags; unsigned int i; /* Stop all channels. */ @@ -745,9 +746,9 @@ static void rcar_dmac_abort(struct rcar_dmac *dmac) struct rcar_dmac_chan *chan = &dmac->channels[i]; /* Stop and reinitialize the channel. */ - spin_lock(&chan->lock); + spin_lock_irqsave(&chan->lock, flags); rcar_dmac_chan_halt(chan); - spin_unlock(&chan->lock); + spin_unlock_irqrestore(&chan->lock, flags); rcar_dmac_chan_reinit(chan); } @@ -1318,9 +1319,10 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void *dev) u32 mask = RCAR_DMACHCR_DSE | RCAR_DMACHCR_TE; struct rcar_dmac_chan *chan = dev; irqreturn_t ret = IRQ_NONE; + unsigned long flags; u32 chcr; - spin_lock(&chan->lock); + spin_lock_irqsave(&chan->lock, flags); chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR); if (chcr & RCAR_DMACHCR_TE) @@ -1333,7 +1335,7 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void *dev) if (chcr & RCAR_DMACHCR_TE) ret |= rcar_dmac_isr_transfer_end(chan); - spin_unlock(&chan->lock); + spin_unlock_irqrestore(&chan->lock, flags); return ret; }
The rcar_dmac_desc_put() function is called in interrupt context and must thus use spin_lock_irqsave() instead of spin_lock_irq(). Furthermore, as the driver can be used on SMP systems, the IRQ handlers must use spin_lock_irqsave() instead of spin_lock(). Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/dma/sh/rcar-dmac.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)