Skip to contentSkip to conversation

mmc: block: fix read single on recovery logic

PATCH Superseded Christian Löhle 3 participants 8 messages Jan 5, 2022 – Feb 8, 2022

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mmc: block: fix read single on recovery logic

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.

The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining,
or BLK_STS_OK if the single read was successful in the meantime.

---
 drivers/mmc/core/block.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/mmc/core/block.c +14 -14
index 90e1bcd03b46..d7d880ce0f8a 100644
@@ -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 && !mrq->data->error)
+				break;
+		}
 
 		if (mrq->cmd->error ||
 		    mrq->data->error ||
Signature · 4 lines
2.34.1
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
#1 lore raw

So could anyone take a long at this so far?

From: Christian Löhle
Sent: Wednesday, January 5, 2022 5:43 PM
To: [email protected]; Christian Löhle; [email protected]; [email protected]
Cc: Avri Altman
Subject: [PATCH] mmc: block: fix read single on recovery logic

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.

The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining,
or BLK_STS_OK if the single read was successful in the meantime.

---
 drivers/mmc/core/block.c | 28 ++++++++++++++--------------

 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/mmc/core/block.c +0 -0
index 90e1bcd03b46..d7d880ce0f8a 100644
@@ -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 && !mrq->data->error) +                               break;
+               }

                 if (mrq->cmd->error ||
                     mrq->data->error ||

Signature · 5 lines
2.34.1
    =
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
#2 lore raw

+ Adrian

Quoted 3 lines · Christian Löhle

So could anyone take a long at this so far?

Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list.

I have added Adrian too, who knows this code very well too.

Kind regards
Uffe

Quoted 89 lines

From: Christian Löhle
Sent: Wednesday, January 5, 2022 5:43 PM
To: [email protected]; Christian Löhle; [email protected]; [email protected]
Cc: Avri Altman
Subject: [PATCH] mmc: block: fix read single on recovery logic

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.

The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining,
or BLK_STS_OK if the single read was successful in the meantime.

---
 drivers/mmc/core/block.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/mmc/core/block.c +14 -15
index 90e1bcd03b46..d7d880ce0f8a 100644
@@ -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 && !mrq->data->error)
+                               break;
+               }
 
                 if (mrq->cmd->error ||
                     mrq->data->error ||
--

2.34.1

    =

Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782

#3 lore raw
Quoted 7 lines · Ulf Hansson

+ Adrian

Quoted 3 lines · Christian Löhle

So could anyone take a long at this so far?

Quoted 35 lines

Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list.

I have added Adrian too, who knows this code very well too.

Kind regards
Uffe

Quoted 28 lines

From: Christian Löhle
Sent: Wednesday, January 5, 2022 5:43 PM
To: [email protected]; Christian Löhle; [email protected]; [email protected]
Cc: Avri Altman
Subject: [PATCH] mmc: block: fix read single on recovery logic

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.

The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining,
or BLK_STS_OK if the single read was successful in the meantime.

Thanks for the patch.

Looks OK, although a couple of comments below, plus it needs a Fixes tag, and Cc for stable.

Fixes 81196976ed946c ("mmc: block: Add blk-mq support")
Quoted 14 lines
Quoted 14 lines
---
 drivers/mmc/core/block.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/mmc/core/block.c +0 -1
index 90e1bcd03b46..d7d880ce0f8a 100644
@@ -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;
 
Quoted 7 lines

drivers/mmc/core/block.c static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) · line 1691

