[PATCH] mmc_spi: use EILSEQ for possible transmission errors

[PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

From: Wolfgang Muees <wolfgang.mues@auerswald.de>

o This patch changes the reported error code for the responses
  to a command from EINVAL/EIO to EILSEQ, as EINVAL is reserved
  for non-recoverable host errors, and the responses from
  the SD/MMC card may be because of recoverable transmission
  errors in the command or in the response. Response codes
  are NOT protected by a checksum, so don't trust them.

Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>

---
diff -uprN 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c
--- 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c	2009-04-08 11:11:20.000000000 +0200
+++ 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c	2009-05-14 12:49:42.000000000 +0200
@@ -334,17 +334,18 @@ checkstatus:
 	cmd->error = 0;

 	/* Status byte: the entire seven-bit R1 response.  */
-	if (cmd->resp[0] != 0) {
-		if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS
-				      | R1_SPI_ILLEGAL_COMMAND)
-				& cmd->resp[0])
-			value = -EINVAL;
-		else if (R1_SPI_COM_CRC & cmd->resp[0])
-			value = -EILSEQ;
-		else if ((R1_SPI_ERASE_SEQ | R1_SPI_ERASE_RESET)
-				& cmd->resp[0])
-			value = -EIO;
-		/* else R1_SPI_IDLE, "it's resetting" */
+	/*
+	 * Note that we have a problem here: as the response is NOT protected
+	 * by a CRC or checksum, a transmission error in the response will
+	 * be interpreted as an error code. So we map all error codes to
+	 * EILSEQ here, to allow for the upper layer to retry the command.
+	 * If one of these error codes is a non-recoverable error, retries
+	 * will do no harm.
+	 */
+
+	/* Allow only 0 and R1_SPI_IDLE here */
+	if (cmd->resp[0] & ~R1_SPI_IDLE) {
+		value = -EILSEQ;
 	}

 	switch (mmc_spi_resp_type(cmd)) {
---
regards

i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Matt Fleming]

On Thu, May 14, 2009 at 12:24:27PM +0100, Wolfgang Mües wrote:
> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> o This patch changes the reported error code for the responses
>   to a command from EINVAL/EIO to EILSEQ, as EINVAL is reserved
>   for non-recoverable host errors, and the responses from
>   the SD/MMC card may be because of recoverable transmission
>   errors in the command or in the response. Response codes
>   are NOT protected by a checksum, so don't trust them.
> 
> Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> ---
> diff -uprN 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c
> --- 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c	2009-04-08 11:11:20.000000000 +0200
> +++ 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c	2009-05-14 12:49:42.000000000 +0200
> @@ -334,17 +334,18 @@ checkstatus:
>  	cmd->error = 0;
> 
>  	/* Status byte: the entire seven-bit R1 response.  */
> -	if (cmd->resp[0] != 0) {
> -		if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS
> -				      | R1_SPI_ILLEGAL_COMMAND)
> -				& cmd->resp[0])
> -			value = -EINVAL;
> -		else if (R1_SPI_COM_CRC & cmd->resp[0])
> -			value = -EILSEQ;
> -		else if ((R1_SPI_ERASE_SEQ | R1_SPI_ERASE_RESET)
> -				& cmd->resp[0])
> -			value = -EIO;
> -		/* else R1_SPI_IDLE, "it's resetting" */
> +	/*
> +	 * Note that we have a problem here: as the response is NOT protected
> +	 * by a CRC or checksum, a transmission error in the response will
> +	 * be interpreted as an error code. So we map all error codes to
> +	 * EILSEQ here, to allow for the upper layer to retry the command.
> +	 * If one of these error codes is a non-recoverable error, retries
> +	 * will do no harm.
> +	 */
> +
> +	/* Allow only 0 and R1_SPI_IDLE here */
> +	if (cmd->resp[0] & ~R1_SPI_IDLE) {
> +		value = -EILSEQ;
>  	}
> 
>  	switch (mmc_spi_resp_type(cmd)) {

Hmm, always returning -EILSEQ is devious. What happens if we sent an
illegal command? The value of "value" is passed up to the callers via
cmd->error and so may eventually get printed in the pr_debug() call in
mmc_request_done(), line 86.

Whereas before the error would display EINVAL for an illegal command
now it'll display EILSEQ, which makes no sense. Seeing EILSEQ in my
log when really the error is EINVAL is gonna really confuse me.

IMHO always assuming that command errors are caused by transmission
problems is not the right solution.

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

Matt,

Am Dienstag, 19. Mai 2009 schrieb Matt Fleming:
> On Thu, May 14, 2009 at 12:24:27PM +0100, Wolfgang Mües wrote:
> > o This patch changes the reported error code for the responses
> >   to a command from EINVAL/EIO to EILSEQ

> Hmm, always returning -EILSEQ is devious. What happens if we sent an
> illegal command?
Isn't this a theoretical question?
Sending an illegal command will never work. Even the programmer of this piece 
of software will notice, and such a software will not find it's way into the 
kernel.

And IF it would, and really it's an illegal command, then it does no harm if 
it is retried several times. It will never work.

Matt, I do not like this patch either.

But given the fact that a response is NOT checksum-protected, there is NO WAY 
we can tell if the received response is a transmission error or a real error 
code from the SD/MMC card.

My first patch has not changed the error codes in mmc_spi, but only changed 
the actions in block.c.

But Pierre has objected against this: he stated that EINVAL is used by all 
(non-SPI) drivers only for true non-recoverable errors, and requested that 
another error code should be used by the mmc_spi driver. The apropriate code 
for transmission errors is EILSEQ.

Maybe a solution-in-the-middle is to use different error codes in the mmc_spi 
framework, which allows to differentiate between individual response codes, 
and treat them all as transmission errors in block.c. What error codes can we 
choose?

> IMHO always assuming that command errors are caused by transmission
> problems is not the right solution.

It's not your or my fault, it's the fault of the spi SD/MMC card protocol 
designer. You may send him to me, already prepared: naked and in chains ;-)

