[PATCH] mmc_spi: do propper retry managment in the block layer - 3rd try

[PATCH] mmc_spi: do propper retry managment in the block layer - 3rd try

[Wolfgang Mües]

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

o This patch adds a propper retry managment for reading
  and writing data blocks for mmc and mmc_spi. Blocks are
  retransmitted if the cause of failure might be a transmission
  fault. After a permanent failure, we try to read all other
  pending sectors, but terminate on write.
  This patch was tested with induced transmission errors
  by ESD pulses (and survived).

This patch is based on 
[PATCH] mmc_spi: don't use EINVAL for possible transmission errors

Suggested changes from Matt Fleming and David Brownell are
incorporated.

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

---
diff -uprN 2_6_29_rc7_patch_EILSEQ/drivers/mmc/card/block.c 2_6_29_rc7_patch_retries/drivers/mmc/card/block.c
--- 2_6_29_rc7_patch_EILSEQ/drivers/mmc/card/block.c	2009-03-09 17:10:55.000000000 +0100
+++ 2_6_29_rc7_patch_retries/drivers/mmc/card/block.c	2009-05-18 15:00:41.000000000 +0200
@@ -230,12 +230,14 @@ static int mmc_blk_issue_rq(struct mmc_q
 	struct mmc_card *card = md->queue.card;
 	struct mmc_blk_request brq;
 	int ret = 1, disable_multi = 0;
+	int retries = 3;

 	mmc_claim_host(card->host);

 	do {
 		struct mmc_command cmd;
 		u32 readcmd, writecmd, status = 0;
+		int error = 0;

 		memset(&brq, 0, sizeof(struct mmc_blk_request));
 		brq.mrq.cmd = &brq.cmd;
@@ -251,10 +253,8 @@ static int mmc_blk_issue_rq(struct mmc_q
 		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 		brq.data.blocks = req->nr_sectors;

-		/*
-		 * After a read error, we redo the request one sector at a time
-		 * in order to accurately determine which sectors can be read
-		 * successfully.
+		/* After an transmission error, we switch to single block
+		 * transfer until the problem is solved or we fail.
 		 */
 		if (disable_multi && brq.data.blocks > 1)
 			brq.data.blocks = 1;
@@ -262,10 +262,12 @@ static int mmc_blk_issue_rq(struct mmc_q
 		if (brq.data.blocks > 1) {
 			/* SPI multiblock writes terminate using a special
 			 * token, not a STOP_TRANSMISSION request.
+			 * Here, this request is set for ALL types of
+			 * hosts, so that we can use it in the lower
+			 * layers if the data transfer stage has failed
+			 * and the card is not able to accept the token.
 			 */
-			if (!mmc_host_is_spi(card->host)
-					|| rq_data_dir(req) == READ)
-				brq.mrq.stop = &brq.stop;
+			brq.mrq.stop = &brq.stop;
 			readcmd = MMC_READ_MULTIPLE_BLOCK;
 			writecmd = MMC_WRITE_MULTIPLE_BLOCK;
 		} else {
@@ -311,52 +313,71 @@ static int mmc_blk_issue_rq(struct mmc_q
 		mmc_wait_for_req(card->host, &brq.mrq);

 		mmc_queue_bounce_post(mq);
-
-		/*
-		 * Check for errors here, but don't jump to cmd_err
-		 * until later as we need to wait for the card to leave
-		 * programming mode even when things go wrong.
-		 */
-		if (brq.cmd.error || brq.data.error || brq.stop.error) {
-			if (brq.data.blocks > 1 && rq_data_dir(req) == READ) {
-				/* Redo read one sector at a time */
-				printk(KERN_WARNING "%s: retrying using single "
-				       "block read\n", req->rq_disk->disk_name);
-				disable_multi = 1;
-				continue;
-			}
-			status = get_card_status(card, req);
-		}
-
+#if 0
+		printk(KERN_INFO "%s transfer sector %d count %d\n",
+			(rq_data_dir(req) == READ) ? "Read" : "Write",
+			(int)req->sector, (int)brq.data.blocks);
+#endif
+		/* Error reporting */
 		if (brq.cmd.error) {
+			error = brq.cmd.error;
+			status = get_card_status(card, req);
 			printk(KERN_ERR "%s: error %d sending read/write "
 			       "command, response %#x, card status %#x\n",
 			       req->rq_disk->disk_name, brq.cmd.error,
 			       brq.cmd.resp[0], status);
-		}
-
-		if (brq.data.error) {
-			if (brq.data.error == -ETIMEDOUT && brq.mrq.stop)
+		} else if (brq.data.error) {
+			error = brq.data.error;
+			if ((brq.data.error == -ETIMEDOUT)
+			    && brq.mrq.stop && !brq.stop.error)
 				/* 'Stop' response contains card status */
 				status = brq.mrq.stop->resp[0];
+			else
+				status = get_card_status(card, req);
 			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);
-		}
-
-		if (brq.stop.error) {
+		} else if (brq.stop.error) {
+			error = brq.stop.error;
+			status = get_card_status(card, req);
 			printk(KERN_ERR "%s: error %d sending stop command, "
 			       "response %#x, card status %#x\n",
 			       req->rq_disk->disk_name, brq.stop.error,
 			       brq.stop.resp[0], status);
 		}

+		/*
+		 * If this is an SD card and we're writing, we can first
+		 * mark the known good sectors as ok.
+		 *
+		 * If the card is not SD, we can still ok written sectors
+		 * as reported by the controller (which might be less than
+		 * the real number of written sectors, but never more).
+		 *
+		 * For read transfers, we report the number of tranfered bytes.
+		 */
+		if (error && mmc_card_sd(card) && (rq_data_dir(req) != READ)) {
+			u32 blocks;
+
+			blocks = mmc_sd_num_wr_blocks(card);
+			if (blocks != (u32)-1)
+				brq.data.bytes_xfered = blocks << 9;
+		}
+		if (brq.data.bytes_xfered) {
+			spin_lock_irq(&md->lock);
+			ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
+			spin_unlock_irq(&md->lock);
+		}
+
+		/* Wait until card is ready for new data. This information is
+		 * only available in non-SPI mode. In SPI mode, the busy
+		 * handling is done in the SPI driver.
+		 */
 		if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
 			do {
 				int err;
-
 				cmd.opcode = MMC_SEND_STATUS;
 				cmd.arg = card->rca << 16;
 				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -374,66 +395,63 @@ static int mmc_blk_issue_rq(struct mmc_q
 			} while (!(cmd.resp[0] & R1_READY_FOR_DATA) ||
 				(R1_CURRENT_STATE(cmd.resp[0]) == 7));

-#if 0
-			if (cmd.resp[0] & ~0x00000900)
-				printk(KERN_ERR "%s: status = %08x\n",
-				       req->rq_disk->disk_name, cmd.resp[0]);
-			if (mmc_decode_status(cmd.resp))
-				goto cmd_err;
-#endif
 		}

-		if (brq.cmd.error || brq.stop.error || brq.data.error) {
-			if (rq_data_dir(req) == READ) {
-				/*
-				 * After an error, we redo I/O one sector at a
-				 * time, so we only reach here after trying to
-				 * read a single sector.
-				 */
-				spin_lock_irq(&md->lock);
-				ret = __blk_end_request(req, -EIO, brq.data.blksz);
-				spin_unlock_irq(&md->lock);
-				continue;
-			}
-			goto cmd_err;
-		}
+		/* Do retries for all sort of transmission errors */
+		switch (error) {

-		/*
-		 * A block was successfully transferred.
+		case 0:	/* no error: continue, reset error variables */
+			disable_multi = 0;
+			retries = 3;
+			break;
+
+		/* Card has not understand command. As we do only send
+		 * valid commands, this must be a transmission error. */
+		case -EPROTO:	/* fall through */
+
+		/* Transmission error */
+		case -EILSEQ:	/* fall through */
+
+		/* Timeout, no answer. This might be a transmission error
+		 * host -> card or a real timeout. If we repeat the command,
+		 * we do an overall slowdown and have good chances to
+		 * complete the transfer even in case of a real timeout.
 		 */
-		spin_lock_irq(&md->lock);
-		ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
-		spin_unlock_irq(&md->lock);
+		case -ETIMEDOUT:
+			disable_multi = 1;
+			if (--retries > 0)
+				break;
+			/* Fall through */
+
+		/* non-recoverable error or retries exhausted */
+		default:
+			/* Terminate, if transfer direction is write.
+			 * We are not allowed to continue after a non-
+			 * recoverable write!
+			 */
+			if (rq_data_dir(req) != READ)
+				goto cmd_err;
+			/* If transfer direction is read, report error
+			 * for this sector and continue to read
+			 * the rest. Get all available information!
+			 */
+			spin_lock_irq(&md->lock);
+			ret = __blk_end_request(req, -EIO, brq.data.blksz);
+			spin_unlock_irq(&md->lock);
+			/* reset error variables */
+			disable_multi = 0;
+			retries = 3;
+			break;
+		}
+
+	/* repeat until no more sectors left */
 	} while (ret);

 	mmc_release_host(card->host);

 	return 1;

- cmd_err:
- 	/*
- 	 * If this is an SD card and we're writing, we can first
- 	 * mark the known good sectors as ok.
- 	 *
-	 * If the card is not SD, we can still ok written sectors
-	 * as reported by the controller (which might be less than
-	 * the real number of written sectors, but never more).
-	 */
-	if (mmc_card_sd(card)) {
-		u32 blocks;
-
-		blocks = mmc_sd_num_wr_blocks(card);
-		if (blocks != (u32)-1) {
-			spin_lock_irq(&md->lock);
-			ret = __blk_end_request(req, 0, blocks << 9);
-			spin_unlock_irq(&md->lock);
-		}
-	} else {
-		spin_lock_irq(&md->lock);
-		ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
-		spin_unlock_irq(&md->lock);
-	}
-
+cmd_err:
 	mmc_release_host(card->host);

 	spin_lock_irq(&md->lock);
diff -uprN 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c
--- 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c	2009-05-14 12:49:42.000000000 +0200
+++ 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c	2009-05-14 15:01:15.000000000 +0200
@@ -743,12 +743,11 @@ mmc_spi_writeblock(struct mmc_spi_host *
 	if (status != 0) {
 		dev_dbg(&spi->dev, "write error %02x (%d)\n",
 			scratch->status[0], status);
-		return status;
-	}
-
+	} else {
 	t->tx_buf += t->len;
 	if (host->dma_dev)
 		t->tx_dma += t->len;
+	}

 	/* Return when not busy.  If we didn't collect that status yet,
 	 * we'll need some more I/O.
@@ -756,9 +755,12 @@ mmc_spi_writeblock(struct mmc_spi_host *
 	for (i = 4; i < sizeof(scratch->status); i++) {
 		/* card is non-busy if the most recent bit is 1 */
 		if (scratch->status[i] & 0x01)
-			return 0;
+		return status;
 	}
-	return mmc_spi_wait_unbusy(host, timeout);
+	i = mmc_spi_wait_unbusy(host, timeout);
+	if (!status)
+		status = i;
+	return status;
 }

 /*
@@ -1087,7 +1089,15 @@ static void mmc_spi_request(struct mmc_h
 	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
 	if (status == 0 && mrq->data) {
 		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
-		if (mrq->stop)
+		/* filter-out the stop command for multiblock writes,
+		* only if the data stage has no transmission error.
+		* If the data stage has a transmission error, send the
+		* STOP command because there is a great chance that the
+		* SPI stop token was not accepted by the card.
+		*/
+		if (mrq->stop &&
+		  ((mrq->data->flags & MMC_DATA_READ)
+		 || mrq->data->error))
 			status = mmc_spi_command_send(host, mrq, mrq->stop, 0);
 		else
 			mmc_cs_off(host);
---
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: do propper retry managment in the block layer - 3rd try

[Pierre Ossman]

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

On Tue, 26 May 2009 12:11:45 +0100
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> From: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 
> o This patch adds a propper retry managment for reading
>   and writing data blocks for mmc and mmc_spi. Blocks are
>   retransmitted if the cause of failure might be a transmission
>   fault. After a permanent failure, we try to read all other
>   pending sectors, but terminate on write.
>   This patch was tested with induced transmission errors
>   by ESD pulses (and survived).
> 
> This patch is based on 
> [PATCH] mmc_spi: don't use EINVAL for possible transmission errors
> 
> Suggested changes from Matt Fleming and David Brownell are
> incorporated.
> 
> Signed-off-by: Wolfgang Muees <wolfgang.mues@auerswald.de>
> 

I think this sounds good in principle. I just need to think a bit more
about the scenarios and make sure we don't miss anything.

FYI, the situation that we must avoid to not screw up the assumptions
of the filesystems is to end up with a situation like this on the card:

 NNNNBNNNOOOO

 N = block with new data
 O = block with previous data
 B = block with damaged data

This could in theory happen when the card has some internal problem
(e.g. bad flash cell) as well go through these states:

 NNNNNNNNOOOO (first write, response to stop command is lost)
 NNNNBNNNOOOO (second write, card has internal problem halfway through)


And please leave out the "o " and indentation in the commit text. :)


> @@ -251,10 +253,8 @@ static int mmc_blk_issue_rq(struct mmc_q
>  		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>  		brq.data.blocks = req->nr_sectors;
> 
> -		/*
> -		 * After a read error, we redo the request one sector at a time
> -		 * in order to accurately determine which sectors can be read
> -		 * successfully.
> +		/* After an transmission error, we switch to single block
> +		 * transfer until the problem is solved or we fail.
>  		 */
>  		if (disable_multi && brq.data.blocks > 1)
>  			brq.data.blocks = 1;

I think we can keep the logic where it uses single blocks for the
remainder of the request. There will be a lot of switching back and
forth to handle the scenario the fallback was originally written for
otherwise. And the system will go back to large transfers in the next
request anyway.

> +#if 0
> +		printk(KERN_INFO "%s transfer sector %d count %d\n",
> +			(rq_data_dir(req) == READ) ? "Read" : "Write",
> +			(int)req->sector, (int)brq.data.blocks);
> +#endif

No dead code please.

> +		/* Error reporting */
>  		if (brq.cmd.error) {
> +			error = brq.cmd.error;
> +			status = get_card_status(card, req);
>  			printk(KERN_ERR "%s: error %d sending read/write "
>  			       "command, response %#x, card status %#x\n",
>  			       req->rq_disk->disk_name, brq.cmd.error,
>  			       brq.cmd.resp[0], status);
> -		}
> -
> -		if (brq.data.error) {
> -			if (brq.data.error == -ETIMEDOUT && brq.mrq.stop)
> +		} else if (brq.data.error) {
> +			error = brq.data.error;
> +			if ((brq.data.error == -ETIMEDOUT)
> +			    && brq.mrq.stop && !brq.stop.error)
>  				/* 'Stop' response contains card status */
>  				status = brq.mrq.stop->resp[0];
> +			else
> +				status = get_card_status(card, req);
>  			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);
> -		}
> -
> -		if (brq.stop.error) {
> +		} else if (brq.stop.error) {
> +			error = brq.stop.error;
> +			status = get_card_status(card, req);
>  			printk(KERN_ERR "%s: error %d sending stop command, "
>  			       "response %#x, card status %#x\n",
>  			       req->rq_disk->disk_name, brq.stop.error,
>  			       brq.stop.resp[0], status);
>  		}
> 

The point of having the continue before this code was to make sure we
didn't get a lot of noise in dmesg for the cases where a retry solved
the issue.

It was also per design that all three errors were printed separately.

> +		/*
> +		 * If this is an SD card and we're writing, we can first
> +		 * mark the known good sectors as ok.
> +		 *
> +		 * If the card is not SD, we can still ok written sectors
> +		 * as reported by the controller (which might be less than
> +		 * the real number of written sectors, but never more).
> +		 *
> +		 * For read transfers, we report the number of tranfered bytes.
> +		 */
> +		if (error && mmc_card_sd(card) && (rq_data_dir(req) != READ)) {
> +			u32 blocks;
> +
> +			blocks = mmc_sd_num_wr_blocks(card);
> +			if (blocks != (u32)-1)
> +				brq.data.bytes_xfered = blocks << 9;
> +		}
> +		if (brq.data.bytes_xfered) {
> +			spin_lock_irq(&md->lock);
> +			ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
> +			spin_unlock_irq(&md->lock);
> +		}

Perhaps this fix can be moved into its own patch? I think it might keep
things cleaner.

> +
> +		/* Wait until card is ready for new data. This information is
> +		 * only available in non-SPI mode. In SPI mode, the busy
> +		 * handling is done in the SPI driver.
> +		 */

Strictly speaking it should be handled in all drivers. Some hardware
doesn't do it properly though so we have this safeguard.

>  		if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
>  			do {
>  				int err;
> -
>  				cmd.opcode = MMC_SEND_STATUS;
>  				cmd.arg = card->rca << 16;
>  				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;

This seems like an unnecessary and negative change.

> @@ -374,66 +395,63 @@ static int mmc_blk_issue_rq(struct mmc_q
>  			} while (!(cmd.resp[0] & R1_READY_FOR_DATA) ||
>  				(R1_CURRENT_STATE(cmd.resp[0]) == 7));
> 
> -#if 0
> -			if (cmd.resp[0] & ~0x00000900)
> -				printk(KERN_ERR "%s: status = %08x\n",
> -				       req->rq_disk->disk_name, cmd.resp[0]);
> -			if (mmc_decode_status(cmd.resp))
> -				goto cmd_err;
> -#endif
>  		}
> 

Might want to move this out as well in order to make this somewhat
complex diff more readable.

> +		/* Do retries for all sort of transmission errors */
> +		switch (error) {
> 
> -		/*
> -		 * A block was successfully transferred.
> +		case 0:	/* no error: continue, reset error variables */
> +			disable_multi = 0;
> +			retries = 3;
> +			break;
> +
> +		/* Card has not understand command. As we do only send
> +		 * valid commands, this must be a transmission error. */
> +		case -EPROTO:	/* fall through */

This indicates a layering problem. The host driver should not be aware
of anything but pure bit errors.

Also, this special meaning should be documented in core.h should we
decide to keep it.

> +		/* non-recoverable error or retries exhausted */
> +		default:
> +			/* Terminate, if transfer direction is write.
> +			 * We are not allowed to continue after a non-
> +			 * recoverable write!
> +			 */
> +			if (rq_data_dir(req) != READ)
> +				goto cmd_err;
> +			/* If transfer direction is read, report error
> +			 * for this sector and continue to read
> +			 * the rest. Get all available information!
> +			 */
> +			spin_lock_irq(&md->lock);
> +			ret = __blk_end_request(req, -EIO, brq.data.blksz);
> +			spin_unlock_irq(&md->lock);
> +			/* reset error variables */
> +			disable_multi = 0;
> +			retries = 3;
> +			break;

This looks wrong. What happened to the original logic of retrying reads
on any error?

> diff -uprN 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c
> --- 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c	2009-05-14 12:49:42.000000000 +0200
> +++ 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c	2009-05-14 15:01:15.000000000 +0200
> @@ -743,12 +743,11 @@ mmc_spi_writeblock(struct mmc_spi_host *
>  	if (status != 0) {
>  		dev_dbg(&spi->dev, "write error %02x (%d)\n",
>  			scratch->status[0], status);
> -		return status;
> -	}
> -
> +	} else {
>  	t->tx_buf += t->len;
>  	if (host->dma_dev)
>  		t->tx_dma += t->len;
> +	}
> 
>  	/* Return when not busy.  If we didn't collect that status yet,
>  	 * we'll need some more I/O.

Bad indentation. And chunk seems unrelated to the rest of the patch.

> @@ -756,9 +755,12 @@ mmc_spi_writeblock(struct mmc_spi_host *
>  	for (i = 4; i < sizeof(scratch->status); i++) {
>  		/* card is non-busy if the most recent bit is 1 */
>  		if (scratch->status[i] & 0x01)
> -			return 0;
> +		return status;
>  	}
> -	return mmc_spi_wait_unbusy(host, timeout);
> +	i = mmc_spi_wait_unbusy(host, timeout);
> +	if (!status)
> +		status = i;
> +	return status;
>  }
> 
>  /*

Same here.

> @@ -1087,7 +1089,15 @@ static void mmc_spi_request(struct mmc_h
>  	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
>  	if (status == 0 && mrq->data) {
>  		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
> -		if (mrq->stop)
> +		/* filter-out the stop command for multiblock writes,
> +		* only if the data stage has no transmission error.
> +		* If the data stage has a transmission error, send the
> +		* STOP command because there is a great chance that the
> +		* SPI stop token was not accepted by the card.
> +		*/
> +		if (mrq->stop &&
> +		  ((mrq->data->flags & MMC_DATA_READ)
> +		 || mrq->data->error))
>  			status = mmc_spi_command_send(host, mrq, mrq->stop, 0);
>  		else
>  			mmc_cs_off(host);

