Re: [PATCH 2/2] mmc_block: ensure all sectors that do not have errors are read

[Pierre Ossman <drzeus@drzeus.cx>]

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

On Thu, 16 Oct 2008 16:26:57 +0300
Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> If a card encounters an ECC error while reading a sector it will
> timeout.  Instead of reporting the entire I/O request as having
> an error, redo the I/O one sector at a time so that all readable
> sectors are provided to the upper layers.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---

We actually had something like this on the table some time ago. It got
scrapped because of data integrity problems. This is just for reads
though, so I guess it should be safe.

> @@ -278,6 +279,9 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>  		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>  		brq.data.blocks = req->nr_sectors;
>  
> +		if (disable_multi && brq.data.blocks > 1)
> +			brq.data.blocks = 1;
> +

A comment here would be nice.

You also need to adjust the sg list when you change the block count.
There was code there that did that previously, but it got removed in
2.6.27-rc1.

> @@ -312,6 +318,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>  
>  		mmc_queue_bounce_post(mq);
>  
> +		if (multi && rq_data_dir(req) == READ &&
> +		    brq.data.error == -ETIMEDOUT) {
> +			/* Redo read one sector at a time */
> +			disable_multi = 1;
> +			continue;
> +		}
> +

Some concerns here:

1. "brq.data.blocks > 1" doesn't need to be optimised into its own
variable. It just obscures things.

2. A comment here as well. Explain what this does and why it is safe
(so people don't try to extend it to writes)

3. You should check all errors, not just data.error and ETIMEDOUT.

4. You should first report the successfully transferred blocks as ok.

> @@ -360,14 +373,21 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>  #endif
>  		}
>  
> -		if (brq.cmd.error || brq.data.error || brq.stop.error)
> +		if (brq.cmd.error || brq.stop.error)
>  			goto cmd_err;

Move your code to inside this if clause and you'll solve 3. and 4. in a
neat manner. You might also want to print something so that it is
visible that the driver retried the transfer.

>  
> -		/*
> -		 * A block was successfully transferred.
> -		 */
> +		if (brq.data.error) {
> +			if (brq.data.error == -ETIMEDOUT &&
> +			    rq_data_dir(req) == READ) {
> +				err = -EIO;
> +				brq.data.bytes_xfered = brq.data.blksz;
> +			} else
> +				goto cmd_err;
> +		} else
> +			err = 0;
> +
>  		spin_lock_irq(&md->lock);
> -		ret = __blk_end_request(req, 0, brq.data.bytes_xfered);
> +		ret = __blk_end_request(req, err, brq.data.bytes_xfered);
>  		spin_unlock_irq(&md->lock);
>  	} while (ret);
>  

Instead of this big song and dance routine, just have a dedicated piece
of code for calling __blk_end_request() for the single sector failure.

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