Quoted 7 lines
drivers/mmc/core/block.c +2 -1
         do {
                 u32 status;
                 int err;
+               int retries = 0;
 
-               mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
+               while (retries++ < MMC_READ_SINGLE_RETRIES) {

Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before? So, maybe:

		while (retries++ <= MMC_READ_SINGLE_RETRIES) {
Quoted 26 lines
Quoted 26 lines
drivers/mmc/core/block.c +9 -11
+                       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;
+                       }
 
Quoted 2 lines

drivers/mmc/core/block.c static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) · line 1707

Quoted 2 lines
drivers/mmc/core/block.c +1 -1
-               retries = 0;
+                       if (!mrq->cmd->error && !mrq->data->error)

We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just

			if (!mrq->cmd->error)
Quoted 12 lines
Quoted 12 lines
drivers/mmc/core/block.c +2 -1
+                               break;
+               }
 
                 if (mrq->cmd->error ||
                     mrq->data->error ||
--

2.34.1

    =

Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782

#4 lore raw

Thanks for the comments Adrian!

Quoted 6 lines
Quoted 1 line

+ while (retries++ < MMC_READ_SINGLE_RETRIES) {

Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before? So, maybe:

              while (retries++ <= MMC_READ_SINGLE_RETRIES) {

Yes, you are correct. Will be fixed in v2.

Quoted 6 lines
Quoted 1 line

+ if (!mrq->cmd->error && !mrq->data->error)

We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just

                       if (!mrq->cmd->error)

That was intentional by me, it was very unintuitive to my you would not retry for data errors. (Considering a data error is likely how you got into the whole recovery in the first place.) But yes I see your point, a very large request might block this for quite a while. Will change in v2, too.

From: Adrian Hunter <[email protected]>
Sent: Friday, February 4, 2022 12:26 PM
To: Ulf Hansson; Christian Löhle
Cc: [email protected]; [email protected]; Avri Altman
Subject: Re: [PATCH] mmc: block: fix read single on recovery logic

Quoted 7 lines · Ulf Hansson

+ Adrian

Quoted 3 lines · Christian Löhle

So could anyone take a long at this so far?

Quoted 35 lines

Thanks for pinging. Apologize for the delay, it's on top of my "to-review" list.

I have added Adrian too, who knows this code very well too.

Kind regards
Uffe

Quoted 28 lines

From: Christian Löhle
Sent: Wednesday, January 5, 2022 5:43 PM
To: [email protected]; Christian Löhle; [email protected]; [email protected]
Cc: Avri Altman
Subject: [PATCH] mmc: block: fix read single on recovery logic

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.

The patch now will count both command and data errors as retries and send BLK_STS_IOERR if there are no retries remaining,
or BLK_STS_OK if the single read was successful in the meantime.

Thanks for the patch.

Looks OK, although a couple of comments below, plus it needs a Fixes tag, and Cc for stable.

Fixes 81196976ed946c ("mmc: block: Add blk-mq support")
Quoted 14 lines
Quoted 14 lines

---
  drivers/mmc/core/block.c | 28 ++++++++++++++--------------

  1 file changed, 14 insertions(+), 14 deletions(-)
drivers/mmc/core/block.c +0 -1
index 90e1bcd03b46..d7d880ce0f8a 100644
@@ -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;
 
Quoted 7 lines

drivers/mmc/core/block.c static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) · line 1691

Quoted 7 lines
drivers/mmc/core/block.c +2 -1
           do {
                   u32 status;
                   int err;
+               int retries = 0;
 
-               mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
+               while (retries++ < MMC_READ_SINGLE_RETRIES) {

Because this is now checked at the top of the loop, wouldn't that result in one fewer retries than before?  So, maybe:

                while (retries++ <= MMC_READ_SINGLE_RETRIES) {

Quoted 26 lines
Quoted 26 lines
drivers/mmc/core/block.c +9 -11
+                       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;
+                       }
 
Quoted 2 lines

drivers/mmc/core/block.c static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req) · line 1707

Quoted 2 lines
drivers/mmc/core/block.c +1 -1
-               retries = 0;
+                       if (!mrq->cmd->error && !mrq->data->error)

We weren't retrying for data errors before, and I don't think we want to because single block read can be very slow. i.e. just

                        if (!mrq->cmd->error)

Quoted 12 lines
Quoted 12 lines
drivers/mmc/core/block.c +2 -0
+                               break;
+               }
 

                  if (mrq->cmd->error || mrq->data->error ||
--
2.34.1
     =
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782

    =

Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782

#5 lore raw

PATCH v2 1/1 mmc: block: fix read single on recovery logic

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")

---
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(-)
drivers/mmc/core/block.c +14 -14
index 4e61b28a002f..8d718aa56d33 100644
@@ -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 ||
Signature · 4 lines
2.34.1
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
Reviewed-by on v2 1/1
#6 lore raw
Quoted 15 lines · Christian Löhle

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.

Quoted 3 lines
Fixes 81196976ed946c ("mmc: block: Add blk-mq support")
Quoted 61 lines

---
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(-)
drivers/mmc/core/block.c +14 -14
index 4e61b28a002f..8d718aa56d33 100644
@@ -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 ||
#7 lore raw
Quoted 17 lines · Adrian Hunter
Quoted 15 lines · Christian Löhle

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.

Quoted 5 lines
Quoted 3 lines
Fixes 81196976ed946c ("mmc: block: Add blk-mq support")

Applied for fixes, thanks to both of you!

Kind regards
Uffe

Quoted 63 lines
Quoted 62 lines

---
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(-)
drivers/mmc/core/block.c +14 -14
index 4e61b28a002f..8d718aa56d33 100644
@@ -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 ||