[PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[Justin Wang (王丁)]

From 05849da563c80c20597ab6275d5881a8ed426f96 Mon Sep 17 00:00:00 2001
From: justin.wang <justin.wang@spreadtrum.com>
Date: Mon, 18 May 2015 20:14:15 +0800
Subject: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

The current handler of MMC_BLK_CMD_ERR in mmc_blk_issue_rw_rq function
may cause new coming request permanent missing when the ongoing
request (previoulsy started) complete end.

This would cause the process related to the missing request stay at 'D'
state forever.

Signed-off-by: Ding Wang <justin.wang@spreadtrum.com>
---
 drivers/mmc/card/block.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 60f7141..f05cd1f 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1910,9 +1910,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			break;
 		case MMC_BLK_CMD_ERR:
 			ret = mmc_blk_cmd_err(md, card, brq, req, ret);
-			if (!mmc_blk_reset(md, card->host, type))
-				break;
-			goto cmd_abort;
+			if (mmc_blk_reset(md, card->host, type))
+				goto cmd_abort;
+			if (!ret)
+				goto start_new_req;
+			break;
 		case MMC_BLK_RETRY:
 			if (retry++ < 5)
 				break;
-- 
1.7.4.1

Re: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[Ulf Hansson]

On 19 May 2015 at 08:32, Justin Wang (王丁) <Justin.Wang@spreadtrum.com> wrote:
> From 05849da563c80c20597ab6275d5881a8ed426f96 Mon Sep 17 00:00:00 2001
> From: justin.wang <justin.wang@spreadtrum.com>
> Date: Mon, 18 May 2015 20:14:15 +0800
> Subject: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq
>
> The current handler of MMC_BLK_CMD_ERR in mmc_blk_issue_rw_rq function
> may cause new coming request permanent missing when the ongoing
> request (previoulsy started) complete end.

I think you need to elaborate a bit more here. In exactly what
scenario does this problem occur?

>
> This would cause the process related to the missing request stay at 'D'
> state forever.

'D' state?

I suppose it waits for a response for its IO request. Doesn't also the
block layer complain or timeout somehow? Would be interesting to know
a bit more about what really happens when things goes wrong.

>

Seems like we should have a fixes tag here as well, could you possibly
try to find what commit that introduced this bug?

> Signed-off-by: Ding Wang <justin.wang@spreadtrum.com>
> ---
>  drivers/mmc/card/block.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 60f7141..f05cd1f 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1910,9 +1910,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                         break;
>                 case MMC_BLK_CMD_ERR:
>                         ret = mmc_blk_cmd_err(md, card, brq, req, ret);
> -                       if (!mmc_blk_reset(md, card->host, type))
> -                               break;
> -                       goto cmd_abort;
> +                       if (mmc_blk_reset(md, card->host, type))
> +                               goto cmd_abort;
> +                       if (!ret)
> +                               goto start_new_req;
> +                       break;
>                 case MMC_BLK_RETRY:
>                         if (retry++ < 5)
>                                 break;
> --
> 1.7.4.1
>

Kind regards
Uffe

RE: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[Justin Wang (王丁)]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Thursday, May 28, 2015 4:29 PM
> To: Justin Wang (王丁)
> Cc: kuninori.morimoto.gx@renesas.com; jh80.chung@samsung.com;
> akpm@linux-foundation.org; JBottomley@Odin.com; ben@decadent.org.uk;
> chuanxiao.dong@intel.com; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq
> Importance: High
> 
> On 19 May 2015 at 08:32, Justin Wang (王丁) <Justin.Wang@spreadtrum.com>
> wrote:
> > From 05849da563c80c20597ab6275d5881a8ed426f96 Mon Sep 17 00:00:00
> 2001
> > From: justin.wang <justin.wang@spreadtrum.com>
> > Date: Mon, 18 May 2015 20:14:15 +0800
> > Subject: [PATCH] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq
> >
> > The current handler of MMC_BLK_CMD_ERR in mmc_blk_issue_rw_rq
> function
> > may cause new coming request permanent missing when the ongoing
> > request (previoulsy started) complete end.
> 
> I think you need to elaborate a bit more here. In exactly what
> scenario does this problem occur?
> 

Thanks, I will give a detailed description next patch.

> >
> > This would cause the process related to the missing request stay at 'D'
> > state forever.
> 
> 'D' state?
> 
> I suppose it waits for a response for its IO request. Doesn't also the
> block layer complain or timeout somehow? Would be interesting to know
> a bit more about what really happens when things goes wrong.
> 

Yes, process will wait the missed request complete forever, no timeout in block layer.
The related process will be blocked.
> >
> 
> Seems like we should have a fixes tag here as well, could you possibly
> try to find what commit that introduced this bug?
> 
> > Signed-off-by: Ding Wang <justin.wang@spreadtrum.com>
> > ---
> >  drivers/mmc/card/block.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 60f7141..f05cd1f 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -1910,9 +1910,11 @@ static int mmc_blk_issue_rw_rq(struct
> mmc_queue *mq, struct request *rqc)
> >                         break;
> >                 case MMC_BLK_CMD_ERR:
> >                         ret = mmc_blk_cmd_err(md, card, brq, req,
> ret);
> > -                       if (!mmc_blk_reset(md, card->host, type))
> > -                               break;
> > -                       goto cmd_abort;
> > +                       if (mmc_blk_reset(md, card->host, type))
> > +                               goto cmd_abort;
> > +                       if (!ret)
> > +                               goto start_new_req;
> > +                       break;
> >                 case MMC_BLK_RETRY:
> >                         if (retry++ < 5)
> >                                 break;
> > --
> > 1.7.4.1
> >
> 
> Kind regards
> Uffe

[PATCH v2] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[Justin Wang (王丁)]

From 0e01e7546745f39843d54810f74c198e01cb8226 Mon Sep 17 00:00:00 2001
From: justin.wang <justin.wang@spreadtrum.com>
Date: Mon, 18 May 2015 20:14:15 +0800
Subject: [PATCH v2] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

The current handler of MMC_BLK_CMD_ERR in mmc_blk_issue_rw_rq function
may cause new coming request permanent missing when the ongoing
request (previoulsy started) complete end.

The problem scenario is as follows:
(1) Request A is ongoing;
(2) Request B arrived, and finally mmc_blk_issue_rw_rq() is called;
(3) Request A encounters the MMC_BLK_CMD_ERR error;
(4) In the error handling of MMC_BLK_CMD_ERR, suppose mmc_blk_cmd_err()
    end request A completed and return zero. Continue the error handling,
    suppose mmc_blk_reset() reset device success;
(5) Continue the execution, while loop completed because variable ret
    is zero now;
(6) Finally, mmc_blk_issue_rw_rq() return without processing request B.

The process related to the missing request may wait that IO request
complete forever, possibly crashing the application or hanging the system.

Fix this issue by starting new request when reset success.

Signed-off-by: Ding Wang <justin.wang@spreadtrum.com>
---
 drivers/mmc/card/block.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 60f7141..f05cd1f 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1910,9 +1910,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			break;
 		case MMC_BLK_CMD_ERR:
 			ret = mmc_blk_cmd_err(md, card, brq, req, ret);
-			if (!mmc_blk_reset(md, card->host, type))
-				break;
-			goto cmd_abort;
+			if (mmc_blk_reset(md, card->host, type))
+				goto cmd_abort;
+			if (!ret)
+				goto start_new_req;
+			break;
 		case MMC_BLK_RETRY:
 			if (retry++ < 5)
 				break;
-- 
1.7.4.1

Re: [PATCH v2] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

[Ulf Hansson]

On 12 June 2015 at 14:38, Justin Wang (王丁) <Justin.Wang@spreadtrum.com> wrote:
> From 0e01e7546745f39843d54810f74c198e01cb8226 Mon Sep 17 00:00:00 2001
> From: justin.wang <justin.wang@spreadtrum.com>
> Date: Mon, 18 May 2015 20:14:15 +0800
> Subject: [PATCH v2] mmc: card: Fixup request missing in mmc_blk_issue_rw_rq
>
> The current handler of MMC_BLK_CMD_ERR in mmc_blk_issue_rw_rq function
> may cause new coming request permanent missing when the ongoing
> request (previoulsy started) complete end.
>
> The problem scenario is as follows:
> (1) Request A is ongoing;
> (2) Request B arrived, and finally mmc_blk_issue_rw_rq() is called;
> (3) Request A encounters the MMC_BLK_CMD_ERR error;
> (4) In the error handling of MMC_BLK_CMD_ERR, suppose mmc_blk_cmd_err()
>     end request A completed and return zero. Continue the error handling,
>     suppose mmc_blk_reset() reset device success;
> (5) Continue the execution, while loop completed because variable ret
>     is zero now;
> (6) Finally, mmc_blk_issue_rw_rq() return without processing request B.
>
> The process related to the missing request may wait that IO request
> complete forever, possibly crashing the application or hanging the system.
>
> Fix this issue by starting new request when reset success.
>
> Signed-off-by: Ding Wang <justin.wang@spreadtrum.com>

Thanks, applied for next.

I have added a the following fixes tag:
Fixes 67716327eec7 ("mmc: block: add eMMC hardware reset support")
I am not sure, whether the bug were introduced even earlier than that,
but I think $subject patch should be trivial to apply on top that one.

Kind regards
Uffe

> ---
>  drivers/mmc/card/block.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 60f7141..f05cd1f 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1910,9 +1910,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                         break;
>                 case MMC_BLK_CMD_ERR:
>                         ret = mmc_blk_cmd_err(md, card, brq, req, ret);
> -                       if (!mmc_blk_reset(md, card->host, type))
> -                               break;
> -                       goto cmd_abort;
> +                       if (mmc_blk_reset(md, card->host, type))
> +                               goto cmd_abort;
> +                       if (!ret)
> +                               goto start_new_req;
> +                       break;
>                 case MMC_BLK_RETRY:
>                         if (retry++ < 5)
>                                 break;
> --
> 1.7.4.1
>