renesas sdhi driver and DMA

renesas sdhi driver and DMA

[Chris Brandt]

Hello Wolfram,

For my RZ/A1 DMA driver (it's a work in progress), the patch

52ad9a8e854c ("mmc: host: tmio: ensure end of DMA and SD access are in sync")

now causes a "BUG: scheduling while atomic"

Starting logging: BUG: scheduling while atomic: S01logging/434/0x00000100
CPU: 0 PID: 434 Comm: S01logging Not tainted 4.9.49-g845fe5990-dirty #29
Hardware name: Generic R7S72100 (Flattened Device Tree)
[<bf808d41>] (unwind_backtrace) from [<bf8075f3>] (show_stack+0xb/0xc)
[<bf8075f3>] (show_stack) from [<bf82038d>] (__schedule_bug+0x39/0x58)
[<bf82038d>] (__schedule_bug) from [<bfa2fba9>] (__schedule+0x21/0x238)
[<bfa2fba9>] (__schedule) from [<bfa2fe2d>] (schedule+0x51/0x58)
[<bfa2fe2d>] (schedule) from [<bfa313ad>] (schedule_timeout+0x15/0xcc)
[<bfa313ad>] (schedule_timeout) from [<bfa30427>] (wait_for_common+0x9b/0xbc)
[<bfa30427>] (wait_for_common) from [<bf9921e7>] (tmio_mmc_dma_callback+0xb/0x74)
[<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>] (rzadma_tasklet+0x23/0x98)
[<bf908e1b>] (rzadma_tasklet) from [<bf810d9b>] (tasklet_action+0x3f/0x5c)
[<bf810d9b>] (tasklet_action) from [<bf810a47>] (__do_softirq+0x7f/0x144)
[<bf810a47>] (__do_softirq) from [<bf829917>] (__handle_domain_irq+0x51/0x6a)
[<bf829917>] (__handle_domain_irq) from [<bf80128b>] (gic_handle_irq+0x37/0x58)
[<bf80128b>] (gic_handle_irq) from [<bf807d25>] (__irq_svc+0x65/0x94)


I noticed that in rcar-dmac.c, the DMA channel IRQs are threaded:

	ret = devm_request_threaded_irq(dmac->dev, irq, rcar_dmac_isr_channel,
					rcar_dmac_isr_channel_thread, 0,
					irqname, rchan);

Is it now a requirement that I have to register my DMA channel 
interrupts as devm_request_threaded_irq?

Will that fix my issue?

Thank you,
Chris

Re: renesas sdhi driver and DMA

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi Chris,

> [<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>] (rzadma_tasklet+0x23/0x98)

For SYS-DMAC, this is a callback called by the dmaengine core once the
DMA is completed. My guess is you call it directly from an interrupt
handler? It would be easier to talk over code here, I guess.

> Is it now a requirement that I have to register my DMA channel 
> interrupts as devm_request_threaded_irq?
> 
> Will that fix my issue?

I can't say yet if this alone will do. Likely not. Can you share the
code already? Isn't it SYS-DMAC compatible (sigh)?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: renesas sdhi driver and DMA

[Chris Brandt]

Hi Wolfram,

> > [<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>]
> (rzadma_tasklet+0x23/0x98)
> 
> For SYS-DMAC, this is a callback called by the dmaengine core once the
> DMA is completed. My guess is you call it directly from an interrupt
> handler? It would be easier to talk over code here, I guess.
> 
> > Is it now a requirement that I have to register my DMA channel
> > interrupts as devm_request_threaded_irq?
> >
> > Will that fix my issue?
> 
> I can't say yet if this alone will do. Likely not. Can you share the
> code already?

The interrupt handler is starting a tasklet, and then the tasklet calls 
the callback. I thought tasklets didn't run in an interrupt context?????


As a code snippet, here is the irq hander and the tasket. Of course I 
can send the entire file if you want as well if you need to see more.


static void dma_irq_handle_channel(struct rzadma_channel *rzadmac)
{
	u32 chstat, chctrl;
	struct rzadma_engine *rzadma = rzadmac->rzadma;
	int channel = rzadmac->channel;

	chstat = rzadma_ch_readl(rzadmac, CHSTAT, 1);
	if (chstat & CHSTAT_ER) {
		dev_dbg(rzadma->dev, "CHSTAT_%d = %08X\n", channel, chstat);
		rzadma_ch_writel(rzadmac,
				CHCTRL_DEFAULT,
				CHCTRL, 1);
		goto schedule;
	}
	if (!(chstat & CHSTAT_END))
		return;

	chctrl = rzadma_ch_readl(rzadmac, CHCTRL, 1);
	dev_dbg(rzadma->dev,"2. chctrl_%d=%08X\n", channel, chctrl);
	rzadma_ch_writel(rzadmac,
			chctrl | CHCTRL_CLREND | CHCTRL_CLRRQ,
			CHCTRL, 1);
schedule:
	/* Tasklet irq */
	tasklet_schedule(&rzadmac->dma_tasklet);
}


static void rzadma_tasklet(unsigned long data)
{
	struct rzadma_channel *rzadmac = (void *)data;
	struct rzadma_engine *rzadma = rzadmac->rzadma;
	struct rzadma_desc *desc;
	unsigned long flags;

	/* Protection of critical section */
	spin_lock_irqsave(&rzadma->lock, flags);

	if (list_empty(&rzadmac->ld_active)) {
		/* Someone might have called terminate all */
		goto out;
	}

	desc = list_first_entry(&rzadmac->ld_active, struct rzadma_desc, node);

	if (desc->desc.callback)
		desc->desc.callback(desc->desc.callback_param);

	dma_cookie_complete(&desc->desc);

	if (list_empty(&rzadmac->ld_active)) {
		goto out;
	}else{
		list_move_tail(rzadmac->ld_active.next, &rzadmac->ld_free);
	}

	if (!list_empty(&rzadmac->ld_queue)) {
		desc = list_first_entry(&rzadmac->ld_queue, struct rzadma_desc,
					node);
		if (rzadma_xfer_desc(desc) < 0){
			dev_err(rzadma->dev, "%s: channel: %d couldn't xfer desc\n",
				 __func__, rzadmac->channel);
		}else{
			list_move_tail(rzadmac->ld_queue.next, &rzadmac->ld_active);
		}
	}
out:
	spin_unlock_irqrestore(&rzadma->lock, flags);
}



> Isn't it SYS-DMAC compatible (sigh)?

My first goal is to just get this DMA platform driver we made a couple 
years ago working with DT (ie, you can't go passing in slave IDs anymore 
via platform data structures).

Technically, it is working now (after I revert your patch), but I 
probably have more things that have to change before I can think about 
submitting it upstream for review.


Thank you!

Chris

RE: renesas sdhi driver and DMA

[Chris Brandt]

Hi Wolfram,

On Tuesday, October 10, 2017, Wolfram Sang wrote:
> Isn't it SYS-DMAC compatible (sigh)?

And to answer your question, it's a completely different IP than the 
SYS-DMAC. It's still a multi-channel linked-list descriptor based system 
like SYS-DMAC, but a complete different set of registers.

However, it's a nice piece of IP so we'll probably continue to use it 
for future RZ/A devices.


Chris

Re: renesas sdhi driver and DMA

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]


