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@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> 
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/