On Mon, 25 May 2009 16:59:17 +0200
Wolfgang Mües <[email protected]> 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.
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 <[email protected]>
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 <[email protected]>
---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: [email protected]: 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Jun 10, 2009 at 09:29:49AM +0200, Wolfgang M?es wrote:
>
> From: Wolfgang Muees <[email protected]>
>
> 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 <[email protected]>
Nice patch Wolfgang and well done for sticking this out ;-)
FWIW,
Acked-by: Matt Fleming <[email protected]>
On Wed, 10 Jun 2009 09:29:49 +0200
Wolfgang Mües <[email protected]> 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.