> > Isn't it SYS-DMAC compatible (sigh)?
> 
> And to answer your question, it's a completely different IP than the 
> SYS-DMAC. It's still a multi-channel linked-list descriptor based system 
> like SYS-DMAC, but a complete different set of registers.
> 
> However, it's a nice piece of IP so we'll probably continue to use it 
> for future RZ/A devices.

When asking the question, I mixed up RZ/A with RZ/G, so I wondered why
the latter has something different. For RZ/A, it is obvious to me that
DMA has a different controller. Sorry about the fuzz!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: renesas sdhi driver and DMA

[Chris Brandt]

Hi Wolfram,

Sorry, back on this subject...

> > > [<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>]
> > (rzadma_tasklet+0x23/0x98)
> >
> > For SYS-DMAC, this is a callback called by the dmaengine core once the
> > DMA is completed. My guess is you call it directly from an interrupt
> > handler? It would be easier to talk over code here, I guess.
> >
> > > Is it now a requirement that I have to register my DMA channel
> > > interrupts as devm_request_threaded_irq?
> > >
> > > Will that fix my issue?
> >
> > I can't say yet if this alone will do. Likely not. Can you share the
> > code already?
> 
> The interrupt handler is starting a tasklet, and then the tasklet calls
> the callback. I thought tasklets didn't run in an interrupt context?????


