mmc: Several fixes for bcm2835 driver

This patch series fixes several issues which has been discovered after submission.

Changes in V2:
- add my own signed-off-by to patches #1 and #2

Michal Suchanek (1):
mmc: bcm2835: reset host on timeout

Phil Elwell (1):
mmc: bcm2835: Recover from MMC_SEND_EXT_CSD

Stefan Wahren (5):
mmc: bcm2835: Release DMA channel on driver unload
mmc: bcm2835: Avoid possible races on data requests
mmc: bcm2835: Terminate timeout work synchronously
mmc: bcm2835: Refactor dma_map_sg handling
mmc: bcm2835: Properly handle dmaengine_prep_slave_sg

 drivers/mmc/host/bcm2835.c | 58 ++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

2.7.4

From: Michal Suchanek <[email protected]>

The bcm2835 mmc host tends to lock up for unknown reason so reset it on timeout. The upper mmc block layer tries retransimitting with single blocks which tends to work out after a long wait.

This is better than giving up and leaving the machine broken for no obvious reason.

---
 drivers/mmc/host/bcm2835.c | 3 +++
 1 file changed, 3 insertions(+)

index 768972a..f1712df 100644
@@ -286,6 +286,7 @@ static void bcm2835_reset(struct mmc_host *mmc)
 
 	if (host->dma_chan)
 		dmaengine_terminate_sync(host->dma_chan);
+	host->dma_chan = NULL;
 	bcm2835_reset_internal(host);
 }
 

@@ -837,6 +838,8 @@ static void bcm2835_timeout(struct work_struct *work)
 		dev_err(dev, "timeout waiting for hardware interrupt.\n");
 		bcm2835_dumpregs(host);
 
