diff mbox

mmc: card: Fixup request missing in mmc_blk_issue_rw_rq

Message ID dbf65349aac343abbd215129dd3ceefb@SHMBX03.spreadtrum.com (mailing list archive)
State New, archived
Headers show

Commit Message

Justin Wang (??) May 19, 2015, 6:32 a.m. UTC 
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(-)

-- 
1.7.4.1

Comments

Ulf Hansson May 28, 2015, 8:29 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Justin Wang (??) June 2, 2015, 9:14 a.m. UTC | #2 
> -----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
Justin Wang (??) June 12, 2015, 12:38 p.m. UTC | #3 
RnJvbSAwZTAxZTc1NDY3NDVmMzk4NDNkNTQ4MTBmNzRjMTk4ZTAxY2I4MjI2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQ0KRnJvbToganVzdGluLndhbmcgPGp1c3Rpbi53YW5nQHNwcmVhZHRydW0u
Y29tPg0KRGF0ZTogTW9uLCAxOCBNYXkgMjAxNSAyMDoxNDoxNSArMDgwMA0KU3ViamVjdDogW1BB
VENIIHYyXSBtbWM6IGNhcmQ6IEZpeHVwIHJlcXVlc3QgbWlzc2luZyBpbiBtbWNfYmxrX2lzc3Vl
X3J3X3JxDQoNClRoZSBjdXJyZW50IGhhbmRsZXIgb2YgTU1DX0JMS19DTURfRVJSIGluIG1tY19i
bGtfaXNzdWVfcndfcnEgZnVuY3Rpb24NCm1heSBjYXVzZSBuZXcgY29taW5nIHJlcXVlc3QgcGVy
bWFuZW50IG1pc3Npbmcgd2hlbiB0aGUgb25nb2luZw0KcmVxdWVzdCAocHJldmlvdWxzeSBzdGFy
dGVkKSBjb21wbGV0ZSBlbmQuDQoNClRoZSBwcm9ibGVtIHNjZW5hcmlvIGlzIGFzIGZvbGxvd3M6
DQooMSkgUmVxdWVzdCBBIGlzIG9uZ29pbmc7DQooMikgUmVxdWVzdCBCIGFycml2ZWQsIGFuZCBm
aW5hbGx5IG1tY19ibGtfaXNzdWVfcndfcnEoKSBpcyBjYWxsZWQ7DQooMykgUmVxdWVzdCBBIGVu
Y291bnRlcnMgdGhlIE1NQ19CTEtfQ01EX0VSUiBlcnJvcjsNCig0KSBJbiB0aGUgZXJyb3IgaGFu
ZGxpbmcgb2YgTU1DX0JMS19DTURfRVJSLCBzdXBwb3NlIG1tY19ibGtfY21kX2VycigpDQogICAg
ZW5kIHJlcXVlc3QgQSBjb21wbGV0ZWQgYW5kIHJldHVybiB6ZXJvLiBDb250aW51ZSB0aGUgZXJy
b3IgaGFuZGxpbmcsDQogICAgc3VwcG9zZSBtbWNfYmxrX3Jlc2V0KCkgcmVzZXQgZGV2aWNlIHN1
Y2Nlc3M7DQooNSkgQ29udGludWUgdGhlIGV4ZWN1dGlvbiwgd2hpbGUgbG9vcCBjb21wbGV0ZWQg
YmVjYXVzZSB2YXJpYWJsZSByZXQNCiAgICBpcyB6ZXJvIG5vdzsNCig2KSBGaW5hbGx5LCBtbWNf
YmxrX2lzc3VlX3J3X3JxKCkgcmV0dXJuIHdpdGhvdXQgcHJvY2Vzc2luZyByZXF1ZXN0IEIuDQoN
ClRoZSBwcm9jZXNzIHJlbGF0ZWQgdG8gdGhlIG1pc3NpbmcgcmVxdWVzdCBtYXkgd2FpdCB0aGF0
IElPIHJlcXVlc3QNCmNvbXBsZXRlIGZvcmV2ZXIsIHBvc3NpYmx5IGNyYXNoaW5nIHRoZSBhcHBs
aWNhdGlvbiBvciBoYW5naW5nIHRoZSBzeXN0ZW0uDQoNCkZpeCB0aGlzIGlzc3VlIGJ5IHN0YXJ0
aW5nIG5ldyByZXF1ZXN0IHdoZW4gcmVzZXQgc3VjY2Vzcy4NCg0KU2lnbmVkLW9mZi1ieTogRGlu
ZyBXYW5nIDxqdXN0aW4ud2FuZ0BzcHJlYWR0cnVtLmNvbT4NCi0tLQ0KIGRyaXZlcnMvbW1jL2Nh
cmQvYmxvY2suYyB8ICAgIDggKysrKystLS0NCiAxIGZpbGVzIGNoYW5nZWQsIDUgaW5zZXJ0aW9u
cygrKSwgMyBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL2RyaXZlcnMvbW1jL2NhcmQvYmxv
Y2suYyBiL2RyaXZlcnMvbW1jL2NhcmQvYmxvY2suYw0KaW5kZXggNjBmNzE0MS4uZjA1Y2QxZiAx
MDA2NDQNCi0tLSBhL2RyaXZlcnMvbW1jL2NhcmQvYmxvY2suYw0KKysrIGIvZHJpdmVycy9tbWMv
Y2FyZC9ibG9jay5jDQpAQCAtMTkxMCw5ICsxOTEwLDExIEBAIHN0YXRpYyBpbnQgbW1jX2Jsa19p
c3N1ZV9yd19ycShzdHJ1Y3QgbW1jX3F1ZXVlICptcSwgc3RydWN0IHJlcXVlc3QgKnJxYykNCiAJ
CQlicmVhazsNCiAJCWNhc2UgTU1DX0JMS19DTURfRVJSOg0KIAkJCXJldCA9IG1tY19ibGtfY21k
X2VycihtZCwgY2FyZCwgYnJxLCByZXEsIHJldCk7DQotCQkJaWYgKCFtbWNfYmxrX3Jlc2V0KG1k
LCBjYXJkLT5ob3N0LCB0eXBlKSkNCi0JCQkJYnJlYWs7DQotCQkJZ290byBjbWRfYWJvcnQ7DQor
CQkJaWYgKG1tY19ibGtfcmVzZXQobWQsIGNhcmQtPmhvc3QsIHR5cGUpKQ0KKwkJCQlnb3RvIGNt
ZF9hYm9ydDsNCisJCQlpZiAoIXJldCkNCisJCQkJZ290byBzdGFydF9uZXdfcmVxOw0KKwkJCWJy
ZWFrOw0KIAkJY2FzZSBNTUNfQkxLX1JFVFJZOg0KIAkJCWlmIChyZXRyeSsrIDwgNSkNCiAJCQkJ
YnJlYWs7DQotLSANCjEuNy40LjENCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson June 15, 2015, 9:55 a.m. UTC | #4 
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;