Re: [PATCH 1/2] mmc_block: print better data error message after timeout

[Pierre Ossman <drzeus@drzeus.cx>]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Wed, 29 Oct 2008 12:29:48 +0200
Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> Add command response and card status to error
> messages.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---
>  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).

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

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]