regards
 
i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Matt Fleming]

On Tue, May 19, 2009 at 09:49:07PM -0700, David Brownell wrote:
> On Tuesday 19 May 2009, Matt Fleming wrote:
> > Hmm, always returning -EILSEQ is devious. What happens if we sent an
> > illegal command? The value of "value" is passed up to the callers via
> > cmd->error and so may eventually get printed in the pr_debug() call in
> > mmc_request_done(), line 86.
> 
> True, but a pr_debug from mmc_spi could help that.  A patch doing
> that would need to be less aggressive about ripping out the current
> fault-parsing logic, but it could continue reporting -EILSEQ to
> cope with the possible response mangling.
> 

Yes, a less aggressive patch would be fine.

> > Whereas before the error would display EINVAL for an illegal command
> > now it'll display EILSEQ, which makes no sense. Seeing EILSEQ in my
> > log when really the error is EINVAL is gonna really confuse me.
> > 
> > IMHO always assuming that command errors are caused by transmission
> > problems is not the right solution.
> 
> Do you have a better solution to propose though?  If Wolfgang
> is actually observing transmission errors there, I'm not sure
> a better one is to be had.
> 

You're right. It's not cool for me to criticise and not give any
suggestions. I've been trying to come up with some middle-ground
solution since yesterday.

I think if the patch was less aggressive at returning EILSEQ, it'd be
fine. For instance, returning EILSEQ if the response you get doesn't
make any sense given the command you sent or if the response is just
garbage.

It might even make sense to retry commands (say twice, and only if
it's safe to do so) and if the response codes differ in bizarre ways
then the status bits have probably been garbled on the bus and EILSEQ
is the correct error code to use. I'm less sure about this bit though,
it depends on exactly what we're trying to fix.

Wolfgang, is there a particular scenario where you're seeing the
status bits trashed by noise on the line? Or is this just a cautionary
patch?

On Tue, May 19, 2009 at 01:47:45PM +0200, Wolfgang Mües wrote:
> 
> > IMHO always assuming that command errors are caused by transmission
> > problems is not the right solution.
> 
> It's not your or my fault, it's the fault of the spi SD/MMC card protocol 
> designer. You may send him to me, already prepared: naked and in chains ;-)

This is the funniest reply I've ever read.

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[David Brownell]

On Tuesday 19 May 2009, Matt Fleming wrote:
> Hmm, always returning -EILSEQ is devious. What happens if we sent an
> illegal command? The value of "value" is passed up to the callers via
> cmd->error and so may eventually get printed in the pr_debug() call in
> mmc_request_done(), line 86.