So here's what I found.

According to:

https://www.kernel.org/doc/Documentation/dmaengine/client.txt

It says:
"Note that callbacks will always be invoked from the DMA engines 
tasklet, never from interrupt context."


My DMA driver was calling the SDHI callback from within a tasklet (ie, 
softirq). Almost all the other DMA drivers doing the same thing.

All was fine until that patch you submitted (52ad9a8e854c) that now adds
a wait_for_completion() in the SDHI callback.

Now I get the "BUG: scheduling while atomic".

The R-Car driver uses a threaded irq to call the callbacks, not a 
tasklet.

So I hacked my code to also call the callbacks from a threaded irq like 
rcar-dma.c.
And....that works (no more scheduling while atomic).

So, the question is, which of these statements are correct?

  1. wait_for_completion should not be called in a DMA callback

  2. DMA engine drivers should use threaded IRQs instead of tasklets


The issue with #2 is that mic_x100_dma.c, sh/rcar-dmac.c and 
sh/shdma-base.c are the only ones calling request_threaded_irq,
so you would think that tasklets are the correct method.


Thanks,
Chris

Re: renesas sdhi driver and DMA

[Niklas Söderlund]

Hi Chris,

On 2017-10-27 21:11:39 +0000, Chris Brandt wrote:
> Hi Wolfram,
> 
> Sorry, back on this subject...
> 
> > > > [<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>]
> > > (rzadma_tasklet+0x23/0x98)
> > >
> > > For SYS-DMAC, this is a callback called by the dmaengine core once the
> > > DMA is completed. My guess is you call it directly from an interrupt
> > > handler? It would be easier to talk over code here, I guess.
> > >
> > > > Is it now a requirement that I have to register my DMA channel
> > > > interrupts as devm_request_threaded_irq?
> > > >
> > > > Will that fix my issue?
> > >
> > > I can't say yet if this alone will do. Likely not. Can you share the
> > > code already?
> > 
> > The interrupt handler is starting a tasklet, and then the tasklet calls
> > the callback. I thought tasklets didn't run in an interrupt context?????
> 
> 
> So here's what I found.
> 
> According to:
> 
> https://www.kernel.org/doc/Documentation/dmaengine/client.txt
> 
> It says:
> "Note that callbacks will always be invoked from the DMA engines 
> tasklet, never from interrupt context."
> 
> 
> My DMA driver was calling the SDHI callback from within a tasklet (ie, 
> softirq). Almost all the other DMA drivers doing the same thing.
> 
> All was fine until that patch you submitted (52ad9a8e854c) that now adds
> a wait_for_completion() in the SDHI callback.

This is a interesting finding, I did some bisecting a while back to try 
and understand why a known problematic SD card started to have more 
issues then last time I tested it. That investigation also ended up 
pointing to 52ad9a8e854c.

> 
> Now I get the "BUG: scheduling while atomic".
> 
> The R-Car driver uses a threaded irq to call the callbacks, not a 
> tasklet.
> 
> So I hacked my code to also call the callbacks from a threaded irq like 
> rcar-dma.c.
> And....that works (no more scheduling while atomic).

Can you post that hack or just send it to me privately if it's to 
hackish for a mailinglist :-) I'm still interested in my problematic SD 
card and would just like to see what happens with your hack applied.

> 
> So, the question is, which of these statements are correct?
> 
>   1. wait_for_completion should not be called in a DMA callback
> 
>   2. DMA engine drivers should use threaded IRQs instead of tasklets
> 
> 
> The issue with #2 is that mic_x100_dma.c, sh/rcar-dmac.c and 
> sh/shdma-base.c are the only ones calling request_threaded_irq,
> so you would think that tasklets are the correct method.
> 
> 
> Thanks,
> Chris
> 

-- 
Regards,
Niklas S�derlund

Re: renesas sdhi driver and DMA

[Niklas Söderlund]

Hi Chris,