I'm not entirely pleased with this hack, but I'm not sure we can do
much better. Could you add a comment in include/mmc/core.h for *stop in
struct mmc_request that SPI will only use this command on errors.

Could you also move those modifications to its own 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: do propper retry managment in the block layer - 3rd try

[Wolfgang Mües]

Pierre,

Am Mittwoch, 27. Mai 2009 schrieb Pierre Ossman:
> I think this sounds good in principle.

OK. Good.

> FYI, the situation that we must avoid to not screw up the assumptions
> of the filesystems is to end up with a situation like this on the card:
>
>  NNNNBNNNOOOO
>
>  N = block with new data
>  O = block with previous data
>  B = block with damaged data
>
> This could in theory happen when the card has some internal problem
> (e.g. bad flash cell) as well go through these states:
>
>  NNNNNNNNOOOO (first write, response to stop command is lost)
>  NNNNBNNNOOOO (second write, card has internal problem halfway through)

For SPI and for any controller which gives you a correct bytes_xfered, this 
will only be NNNNNNNNNNNNBOOOOOOOOOOO, because after each transfered sector, 
you get a response from the card. So you will not start from the beginning 
any more.

If a host controller does not give a correct bytes_xfered, you might end in 
the scenario you have pointed out, only after exhausting all retries. This is 
equivalent to a bad of multiple sectors. NBNBNBNB000 == NBBBBBBBOOO.

