[PATCH] mmc: block: fix read single on recovery logic

[PATCH] mmc: block: fix read single on recovery logic

[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.

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.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 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 90e1bcd03b46..d7d880ce0f8a 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 && !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

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

[Christian Löhle]

So could anyone take a long at this so far?



From: Christian Löhle
Sent: Wednesday, January 5, 2022 5:43 PM
To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
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.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 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 90e1bcd03b46..d7d880ce0f8a 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 && !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

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

[Ulf Hansson]

+ Adrian

On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> 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

>
>
> From: Christian Löhle
> Sent: Wednesday, January 5, 2022 5:43 PM
> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> 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.
>
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  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 90e1bcd03b46..d7d880ce0f8a 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 && !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
>

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

[Adrian Hunter]

On 04/02/2022 11:47, Ulf Hansson wrote:
> + Adrian
> 
> On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote:
>>
>> 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
> 
>>
>>
>> From: Christian Löhle
>> Sent: Wednesday, January 5, 2022 5:43 PM
>> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
>> 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.
>>
>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>

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")
Cc: stable@vger.kernel.org

>> ---
>>  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 90e1bcd03b46..d7d880ce0f8a 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) {

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

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

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)

>> +                               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
>>

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

[Christian Löhle]

Thanks for the comments Adrian!

>> +               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.

>> +                       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 <adrian.hunter@intel.com>
Sent: Friday, February 4, 2022 12:26 PM
To: Ulf Hansson; Christian Löhle
Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Avri Altman
Subject: Re: [PATCH] mmc: block: fix read single on recovery logic
    
On 04/02/2022 11:47, Ulf Hansson wrote:
> + Adrian
> 
> On Thu, 3 Feb 2022 at 11:09, Christian Löhle <CLoehle@hyperstone.com> wrote:
>>
>> 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
> 
>>
>>
>> From: Christian Löhle
>> Sent: Wednesday, January 5, 2022 5:43 PM
>> To: ulf.hansson@linaro.org; Christian Löhle; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
>> 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.
>>
>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>

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")
Cc: stable@vger.kernel.org

>> ---
>>  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 90e1bcd03b46..d7d880ce0f8a 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) {

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

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

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)

>> +                               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

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

[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.

Fixes: 81196976ed946c ("mmc: block: Add blk-mq support")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
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 ||
-- 
2.34.1
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

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@vger.kernel.org
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>

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

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

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

[Ulf Hansson]

On Tue, 8 Feb 2022 at 07:37, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> 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@vger.kernel.org
> > Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Applied for fixes, thanks to both of you!

Kind regards
Uffe


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

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

[Greg Kroah-Hartman]

On Fri, Feb 18, 2022 at 11:05:21AM +0000, Christian Löhle wrote:
> You're right, I did not get an email about 5.4, maybe it was caught up somewhere.
> Greg, can you apply this to 5.4, too?

Oops, forgot that one, now queued up.

greg k-h

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

[Christian Löhle]

You're right, I did not get an email about 5.4, maybe it was caught up somewhere.
Greg, can you apply this to 5.4, too?






From: Adrian Hunter <adrian.hunter@intel.com>
Sent: Friday, February 18, 2022 12:02 PM
To: Christian Löhle
Subject: Re: [PATCH] mmc: block: fix read single on recovery logic
    
On 18/02/2022 12:36, Christian Löhle wrote:
> This is the backport for 4.19, it applied for more recent branches and is not applicable to 4.14.

Was 5.4 done?  There was a "failed to apply email for that also"


> 
> 
> 
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Friday, February 18, 2022 11:33 AM
> To: Christian Löhle
> Cc: stable@vger.kernel.org; Ulf Hansson; Adrian Hunter
> Subject: Re: [PATCH] mmc: block: fix read single on recovery logic
>     
> On Fri, Feb 18, 2022 at 09:50:54AM +0000, Christian Löhle wrote:
>> commit 54309fde1a352ad2674ebba004a79f7d20b9f037 upstream.
>>
>> 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")
>>
>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
>> ---
>>   drivers/mmc/core/block.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> What stable branch(s) is this backport for?
> 
> thanks,
> 
> greg k-h
>     =
> 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

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

[gregkh]

On Fri, Feb 18, 2022 at 10:36:47AM +0000, Christian Löhle wrote:
> This is the backport for 4.19, it applied for more recent branches and
> is not applicable to 4.14.

Thanks, now queued up.

greg k-h

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

[Christian Löhle]

This is the backport for 4.19, it applied for more recent branches and is not applicable to 4.14.



From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
Sent: Friday, February 18, 2022 11:33 AM
To: Christian Löhle
Cc: stable@vger.kernel.org; Ulf Hansson; Adrian Hunter
Subject: Re: [PATCH] mmc: block: fix read single on recovery logic
    
On Fri, Feb 18, 2022 at 09:50:54AM +0000, Christian Löhle wrote:
> commit 54309fde1a352ad2674ebba004a79f7d20b9f037 upstream.
> 
> 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")
> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/block.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

What stable branch(s) is this backport for?

thanks,

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

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

[gregkh]

On Fri, Feb 18, 2022 at 09:50:54AM +0000, Christian Löhle wrote:
> commit 54309fde1a352ad2674ebba004a79f7d20b9f037 upstream.
> 
> 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")
> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/block.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

What stable branch(s) is this backport for?

thanks,

greg k-h

[PATCH] mmc: block: fix read single on recovery logic

[Christian Löhle]

commit 54309fde1a352ad2674ebba004a79f7d20b9f037 upstream.

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

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 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 d1cc0fdbc51c..630f3bcba56d 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1678,31 +1678,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_blk_in_tran_state(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_blk_in_tran_state(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 ||
-- 
2.34.1

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