mmc_block: print better data error message after timeout
PATCH 1/2 mmc_block: print better data error message after timeout
In particular, if the card gets an ECC error it will timeout, in which case it is much more helpful to see an ECC error rather than a timeout error.
---drivers/mmc/card/block.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 43 insertions(+), 4 deletions(-)
drivers/mmc/card/block.c +0 -0
@@ -210,6 +210,47 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
return blocks; }
+static void print_data_error(struct mmc_blk_request *brq, struct mmc_card *card,
+ struct request *req)
+{
+ struct mmc_command cmd;
+ char *emsg;
+ u32 status;
+ int status_err = 0;
+
+ if (brq->data.error != -ETIMEDOUT || mmc_host_is_spi(card->host))
+ goto out_print;
+
+ if (brq->mrq.stop)
+ /* 'Stop' response contains card status */
+ status = brq->mrq.stop->resp[0];
+ else {
+ cmd.opcode = MMC_SEND_STATUS;
+ cmd.arg = card->rca << 16;
+ cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+ status_err = mmc_wait_for_cmd(card->host, &cmd, 0);
+ if (status_err)
+ goto out_print;
+ status = cmd.resp[0];
+ }
+
+ emsg = (status & R1_CARD_ECC_FAILED) ? "ECC" : "I/O";
+
+ printk(KERN_ERR "%s: %s error transferring data, sector %u, "
+ "card status %#x\n", req->rq_disk->disk_name, emsg,
+ (unsigned)req->sector, status);
+
+ return;
+
+out_print:
+ printk(KERN_ERR "%s: error %d transferring data, sector %u, nr %u\n",
+ req->rq_disk->disk_name, brq->data.error, (unsigned)req->sector,
+ (unsigned)req->nr_sectors);
+ if (status_err)
+ printk(KERN_ERR "%s: error %d requesting card status\n",
+ req->rq_disk->disk_name, status_err);
+}
+
static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
{
struct mmc_blk_data *md = mq->data;
@@ -281,10 +322,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
req->rq_disk->disk_name, brq.cmd.error);
}- if (brq.data.error) {
- printk(KERN_ERR "%s: error %d transferring data\n",
- req->rq_disk->disk_name, brq.data.error);
- }
+ if (brq.data.error)
+ print_data_error(&brq, card, req);
if (brq.stop.error) {
printk(KERN_ERR "%s: error %d sending stop command\n",Signature · 1 line
1.5.4.3
Quoted 6 lines · On Thu, 16 Oct 2008 16:26:55 +0300 Adrian Hunter <[email protected]>…
In particular, if the card gets an ECC error it will timeout, in which case it is much more helpful to see an ECC error rather than a timeout error.
Signed-off-by Adrian Hunter <[email protected]>---
Woo. I think you're the first I've seen that has been able to trigger an actual card error. :)
As for the patch, I like the idea but I'm not entirely satisfied with the implementation.
Review
Quoted 11 lines
(quoted diff) +11
-0+static void print_data_error(struct mmc_blk_request *brq, struct mmc_card *card, + struct request *req) +{ + struct mmc_command cmd; + char *emsg; + u32 status; + int status_err = 0; + + if (brq->data.error != -ETIMEDOUT || mmc_host_is_spi(card->host)) + goto out_print; +
The error codes are more of a hint than anything else, so you should check the status for all non-zero codes. You should also not just check data.error, but all of them.
And why exclude spi?
Review
Quoted 12 lines
(quoted diff) +12
-0+ if (brq->mrq.stop) + /* 'Stop' response contains card status */ + status = brq->mrq.stop->resp[0]; + else { + cmd.opcode = MMC_SEND_STATUS; + cmd.arg = card->rca << 16; + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; + status_err = mmc_wait_for_cmd(card->host, &cmd, 0); + if (status_err) + goto out_print; + status = cmd.resp[0]; + }
Errors can occur on writes as well, so I suggest accumulating the status bits instead of trying to get the entire set in one go. I.e.:
status = 0;
if (mrq.stop)
status |= mrq.stop.resp[0]; while (card not ready) status |= send_status();
IOW, you should do this in the main handler (that already has the status loop for writes).
Review
Quoted 3 lines
(quoted diff) +3
-0+ + emsg = (status & R1_CARD_ECC_FAILED) ? "ECC" : "I/O"; +
There are also some other error codes that can be of interest. "Internal controller error" for example.
(it's probably also better to state "Unknown" error and not "I/O" for the fallback)
Review static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) · line 327
Quoted 11 lines
(quoted diff) +2
-4@@ -281,10 +322,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) req->rq_disk->disk_name, brq.cmd.error); } - if (brq.data.error) { - printk(KERN_ERR "%s: error %d transferring data\n", - req->rq_disk->disk_name, brq.data.error); - } + if (brq.data.error) + print_data_error(&brq, card, req);
Please keep the old message and add this as a new extra piece of information. It is helpful for debugging to see both what the driver and the card reported.
Rgds
Signature · 6 lines
-- Pierre Ossman WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.
Quoted 9 lines · Pierre Ossman
Quoted 6 lines · On Thu, 16 Oct 2008 16:26:55 +0300 Adrian Hunter <[email protected]>…
In particular, if the card gets an ECC error it will timeout, in which case it is much more helpful to see an ECC error rather than a timeout error.
Signed-off-by Adrian Hunter <[email protected]>---
Thank you for looking at the patch.
Quoted 2 lines
Woo. I think you're the first I've seen that has been able to trigger an actual card error. :)
Well, my impression is that they are quite easy to break
Quoted 18 lines
As for the patch, I like the idea but I'm not entirely satisfied with the implementation.
Review
Quoted 11 lines
(quoted diff) +11
-0+static void print_data_error(struct mmc_blk_request *brq, struct mmc_card *card, + struct request *req) +{ + struct mmc_command cmd; + char *emsg; + u32 status; + int status_err = 0; + + if (brq->data.error != -ETIMEDOUT || mmc_host_is_spi(card->host)) + goto out_print; +The error codes are more of a hint than anything else, so you should check the status for all non-zero codes. You should also not just check data.error, but all of them.
OK but the error bits are cleared as soon as the status has been reported, which is in the command response or, in the case of timeout, the response to the next command. Reading the status after that shows nothing. The status would have to be fished out of brq->mrq.cmd->resp[0].
ETIMEDOUT is a special case because it is not a hint for the error, it is rather the normal response to an error - so it seemed very misleading to me.
Quoted 1 line
And why exclude spi?
I don't have SPI, so I can't test it. AFAICT timeouts are not meant to happen with SPI so it is a genuine error.
The SPI error reporting is a little different. The non-SPI code that I wrote certainly won't work for SPI. I may look at doing something for SPI.
Quoted 16 lines
Review
Quoted 12 lines
(quoted diff) +12
-0+ if (brq->mrq.stop) + /* 'Stop' response contains card status */ + status = brq->mrq.stop->resp[0]; + else { + cmd.opcode = MMC_SEND_STATUS; + cmd.arg = card->rca << 16; + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; + status_err = mmc_wait_for_cmd(card->host, &cmd, 0); + if (status_err) + goto out_print; + status = cmd.resp[0]; + }Errors can occur on writes as well, so I suggest accumulating the status bits instead of trying to get the entire set in one go. I.e.:
Quoted 9 lines
status = 0;
if (mrq.stop)status |= mrq.stop.resp[0]; while (card not ready) status |= send_status();IOW, you should do this in the main handler (that already has the status loop for writes).
The nesting may get too deep and the control paths too complicated if it is done in the main handler, but I will see what I can do. Ideally the loop needs some limit to prevent a misbehaving card locking up the driver.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + emsg = (status & R1_CARD_ECC_FAILED) ? "ECC" : "I/O"; +There are also some other error codes that can be of interest. "Internal controller error" for example.
Do you mean CC_ERROR? I picked out ECC errors because NAND will get ECC errors if the NAND page was not written cleanly (e.g. if the power went off during the write) I guess a good card should recover the old data and never report ECC errors, but that is a separate issue.
Quoted 2 lines
(it's probably also better to state "Unknown" error and not "I/O" for the fallback)
"Unknown" is for unknown error codes, so I don't think it is appropriate. In this case we are simply not bothering to break down all the errors bits.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -281,10 +322,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) req->rq_disk->disk_name, brq.cmd.error); }
Quoted 11 lines
Review static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) · line 327
Quoted 7 lines
(quoted diff) +2
-4- if (brq.data.error) { - printk(KERN_ERR "%s: error %d transferring data\n", - req->rq_disk->disk_name, brq.data.error); - } + if (brq.data.error) + print_data_error(&brq, card, req);Please keep the old message and add this as a new extra piece of information. It is helpful for debugging to see both what the driver and the card reported.
It already does that.
Add command response and card status to error
messages.
---drivers/mmc/card/block.c | 56 ++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 49 insertions(+), 7 deletions(-)
Pierre
This patch amends error messages for all error codes and is independent of SPI. Status values are not OR'd together from different commands as that would muddy the waters for anyone trying to understand the nature of the error. Waiting for the card to become ready after an error is also not included, as it is not in the standards and is a separate issue anyway.
Regards
Adrian
drivers/mmc/card/block.c +0 -0
@@ -209,6 +209,27 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
return blocks; }
+static u32 get_card_status(struct mmc_card *card, struct request *req) +{
+ struct mmc_command cmd;
+ int err;
+
+ /* SEND STATUS command is not supported by SDIO */
+ if (mmc_card_sdio(card))
+ return 0;
+
+ memset(&cmd, 0, sizeof(struct mmc_command));
+ cmd.opcode = MMC_SEND_STATUS;
+ if (!mmc_host_is_spi(card->host))
+ cmd.arg = card->rca << 16;
+ cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
+ err = mmc_wait_for_cmd(card->host, &cmd, 0);
+ if (err)
+ printk(KERN_ERR "%s: error %d sending status comand",
+ req->rq_disk->disk_name, err);
+ return cmd.resp[0];
+}
+
static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
{
struct mmc_blk_data *md = mq->data;@@ -220,7 +241,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
do {
struct mmc_command cmd;- u32 readcmd, writecmd;
+ u32 readcmd, writecmd, status = 0;
memset(&brq, 0, sizeof(struct mmc_blk_request)); brq.mrq.cmd = &brq.cmd;
@@ -275,19 +296,40 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
* until later as we need to wait for the card to leave
* programming mode even when things go wrong.
*/
+ if (brq.cmd.error || brq.data.error || brq.stop.error)
+ status = get_card_status(card, req);
+
if (brq.cmd.error) {
- printk(KERN_ERR "%s: error %d sending read/write command\n",
- req->rq_disk->disk_name, brq.cmd.error);
+ printk(KERN_ERR "%s: error %d sending read/write "
+ "command, response %#x, card status %#x\n",
+ req->rq_disk->disk_name, brq.cmd.error,
+ brq.cmd.resp[0], status);
}
if (brq.data.error) {- printk(KERN_ERR "%s: error %d transferring data\n",
- req->rq_disk->disk_name, brq.data.error);
+ if (brq.data.error == -ETIMEDOUT) {
+ /* 'Stop' response contains card status */
+ if (brq.mrq.stop)
+ status = brq.mrq.stop->resp[0];
+ printk(KERN_ERR "%s: error transferring data,"
+ " sector %u, nr %u, card status %#x\n",
+ req->rq_disk->disk_name,
+ (unsigned)req->sector,
+ (unsigned)req->nr_sectors, status);
+ } else {
+ printk(KERN_ERR "%s: error %d transferring data,"
+ " sector %u, nr %u, card status %#x\n",
+ req->rq_disk->disk_name, brq.data.error,
+ (unsigned)req->sector,
+ (unsigned)req->nr_sectors, status);
+ }
}
if (brq.stop.error) {
- printk(KERN_ERR "%s: error %d sending stop command\n",
- req->rq_disk->disk_name, brq.stop.error);
+ printk(KERN_ERR "%s: error %d sending stop command, "
+ "response %#x, card status %#x\n",
+ req->rq_disk->disk_name, brq.stop.error,
+ brq.stop.resp[0], status);
}
if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {Signature · 1 line
1.5.4.3
Quoted 12 lines · Adrian Hunter
Add command response and card status to error
messages.Signed-off-by Adrian Hunter <[email protected]>---
drivers/mmc/card/block.c| 56
++++++++++++++++++++++++++++++++++++++++-----1 files changed, 49 insertions(+), 7 deletions(-)Pierre
Quoted 8 lines
This patch amends error messages for all error codes and is independent of SPI. Status values are not OR'd together from different commands as that would muddy the waters for anyone trying to understand the nature of the error. Waiting for the card to become ready after an error is also not included, as it is not in the standards and is a separate issue anyway.
Regards
Adrian
What is the status of this patch?
Quoted 3 lines · On Mon, 10 Nov 2008 10:20:23 +0200 Adrian Hunter <[email protected]>…
What is the status of this patch?
Waiting for me to have time to look closer at it.
Rgds
Signature · 6 lines
-- Pierre Ossman WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.
Quoted 4 lines · On Wed, 29 Oct 2008 12:29:48 +0200 Adrian Hunter <[email protected]>…
Add command response and card status to error
messages.Signed-off-by Adrian Hunter <[email protected]>
Quoted 8 lines
--- drivers/mmc/card/block.c | 56 ++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 49 insertions(+), 7 deletions(-)Pierre
This patch amends error messages for all error codes and is independent of SPI.
Looks good. It's ready to queue up except for two very small details (see below).
Quoted 2 lines
Status values are not OR'd together from different commands as that would muddy the waters for anyone trying to understand the nature of the error.
I suppose. I'm just generally uneasy about error handling in SD/MMC as the specs aren't that clear. :)
Quoted 2 lines
Waiting for the card to become ready after an error is also not included, as it is not in the standards and is a separate issue anyway.
What do you mean?
This will handle your case of seeing read errors, but for write errors we probably need to wait for the write to complete fully. But that's something that can be added later...
Review
Quoted 10 lines
(quoted diff) +9
-0+static u32 get_card_status(struct mmc_card *card, struct request *req) +{ + struct mmc_command cmd; + int err; + + /* SEND STATUS command is not supported by SDIO */ + if (mmc_card_sdio(card)) + return 0; +
This is redundant. We never bind to a SDIO card.
Quoted 10 lines
(quoted diff) +7
-2if (brq.data.error) { - printk(KERN_ERR "%s: error %d transferring data\n", - req->rq_disk->disk_name, brq.data.error); + if (brq.data.error == -ETIMEDOUT) { + /* 'Stop' response contains card status */ + if (brq.mrq.stop) + status = brq.mrq.stop->resp[0]; + printk(KERN_ERR "%s: error transferring data," + " sector %u, nr %u, card status %#x\n", + req->rq_disk->disk_name,
Review
Quoted 10 lines
(quoted diff) +9
-0+ (unsigned)req->sector, + (unsigned)req->nr_sectors, status); + } else { + printk(KERN_ERR "%s: error %d transferring data," + " sector %u, nr %u, card status %#x\n", + req->rq_disk->disk_name, brq.data.error, + (unsigned)req->sector, + (unsigned)req->nr_sectors, status); + } }
Do we need the two different printk:s? It would reduce the code a bit if you just let the if clause modify the status value.
Rgds
Signature · 6 lines
-- Pierre Ossman WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.
Quoted 12 lines · Pierre Ossman
Quoted 9 lines · On Wed, 29 Oct 2008 12:29:48 +0200 Adrian Hunter <[email protected]>…
Add command response and card status to error
messages.Signed-off-by Adrian Hunter <[email protected]>--- drivers/mmc/card/block.c | 56 ++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 49 insertions(+), 7 deletions(-)
Quoted 6 lines
Quoted 3 lines
Pierre
This patch amends error messages for all error codes and is independent of SPI.
Looks good. It's ready to queue up except for two very small details (see below).
Sorry for not responding sooner. The mail got lost and I only just noticed it.
Quoted 10 lines
Quoted 2 lines
Status values are not OR'd together from different commands as that would muddy the waters for anyone trying to understand the nature of the error.
I suppose. I'm just generally uneasy about error handling in SD/MMC as the specs aren't that clear. :)
Quoted 2 lines
Waiting for the card to become ready after an error is also not included, as it is not in the standards and is a separate issue anyway.
What do you mean?
You had suggested
while (card not ready) status |= send_status();
and I didn't do it, because of the reasons given above.
Quoted 3 lines
This will handle your case of seeing read errors, but for write errors we probably need to wait for the write to complete fully. But that's something that can be added later...
Multiblock writes are terminated with a stop command when an error occurs, and there is a loop waiting on the status whether an error occurs or not.
Quoted 12 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+static u32 get_card_status(struct mmc_card *card, struct request *req) +{ + struct mmc_command cmd; + int err; + + /* SEND STATUS command is not supported by SDIO */ + if (mmc_card_sdio(card)) + return 0; +This is redundant. We never bind to a SDIO card.
OK
Quoted 15 lines
Quoted 15 lines
(quoted diff) +12
-2if (brq.data.error) { - printk(KERN_ERR "%s: error %d transferring data\n", - req->rq_disk->disk_name, brq.data.error); + if (brq.data.error == -ETIMEDOUT) { + /* 'Stop' response contains card status */ + if (brq.mrq.stop) + status = brq.mrq.stop->resp[0]; + printk(KERN_ERR "%s: error transferring data," + " sector %u, nr %u, card status %#x\n", + req->rq_disk->disk_name, + (unsigned)req->sector, + (unsigned)req->nr_sectors, status); + } else { + printk(KERN_ERR "%s: error %d transferring data," + " sector %u, nr %u, card status %#x\n",
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ req->rq_disk->disk_name, brq.data.error, + (unsigned)req->sector, + (unsigned)req->nr_sectors, status); + } }Do we need the two different printk:s? It would reduce the code a bit if you just let the if clause modify the status value.
OK
From: Adrian Hunter <[email protected]>
Date: Thu, 16 Oct 2008 12:55:25 +0300
Subject: [PATCH] mmc_block: print better error messages
Add command response and card status to error
messages.
---drivers/mmc/card/block.c | 44 +++++++++++++++++++++++++++++++++++++-------
1 files changed, 37 insertions(+), 7 deletions(-)
drivers/mmc/card/block.c +0 -0
@@ -209,6 +209,23 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
return blocks; }
+static u32 get_card_status(struct mmc_card *card, struct request *req) +{
+ struct mmc_command cmd;
+ int err;
+
+ memset(&cmd, 0, sizeof(struct mmc_command));
+ cmd.opcode = MMC_SEND_STATUS;
+ if (!mmc_host_is_spi(card->host))
+ cmd.arg = card->rca << 16;
+ cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
+ err = mmc_wait_for_cmd(card->host, &cmd, 0);
+ if (err)
+ printk(KERN_ERR "%s: error %d sending status comand",
+ req->rq_disk->disk_name, err);
+ return cmd.resp[0];
+}
+
static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
{
struct mmc_blk_data *md = mq->data;@@ -220,7 +237,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
do {
struct mmc_command cmd;- u32 readcmd, writecmd;
+ u32 readcmd, writecmd, status = 0;
memset(&brq, 0, sizeof(struct mmc_blk_request)); brq.mrq.cmd = &brq.cmd;
@@ -275,19 +292,32 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
* until later as we need to wait for the card to leave
* programming mode even when things go wrong.
*/
+ if (brq.cmd.error || brq.data.error || brq.stop.error)
+ status = get_card_status(card, req);
+
if (brq.cmd.error) {
- printk(KERN_ERR "%s: error %d sending read/write command\n",
- req->rq_disk->disk_name, brq.cmd.error);
+ printk(KERN_ERR "%s: error %d sending read/write "
+ "command, response %#x, card status %#x\n",
+ req->rq_disk->disk_name, brq.cmd.error,
+ brq.cmd.resp[0], status);
}
if (brq.data.error) {- printk(KERN_ERR "%s: error %d transferring data\n",
- req->rq_disk->disk_name, brq.data.error);
+ if (brq.data.error == -ETIMEDOUT && brq.mrq.stop)
+ /* 'Stop' response contains card status */
+ status = brq.mrq.stop->resp[0];
+ printk(KERN_ERR "%s: error %d transferring data,"
+ " sector %u, nr %u, card status %#x\n",
+ req->rq_disk->disk_name, brq.data.error,
+ (unsigned)req->sector,
+ (unsigned)req->nr_sectors, status);
}
if (brq.stop.error) {
- printk(KERN_ERR "%s: error %d sending stop command\n",
- req->rq_disk->disk_name, brq.stop.error);
+ printk(KERN_ERR "%s: error %d sending stop command, "
+ "response %#x, card status %#x\n",
+ req->rq_disk->disk_name, brq.stop.error,
+ brq.stop.resp[0], status);
}
if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {Signature · 1 line
1.5.4.3
Pierre Ossman
AppliedQuoted 7 lines · On Thu, 04 Dec 2008 15:35:43 +0200 Adrian Hunter <[email protected]>…
Quoted 6 lines · Pierre Ossman
Quoted 2 lines · On Wed, 29 Oct 2008 12:29:48 +0200 Adrian Hunter <[email protected]>…
Waiting for the card to become ready after an error is also not included, as it is not in the standards and is a separate issue anyway.
Quoted 9 lines
Quoted 1 line
What do you mean?
You had suggested
while (card not ready) status |= send_status();and I didn't do it, because of the reasons given above.
What confused me was that you sait "it is not in the standards", but perhaps you were just referring to the read case? In the write case the specs are clear that errors will be done during execution, which can be delayed for writes.
Quoted 9 lines
From: Adrian Hunter <[email protected]>
Date: Thu, 16 Oct 2008 12:55:25 +0300
Subject: [PATCH]mmc_block: print better error messagesAdd command response and card status to error
messages.Signed-off-by Adrian Hunter <[email protected]>---
Queued. Thanks for all the work. :)
Rgds
Signature · 6 lines
-- Pierre Ossman WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.