I think we should code according to the expectations of the user.
A nonrecoverable write results in a data loss for the user. And I think that 
the wish of the user to get the newly aquired data down to the card has 
preceedence against the possible loss of some old data on the card. Remember 
that the "old data" will be unused sectors most of the time. In embedded 
systems, the user often has no chance to repeat the storage process or choose 
another medium, and the data ist lost if the write to SD card fails.

> And please leave out the "o " and indentation in the commit text. :)

Hey, you have accepted 9 patches from me without complaining!

I will do next time.
>
> > @@ -251,10 +253,8 @@ static int mmc_blk_issue_rq(struct mmc_q
> >  		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> >  		brq.data.blocks = req->nr_sectors;
> >
> > -		/*
> > -		 * After a read error, we redo the request one sector at a time
> > -		 * in order to accurately determine which sectors can be read
> > -		 * successfully.
> > +		/* After an transmission error, we switch to single block
> > +		 * transfer until the problem is solved or we fail.
> >  		 */
> >  		if (disable_multi && brq.data.blocks > 1)
> >  			brq.data.blocks = 1;
>
> I think we can keep the logic where it uses single blocks for the
> remainder of the request.

But this will increase wear leveling problems for the rest of the request.
And this is for a mean value of 64 sectors!

Why on earth should we do this if it is so easy to avoid?

As transmission errors are seldom, we get a request splitted out in 3 blocks: 
n blocks before the error, one single-block transfer, m blocks after the 
error. And you almost will not notice a slowdown in transmission speed.

> > +#if 0
> > +		printk(KERN_INFO "%s transfer sector %d count %d\n",
> > +			(rq_data_dir(req) == READ) ? "Read" : "Write",
> > +			(int)req->sector, (int)brq.data.blocks);
> > +#endif
>
> No dead code please.

Argggh! Matt Flemming requested this to be a pr_debug, and somehow this was 
not getting into the patch. Please drop the patch; I will resend.

>
> > +		/* Error reporting */
> >  		if (brq.cmd.error) {
> > +			error = brq.cmd.error;
> > +			status = get_card_status(card, req);
> >  			printk(KERN_ERR "%s: error %d sending read/write "
> >  			       "command, response %#x, card status %#x\n",
> >  			       req->rq_disk->disk_name, brq.cmd.error,
> >  			       brq.cmd.resp[0], status);
> > -		}
> > -
> > -		if (brq.data.error) {
> > -			if (brq.data.error == -ETIMEDOUT && brq.mrq.stop)
> > +		} else if (brq.data.error) {
> > +			error = brq.data.error;
> > +			if ((brq.data.error == -ETIMEDOUT)
> > +			    && brq.mrq.stop && !brq.stop.error)
> >  				/* 'Stop' response contains card status */
> >  				status = brq.mrq.stop->resp[0];
> > +			else
> > +				status = get_card_status(card, req);
> >  			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);
> > -		}
> > -
> > -		if (brq.stop.error) {
> > +		} else if (brq.stop.error) {
> > +			error = brq.stop.error;
> > +			status = get_card_status(card, req);
> >  			printk(KERN_ERR "%s: error %d sending stop command, "
> >  			       "response %#x, card status %#x\n",
> >  			       req->rq_disk->disk_name, brq.stop.error,
> >  			       brq.stop.resp[0], status);
> >  		}
>
> The point of having the continue before this code was to make sure we
> didn't get a lot of noise in dmesg for the cases where a retry solved
> the issue.

