lkml.org 
[lkml]   [2008]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/2] mmc_block: print better data error message after timeout
On Thu, 16 Oct 2008 16:26:55 +0300
Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> 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 <ext-adrian.hunter@nokia.com>
> ---

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.

> +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?

> + 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).

> +
> + 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)

> @@ -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
--
-- 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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2008-10-26 12:15    [W:0.196 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site