Re: [PATCHv2] mmc: block: fix read single on recovery logic

[Adrian Hunter]

On 04/02/2022 17:11, Christian Löhle wrote:
> On reads with MMC_READ_MULTIPLE_BLOCK that fail,
> the recovery handler will use MMC_READ_SINGLE_BLOCK for
> each of the blocks, up to MMC_READ_SINGLE_RETRIES times each.
> The logic for this is fixed to never report unsuccessful reads
> as success to the block layer.
> 
> On command error with retries remaining, blk_update_request was
> called with whatever value error was set last to.
> In case it was last set to BLK_STS_OK (default), the read will be
> reported as success, even though there was no data read from the device.
> This could happen on a CRC mismatch for the response,
> a card rejecting the command (e.g. again due to a CRC mismatch).
> In case it was last set to BLK_STS_IOERR, the error is reported correctly,
> but no retries will be attempted.
> 
> Fixes: 81196976ed946c ("mmc: block: Add blk-mq support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Loehle <cloehle@xxxxxxxxxxxxxx>

Reviewed-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

> ---
> v2:
>   - Do not allow data error retries
>   - Actually retry MMC_READ_SINGLE_RETRIES times instead of
>   MMC_READ_SINGLE_RETRIES-1
> 
> 
>  drivers/mmc/core/block.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 4e61b28a002f..8d718aa56d33 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req)
>  	struct mmc_card *card = mq->card;
>  	struct mmc_host *host = card->host;
>  	blk_status_t error = BLK_STS_OK;
> -	int retries = 0;
>  
>  	do {
>  		u32 status;
>  		int err;
> +		int retries = 0;
>  
> -		mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
> +		while (retries++ <= MMC_READ_SINGLE_RETRIES) {
> +			mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
>  
> -		mmc_wait_for_req(host, mrq);
> +			mmc_wait_for_req(host, mrq);
>  
> -		err = mmc_send_status(card, &status);
> -		if (err)
> -			goto error_exit;
> -
> -		if (!mmc_host_is_spi(host) &&
> -		    !mmc_ready_for_data(status)) {
> -			err = mmc_blk_fix_state(card, req);
> +			err = mmc_send_status(card, &status);
>  			if (err)
>  				goto error_exit;
> -		}
>  
> -		if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES)
> -			continue;
> +			if (!mmc_host_is_spi(host) &&
> +			    !mmc_ready_for_data(status)) {
> +				err = mmc_blk_fix_state(card, req);
> +				if (err)
> +					goto error_exit;
> +			}
>  
> -		retries = 0;
> +			if (!mrq->cmd->error)
> +				break;
> +		}
>  
>  		if (mrq->cmd->error ||
>  		    mrq->data->error ||
>