OK. I will change this behaviour.

> It was also per design that all three errors were printed separately.

Hm. A stop error may occur as consequence of a data error. I will change it.

> > +		/*
> > +		 * If this is an SD card and we're writing, we can first
> > +		 * mark the known good sectors as ok.
> > +		 *
> > +		 * If the card is not SD, we can still ok written sectors
> > +		 * as reported by the controller (which might be less than
> > +		 * the real number of written sectors, but never more).
> > +		 *
> > +		 * For read transfers, we report the number of tranfered bytes.
> > +		 */
> > +		if (error && mmc_card_sd(card) && (rq_data_dir(req) != READ)) {
> > +			u32 blocks;
> > +
> > +			blocks = mmc_sd_num_wr_blocks(card);
> > +			if (blocks != (u32)-1)
> > +				brq.data.bytes_xfered = blocks << 9;
> > +		}
> > +		if (brq.data.bytes_xfered) {
> > +			spin_lock_irq(&md->lock);
> > +			ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
> > +			spin_unlock_irq(&md->lock);
> > +		}
>
> Perhaps this fix can be moved into its own patch? I think it might keep
> things cleaner.

Which fix? The code was only moved and restructured. No new behaviour.

> > +
> > +		/* Wait until card is ready for new data. This information is
> > +		 * only available in non-SPI mode. In SPI mode, the busy
> > +		 * handling is done in the SPI driver.
> > +		 */
>
> Strictly speaking it should be handled in all drivers. Some hardware
> doesn't do it properly though so we have this safeguard.

