| 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
| ||
|
Message-ID: <4F8EEEAD.6090306@metasoft.pl>
Date: Wed, 18 Apr 2012 18:41:17 +0200
From: Rafal Prylowski <prylowski@...asoft.pl>
To: H Hartley Sweeten <hartleys@...ionengravers.com>
CC: "vinod.koul@...el.com" <vinod.koul@...el.com>,
Mika Westerberg <mika.westerberg@....fi>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"rmallon@...il.com" <rmallon@...il.com>
Subject: Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels
On 2012-04-17 22:51, H Hartley Sweeten wrote:
> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>
>> Rafal,
>>
>> Here's the output:
>>
>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>> M2M: 20c3
>> M2M: 20c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>>
>> The "M2M: 21c3" keeps getting output until the system is turned off.
>
> Rafal,
>
> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
>
> I added a prink in msm_hw_submit():
>
> if (ep93xx_dma_advance_active(edmac)) {
> m2m_fill_desc(edmac);
> control |= M2M_CONTROL_NFBINT;
> printk("%s: NFB enabled\n", __func__);
> }
>
> This message is never displayed.
>
> And for some reason the txd.cookie is not getting set correctly to detect
> the last entry in m2m_hw_interrupt.
>
> /*
> * Check whether we are done with descriptors or not. This, together
> * with DMA channel state, determines action to take in interrupt.
> */
> last = list_first_entry(edmac->active.next,
> struct ep93xx_dma_desc, node)->txd.cookie;
>
> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
> even though a "next" buffer does not exist.
>
Well, I thought this is correct even if we have only one descriptor in active
queue (when NFBINT bit is not set). edmac->active.next should point to the same
element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
this.
Following is a double buffering patch, which should solve your issues with interrupt
storm. It's similar to your hack, but hopefully more optimal, and (the most
important thing for me) it never tries to disable the channel without ensuring
that it is in proper state. If it happens that we are done with descriptors
but Buffer and Control FSM informs that the channel is still operating, we just
wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
always issues next interrupt, and sets DONE bit again).
Patch applies to current mainline.
Implement double buffering for M2M DMA channels.
Signed-off-by: Rafal Prylowski <prylowski@...asoft.pl>
Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -71,6 +71,7 @@
#define M2M_CONTROL_TM_SHIFT 13
#define M2M_CONTROL_TM_TX (1 << M2M_CONTROL_TM_SHIFT)
#define M2M_CONTROL_TM_RX (2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT BIT(21)
#define M2M_CONTROL_RSS_SHIFT 22
#define M2M_CONTROL_RSS_SSPRX (1 << M2M_CONTROL_RSS_SHIFT)
#define M2M_CONTROL_RSS_SSPTX (2 << M2M_CONTROL_RSS_SHIFT)
@@ -79,7 +80,23 @@
#define M2M_CONTROL_PWSC_SHIFT 25
#define M2M_INTERRUPT 0x0004
-#define M2M_INTERRUPT_DONEINT BIT(1)
+#define M2M_INTERRUPT_MASK 6
+
+#define M2M_STATUS 0x000c
+#define M2M_STATUS_CTL_SHIFT 1
+#define M2M_STATUS_CTL_IDLE (0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL (1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD (2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR (3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT (4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK (7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT 4
+#define M2M_STATUS_BUF_NO (0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON (1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT (2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK (3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE BIT(6)
+
#define M2M_BCR0 0x0010
#define M2M_BCR1 0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
/*
* M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
*/
static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
m2m_fill_desc(edmac);
control |= M2M_CONTROL_DONEINT;
+ if (ep93xx_dma_advance_active(edmac)) {
+ m2m_fill_desc(edmac);
+ control |= M2M_CONTROL_NFBINT;
+ }
+
/*
* Now we can finally enable the channel. For M2M channel this must be
* done _after_ the BCRx registers are programmed.
@@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
}
}
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
{
+ u32 status = readl(edmac->regs + M2M_STATUS);
+ u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+ u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+ bool done = status & M2M_STATUS_DONE;
+ bool last_done;
u32 control;
+ struct ep93xx_dma_desc *desc;
- if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+ /* Accept only DONE and NFB interrupts */
+ if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
return INTERRUPT_UNKNOWN;
- /* Clear the DONE bit */
- writel(0, edmac->regs + M2M_INTERRUPT);
+ if (done)
+ /* Clear the DONE bit */
+ writel(0, edmac->regs + M2M_INTERRUPT);
- /* Disable interrupts and the channel */
- control = readl(edmac->regs + M2M_CONTROL);
- control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
- writel(control, edmac->regs + M2M_CONTROL);
+ /*
+ * Check whether we are done with descriptors or not. This, together
+ * with DMA channel state, determines action to take in interrupt.
+ */
+ desc = ep93xx_dma_get_active(edmac);
+ last_done = !desc || desc->txd.cookie;
/*
- * Since we only get DONE interrupt we have to find out ourselves
- * whether there still is something to process. So we try to advance
- * the chain an see whether it succeeds.
+ * Use M2M DMA Buffer FSM and Control FSM to check current state of
+ * DMA channel. Using DONE and NFB bits from channel status register
+ * or bits from channel interrupt register is not reliable.
*/
- if (ep93xx_dma_advance_active(edmac)) {
- edmac->edma->hw_submit(edmac);
- return INTERRUPT_NEXT_BUFFER;
+ if (!last_done &&
+ (buf_fsm == M2M_STATUS_BUF_NO ||
+ buf_fsm == M2M_STATUS_BUF_ON)) {
+ /*
+ * Two buffers are ready for update when Buffer FSM is in
+ * DMA_NO_BUF state. Only one buffer can be prepared without
+ * disabling the channel or polling the DONE bit.
+ * To simplify things, always prepare only one buffer.
+ */
+ if (ep93xx_dma_advance_active(edmac)) {
+ m2m_fill_desc(edmac);
+ if (done && !edmac->chan.private) {
+ /* Software trigger for memcpy channel */
+ control = readl(edmac->regs + M2M_CONTROL);
+ control |= M2M_CONTROL_START;
+ writel(control, edmac->regs + M2M_CONTROL);
+ }
+ return INTERRUPT_NEXT_BUFFER;
+ } else
+ last_done = true;
+ }
+
+ /*
+ * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+ * and Control FSM is in DMA_STALL state.
+ */
+ if (last_done &&
+ buf_fsm == M2M_STATUS_BUF_NO &&
+ ctl_fsm == M2M_STATUS_CTL_STALL) {
+ /* Disable interrupts and the channel */
+ control = readl(edmac->regs + M2M_CONTROL);
+ control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+ | M2M_CONTROL_ENABLE);
+ writel(control, edmac->regs + M2M_CONTROL);
+ return INTERRUPT_DONE;
}
- return INTERRUPT_DONE;
+ /*
+ * Nothing to do this time.
+ */
+ return INTERRUPT_NEXT_BUFFER;
}
/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists