Messages in this thread | | | Date | Sun, 30 Nov 2008 19:48:45 +0100 | From | Pierre Ossman <> | 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@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. [unhandled content-type:application/pgp-signature] | |