IMHO no driver and framework is doing this correct. It should not be done 
after a write request, but before the _next_ request. All I have done here 
was to add some documentation about SPI, and leave the previous code intact.
>
> >  		if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
> >  			do {
> >  				int err;
> > -
> >  				cmd.opcode = MMC_SEND_STATUS;
> >  				cmd.arg = card->rca << 16;
> >  				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>
> This seems like an unnecessary and negative change.

Removed in the updated patch I will send.

> > @@ -374,66 +395,63 @@ static int mmc_blk_issue_rq(struct mmc_q
> >  			} while (!(cmd.resp[0] & R1_READY_FOR_DATA) ||
> >  				(R1_CURRENT_STATE(cmd.resp[0]) == 7));
> >
> > -#if 0
> > -			if (cmd.resp[0] & ~0x00000900)
> > -				printk(KERN_ERR "%s: status = %08x\n",
> > -				       req->rq_disk->disk_name, cmd.resp[0]);
> > -			if (mmc_decode_status(cmd.resp))
> > -				goto cmd_err;
> > -#endif
> >  		}
>
> Might want to move this out as well in order to make this somewhat
> complex diff more readable.

OK, I will leave it in for now.

> > +		/* Do retries for all sort of transmission errors */
> > +		switch (error) {
> >
> > -		/*
> > -		 * A block was successfully transferred.
> > +		case 0:	/* no error: continue, reset error variables */
> > +			disable_multi = 0;
> > +			retries = 3;
> > +			break;
> > +
> > +		/* Card has not understand command. As we do only send
> > +		 * valid commands, this must be a transmission error. */
> > +		case -EPROTO:	/* fall through */
>
> This indicates a layering problem. The host driver should not be aware
> of anything but pure bit errors.
>
> Also, this special meaning should be documented in core.h should we
> decide to keep it.

This is not MY error code. EPROTO is send from mmc_spi_writeblock(), and I 
have listed it here because it is a transmission error.

So there seems to be contrary objections: Matt Flemming has requested that the 
exact cause of error is reported by the driver (because otherwise the caller 
will loose information), and you requested to distinguish only between 
transmission errors and the rest.

Matt Flemming has pointed out that further changes in the code will request to 
get the exact cause of error in the block layer, and that error codes might 
have interpreted different according to the command class, so IMHO it is 
better to transport the exact error cause into block.c and do the error 
handling here, according to the type of request.

> > +		/* non-recoverable error or retries exhausted */
> > +		default:
> > +			/* Terminate, if transfer direction is write.
> > +			 * We are not allowed to continue after a non-
> > +			 * recoverable write!
> > +			 */
> > +			if (rq_data_dir(req) != READ)
> > +				goto cmd_err;
> > +			/* If transfer direction is read, report error
> > +			 * for this sector and continue to read
> > +			 * the rest. Get all available information!
> > +			 */
> > +			spin_lock_irq(&md->lock);
> > +			ret = __blk_end_request(req, -EIO, brq.data.blksz);
> > +			spin_unlock_irq(&md->lock);
> > +			/* reset error variables */
> > +			disable_multi = 0;
> > +			retries = 3;
> > +			break;
>
> This looks wrong. What happened to the original logic of retrying reads
> on any error?

You mean error codes like EINVAL, ENOMEDIUM, ENODEV?
I there any sense in retrying non-recoverable errors? Or should I list the 
non-recoverable errors inside the switch (and do nothing), and do a retry for 
read calls on any other error code, so a new coded driver with a new error 
code will get a read retry (but no write retry!) for this new code regardless 
of making sense?

> > diff -uprN 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c
> > 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c ---
> > 2_6_29_rc7_patch_EILSEQ/drivers/mmc/host/mmc_spi.c	2009-05-14
> > 12:49:42.000000000 +0200 +++
> > 2_6_29_rc7_patch_retries/drivers/mmc/host/mmc_spi.c	2009-05-14
> > 15:01:15.000000000 +0200 @@ -743,12 +743,11 @@ mmc_spi_writeblock(struct
> > mmc_spi_host *
> >  	if (status != 0) {
> >  		dev_dbg(&spi->dev, "write error %02x (%d)\n",
> >  			scratch->status[0], status);
> > -		return status;
> > -	}
> > -
> > +	} else {
> >  	t->tx_buf += t->len;
> >  	if (host->dma_dev)
> >  		t->tx_dma += t->len;
> > +	}
> >
> >  	/* Return when not busy.  If we didn't collect that status yet,
> >  	 * we'll need some more I/O.
>
> Bad indentation.
Yepp.

> And chunk seems unrelated to the rest of the patch. 

Ah... no. This is part of sending the STOP command in case of an error.
And sending the STOP command in case of an error is needed for a working retry 
of write commands in SPI mode. Without a STOP command, you can not retry a 
broken write sequence because the card missed the STOP token. 

> I'm not entirely pleased with this hack, but I'm not sure we can do
> much better.

This is the best reaction we can do. The STOP will close/terminate any pending 
write actions. And the card must accept this command in SD mode and in SPI 
read mode, so it is very likely that it will behave if we send it in write 
mode too. And my tests with SD cards were positive.

> Could you add a comment in include/mmc/core.h for *stop in 
> struct mmc_request that SPI will only use this command on errors.
... and for multiblock reads, of course. Yes, I can.

> Could you also move those modifications to its own patch?

Why? It is meaningless without a retry logic for block writes.

Puuhhh... Many points to settle down. Hope we can reach consensus on all open 
points.

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: do propper retry managment in the block layer - 3rd try

[Matt Fleming]

On Thu, May 28, 2009 at 10:28:29AM +0200, Wolfgang Mües wrote:
> Pierre,
> 
> Am Mittwoch, 27. Mai 2009 schrieb Pierre Ossman:
>

[...]

> > > +		/* Do retries for all sort of transmission errors */
> > > +		switch (error) {
> > >
> > > -		/*
> > > -		 * A block was successfully transferred.
> > > +		case 0:	/* no error: continue, reset error variables */
> > > +			disable_multi = 0;
> > > +			retries = 3;
> > > +			break;
> > > +
> > > +		/* Card has not understand command. As we do only send
> > > +		 * valid commands, this must be a transmission error. */
> > > +		case -EPROTO:	/* fall through */
> >
> > This indicates a layering problem. The host driver should not be aware
> > of anything but pure bit errors.
> >
> > Also, this special meaning should be documented in core.h should we
> > decide to keep it.
> 
> This is not MY error code. EPROTO is send from mmc_spi_writeblock(), and I 
> have listed it here because it is a transmission error.
> 
> So there seems to be contrary objections: Matt Flemming has requested that the 
> exact cause of error is reported by the driver (because otherwise the caller 
> will loose information), and you requested to distinguish only between 
> transmission errors and the rest.
> 
> Matt Flemming has pointed out that further changes in the code will request to 
> get the exact cause of error in the block layer, and that error codes might 
> have interpreted different according to the command class, so IMHO it is 
> better to transport the exact error cause into block.c and do the error 
> handling here, according to the type of request.
> 

When I said "report the correct error" I was objecting to using EILSEQ
as The One True Error Code based on the fact that a transmission error
may, or may not, have occurred.

That is not contrary to Pierre's comments. I was not advocating
layering violations ;-)

Re: [PATCH] mmc_spi: do propper retry managment in the block layer - 3rd try

[Wolfgang Mües]

Matt,

Am Donnerstag, 28. Mai 2009 schrieb Matt Fleming:
> When I said "report the correct error" I was objecting to using EILSEQ
> as The One True Error Code based on the fact that a transmission error
> may, or may not, have occurred.

Yes, and we have discussed the problem of error code interpretion based on 
request types.

So, in block.c, if I get a -EIO (== erase error), I can handle this as 
transmission error inside the read/write data function.

If there will be a (future) erase request function in block.c, I will handle 
the same error code as erase error, not as transmission error. No big 
problems here.

To mimic the same behaviour inside the driver, inside mmc_spi_response_get,
I will have to code:

	if (cmd->resp[0] != 0) {
		if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS)
				& cmd->resp[0])