True, but a pr_debug from mmc_spi could help that.  A patch doing
that would need to be less aggressive about ripping out the current
fault-parsing logic, but it could continue reporting -EILSEQ to
cope with the possible response mangling.


> Whereas before the error would display EINVAL for an illegal command
> now it'll display EILSEQ, which makes no sense. Seeing EILSEQ in my
> log when really the error is EINVAL is gonna really confuse me.
> 
> IMHO always assuming that command errors are caused by transmission
> problems is not the right solution.

Do you have a better solution to propose though?  If Wolfgang
is actually observing transmission errors there, I'm not sure
a better one is to be had.

Though I wonder what this would do for anyone trying SDIO over
the mmc_spi driver.

- Dave

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

Am Mittwoch, 20. Mai 2009 schrieb David Brownell:
> On Tuesday 19 May 2009, Matt Fleming wrote:
> > Hmm, always returning -EILSEQ is devious. What happens if we sent an
> > illegal command? The value of "value" is passed up to the callers via
> > cmd->error and so may eventually get printed in the pr_debug() call in
> > mmc_request_done(), line 86.
>
> True, but a pr_debug from mmc_spi could help that.  A patch doing
> that would need to be less aggressive about ripping out the current
> fault-parsing logic, but it could continue reporting -EILSEQ to
> cope with the possible response mangling.

what about the following proposal which is not so aggressive?

The current mapping in mmc_spi.c is:

R1_SPI_PARAMETER => EINVAL
R1_SPI_ADDRESS => EINVAL
R1_SPI_ILLEGAL_COMMAND => EINVAL
R1_SPI_COM_CRC => EILSEQ
R1_SPI_ERASE_SEQ => EIO
R1_SPI_ERASE_RESET => EIO

R1_SPI_COM_CRC is a transmission error - no doubt. Mapping to EILSEQ is OK.

R1_SPI_ERASE_SEQ and R1_SPI_ERASE_RESET are responses to a sector erase 
command. block.c is not sending such commands, so block.c can safely assume 
that each such response is a transmission error.

R1_SPI_PARAMETER and R1_SPI_ADDRESS are errors from the card controller
because a parameter or an address is out of range/invalid. It is mapped to 
EINVAL, and so the caller can not distinguish between errors from the host 
controller and driver or the card. This has to be changed to some other code,
maybe EFAULT /* Bad address */.

R1_SPI_ILLEGAL_COMMAND is mapped to EINVAL, and I suggest it to replace with
ENOSYS /* Function not implemented */.

A quick scan in the mmc directory for EINVAL reveals that there are no callers 
which need to be changed.

As block.c does not send a command, parameter or address which is invalid/out 
of range, these errors must be considered transmission errors in the command 
(if CRC is off) or in the response.

regards

i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[David Brownell]

On Wednesday 20 May 2009, Wolfgang Mües wrote:
> what about the following proposal which is not so aggressive?

Seems plausible to me, though note one comment at the end...


> The current mapping in mmc_spi.c is:
> 
> R1_SPI_PARAMETER => EINVAL
> R1_SPI_ADDRESS => EINVAL
> R1_SPI_ILLEGAL_COMMAND => EINVAL
> R1_SPI_COM_CRC => EILSEQ
> R1_SPI_ERASE_SEQ => EIO
> R1_SPI_ERASE_RESET => EIO

Yeah, that took a bit of head-scratching to come up with.


> R1_SPI_COM_CRC is a transmission error - no doubt. Mapping to EILSEQ is OK.
> 
> R1_SPI_ERASE_SEQ and R1_SPI_ERASE_RESET are responses to a sector erase 
> command. block.c is not sending such commands, so block.c can safely assume 
> that each such response is a transmission error.

Hmm, would hope that's not a long-term plan.  Remember that the
firmware in the card can leverage "that's erased" knowledge for
things like wear leveling.  SSDs and other storage devices would
likewise benefit from such knowledge.  I'm quite certain there's
been discussion about adding support for that in the block layer.


> R1_SPI_PARAMETER and R1_SPI_ADDRESS are errors from the card controller
> because a parameter or an address is out of range/invalid. It is mapped to 
> EINVAL, and so the caller can not distinguish between errors from the host 
> controller and driver or the card. This has to be changed to some other code,
> maybe EFAULT /* Bad address */.

OK.


