Make the i.MX MMC device using DMA again

Make the i.MX MMC device using DMA again

[Juergen Borleis]

These patches try to repair the MMC DMA use on i.MX27 based platforms. Its worth the
effort since it doubles the speed of SD card operations for example.

I stumbled across this issue, while a system shutdown:

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = 3be189ab
[00000018] *pgd=a1569831, *pte=00000000, *ppte=00000000
Internal error: Oops: 817 [#1] PREEMPT ARM
CPU: 0 PID: 154 Comm: init Tainted: G        W         5.10.51-00002-gf0033953d4b5-dirty #4
Hardware name: Freescale i.MX27 (Device Tree Support)
PC is at mxcmci_start_cmd+0x190/0x200
LR is at mxcmci_start_cmd+0x1c8/0x200
pc : [<c044a34c>]    lr : [<c044a384>]    psr: 40000013
sp : c0f8bd68  ip : 40000013  fp : 00000000
r10: c14d384c  r9 : c0940288  r8 : c0e4ee80
r7 : 00000200  r6 : c0f8be18  r5 : 00000200  r4 : c0e4ee80
r3 : 00000000  r2 : c044a6ac  r1 : 00000004  r0 : 00000000
Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 0005317f  Table: a1554000  DAC: 00000051
Process init (pid: 154, stack limit = 0xf5d79e18)
Stack: (0xc0f8bd68 to 0xc0f8c000)
bd60:                   c0e4ec00 00000000 c0f8bdcc c044a87c c0f8bdac c057ab78
bd80: 00000000 c0e4ec00 c0f8bdcc 00000000 c0f8a000 c0930018 c0940288 c14d384c
bda0: 00000000 c0436ca8 c0f8bdcc c0e4ec00 00000003 c0436e7c c0f8be18 c0e4ec00
bdc0: 00000003 c0436f50 c0f8bddc 00000000 c0f8be18 00000000 00000000 00000000
bde0: c0f8bde0 c0f8bde0 00000000 c0f8bdec c0f8bdec c04367b4 00000000 00000000
be00: 00000000 00000000 c0e4ec00 c0e4ec08 c14d3808 c043db80 00000007 00000000
be20: 00000000 00000000 00000000 00000000 00000000 00000003 00000000 00000000
be40: 00000000 c0f8bdcc c0e4ec00 c043f230 c0e4ec00 c0e4ec08 c14d3808 c0439734
be60: c14d380c c039cbfc 00000000 fee1dead 00000000 c09090a8 c0100224 c0f8a000
be80: 00000000 c012dd90 01234567 c012e038 00000000 00000000 00000000 00000000
bea0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 0008ba44
bfa0: 00000058 c0100040 00000000 00000000 fee1dead 28121969 01234567 00000000
bfc0: 00000000 00000000 0008ba44 00000058 00000000 00000000 b6f52000 00000000
bfe0: 0008b138 bee3bcc8 0005d3b4 b6ea02b8 60000010 fee1dead 00000000 00000000
[<c044a34c>] (mxcmci_start_cmd) from [<c044a87c>] (mxcmci_request+0x114/0x268)
[<c044a87c>] (mxcmci_request) from [<c0436ca8>] (mmc_start_request+0x70/0xa0)
[<c0436ca8>] (mmc_start_request) from [<c0436e7c>] (mmc_wait_for_req+0x58/0xd0)
[<c0436e7c>] (mmc_wait_for_req) from [<c0436f50>] (mmc_wait_for_cmd+0x5c/0x84)
[<c0436f50>] (mmc_wait_for_cmd) from [<c043db80>] (mmc_deselect_cards+0x34/0x3c)
[<c043db80>] (mmc_deselect_cards) from [<c043f230>] (_mmc_sd_suspend+0x3c/0x70)
[<c043f230>] (_mmc_sd_suspend) from [<c0439734>] (mmc_bus_shutdown+0x40/0x68)
[<c0439734>] (mmc_bus_shutdown) from [<c039cbfc>] (device_shutdown+0x150/0x23c)
[<c039cbfc>] (device_shutdown) from [<c012dd90>] (kernel_restart+0x30/0xa0)
[<c012dd90>] (kernel_restart) from [<c012e038>] (__do_sys_reboot+0xb4/0x1b8)
[<c012e038>] (__do_sys_reboot) from [<c0100040>] (ret_fast_syscall+0x0/0x50)
Exception stack(0xc0f8bfa8 to 0xc0f8bff0)
bfa0:                   00000000 00000000 fee1dead 28121969 01234567 00000000
bfc0: 00000000 00000000 0008ba44 00000058 00000000 00000000 b6f52000 00000000
bfe0: 0008b138 bee3bcc8 0005d3b4 b6ea02b8
Code: e3530000 0a00000b e59f2060 e3a01004 (e5832018)
---[ end trace 2eac4fc5f6f6ce33 ]---

In this case it hits the line

   host->desc->callback = mxcmci_dma_callback;

in mxcmci_start_cmd(), where host->desc isn't setup because of bugs in the
corresponding i.MX27 DMA driver and this MMC driver.

The state machine in mxcmmc.c is broken and always expects a working DMA if a
call to dma_request_chan() in its probe function was successfull. Since the
i.MX27 DMA driver (imx-dma.c) is broken as well regarding its channel configuration,
DMA wasn't possible since commit dea7a9f (at least for a generic DMA type).
I don't know, why the MMC driver worked at regular SD card usage (e.g. mounted
filesystem), but a system shutdown seems to hit a corner case and crashes.