On 2017-10-27 21:11:39 +0000, Chris Brandt wrote:
> Hi Wolfram,
> 
> Sorry, back on this subject...
> 
> > > > [<bf9921e7>] (tmio_mmc_dma_callback) from [<bf908e1b>]
> > > (rzadma_tasklet+0x23/0x98)
> > >
> > > For SYS-DMAC, this is a callback called by the dmaengine core once the
> > > DMA is completed. My guess is you call it directly from an interrupt
> > > handler? It would be easier to talk over code here, I guess.
> > >
> > > > Is it now a requirement that I have to register my DMA channel
> > > > interrupts as devm_request_threaded_irq?
> > > >
> > > > Will that fix my issue?
> > >
> > > I can't say yet if this alone will do. Likely not. Can you share the
> > > code already?
> > 
> > The interrupt handler is starting a tasklet, and then the tasklet calls
> > the callback. I thought tasklets didn't run in an interrupt context?????
> 
> 
> So here's what I found.
> 
> According to:
> 
> https://www.kernel.org/doc/Documentation/dmaengine/client.txt
> 
> It says:
> "Note that callbacks will always be invoked from the DMA engines 
> tasklet, never from interrupt context."
> 
> 
> My DMA driver was calling the SDHI callback from within a tasklet (ie, 
> softirq). Almost all the other DMA drivers doing the same thing.
> 
> All was fine until that patch you submitted (52ad9a8e854c) that now adds
> a wait_for_completion() in the SDHI callback.

This is a interesting finding, I did some bisecting a while back to try 
and understand why a known problematic SD card started to have more 
issues then last time I tested it. That investigation also ended up 
pointing to 52ad9a8e854c.

> 
> Now I get the "BUG: scheduling while atomic".
> 
> The R-Car driver uses a threaded irq to call the callbacks, not a 
> tasklet.
> 
> So I hacked my code to also call the callbacks from a threaded irq like 
> rcar-dma.c.
> And....that works (no more scheduling while atomic).

Can you post that hack or just send it to me privately if it's to 
hackish for a mailinglist :-) I'm still interested in my problematic SD 
card and would just like to see what happens with your hack applied.

> 
> So, the question is, which of these statements are correct?
> 
>   1. wait_for_completion should not be called in a DMA callback
> 
>   2. DMA engine drivers should use threaded IRQs instead of tasklets
> 
> 
> The issue with #2 is that mic_x100_dma.c, sh/rcar-dmac.c and 
> sh/shdma-base.c are the only ones calling request_threaded_irq,
> so you would think that tasklets are the correct method.
> 
> 
> Thanks,
> Chris
> 

-- 
Regards,
Niklas Söderlund

RE: renesas sdhi driver and DMA

[Chris Brandt]

Hi Niklas,

On Saturday, October 28, 2017, Niklas Söderlund wrote:
> > All was fine until that patch you submitted (52ad9a8e854c) that now adds
> > a wait_for_completion() in the SDHI callback.
> 
> This is a interesting finding, I did some bisecting a while back to try
> and understand why a known problematic SD card started to have more
> issues then last time I tested it. That investigation also ended up
> pointing to 52ad9a8e854c.
> 
> >
> > Now I get the "BUG: scheduling while atomic".
> >
> > The R-Car driver uses a threaded irq to call the callbacks, not a
> > tasklet.
> >
> > So I hacked my code to also call the callbacks from a threaded irq like
> > rcar-dma.c.
> > And....that works (no more scheduling while atomic).
> 
> Can you post that hack or just send it to me privately if it's to
> hackish for a mailinglist :-) I'm still interested in my problematic SD
> card and would just like to see what happens with your hack applied.

I hacked my RZ/A DMA driver, not the SDCARD driver. I assume you are 
using R-Car, so that driver already uses a threaded IRQ handler (not a
tasklet).

But, what I did not try yet was rewriting the patch for SDHI 
(52ad9a8e854c) so that instead of using wait_for_completion() in the callback, it 
creates a kernel thread to do the waiting. In that case, the DMA tasklet 
would complete immediately and my DMA driver could remain using tasklets
instead of threaded irqs.

I am worried about the scheduling latencies of using a kernel thread for
the SDHI callback, but, I guess that's no different than using a 
threaded irq handler.

However...I'm wondering if I'm missing something obvious here and doing 
extra work for nothing.


Chris