> R1_SPI_ILLEGAL_COMMAND is mapped to EINVAL, and I suggest it to replace with
> ENOSYS /* Function not implemented */.
> 
> A quick scan in the mmc directory for EINVAL reveals that there are no callers 
> which need to be changed.
> 
> As block.c does not send a command, parameter or address which is invalid/out 
> of range,

That is, *today* it doesn't.  For the moment I suspect just logging
the actual fault code (a pr_debug) may will suffice; this is the kind
of stuff that shouldn't get buried too deeply.


> these errors must be considered transmission errors in the command  
> (if CRC is off) or in the response.

And the key issue is -- correct me if I'm wrong -- that returning
EINVAL makes trouble, so you need to change those three codes.  The
others don't much matter, right?

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Pierre Ossman]

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

On Wed, 20 May 2009 02:20:46 -0700
David Brownell <david-b@pacbell.net> wrote:

> On Wednesday 20 May 2009, Wolfgang Mües wrote:
> > R1_SPI_COM_CRC is a transmission error - no doubt. Mapping to EILSEQ is OK.
> > 
> > R1_SPI_ERASE_SEQ and R1_SPI_ERASE_RESET are responses to a sector erase 
> > command. block.c is not sending such commands, so block.c can safely assume 
> > that each such response is a transmission error.
> 
> Hmm, would hope that's not a long-term plan.  Remember that the
> firmware in the card can leverage "that's erased" knowledge for
> things like wear leveling.  SSDs and other storage devices would
> likewise benefit from such knowledge.  I'm quite certain there's
> been discussion about adding support for that in the block layer.
> 

It's already in AFAIK. I even had code for hooking it up to mmc_block.
But it didn't produce any measurable results so I never merged it.

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: 198 bytes --]

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[David Brownell]

On Wednesday 20 May 2009, Pierre Ossman wrote:
> >		 Remember that the
> > firmware in the card can leverage "that's erased" knowledge for
> > things like wear leveling.  SSDs and other storage devices would
> > likewise benefit from such knowledge.  I'm quite certain there's
> > been discussion about adding support for that in the block layer.
> 
> It's already in AFAIK. I even had code for hooking it up to mmc_block.
> But it didn't produce any measurable results so I never merged it.

I'd expect the results would take time to show.  As in, they would
facilitate wear leveling logic, which may be difficult to measure
except by testing various cards to destruction ... even for vendors
that *do* have decent wear leveling.  :)

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

David,

Am Donnerstag, 21. Mai 2009 schrieb David Brownell:
> On Wednesday 20 May 2009, Pierre Ossman wrote:
> > >		 Remember that the
> > > firmware in the card can leverage "that's erased" knowledge for
> > > things like wear leveling.  SSDs and other storage devices would
> > > likewise benefit from such knowledge.  I'm quite certain there's
> > > been discussion about adding support for that in the block layer.
> >
> > It's already in AFAIK. I even had code for hooking it up to mmc_block.
> > But it didn't produce any measurable results so I never merged it.
>
> I'd expect the results would take time to show.  As in, they would
> facilitate wear leveling logic, which may be difficult to measure
> except by testing various cards to destruction ... even for vendors
> that *do* have decent wear leveling.  :)

Can someone please explain for me the purpose and the implementation of this 
wear level logic in block.c?

I can not see why a sector erase and the sector erase result codes of a MMC/SD 
card can be used to get any usefull information about wear leveling in the 
card. The mapping of physical to logical blocks is not reported by the card, 
and the number of erase cycles for each block is also not reported. SO how 
can the host be able to optimize the wear leveling?

As far as I can see, the best wear level handling for MMC/SD cards is to 
optimize the size of the chunks of data to write. I have posted a patch
(mmc_spi: speedup for slow cards, less wear-out), and it was accepted by 
Pierre at 11th of april.

best regards
 
i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[David Brownell]

On Monday 25 May 2009, Wolfgang Mües wrote:
> Can someone please explain for me the purpose and the implementation of this 
> wear level logic in block.c?

I think you're misinterpreting what I said.  The wear leveling
logic is in the microcontroller *INSIDE THE MMC/SD DEVICE* not
on the Linux side driver.


> I can not see why a sector erase and the sector erase result codes of a MMC/SD 
> card can be used to get any usefull information about wear leveling in the 
> card. The mapping of physical to logical blocks is not reported by the card, 
> and the number of erase cycles for each block is also not reported. SO how 
> can the host be able to optimize the wear leveling?

