4.13 on thinkpad x220: oops when writing to SD card

Commit Message

Adrian Hunter Sept. 7, 2017, 7:53 a.m. UTC

On 07/09/17 10:18, Ulf Hansson wrote:
> + Linus
> 
> On 6 September 2017 at 08:03, Adrian Hunter <[e-mail]> wrote:
>> On 06/09/17 05:44, Shawn Lin wrote:
>>> + Seraphime
>>>
>>> On 2017/9/6 3:47, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>> I tried to write to the MMC card; process hung and I got this in the
>>>> dmesg.
>>>
>>>
>>> A similar report for 4.13 cycle was here:
>>>
>>> https://lkml.org/lkml/2017/8/10/824
>>>
>>> Seems 4.13-rc4 was already broken for that but unfortuantely I didn't
>>> reproduce that. So maybe Seraphime can do git-bisect as he said "I get
>>> it everytime" for which I assume it could be easy for him to find out
>>> the problematic commit?
>>>
>>
>> One obvious weakness in the new mmc_init_request() is the possibility
>> that it might be called before card->bouncesz is set up.  That could
>> result in bouncing being done but mq_rq->bounce_sg is null.
>> This might help:
>>
>>
>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
>> index affa7370ba82..ad3e53e63abb 100644
>> --- a/drivers/mmc/core/queue.c
>> +++ b/drivers/mmc/core/queue.c
>> @@ -242,6 +242,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>>         if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
>>                 limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
>>
>> +       card->bouncesz = mmc_queue_calc_bouncesz(host);
>> +
>>         mq->card = card;
>>         mq->queue = blk_alloc_queue(GFP_KERNEL);
>>         if (!mq->queue)
>> @@ -265,7 +267,6 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>>         if (mmc_can_erase(card))
>>                 mmc_queue_setup_discard(mq->queue, card);
>>
>> -       card->bouncesz = mmc_queue_calc_bouncesz(host);
>>         if (card->bouncesz) {
>>                 blk_queue_max_hw_sectors(mq->queue, card->bouncesz / 512);
>>                 blk_queue_max_segments(mq->queue, card->bouncesz / 512);
>>
> 
> Even if this fixes the problem it seems like we are papering over the
> real issue, which earlier fixes also did during the release cycle for
> v4.13.

blk_init_allocated_queue() allocates 1 request for flush and 4 requests
for a memory pool.  The memory pool requests only get used under memory
pressure.  That is why the error doesn't come up straight away.

> 
> Anyway I am happy to apply this as fix for 4.14, if Seraphime/Pavel
> can report it solved the problem. Could you send a proper patch with
> some changlog please?
> 
> I would also appreciate if can add you a small comment in the code,
> why moving this line is needed.

From: Adrian Hunter <[e-mail]>
Date: Thu, 7 Sep 2017 10:40:35 +0300
Subject: [PATCH] mmc: block: Fix incorrectly initialized requests

mmc_init_request() depends on card->bouncesz so it must be calculated
before blk_init_allocated_queue() starts allocating requests.

Reported-by: Seraphime Kirkovski <[e-mail]>
Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
Signed-off-by: Adrian Hunter <[e-mail]>
---
 drivers/mmc/core/queue.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Seraphime Kirkovski Sept. 7, 2017, 10:47 a.m. UTC | #1

> blk_init_allocated_queue() allocates 1 request for flush and 
> 4 requests
> for a memory pool.  The memory pool requests only get used under memory
> pressure.  That is why the error doesn't come up straight away.
This seems correct, I can "trivially" trigger the bug with
a while-malloc loop + firefox.

> Reported-by: Seraphime Kirkovski <[e-mail]>
> Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
> Signed-off-by: Adrian Hunter <[e-mail]>

As I said, this fixes it for me, you can add

Tested-By: Seraphime Kirkovski <[e-mail]>

Although I'm not sure this covers the same bug Pavel encountered.
My kernel doesn't panic, it makes KASAN scream + #GP eventually followed 
by a lockup.

Anyway, thanks for the fix,
Seraphime
--
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

Ulf Hansson Sept. 7, 2017, 12:06 p.m. UTC | #2

[...]

>
> From: Adrian Hunter <[e-mail]>
> Date: Thu, 7 Sep 2017 10:40:35 +0300
> Subject: [PATCH] mmc: block: Fix incorrectly initialized requests
>
> mmc_init_request() depends on card->bouncesz so it must be calculated
> before blk_init_allocated_queue() starts allocating requests.
>
> Reported-by: Seraphime Kirkovski <[e-mail]>
> Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
> Signed-off-by: Adrian Hunter <[e-mail]>

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>  drivers/mmc/core/queue.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index affa7370ba82..74c663b1c0a7 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -242,6 +242,12 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>         if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
>                 limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
>
> +       /*
> +        * mmc_init_request() depends on card->bouncesz so it must be calculated
> +        * before blk_init_allocated_queue() starts allocating requests.
> +        */
> +       card->bouncesz = mmc_queue_calc_bouncesz(host);
> +
>         mq->card = card;
>         mq->queue = blk_alloc_queue(GFP_KERNEL);
>         if (!mq->queue)
> @@ -265,7 +271,6 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>         if (mmc_can_erase(card))
>                 mmc_queue_setup_discard(mq->queue, card);
>
> -       card->bouncesz = mmc_queue_calc_bouncesz(host);
>         if (card->bouncesz) {
>                 blk_queue_max_hw_sectors(mq->queue, card->bouncesz / 512);
>                 blk_queue_max_segments(mq->queue, card->bouncesz / 512);
> --
> 1.9.1
>
--
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

Pavel Machek Sept. 7, 2017, 12:55 p.m. UTC | #3

On Thu 2017-09-07 14:06:52, Ulf Hansson wrote:
> [...]
> 
> >
> > From: Adrian Hunter <[e-mail]>
> > Date: Thu, 7 Sep 2017 10:40:35 +0300
> > Subject: [PATCH] mmc: block: Fix incorrectly initialized requests
> >
> > mmc_init_request() depends on card->bouncesz so it must be calculated
> > before blk_init_allocated_queue() starts allocating requests.
> >
> > Reported-by: Seraphime Kirkovski <[e-mail]>
> > Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
> > Signed-off-by: Adrian Hunter <[e-mail]>
> 
> Thanks, applied for fixes!

Thanks. I believe this one should get cc: stable markups, so
eventually 4.13 does get fixed, too....
								Pavel

Ulf Hansson Sept. 7, 2017, 3:03 p.m. UTC | #4

On 7 September 2017 at 14:55, Pavel Machek <[e-mail]> wrote:
> On Thu 2017-09-07 14:06:52, Ulf Hansson wrote:
>> [...]
>>
>> >
>> > From: Adrian Hunter <[e-mail]>
>> > Date: Thu, 7 Sep 2017 10:40:35 +0300
>> > Subject: [PATCH] mmc: block: Fix incorrectly initialized requests
>> >
>> > mmc_init_request() depends on card->bouncesz so it must be calculated
>> > before blk_init_allocated_queue() starts allocating requests.
>> >
>> > Reported-by: Seraphime Kirkovski <[e-mail]>
>> > Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
>> > Signed-off-by: Adrian Hunter <[e-mail]>
>>
>> Thanks, applied for fixes!
>
> Thanks. I believe this one should get cc: stable markups, so
> eventually 4.13 does get fixed, too....
>                                                                 Pavel

Yeah, correct and added!

Kind regards
Uffe
--
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

Linus Walleij Sept. 7, 2017, 7:58 p.m. UTC | #5

On Thu, Sep 7, 2017 at 9:53 AM, Adrian Hunter <[e-mail]> wrote:

> From: Adrian Hunter <[e-mail]>
> Date: Thu, 7 Sep 2017 10:40:35 +0300
> Subject: [PATCH] mmc: block: Fix incorrectly initialized requests
>
> mmc_init_request() depends on card->bouncesz so it must be calculated
> before blk_init_allocated_queue() starts allocating requests.
>
> Reported-by: Seraphime Kirkovski <[e-mail]>
> Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
> Signed-off-by: Adrian Hunter <[e-mail]>

Reviewed-by: Linus Walleij <[e-mail]>

Really neat and quick fix, thanks a lot Adrian.

My fault for not finding more systems actually *using* these
bounce buffers. :( :(

Yours,
Linus Walleij
--
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

Pavel Machek Sept. 8, 2017, 8:51 a.m. UTC | #6

On Thu 2017-09-07 14:06:52, Ulf Hansson wrote:
> [...]
> 
> >
> > From: Adrian Hunter <[e-mail]>
> > Date: Thu, 7 Sep 2017 10:40:35 +0300
> > Subject: [PATCH] mmc: block: Fix incorrectly initialized requests
> >
> > mmc_init_request() depends on card->bouncesz so it must be calculated
> > before blk_init_allocated_queue() starts allocating requests.
> >
> > Reported-by: Seraphime Kirkovski <[e-mail]>
> > Fixes: 304419d8a7e92 ("mmc: core: Allocate per-request data using the block layer core")
> > Signed-off-by: Adrian Hunter <[e-mail]>
> 
> Thanks, applied for fixes!

Tested-by: Pavel Machek <[e-mail]>

Thanks,
									Pavel

Patch

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index affa7370ba82..74c663b1c0a7 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -242,6 +242,12 @@  int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
 	if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
 		limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
 
+	/*
+	 * mmc_init_request() depends on card->bouncesz so it must be calculated
+	 * before blk_init_allocated_queue() starts allocating requests.
+	 */
+	card->bouncesz = mmc_queue_calc_bouncesz(host);
+
 	mq->card = card;
 	mq->queue = blk_alloc_queue(GFP_KERNEL);
 	if (!mq->queue)
@@ -265,7 +271,6 @@  int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
 	if (mmc_can_erase(card))
 		mmc_queue_setup_discard(mq->queue, card);
 
-	card->bouncesz = mmc_queue_calc_bouncesz(host);
 	if (card->bouncesz) {
 		blk_queue_max_hw_sectors(mq->queue, card->bouncesz / 512);
 		blk_queue_max_segments(mq->queue, card->bouncesz / 512);