// TODO: transmission error if parameter or address was not out of range.
			value = -EFAULT; /* Bad address */
		else if (R1_SPI_ILLEGAL_COMMAND & cmd->resp[0])
// TODO: transmission error if mandatory command
			value = -ENOSYS; /* Function not implemented */
		else if (R1_SPI_COM_CRC & cmd->resp[0])
			value = -EILSEQ; /* Illegal byte sequence */
		else if ((R1_SPI_ERASE_SEQ | R1_SPI_ERASE_RESET)
				& cmd->resp[0])
// START
			switch (cmd->opcode) {
			case xxx: /* erase commands */
			case yyy:
				value = -EIO;    /* I/O error */
				break;
			default: /* non-erase commands */
				value = -EILSEQ; /* Illegal byte sequence */
				break;
			}
// STOP
		/* else R1_SPI_IDLE, "it's resetting" */
	}

So somewhere I will need to have an error code filter based on the issued 
command (class). Should this be in the driver(s)? Or should it be at the 
location of the caller, in block.c?

The advantages of putting it in block.c is that
a) The command (class) is typically implicit given in the function, and no 
need for a switch() statement.
b) Only one handling for different drivers, not scattered through all(?) host 
drivers.

I must admit that I have difficulties to see a clear layering violation.
There is no clear definition of which error codes should be reported to the 
block layer. There is only a short list of codes with special meaning, but 
not a full list of all used codes.