It can't.  But when the card controller knows that for example
certain logical blocks are not holding data, it can use that
for wear leveling ... it's got more physical blocks to use to
even out the writes, as well as having any records it kept on
writes, reads, and erasures.

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

David,

Am Montag, 25. Mai 2009 schrieb David Brownell:
> It can't.  But when the card controller knows that for example
> certain logical blocks are not holding data, it can use that
> for wear leveling ... it's got more physical blocks to use to
> even out the writes, as well as having any records it kept on
> writes, reads, and erasures.

OK. So the block layer is issuing erase commands for all sectors which get 
unused, to let the mmc/sd card controller know that these areas are unused.

Are these erase commands requests (as reads and writes)? I suppose...

So if this code gets integrated, we can do:

if (command is erase)
	accept erase response codes
else
	treat erase response codes as transmission errors

Should be no problem...

regards
 
i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Pierre Ossman]

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

On Mon, 25 May 2009 12:18:18 +0200
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> 
> So if this code gets integrated, we can do:
> 
> if (command is erase)
> 	accept erase response codes
> else
> 	treat erase response codes as transmission errors
> 
> Should be no problem...
> 

This furthers the layering violation though. I don't remember why
mmc_spi is looking at responses to begin with. It should be up to the
caller to interpret them.

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: 198 bytes --]

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

Pierre,

Am Montag, 25. Mai 2009 schrieb Pierre Ossman:
> I don't remember why
> mmc_spi is looking at responses to begin with.

Because responses are very different.

First of all, responses in SD mode and SPI mode are different.

Second, responses in SD mode are CRC-7 protected, so that it is easy to check 
for transmission errors.

SPI mode responses (R1...) are without any CRC.

Third, if you have SD mode, you use a hardware host controller which manages 
all command/data/response actions, and what you typically get is the content 
of the status register of the host controller (which is different from the
error code of the SD card).

So it is very natural that every host controller driver mapps the responses 
from the host controller to a common representation (== error code). The 
error codes with special meanings for MMC/SD are summed up in mmc/core.h .

The SPI host driver is a recent addition to the linux kernel, and the fact 
that SPI responses are not CRC-protected was not accounted for in the code.

As it makes no sense to code the retry logic in every driver, the prefered way 
is to code the retry logic in block.c. The non-SPI-mode-drivers will profit 
from this logic, because they are getting retries for EILSEQ (CRC error) and
ETIMEDOUT (card has not understand the command).

The biggest problem in the error codes is that EINVAL is used by the SPI 
driver both for recoverable and for non-recoverable errors, which has to be 
splitted IMHO.

regards
 
i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Pierre Ossman]

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

On Mon, 25 May 2009 16:59:17 +0200
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> Pierre,
> 
> Am Montag, 25. Mai 2009 schrieb Pierre Ossman:
> > I don't remember why
> > mmc_spi is looking at responses to begin with.
> 
> Because responses are very different.
> 
> First of all, responses in SD mode and SPI mode are different.
> 

MMC and SD aren't identical either, but we don't delegate that to the
host drivers.

> Second, responses in SD mode are CRC-7 protected, so that it is easy to check 
> for transmission errors.
> 
> SPI mode responses (R1...) are without any CRC.
> 

This however is more relevant. But OTOH, I don't think mmc_spi has a
better idea if the status can be trusted or not than a higher layer.

> Third, if you have SD mode, you use a hardware host controller which manages 
> all command/data/response actions, and what you typically get is the content 
> of the status register of the host controller (which is different from the
> error code of the SD card).
> 
> So it is very natural that every host controller driver mapps the responses 
> from the host controller to a common representation (== error code). The 
> error codes with special meanings for MMC/SD are summed up in mmc/core.h .
> 

But the other drivers don't attempt to interpret the response from the
card. They just give the response back up and let the next layer deal
with it.

> The SPI host driver is a recent addition to the linux kernel, and the fact 
> that SPI responses are not CRC-protected was not accounted for in the code.
> 
> As it makes no sense to code the retry logic in every driver, the prefered way 
> is to code the retry logic in block.c. The non-SPI-mode-drivers will profit 
> from this logic, because they are getting retries for EILSEQ (CRC error) and
> ETIMEDOUT (card has not understand the command).

