[RFC PATCH v2 0/2] mmc: core: process ECC errors raised in stop cmds

[RFC PATCH v2 0/2] mmc: core: process ECC errors raised in stop cmds

[Wolfram Sang]

Here is the second RFC for handling ECC errors flagged in the stop command
after a multiblock transfer. It is still RFC because I could only test it by
inducing ECC errors in software (see patch for TMIO below). Shimoda-san, can
you try this series with the SD tester again? That would be very kind.

Other than that, I hope the patch descriptions and comments explain the single
steps. Looking forward for thoughts.

Kind regards,

   Wolfram

Changes since RFC v1:

* rebased to mmc/next as of today
* reworded commit message for patch 1
* added tested-tag from Shimoda-san for patch 1
* added patch 2

Wolfram Sang (2):
  mmc: core: check also R1 response for stop commands
  mmc: core: for data errors, take response of stop cmd into account

 drivers/mmc/core/block.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Patch to simulate ECC errors:

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index a2d92f10501bdd..9773c7e5e4d154 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -553,6 +553,8 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 	}
 
 	if (stop) {
+		static unsigned int induce_cnt = 0;
+
 		if (stop->opcode != MMC_STOP_TRANSMISSION || stop->arg)
 			dev_err(&host->pdev->dev, "unsupported stop: CMD%u,0x%x. We did CMD12,0\n",
 				stop->opcode, stop->arg);
@@ -560,6 +562,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
 		/* fill in response from auto CMD12 */
 		stop->resp[0] = sd_ctrl_read16_and_16_as_32(host, CTL_RESPONSE);
 
+		if (induce_cnt++ % 100 == 0)
+			stop->resp[0] |= R1_CARD_ECC_FAILED;
+
 		sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
 	}

-- 
2.11.0

[RFC PATCH v2 1/2] mmc: core: check also R1 response for stop commands

[Wolfram Sang]

To detect errors like ECC errors, we must parse the R1 response bits. Introduce
a helper function to also set the error value of a command when R1 error bits
are set. Add ECC error to list of flags checked. Use the new helper for the
stop command to call mmc_blk_recovery when detecting ECC errors which are only
flagged on the next command after multiblock.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---

If adding R1_CARD_ECC_FAILED to CMD_ERRORS is considered too risky for
side-effects, we could also introduce a new define NEXT_CMD_ERRORS maybe?
Yet, I believe it is OK to be done like this.


 drivers/mmc/core/block.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index ff3da960c47361..bacaa255c4b16a 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1287,9 +1287,18 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
 	 R1_ADDRESS_ERROR |	/* Misaligned address */		\
 	 R1_BLOCK_LEN_ERROR |	/* Transferred block length incorrect */\
 	 R1_WP_VIOLATION |	/* Tried to write to protected block */	\
+	 R1_CARD_ECC_FAILED |	/* Card ECC failed */			\
 	 R1_CC_ERROR |		/* Card controller error */		\
 	 R1_ERROR)		/* General/unknown error */
 
