mmc: core: check also R1 response for stop commands

Commit Message

Yoshihiro Shimoda March 15, 2017, 2:54 a.m. UTC

Hi Wolfram-san, 

> From: Wolfram Sang, Sent: Tuesday, March 14, 2017 7:49 PM
> 
> > > So, since I got no complaints about the RFC, I'll declare it a PATCH now :)
> > > Shimoda-san, is it possible for you to test it with the SD tester you once had
> > > access to? I could only test it by setting the ECC bit in the driver manually.
> >
> > Sure. I am able to bring the SD tester from other department tomorrow.
> > So, I will check this patch tomorrow.
> 
> Good news, thank you very much!

I tested this patch with the SD tester. After I applied this patch, when the R1_CARD_ECC_FAILED
is set, the following message appeared:

mmcblk1: error -5 sending stop command, original cmd response 0x900, card status 0x900

However, I have a question about "ecc_err" in both mmc_blk_err_check() and mmc_blk_cmd_recovery().
Anyway, I tested this patch. And the mmc core said some error happened.
I think this is suitable behavior. So,

Tested-by: Yoshihiro Shimoda <[e-mail]>

< Question >
In mmc_blk_cmd_recovery(), if "brq->stop.resp[0] & R1_CARD_ECC_FAILED" is true, ecc_err is set to true.
However, in mmc_blk_err_check(), ecc_err is only referred when brq->data.error is true.
So, I guess we need a patch like below as another patch. What do you think?


After that, the mmc core also output the following message:

mmcblk1: error 0 transferring data, sector 0, nr 128, cmd response 0x900, card status 0x200b00
mmcblk1: retrying using single block read

Best regards,
Yoshihiro Shimoda

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Wolfram Sang March 16, 2017, 11:33 a.m. UTC | #1

Shimoda-san

> Anyway, I tested this patch. And the mmc core said some error happened.
> I think this is suitable behavior. So,
> 
> Tested-by: Yoshihiro Shimoda <[e-mail]>

Thank you very much for the quick response and testing!

> < Question >
> In mmc_blk_cmd_recovery(), if "brq->stop.resp[0] & R1_CARD_ECC_FAILED" is true, ecc_err is set to true.
> However, in mmc_blk_err_check(), ecc_err is only referred when brq->data.error is true.
> So, I guess we need a patch like below as another patch. What do you think?
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 0e838b0..99c937b 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1374,7 +1374,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_ca
>                 return MMC_BLK_RETRY;
>         }
> 
> -       if (brq->data.error) {
> +       if (brq->data.error || ecc_err) {
>                 if (need_retune && !brq->retune_retry_done) {
>                         pr_debug("%s: retrying because a re-tune was needed\n",
>                                  req->rq_disk->disk_name);
> 
> After that, the mmc core also output the following message:
> 
> mmcblk1: error 0 transferring data, sector 0, nr 128, cmd response 0x900, card status 0x200b00
> mmcblk1: retrying using single block read

I think you are right that we need to change something somewhere in this
code block. I was about to suggest adding "|| brq->stop.error" instead
of "|| ecc_err". However, I am not so sure about this as well, since the
code is a little confusing to me. Especially this:

1381                 if (rq_data_dir(req) == READ) { 
1382                         if (ecc_err) 
1383                                 return MMC_BLK_ECC_ERR; 
1384                         return MMC_BLK_DATA_ERR;
1385                 } else {
1386                         return MMC_BLK_CMD_ERR;
1387                 }

If the data_directions is WRITE, then we return a CMD_ERR instead of a
DATA_ERR? And all that in a code path which is only run when
brq->data.error, i.e. brq->cmd.err is not even touched?

I'd think the if-block in question wants to handle errors which happened
while transferring data. As such, I'd still think adding
"|| brq->stop.error" might be a good idea. And revisiting if this block
should really return CMD_ERR.

But I may be wrong and misunderstanding, so I am happy for further
opinions. Maybe the code needs just some more comments.

Thanks and regards,

   Wolfram

Patch

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 0e838b0..99c937b 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1374,7 +1374,7 @@  static enum mmc_blk_status mmc_blk_err_check(struct mmc_ca
                return MMC_BLK_RETRY;
        }

-       if (brq->data.error) {
+       if (brq->data.error || ecc_err) {
                if (need_retune && !brq->retune_retry_done) {
                        pr_debug("%s: retrying because a re-tune was needed\n",
                                 req->rq_disk->disk_name);