And some drivers are reporting codes like ENOMEM etc...

I see that Pierre wants to have a more smaller interface between drivers and 
the upper layer, reporting only classes of errors, to have a more smaller and 
cleaner code in the upper layer. But I think that this is a patch of its own, 
and not in the context of the retry patch.

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: do propper retry managment in the block layer - 3rd try

[Matt Fleming]

On Thu, May 28, 2009 at 11:52:26AM +0200, Wolfgang Mües wrote:
> 
> So somewhere I will need to have an error code filter based on the issued 
> command (class). Should this be in the driver(s)? Or should it be at the 
> location of the caller, in block.c?
> 

It should be in the function/file that makes the most sense.

> The advantages of putting it in block.c is that
> a) The command (class) is typically implicit given in the function, and no 
> need for a switch() statement.
> b) Only one handling for different drivers, not scattered through all(?) host 
> drivers.
> 

You know what, I was going to say that only block transaction stuff is
in block.c, but that's not true, there's loads of MMC protocol
knowledge in there too. I don't think there's a better place than
mmc/drivers/card/block.c, currently.

I would expect all this error handling and intimate knowledge of the
MMC/SD protocol to be in drivers/mmc/core, but that's not the
case. Which just seems strange to me.

> I must admit that I have difficulties to see a clear layering violation.
> There is no clear definition of which error codes should be reported to the 
> block layer. There is only a short list of codes with special meaning, but 
> not a full list of all used codes.
> 
> And some drivers are reporting codes like ENOMEM etc...
> 
> I see that Pierre wants to have a more smaller interface between drivers and 
> the upper layer, reporting only classes of errors, to have a more smaller and 
> cleaner code in the upper layer. But I think that this is a patch of its own, 
> and not in the context of the retry patch.
> 

I agree. That could be a separate patch.

Re: [PATCH] mmc_spi: do propper retry managment in the block layer - 3rd try

[Pierre Ossman]

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

(I haven't compared to your latest versions some some of my comments
here might not be relevant for the existing patch)

On Thu, 28 May 2009 10:28:29 +0200
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> Am Mittwoch, 27. Mai 2009 schrieb Pierre Ossman:
> > FYI, the situation that we must avoid to not screw up the assumptions
> > of the filesystems is to end up with a situation like this on the card:
> >
> >  NNNNBNNNOOOO
> >
> >  N = block with new data
> >  O = block with previous data
> >  B = block with damaged data
> >
> > This could in theory happen when the card has some internal problem
> > (e.g. bad flash cell) as well go through these states:
> >
> >  NNNNNNNNOOOO (first write, response to stop command is lost)
> >  NNNNBNNNOOOO (second write, card has internal problem halfway through)
> 
> For SPI and for any controller which gives you a correct bytes_xfered, this 
> will only be NNNNNNNNNNNNBOOOOOOOOOOO, because after each transfered sector, 
> you get a response from the card. So you will not start from the beginning 
> any more.
> 
> If a host controller does not give a correct bytes_xfered, you might end in 
> the scenario you have pointed out, only after exhausting all retries. This is 
> equivalent to a bad of multiple sectors. NBNBNBNB000 == NBBBBBBBOOO.
> 