+		bcm2835_reset(host->mmc);
+
 		if (host->data) {
 			host->data->error = -ETIMEDOUT;
 			bcm2835_finish_data(host);

2.7.4

From: Phil Elwell <[email protected]>

If the user issues an "mmc extcsd read", the SD controller receives what it thinks is a SEND_IF_COND command with an unexpected data block. The resulting operations leave the FSM stuck in READWAIT, a state which persists until the MMC framework resets the controller, by which point the root filesystem is likely to have been unmounted.

A less heavyweight solution is to detect the condition and nudge the FSM by asserting the (self-clearing) FORCE_DATA_MODE bit.

---
 drivers/mmc/host/bcm2835.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index f1712df..a251be2 100644
@@ -773,6 +773,8 @@ static void bcm2835_finish_command(struct bcm2835_host *host)
 
 		if (!(sdhsts & SDHSTS_CRC7_ERROR) ||
 		    (host->cmd->opcode != MMC_SEND_OP_COND)) {
+			u32 edm, fsm;
+
 			if (sdhsts & SDHSTS_CMD_TIME_OUT) {
 				host->cmd->error = -ETIMEDOUT;
 			} else {

@@ -781,6 +783,13 @@ static void bcm2835_finish_command(struct bcm2835_host *host)
 				bcm2835_dumpregs(host);
 				host->cmd->error = -EILSEQ;
 			}
+			edm = readl(host->ioaddr + SDEDM);
+			fsm = edm & SDEDM_FSM_MASK;
+			if (fsm == SDEDM_FSM_READWAIT ||
+			    fsm == SDEDM_FSM_WRITESTART1)
+				/* Kick the FSM out of its wait */
+				writel(edm | SDEDM_FORCE_DATA_MODE,
+				       host->ioaddr + SDEDM);
 			bcm2835_finish_request(host);
 			return;
 		}

2.7.4

We need to release the slave DMA channel during driver unload.

---
 drivers/mmc/host/bcm2835.c | 3 +++
 1 file changed, 3 insertions(+)

index a251be2..45724ef 100644
@@ -1457,6 +1457,9 @@ static int bcm2835_remove(struct platform_device *pdev)
 	cancel_work_sync(&host->dma_work);
 	cancel_delayed_work_sync(&host->timeout_work);
 
+	if (host->dma_chan_rxtx)
+		dma_release_channel(host->dma_chan_rxtx);
+
 	mmc_free_host(host->mmc);
 	platform_set_drvdata(pdev, NULL);
 

2.7.4

There are two accesses on the data requests which are not protected by the mutex. So fix this accordingly.

---
 drivers/mmc/host/bcm2835.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index 45724ef..1c60798 100644
@@ -1064,10 +1064,12 @@ static void bcm2835_dma_complete_work(struct work_struct *work)
 {
 	struct bcm2835_host *host =
 		container_of(work, struct bcm2835_host, dma_work);
-	struct mmc_data *data = host->data;
+	struct mmc_data *data;
 
 	mutex_lock(&host->mutex);
 
+	data = host->data;
+
 	if (host->dma_chan) {
 		dma_unmap_sg(host->dma_chan->device->dev,
 			     data->sg, data->sg_len,

@@ -1192,9 +1194,6 @@ static void bcm2835_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
-	if (host->use_dma && mrq->data && (mrq->data->blocks > PIO_THRESHOLD))
-		bcm2835_prepare_dma(host, mrq->data);
-
 	mutex_lock(&host->mutex);
 
 	WARN_ON(host->mrq);

@@ -1218,6 +1217,9 @@ static void bcm2835_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
+	if (host->use_dma && mrq->data && (mrq->data->blocks > PIO_THRESHOLD))
+		bcm2835_prepare_dma(host, mrq->data);
+
 	host->use_sbc = !!mrq->sbc && host->mrq->data &&
 			(host->mrq->data->flags & MMC_DATA_READ);
 	if (host->use_sbc) {

2.7.4

It's better to make sure that the timeout work is really terminated before calling mmc_request_done.

---
 drivers/mmc/host/bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1c60798..2c7589f 100644
@@ -608,7 +608,7 @@ static void bcm2835_finish_request(struct bcm2835_host *host)
 	struct dma_chan *terminate_chan = NULL;
 	struct mmc_request *mrq;
 
-	cancel_delayed_work(&host->timeout_work);
+	cancel_delayed_work_sync(&host->timeout_work);
 
 	mrq = host->mrq;
 

2.7.4

There are two variables len within bcm2835_prepare_dma. So rename the result of dma_map_sg to sg_len. While we are at this add a bail out to simplify the following change.

---
 drivers/mmc/host/bcm2835.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

index 2c7589f..c42bdaa 100644
@@ -464,7 +464,7 @@ static void bcm2835_transfer_pio(struct bcm2835_host *host)
 static
 void bcm2835_prepare_dma(struct bcm2835_host *host, struct mmc_data *data)
 {
-	int len, dir_data, dir_slave;
+	int sg_len, dir_data, dir_slave;
 	struct dma_async_tx_descriptor *desc = NULL;
 	struct dma_chan *dma_chan;
 

@@ -510,15 +510,13 @@ void bcm2835_prepare_dma(struct bcm2835_host *host, struct mmc_data *data)
 				     &host->dma_cfg_rx :
 				     &host->dma_cfg_tx);
 
-	len = dma_map_sg(dma_chan->device->dev, data->sg, data->sg_len,
-			 dir_data);
+	sg_len = dma_map_sg(dma_chan->device->dev, data->sg, data->sg_len,
+			    dir_data);
+	if (!sg_len)
+		return;
 
-	if (len > 0) {
-		desc = dmaengine_prep_slave_sg(dma_chan, data->sg,
-					       len, dir_slave,
-					       DMA_PREP_INTERRUPT |
-					       DMA_CTRL_ACK);
-	}
+	desc = dmaengine_prep_slave_sg(dma_chan, data->sg, sg_len, dir_slave,
+				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 
 	if (desc) {
 		desc->callback = bcm2835_dma_complete;

2.7.4

In case dmaengine_prep_slave_sg fails we need to call dma_unmap_sg.

---
 drivers/mmc/host/bcm2835.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index c42bdaa..82e4b08 100644
@@ -518,13 +518,16 @@ void bcm2835_prepare_dma(struct bcm2835_host *host, struct mmc_data *data)
 	desc = dmaengine_prep_slave_sg(dma_chan, data->sg, sg_len, dir_slave,
 				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 
-	if (desc) {
-		desc->callback = bcm2835_dma_complete;
-		desc->callback_param = host;
-		host->dma_desc = desc;
-		host->dma_chan = dma_chan;
-		host->dma_dir = dir_data;
+	if (!desc) {
+		dma_unmap_sg(dma_chan->device->dev, data->sg, sg_len, dir_data);
+		return;
 	}
+
+	desc->callback = bcm2835_dma_complete;
+	desc->callback_param = host;
+	host->dma_desc = desc;
+	host->dma_chan = dma_chan;
+	host->dma_dir = dir_data;
 }
 
 static void bcm2835_start_dma(struct bcm2835_host *host)

2.7.4

This series is:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181128/0d646466/attachment.sig>

Hi Ulf,

should i resend with Eric's Ack?

Applied for next, thanks!

Kind regards
Uffe

Hello,

thanks for the patches.

I tried to replace the bcm2835 sdhost driver in my 4.4 kernel with the upstream driver + these updates but the 16GB orange EVO card still locks up the mmc controller. It seems it locks up much less but is certainly not solid.

Thanks

Michal

Hi Michal,

Am 21.03.19 um 21:03 schrieb Michal Suchánek:

could you please retry with mainline kernel 5.0?

Maybe this is related:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2019-February/008542.html

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I can try that. What I have is pretty much 5.0 anyway so I don't expect much difference:

660fc733bd7436f4fa1a351376493e635514ed64  mmc: bcm2835: Add new driver for the sdhost controller.
bf3240bada0211b4a555d75f027181c8432b2d20  mmc: bcm2835: Fix possible NULL ptr dereference in
c00a231ba053a9b0be8d512957b99395b92bc620  mmc: bcm2835: fix potential null pointer dereferences
2c9e89a1d602c12a4f2bd4c7a57a3315247e3f21  mmc: bcm2835: constify mmc_host_ops structures
118032be389009b07ecb5a03ffe219a89d421def  mmc: bcm2835: Don't overwrite max frequency unconditionally
f6000a4eb34e6462bc0dd39809c1bb99f9633269  mmc: bcm2835: reset host on timeout
07d405769afea5718529fc9e341f0b13b3189b6f  mmc: bcm2835: Recover from MMC_SEND_EXT_CSD
5eae252db3856e62c778832d4d59f6efc5b0aaf9  mmc: bcm2835: Release DMA channel on driver unload
af19b7ce76ba220f358c82b0a5e7d68909a23aa5  mmc: bcm2835: Avoid possible races on data requests
37fefadee8bb665ae337a15aa635dabff9f66ade  mmc: bcm2835: Terminate timeout work synchronously
6dc6f2619017109e45550accc120f823fdc31c3e  mmc: bcm2835: Refactor dma_map_sg handling
2f5da678351f0d504966fab113968202aa5713fb  mmc: bcm2835: Properly handle dmaengine_prep_slave_sg
8c9620b1cc9b69e82fa8d4081d646d0016b602e7  mmc: bcm2835: Fix DMA channel leak on probe error
e5c1e63c932379b89d7404d4e5fde1bf8abff951  mmc: bcm2835: Drop DMA channel error pointer check

What I had before was some previous draft of 660fc733bd7436f4fa1a351376493e635514ed64 + f6000a4eb34e6462bc0dd39809c1bb99f9633269 which would eventually recover on the error but the errors were more frequent with my card.

I suspect that one of the locking fixes that went into mainline recently prevents recovering from the error but I did not try reverting them yet. It takes quite a while to reimage, boot different kernel, run system update (which now invariably crashes/locks up with any kernel).

Thanks

Michal

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Michal,

as long as the issue lies in the sdhost driver code. There also has been a lot of fixes by Lukas Wunner to the DMA engine driver. I prefer a well defined source base.

Would be nice if you can find this regression.

That's why i prefer Raspbian during development, where i can simply replace the kernel / modules with a sd card reader on a PC:

https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75

This should reduce the round-trip, but this could accidently hide the problem as well.

Thanks
Stefan

_______________________________________________
linux-rpi-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Does not look that good.

# bad: [a48caea1745f30e87ab5a8dba5e365d0346aa600] mmc: bcm2835: Drop DMA channel error pointer check (bsc#983145). # good: [c6b26547caa816608ea5c5717b29c78769a22972] mmc: bcm2835: reset host on timeout (bsc#983145, bsc#1070872). git bisect start 'a48caea1745f30e87ab5a8dba5e365d0346aa600' 'c6b26547caa816608ea5c5717b29c78769a22972'
# good: [3eb1fe752f52865eff0f9b8edd95b61b6a9c1010] mmc: bcm2835: Terminate timeout work synchronously (bsc#983145, bsc#1070872). git bisect good 3eb1fe752f52865eff0f9b8edd95b61b6a9c1010
# good: [50b4dd03bd11fbb647f0edbed8501290f6f9ea46] mmc: bcm2835: Properly handle dmaengine_prep_slave_sg (bsc#983145). git bisect good 50b4dd03bd11fbb647f0edbed8501290f6f9ea46

On the good commits a few timeouts occur and are recovered. This leaves

8c9620b1cc9b69e82fa8d4081d646d0016b602e7 mmc: bcm2835: Fix DMA channel leak on probe error e5c1e63c932379b89d7404d4e5fde1bf8abff951 mmc: bcm2835: Drop DMA channel error pointer check

which are not overly suspect. The latter should be noop at least. So maybe the indefinite lockup depends on card state or some other factor. Also the recovery in the case when it does recover does take quite long (like minutes) with the system pretty much completely non-responsive. Not waiting long enough for the system to recover might be also a factor.

I can replace the kernel easily but without going back to the obsolete system state I lose the system upgrade as test case.

Thanks

Michal

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Michal,

could you please try this patch:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2019-March/008627.html

Stefan

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello,

So I suspect we have two different issues here:

mmc1: Card stuck in programming state! mmcblk0 card_busy_detect

this is an issue to which particular card models are more suspectible and the workaround is to reset the controller which we already do. It typically happens under some IO load. 90% of the time I get this when doing system upgrade when system is installed on a particular orange card.

The other which is even harder to reproduce is

sdhost-bcm2835 3f202000.sdhost: timeout waiting for hardware interrupt.

It typically happens shortly after imaging the card for me.

I can try that but the question is what symptom is it supposed to cure and what is the rationale for the change.

Thanks

Michal

_______________________________________________
linux-rpi-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

So I updated the dma driver to be able to apply this patch, applied it, and the system locked up. It even stopped logging over the network - only the serial console shows the message:

Linux version 4.4.176-1.g755499f-default (geeko@buildhost) (gcc version 4.8.5 (SUSE Linux) )
....

(  7/526) Installing: info2html-2.0-223.1.noarch .........................[done]
(  8/526) Installing: libX11-data-1.6.3-10.3.1.noarch ...............<89%>===[|][  664.670582] sdhost-bcm2835 3f202000.sdhost: ti.
[  674.750501] sdhost-bcm2835 3f202000.sdhost: timeout waiting for hardware interrupt.

So at least on 4.4 kernel this does not help.

Thanks

Michal

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel