Daniel Drake | 1 Dec 13:49 2009

Re: crash in mmc subsystem during suspend

On Sat, 2009-11-21 at 12:31 +0000, Matt Fleming wrote:
> Fancy giving this patch a try? I think it's just a case of removing too
> many funcs in the error path.

Thanks!
Yes, I agree that looks like the culprit.
I applied something very similar to your patch and the crash went away.

The one additional change I made is in sdio_bus.c :

 void sdio_remove_func(struct sdio_func *func)
 {
-       if (sdio_func_present(func))
-               device_del(&func->dev);
+       if (!sdio_func_present(func))
+               return;

+       device_del(&func->dev);
        put_device(&func->dev);
 }

I think this is necessary because the error path will go mmc_sdio_remove
--> sdio_remove_func
Hence sdio_remove_func() will be called when sdio_add_func() was never
called beforehand, so there is no func->dev reference to drop.

Do you agree? I'm not certain about this one.

Thanks!
Daniel
(Continue reading)

Matt Fleming | 1 Dec 14:40 2009

Re: crash in mmc subsystem during suspend

On Tue, Dec 01, 2009 at 12:49:26PM +0000, Daniel Drake wrote:
> On Sat, 2009-11-21 at 12:31 +0000, Matt Fleming wrote:
> > Fancy giving this patch a try? I think it's just a case of removing too
> > many funcs in the error path.
> 
> Thanks!
> Yes, I agree that looks like the culprit.
> I applied something very similar to your patch and the crash went away.
> 
> The one additional change I made is in sdio_bus.c :
> 
>  void sdio_remove_func(struct sdio_func *func)
>  {
> -       if (sdio_func_present(func))
> -               device_del(&func->dev);
> +       if (!sdio_func_present(func))
> +               return;
>  
> +       device_del(&func->dev);
>         put_device(&func->dev);
>  }
> 
> I think this is necessary because the error path will go mmc_sdio_remove
> --> sdio_remove_func
> Hence sdio_remove_func() will be called when sdio_add_func() was never
> called beforehand, so there is no func->dev reference to drop.
> 
> Do you agree? I'm not certain about this one.
> 
> Thanks!
(Continue reading)

Matt Fleming | 1 Dec 15:50 2009

[PATCH] sdio: Initialise SDIO functions and update card->sdio_funcs in lockstep

We need to accurately track how many SDIO functions have been
initialised (and keep card->sdio_funcs in sync) so that we don't try to
remove more functions than we initialised if we hit the error path in
mmc_attach_sdio().

Without this patch if we hit the error path in mmc_attach_sdio() we run
the risk of deferencing invalid memory in sdio_remove_func(), leading to
a crash.

Signed-off-by: Matt Fleming <matt <at> console-pimps.org>
---
 drivers/mmc/core/sdio.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..8002797 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
 <at>  <at>  -516,7 +516,8  <at>  <at>  int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
 	 */
-	card->sdio_funcs = funcs = (ocr & 0x70000000) >> 28;
+	funcs = (ocr & 0x70000000) >> 28;
+	card->sdio_funcs = 0;

 	/*
 	 * If needed, disconnect card detection pull-up resistor.
 <at>  <at>  -528,7 +529,7  <at>  <at>  int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	/*
(Continue reading)

Daniel Drake | 1 Dec 16:13 2009

[PATCH] sdio: Fix crash in mmc_attach_sdio() error path

At http://dev.laptop.org/ticket/9707 we are seeing a crash in
the error path of mmc_attach_sdio()

BUG: unable to handle kernel paging request at 6b6b6c57
IP: [<b066d6e2>] sdio_remove_func+0x9/0x27
Call Trace:
[<b066cfb4>] ? mmc_sdio_remove+0x34/0x65
[<b066d1fc>] ? mmc_attach_sdio+0x217/0x240
[<b066a22f>] ? mmc_rescan+0x1a2/0x20f
[<b042e9a0>] ? worker_thread+0x156/0x1e1

Matt Fleming pointed out that this is because more cards are being
removed than have been initialized/added in the error path.

Fix the error path to handle failures gracefully. Based on earlier
patch from Matt.

Signed-off-by: Daniel Drake <dsd <at> laptop.org>
---
 drivers/mmc/core/sdio.c     |    6 ++++--
 drivers/mmc/core/sdio_bus.c |    7 +++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..fa07d4f 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
 <at>  <at>  -516,7 +516,8  <at>  <at>  int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
(Continue reading)

Matt Fleming | 1 Dec 16:53 2009

Re: [PATCH] sdio: Fix crash in mmc_attach_sdio() error path

On Tue, Dec 01, 2009 at 03:13:00PM +0000, Daniel Drake wrote:
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index cdb845b..fa07d4f 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
>  <at>  <at>  -516,7 +516,8  <at>  <at>  int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  	 * The number of functions on the card is encoded inside
>  	 * the ocr.
>  	 */
> -	card->sdio_funcs = funcs = (ocr & 0x70000000) >> 28;
> +	funcs = (ocr & 0x70000000) >> 28;
> +	card->sdio_funcs = 0;
>  
>  	/*
>  	 * If needed, disconnect card detection pull-up resistor.
>  <at>  <at>  -528,10 +529,11  <at>  <at>  int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  	/*
>  	 * Initialize (but don't add) all present functions.
>  	 */
> -	for (i = 0;i < funcs;i++) {
> +	for (i = 0;i < funcs;i++, card->sdio_funcs++) {
>  		err = sdio_init_func(host->card, i + 1);
>  		if (err)
>  			goto remove;
> +		card->sdio_funcs = i + 1;
>  	}

I don't understand what you're trying to do here. The card->sdio_funcs++
should take care of incrementing the sdio_funcs count properly. When the
loop terminates both "i" and "card->sdio_funcs" will be equal to
(Continue reading)

Daniel Drake | 1 Dec 17:00 2009

Re: [PATCH] sdio: Fix crash in mmc_attach_sdio() error path

On Tue, 2009-12-01 at 15:53 +0000, Matt Fleming wrote:
> I don't understand what you're trying to do here. The card->sdio_funcs++
> should take care of incrementing the sdio_funcs count properly. When the
> loop terminates both "i" and "card->sdio_funcs" will be equal to
> "funcs", unless we jump to the remove label. Unless I've missed
> something?

Sorry, that was indeed a small mistake.

> Isn't this hunk below fixing a slightly different bug? Admittedly, it
> could cause a crash, but I think it warrants a separate patch and
> changelog.
> 
> I assumed you would be sending a patch just for this bug below and not
> for the one above (which I is why I submitted one). I wasn't very clear
> about that though ;-)

OK, I'll resend that part alone.

Thanks,
Daniel

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

Daniel Mack | 1 Dec 18:17 2009
Picon

[PATCH] mmc: pxamci: call mmc_remove_host() before freeing resources

mmc_remove_host() will cause the mmc core to switch off the bus power by
eventually calling pxamci_set_ios(). This function uses the regulator or
the GPIO which have been freed already.

This causes the following Oops on module unload.

