* Make the i.MX MMC device using DMA again
@ 2021-07-29 7:29 Juergen Borleis
2021-07-29 7:29 ` [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand Juergen Borleis
2021-07-29 7:29 ` [PATCH 2/2] mmc: mxcmmc: don't expect a DMA operation if DMA seems present Juergen Borleis
0 siblings, 2 replies; 4+ messages in thread
From: Juergen Borleis @ 2021-07-29 7:29 UTC (permalink / raw)
To: linux-mmc
Cc: linux-kernel, festevam, wsa+renesas, dianders, ulf.hansson, kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand
2021-07-29 7:29 Make the i.MX MMC device using DMA again Juergen Borleis
@ 2021-07-29 7:29 ` Juergen Borleis
2021-08-11 11:36 ` Ulf Hansson
2021-07-29 7:29 ` [PATCH 2/2] mmc: mxcmmc: don't expect a DMA operation if DMA seems present Juergen Borleis
1 sibling, 1 reply; 4+ messages in thread
From: Juergen Borleis @ 2021-07-29 7:29 UTC (permalink / raw)
To: linux-mmc
Cc: linux-kernel, festevam, wsa+renesas, dianders, ulf.hansson, kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] mmc: mxcmmc: don't expect a DMA operation if DMA seems present
2021-07-29 7:29 Make the i.MX MMC device using DMA again Juergen Borleis
2021-07-29 7:29 ` [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand Juergen Borleis
@ 2021-07-29 7:29 ` Juergen Borleis
1 sibling, 0 replies; 4+ messages in thread
From: Juergen Borleis @ 2021-07-29 7:29 UTC (permalink / raw)
To: linux-mmc
Cc: linux-kernel, festevam, wsa+renesas, dianders, ulf.hansson, kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand
2021-07-29 7:29 ` [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand Juergen Borleis
@ 2021-08-11 11:36 ` Ulf Hansson
0 siblings, 0 replies; 4+ messages in thread
From: Ulf Hansson @ 2021-08-11 11:36 UTC (permalink / raw)
To: Juergen Borleis
Cc: linux-mmc, Linux Kernel Mailing List, Fabio Estevam,
Wolfram Sang, Doug Anderson, Sascha Hauer
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-11 11:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 7:29 Make the i.MX MMC device using DMA again Juergen Borleis
2021-07-29 7:29 ` [PATCH 1/2] mmc: mxcmmc: really configure the DMA on demand Juergen Borleis
2021-08-11 11:36 ` Ulf Hansson
2021-07-29 7:29 ` [PATCH 2/2] mmc: mxcmmc: don't expect a DMA operation if DMA seems present Juergen Borleis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).