+static bool mmc_blk_has_cmd_err(struct mmc_command *cmd)
+{
+	if (!cmd->error && cmd->resp[0] & CMD_ERRORS)
+		cmd->error = -EIO;
+
+	return cmd->error;
+}
+
 static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 					     struct mmc_async_req *areq)
 {
@@ -1311,7 +1320,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 	 * stop.error indicates a problem with the stop command.  Data
 	 * may have been transferred, or may still be transferring.
 	 */
-	if (brq->sbc.error || brq->cmd.error || brq->stop.error ||
+	if (brq->sbc.error || brq->cmd.error || mmc_blk_has_cmd_err(&brq->stop) ||
 	    brq->data.error) {
 		switch (mmc_blk_cmd_recovery(card, req, brq, &ecc_err, &gen_err)) {
 		case ERR_RETRY:
-- 
2.11.0

[RFC PATCH v2 2/2] mmc: core: for data errors, take response of stop cmd into account

[Wolfram Sang]

Some errors are flagged only with the next command after a multiblock
transfer, e.g. ECC error. So, when checking for data transfer errors,
we check the result from the stop command as well.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index bacaa255c4b16a..ea3dc8a22ad2cb 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1374,7 +1374,8 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 		return MMC_BLK_RETRY;
 	}
 
-	if (brq->data.error) {
+	/* Some errors (ECC) are flagged on the next commmand, so check stop, too */
+	if (brq->data.error || brq->stop.error) {
 		if (need_retune && !brq->retune_retry_done) {
 			pr_debug("%s: retrying because a re-tune was needed\n",
 				 req->rq_disk->disk_name);
@@ -1382,7 +1383,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 			return MMC_BLK_RETRY;
 		}
 		pr_err("%s: error %d transferring data, sector %u, nr %u, cmd response %#x, card status %#x\n",
-		       req->rq_disk->disk_name, brq->data.error,
+		       req->rq_disk->disk_name, brq->data.error ?: brq->stop.error,
 		       (unsigned)blk_rq_pos(req),
 		       (unsigned)blk_rq_sectors(req),
 		       brq->cmd.resp[0], brq->stop.resp[0]);
-- 
2.11.0

Re: [RFC PATCH v2 0/2] mmc: core: process ECC errors raised in stop cmds

[Ulf Hansson]

On 8 April 2017 at 22:20, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> Here is the second RFC for handling ECC errors flagged in the stop command
> after a multiblock transfer. It is still RFC because I could only test it by
> inducing ECC errors in software (see patch for TMIO below). Shimoda-san, can
> you try this series with the SD tester again? That would be very kind.
>
> Other than that, I hope the patch descriptions and comments explain the single
> steps. Looking forward for thoughts.

Apologize for the delay!

I looked into these and the changes seems reasonable. I have applied
them for next to allow them to get some test coverage.

Thanks and kind regards
Uffe

>
> Kind regards,
>
>    Wolfram
>
> Changes since RFC v1:
>
> * rebased to mmc/next as of today
> * reworded commit message for patch 1
> * added tested-tag from Shimoda-san for patch 1
> * added patch 2
>
> Wolfram Sang (2):
>   mmc: core: check also R1 response for stop commands
>   mmc: core: for data errors, take response of stop cmd into account
>
>  drivers/mmc/core/block.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> Patch to simulate ECC errors:
>
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index a2d92f10501bdd..9773c7e5e4d154 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -553,6 +553,8 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
>         }
>
>         if (stop) {
> +               static unsigned int induce_cnt = 0;
> +
>                 if (stop->opcode != MMC_STOP_TRANSMISSION || stop->arg)
>                         dev_err(&host->pdev->dev, "unsupported stop: CMD%u,0x%x. We did CMD12,0\n",
>                                 stop->opcode, stop->arg);
> @@ -560,6 +562,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
>                 /* fill in response from auto CMD12 */
>                 stop->resp[0] = sd_ctrl_read16_and_16_as_32(host, CTL_RESPONSE);
>
> +               if (induce_cnt++ % 100 == 0)
> +                       stop->resp[0] |= R1_CARD_ECC_FAILED;
> +
>                 sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0);
>         }
>
> --
> 2.11.0
>

Re: [RFC PATCH v2 0/2] mmc: core: process ECC errors raised in stop cmds

[Wolfram Sang]

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

On Mon, Jun 12, 2017 at 10:30:27AM +0200, Ulf Hansson wrote:
> On 8 April 2017 at 22:20, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> > Here is the second RFC for handling ECC errors flagged in the stop command
> > after a multiblock transfer. It is still RFC because I could only test it by
> > inducing ECC errors in software (see patch for TMIO below). Shimoda-san, can
> > you try this series with the SD tester again? That would be very kind.
> >
> > Other than that, I hope the patch descriptions and comments explain the single
> > steps. Looking forward for thoughts.
> 
> Apologize for the delay!
> 
> I looked into these and the changes seems reasonable. I have applied
> them for next to allow them to get some test coverage.

Awesome, thanks Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]