[   49.519649] Unable to handle kernel paging request at virtual address 30303a70
[   49.526878] pgd = c7084000
[   49.529563] [30303a70] *pgd=00000000
[   49.533136] Internal error: Oops: 5 [#1]
[   49.537025] last sysfs file: /sys/devices/platform/pxa27x-ohci/usb1/1-1/1-1:1.0/host0/target0:0:0/0:0:0:0/scsi_level
[   49.547471] Modules linked in: pxamci(-) eeti_ts
[   49.552061] CPU: 0    Not tainted  (2.6.32-rc8 #322)
[   49.557001] PC is at regulator_is_enabled+0x3c/0xbc
[   49.561846] LR is at regulator_is_enabled+0x30/0xbc
[   49.566691] pc : [<c01a2448>]    lr : [<c01a243c>]    psr: 60000013
[   49.566702] sp : c7083e70  ip : 30303a30  fp : 00000000
[   49.578093] r10: c705e200  r9 : c7082000  r8 : c705e2e0
[   49.583280] r7 : c7061340  r6 : c7061340  r5 : c7083e70  r4 : 00000000
[   49.589759] r3 : c04dc434  r2 : c04dc434  r1 : c03eecea  r0 : 00000047
[   49.596241] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   49.603329] Control: 0000397f  Table: a7084018  DAC: 00000015
[   49.609031] Process rmmod (pid: 1101, stack limit = 0xc7082278)
[   49.614908] Stack: (0xc7083e70 to 0xc7084000)
[   49.619238] 3e60:                                     c7082000 c703c4f8 c705ea00 c04f4074
[   49.627366] 3e80: 00000000 c705e3a0 ffffffff c0247ddc c70361a0 00000000 c705e3a0 ffffffff
[   49.635499] 3ea0: c705e200 bf006400 c78c4f00 c705e200 c705e3a0 ffffffff c705e200 ffffffff
[   49.643633] 3ec0: c04d8ac8 c02476d0 ffffffff c0247c60 c705e200 c0248678 c705e200 c0249064
[   49.651765] 3ee0: ffffffff bf006204 c04d8ad0 c04d8ad0 c04d8ac8 bf007490 00000880 c00440c4
[   49.659898] 3f00: 0000b748 c01c5708 bf007490 c01c44c8 c04d8ac8 c04d8afc bf007490 c01c4570
(Continue reading)

Wouter van Heyst | 1 Dec 20:57 2009
Picon

Re: [PATCH] mmc: add module parameter to set whether cards are assumed removable

On Sun, Nov 22, 2009 at 12:32:16PM +0000, Ben Hutchings wrote:
> On Sun, 2009-11-22 at 12:42 +0100, Wouter van Heyst wrote:

...

> > I'm running 2.6.32-rc7 with this patch applied and CONFIG_MMC_UNSAFE_RESUME=y
> > That works as desired for my non-removable case. Is it desired that I
> > test if 'removable=1' will thrash my filesystem?
> 
> Please test with CONFIG_MMC_UNSAFE_RESUME=n (which Debian will continue
> to use) and removable=0.

It took a while to get around to it correctly, but yes this works.
Without setting removable=0 my Acer Aspire One hangs while trying to
resume if the mmc card is mounted.

Wouter van Heyst
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

akpm | 1 Dec 22:54 2009

+ mmc-remove-const-from-tmio-mmc-platform-data-v2.patch added to -mm tree


The patch titled
     mmc: remove const from tmio-mmc platform data
has been added to the -mm tree.  Its filename is
     mmc-remove-const-from-tmio-mmc-platform-data-v2.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: mmc: remove const from tmio-mmc platform data
From: Magnus Damm <damm <at> opensource.se>

Remove the const from the tmio-mmc platform data parameter "hclk".  Needed
by the SDHI driver.

Signed-off-by: Magnus Damm <damm <at> opensource.se>
Cc: <linux-mmc <at> vger.kernel.org>
Cc: Paul Mundt <lethal <at> linux-sh.org>
Signed-off-by: Andrew Morton <akpm <at> linux-foundation.org>
(Continue reading)

akpm | 1 Dec 22:54 2009

+ mmc-balance-tmio-mmc-cell-enable-disable-calls.patch added to -mm tree


The patch titled
     mmc: balance tmio-mmc cell enable()/disable() calls
has been added to the -mm tree.  Its filename is
     mmc-balance-tmio-mmc-cell-enable-disable-calls.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: mmc: balance tmio-mmc cell enable()/disable() calls
From: Magnus Damm <damm <at> opensource.se>

Add cell->disable() calls to the tmio-mmc probe() error handling and the
remove() function.

Signed-off-by: Magnus Damm <damm <at> opensource.se>
Cc: <linux-mmc <at> vger.kernel.org>
Cc: Paul Mundt <lethal <at> linux-sh.org>
Signed-off-by: Andrew Morton <akpm <at> linux-foundation.org>
(Continue reading)


Gmane