Comments are welcome.

jb

[PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand

[Juergen Borleis]

Calling mxcmci_use_dma(host) is intended for the next transfer only, not
for generic detection if DMA is possible. Without this change, the DMA gets
never configured and thus, never used.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 drivers/mmc/host/mxcmmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 2fe6fcd..611f827 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -834,7 +834,8 @@ static void mxcmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		burstlen = 4;
 
-	if (mxcmci_use_dma(host) && burstlen != host->burstlen) {
+	if (host->dma != NULL && burstlen != host->burstlen) {
+		/* reconfigure DMA on changes only */
 		host->burstlen = burstlen;
 		ret = mxcmci_setup_dma(mmc);
 		if (ret) {
-- 
2.20.1

[PATCH 2/2] mmc: mxcmmc: don't expect a DMA operation if DMA seems present

[Juergen Borleis]

Change the driver's default behaviour from DMA to PIO for each request.
Preparing DMA can fail or be prevented by other causes. Switch to a DMA
operation only, if it is really possible.

This avoids a NULL pointer dereference on shutdown in mxcmci_start_cmd()
where it tries to store a DMA callback configuration into mxcmci_host::desc
which isn't setup a this point of time.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 drivers/mmc/host/mxcmmc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 611f827..41feea9 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -293,14 +293,13 @@ static int mxcmci_setup_data(struct mxcmci_host *host, struct mmc_data *data)
 	mxcmci_writew(host, blksz, MMC_REG_BLK_LEN);
 	host->datasize = datasize;
 
-	if (!mxcmci_use_dma(host))
-		return 0;
+	if (host->dma == NULL)
+		return 0; /* Keep PIO */
 
+	/* Avoid the use of DMA on short transfers, e.g. non-sectors for example */
 	for_each_sg(data->sg, sg, data->sg_len, i) {
-		if (sg->offset & 3 || sg->length & 3 || sg->length < 512) {
-			host->do_dma = 0;
-			return 0;
-		}
+		if (sg->offset & 3 || sg->length & 3 || sg->length < 512)
+			return 0; /* Keep PIO */
 	}
 
 	if (data->flags & MMC_DATA_READ) {
@@ -325,9 +324,11 @@ static int mxcmci_setup_data(struct mxcmci_host *host, struct mmc_data *data)
 	if (!host->desc) {
 		dma_unmap_sg(host->dma->device->dev, data->sg, data->sg_len,
 				host->dma_dir);
-		host->do_dma = 0;
-		return 0; /* Fall back to PIO */
+		return 0; /* Keep PIO */
 	}
+
+	/* DMA is possible */
+	host->do_dma = 1;
 	wmb();
 
 	dmaengine_submit(host->desc);
@@ -747,8 +748,11 @@ static void mxcmci_request(struct mmc_host *mmc, struct mmc_request *req)
 	host->req = req;
 	host->cmdat &= ~CMD_DAT_CONT_INIT;
 
-	if (host->dma)
-		host->do_dma = 1;
+	/*
+	 * Default always to PIO. DMA will be enabled in
+	 * mxcmci_setup_data() if possible
+	 */
+	host->do_dma = 0;
 
 	if (req->data) {
 		error = mxcmci_setup_data(host, req->data);
-- 
2.20.1

Re: [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand

[Ulf Hansson]

On Thu, 29 Jul 2021 at 09:29, Juergen Borleis <jbe@pengutronix.de> wrote:
>
> Calling mxcmci_use_dma(host) is intended for the next transfer only, not
> for generic detection if DMA is possible. Without this change, the DMA gets
> never configured and thus, never used.

Wow, that's an old bug you found there.

>
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>

It looks like we should add a fixes tag and add a stable tag:

Fixes: f53fbde48ef0 ("mmc: mxcmmc: use dmaengine API")

> ---
>  drivers/mmc/host/mxcmmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 2fe6fcd..611f827 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -834,7 +834,8 @@ static void mxcmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         else
>                 burstlen = 4;
>
> -       if (mxcmci_use_dma(host) && burstlen != host->burstlen) {
> +       if (host->dma != NULL && burstlen != host->burstlen) {
> +               /* reconfigure DMA on changes only */
>                 host->burstlen = burstlen;
>                 ret = mxcmci_setup_dma(mmc);
>                 if (ret) {

A few lines below here, you should not clear host->do_dma, as it can't
be set at this point (thus clearing it just adds confusion).

Also I wonder if patch2 is really needed to fix the bug, or should be
considered as nice cleanup instead?

Kind regards
Uffe