Indeed. But I don't like the fact that we're hiding interpretation of
responses down in the driver. It should be up to the layer issuing the
MMC request how the response should be interpreted.

But that's an issue that can be left for another day...

> 
> The biggest problem in the error codes is that EINVAL is used by the SPI 
> driver both for recoverable and for non-recoverable errors, which has to be 
> splitted IMHO.
> 

Indeed. I'll queue up the patch.

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: 198 bytes --]

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4331 bytes --]

Pierre,
Am Dienstag, 9. Juni 2009 schrieb Pierre Ossman:> This however is more relevant. But OTOH, I don't think mmc_spi has a> better idea if the status can be trusted or not than a higher layer.
> But the other drivers don't attempt to interpret the response from the> card. They just give the response back up and let the next layer deal> with it.
> Indeed. But I don't like the fact that we're hiding interpretation of> responses down in the driver. It should be up to the layer issuing the> MMC request how the response should be interpreted.
OK. I think you are right. The driver should only do a reasonable mapping, and the upper layer should do the interpretion.
Have you noticed that I have posted a revised revision of the patch named 
[PATCH] mmc_spi: don't use EINVAL for possible transmission errors
on 26th of May 2009? This was based on the discussion with Matt Flemming and David Brownell. (I have changed the name of the patch because the oldname was missleading now).
> Indeed. I'll queue up the patch.
Please queue up the new patch. Patch appended for reference here:
From: Wolfgang Muees <wolfgang.mues@auerswald.de>
o This patch changes the reported error code for the responses  to a command from EINVAL to EFAULT/ENOSYS, as EINVAL is reserved  for non-recoverable host errors, and the responses from  the SD/MMC card may be because of recoverable transmission  errors in the command or in the response. Response codes  in SPI mode are NOT protected by a checksum, so don't trust them.
This is a revised, minimal-invasive patch version. As Pierre Ossmanpointed out, EINVAL should only be used for non-recoverable errors(and is used so in the whole rest of the mmc framework).
I have checked every instance of EINVAL in the mmc framework - no changes were needed here. 
This patch is neccessary for doing a sensible retry managment in the mmmc block layer (patch will follow).
Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>
---diff -uprN 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c--- 2_6_29_rc7_patch_wearout_speedup/drivers/mmc/host/mmc_spi.c 2009-04-08 11:11:20.000000000 +0200+++ 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c  2009-05-26 09:37:28.000000000 +0200@@ -335,15 +335,16 @@ checkstatus:
        /* Status byte: the entire seven-bit R1 response.  */        if (cmd->resp[0] != 0) {-               if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS-                                     | R1_SPI_ILLEGAL_COMMAND)+               if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS)                                & cmd->resp[0])-                       value = -EINVAL;+                       value = -EFAULT; /* Bad address */+               else if (R1_SPI_ILLEGAL_COMMAND & cmd->resp[0])+                       value = -ENOSYS; /* Function not implemented */                else if (R1_SPI_COM_CRC & cmd->resp[0])-                       value = -EILSEQ;+                       value = -EILSEQ; /* Illegal byte sequence */                else if ((R1_SPI_ERASE_SEQ | R1_SPI_ERASE_RESET)                                & cmd->resp[0])-                       value = -EIO;+                       value = -EIO;    /* I/O error */                /* else R1_SPI_IDLE, "it's resetting" */        }
---
best regards i. A. Wolfgang Mües-- Auerswald GmbH & Co. KGHardware DevelopmentTelefon: +49 (0)5306 9219 0Telefax: +49 (0)5306 9219 94 E-Mail: Wolfgang.Mues@Auerswald.deWeb: http://www.auerswald.de --------------------------------------------------------------Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 CremlingenRegistriert beim AG Braunschweig HRA 13289p.h.G Auerswald Geschäftsführungsges. mbHRegistriert beim AG Braunschweig HRB 7463Geschäftsführer: Dipl-Ing. Gerhard Auerswaldÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Matt Fleming]