Hmm... unfortunately this is the norm as I don't think any of the other
drivers provide a proper bytes_xfered. That also means that the verdict
for this patch is NAK for now.

> I think we should code according to the expectations of the user.
> A nonrecoverable write results in a data loss for the user. And I think that 
> the wish of the user to get the newly aquired data down to the card has 
> preceedence against the possible loss of some old data on the card. Remember 
> that the "old data" will be unused sectors most of the time. In embedded 
> systems, the user often has no chance to repeat the storage process or choose 
> another medium, and the data ist lost if the write to SD card fails.
> 

The argument was that this method probably worked fine for classical
file systems, but it wrecked havoc if it was done in the log area of
journaling file systems.

I'd like to reiterate that I like the idea and I really think the
kernel should be able to do this. If you can get the VFS and block layer
people to look at this and sign off on it, then we can move forward and
include this. I don't hive time for that particular crusade myself though.

> >
> > I think we can keep the logic where it uses single blocks for the
> > remainder of the request.
> 
> But this will increase wear leveling problems for the rest of the request.
> And this is for a mean value of 64 sectors!
> 
> Why on earth should we do this if it is so easy to avoid?
> 
> As transmission errors are seldom, we get a request splitted out in 3 blocks: 
> n blocks before the error, one single-block transfer, m blocks after the 
> error. And you almost will not notice a slowdown in transmission speed.
> 

That code was not written for transmission errors but for media errors.
That means that if block 8 is the bad one, your method (provided
bytex_xfered is useless, which is the norm) means that it will transfer
n blocks, 1 block, n-1 blocks, 1 block, etc. failing every second
attempt until it gets to the actually damaged sector.

> >
> > Perhaps this fix can be moved into its own patch? I think it might keep
> > things cleaner.
> 
> Which fix? The code was only moved and restructured. No new behaviour.
> 

Never mind, I wasn't paying proper attention. :)

> > > +
> > > +		/* Wait until card is ready for new data. This information is
> > > +		 * only available in non-SPI mode. In SPI mode, the busy
> > > +		 * handling is done in the SPI driver.
> > > +		 */
> >
> > Strictly speaking it should be handled in all drivers. Some hardware
> > doesn't do it properly though so we have this safeguard.
> 
> IMHO no driver and framework is doing this correct. It should not be done 
> after a write request, but before the _next_ request. All I have done here 
> was to add some documentation about SPI, and leave the previous code intact.

Why on the next request? For performance reasons? The downside to that
is increased complexity. The current system means that the card is
always in an idle state after each request.

(the hardware handling also gets trickier as the host controller also
is kept in some "active" state until busy ends)

> > > +		/* Do retries for all sort of transmission errors */
> > > +		switch (error) {
> > >
> > > -		/*
> > > -		 * A block was successfully transferred.
> > > +		case 0:	/* no error: continue, reset error variables */
> > > +			disable_multi = 0;
> > > +			retries = 3;
> > > +			break;
> > > +
> > > +		/* Card has not understand command. As we do only send
> > > +		 * valid commands, this must be a transmission error. */
> > > +		case -EPROTO:	/* fall through */
> >
> > This indicates a layering problem. The host driver should not be aware
> > of anything but pure bit errors.
> >
> > Also, this special meaning should be documented in core.h should we
> > decide to keep it.
> 
> This is not MY error code. EPROTO is send from mmc_spi_writeblock(), and I 
> have listed it here because it is a transmission error.
> 
> So there seems to be contrary objections: Matt Flemming has requested that the 
> exact cause of error is reported by the driver (because otherwise the caller 
> will loose information), and you requested to distinguish only between 
> transmission errors and the rest.
> 
> Matt Flemming has pointed out that further changes in the code will request to 
> get the exact cause of error in the block layer, and that error codes might 
> have interpreted different according to the command class, so IMHO it is 
> better to transport the exact error cause into block.c and do the error 
> handling here, according to the type of request.
> 

Right. As much as possible of the result should be parsed by the same
layer that assembles the request. So I think EPROTO should be left out
here and we can look at getting better integration with mmc_spi
responses in another patch.

> >
> > This looks wrong. What happened to the original logic of retrying reads
> > on any error?
> 
> You mean error codes like EINVAL, ENOMEDIUM, ENODEV?
> I there any sense in retrying non-recoverable errors? Or should I list the 
> non-recoverable errors inside the switch (and do nothing), and do a retry for 
> read calls on any other error code, so a new coded driver with a new error 
> code will get a read retry (but no write retry!) for this new code regardless 
> of making sense?
> 

Yes. E.g. some limited hardware might not be able to report a timeout
properly so the driver will report EIO for many types of errors. We
should be robust enough to handle that.

> > And chunk seems unrelated to the rest of the patch. 
> 
> Ah... no. This is part of sending the STOP command in case of an error.
> And sending the STOP command in case of an error is needed for a working retry 
> of write commands in SPI mode. Without a STOP command, you can not retry a 
> broken write sequence because the card missed the STOP token. 
> 

Ok, "unrelated" might be a bit strong. But it isn't necessary to commit
them together. For the sake of keeping the patch small, I think this
should be a separate patch which will be committed before the retry
changes.

> > Could you also move those modifications to its own patch?
> 
> Why? It is meaningless without a retry logic for block writes.
> 

See above. Reduces the size of this 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 --]