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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081130194845.4abd0cb8@mjolnir.drzeus.cx>
Date:	Sun, 30 Nov 2008 19:48:45 +0100
From:	Pierre Ossman <drzeus@...eus.cx>
To:	Adrian Hunter <ext-adrian.hunter@...ia.com>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mmc_block: print better data error message after
 timeout

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)