On Wed, Jun 10, 2009 at 09:29:49AM +0200, Wolfgang Mües wrote:
> 
> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> o This patch changes the reported error code for the responses
>   to a command from EINVAL to EFAULT/ENOSYS, as EINVAL is reserved
>   for non-recoverable host errors, and the responses from
>   the SD/MMC card may be because of recoverable transmission
>   errors in the command or in the response. Response codes
>   in SPI mode are NOT protected by a checksum, so don't trust them.
> 
> This is a revised, minimal-invasive patch version. As Pierre Ossman
> pointed out, EINVAL should only be used for non-recoverable errors
> (and is used so in the whole rest of the mmc framework).
> 
> I have checked every instance of EINVAL in the mmc framework - no 
> changes were needed here. 
> 
> This patch is neccessary for doing a sensible retry managment in the 
> mmmc block layer (patch will follow).
> 
> Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>

Nice patch Wolfgang and well done for sticking this out ;-)

FWIW,
Acked-by: Matt Fleming <matt@console-pimps.org>

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Pierre Ossman]

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

On Wed, 10 Jun 2009 09:29:49 +0200
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> 
> Have you noticed that I have posted a revised revision of the patch named 
> 
> [PATCH] mmc_spi: don't use EINVAL for possible transmission errors
> 
> on 26th of May 2009? This was based on the discussion with Matt Flemming 
> and David Brownell. (I have changed the name of the patch because the old
> name was missleading now).

I did not. Thanks for pointing this out.

> 
> Please queue up the new patch. Patch appended for reference here:
> 

Done. Thanks for all the work.

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: 198 bytes --]

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Pierre Ossman]

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

On Wed, 20 May 2009 19:02:51 -0700
David Brownell <david-b@pacbell.net> wrote:

> On Wednesday 20 May 2009, Pierre Ossman wrote:
> > >		 Remember that the
> > > firmware in the card can leverage "that's erased" knowledge for
> > > things like wear leveling.  SSDs and other storage devices would
> > > likewise benefit from such knowledge.  I'm quite certain there's
> > > been discussion about adding support for that in the block layer.
> > 
> > It's already in AFAIK. I even had code for hooking it up to mmc_block.
> > But it didn't produce any measurable results so I never merged it.
> 
> I'd expect the results would take time to show.  As in, they would
> facilitate wear leveling logic, which may be difficult to measure
> except by testing various cards to destruction ... even for vendors
> that *do* have decent wear leveling.  :)
> 

Who are these mythical decent vendors you speak of? ;)

Seriously though, the modifications to mmc_block can still be found
here if anyone wants to play with it:

http://git.infradead.org/users/drzeus/discard-2.6.git

I haven't checked if it still applies to the current version of the
kernel though, so you're on your own. ;)

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: 198 bytes --]

Re: [PATCH] mmc_spi: use EILSEQ for possible transmission errors

[Wolfgang Mües]

Matt,

Am Mittwoch, 20. Mai 2009 schrieb David Brownell:
> > R1_SPI_ERASE_SEQ and R1_SPI_ERASE_RESET are responses to a sector erase
> > command. block.c is not sending such commands, so block.c can safely
> > assume that each such response is a transmission error.
>
> Hmm, would hope that's not a long-term plan.  Remember that the
> firmware in the card can leverage "that's erased" knowledge for
> things like wear leveling.  SSDs and other storage devices would
> likewise benefit from such knowledge.  I'm quite certain there's
> been discussion about adding support for that in the block layer.

But if the block layer IS sending such erase commands in the future, it CAN 
treat these error codes different:

if (read or write block command)
   erase errors must be transmission errors
else
   accept erase errors;

As it is not neccessary to change the handling of these errors in mmc_spi.c, 
no information is lost.

> And the key issue is -- correct me if I'm wrong -- that returning
> EINVAL makes trouble

Yes, because EINVAL is used for TWO different purposes:
- reporting an error of the driver or host controller
  (which is by nature non-recoverable, a retry is harmless,
   but makes no sense).
- reporting an error from the SD card (in mmc_spi).
  (which might be a transmission error and retries make
   sense).

> so you need to change those three codes.  

Yepp.

> The others don't much matter, right?

Yes. I can simple treat them as transmission errors in block.c

regards
 
i. A. Wolfgang Mües
-- 
Auerswald GmbH & Co. KG
Hardware Development
Telefon: +49 (0)5306 9219 0
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues@Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald GmbH & Co. KG, Vor den Grashöfen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRA 13289
p.h.G Auerswald Geschäftsführungsges. mbH
Registriert beim AG Braunschweig HRB 7463
Geschäftsführer: Dipl-Ing. Gerhard Auerswald