Subject: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()
Meet one panic as the below:
<1>[ 15.067350] BUG: unable to handle kernel NULL pointer dereference at (null)
<1>[ 15.074455] IP: [<c1496a42>] strlen+0x12/0x20
<4>[ 15.078803] *pde = 00000000
<0>[ 15.081674] Oops: 0000 [#1] PREEMPT SMP
<4>[ 15.101676] Pid: 5, comm: kworker/u:0 Tainted: G C 3.0.34-140729-g7f9d5c5 #1 Intel Corporation Medfield/BKB2
<4>[ 15.112282] EIP: 0060:[<c1496a42>] EFLAGS: 00010046 CPU: 0
<4>[ 15.117760] EIP is at strlen+0x12/0x20
<4>[ 15.121496] EAX: 00000000 EBX: f344cc04 ECX: ffffffff EDX: f344cc04
<4>[ 15.127754] ESI: c12bcee0 EDI: 00000000 EBP: f586fe74 ESP: f586fe70
<4>[ 15.134013] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
<0>[ 15.139406] Process kworker/u:0 (pid: 5, ti=f586e000 task=f585b440 task.ti=f586e000)
<0>[ 15.147140] Stack:
<4>[ 15.149141] f344cc04 f586feb0 c12bcf12 00000000 f586fe9c 00000000 00000007 00000082
<4>[ 15.156877] 00000092 00000002 c1b01ee4 f586feb8 c1635f31 f3b42330 c12bcee0 f344cc04
<4>[ 15.164616] f586fed0 c152f815 f3b42330 f3b42328 00000000 f344cc04 f589b804 00000000
<0>[ 15.172351] Call Trace:
<4>[ 15.174810] [<c12bcf12>] ftrace_raw_event_runtime_pm_status+0x32/0x140
<4>[ 15.181411] [<c1635f31>] ? sdio_enable_wide.part.8+0x61/0x80
<4>[ 15.187145] [<c12bcee0>] ? perf_trace_runtime_pm_usage+0x1a0/0x1a0
<4>[ 15.193407] [<c152f815>] __update_runtime_status+0x65/0x90
<4>[ 15.198968] [<c1531170>] __pm_runtime_set_status+0xe0/0x1b0
<4>[ 15.204621] [<c1637366>] mmc_attach_sdio+0x2f6/0x410
<4>[ 15.209666] [<c162f520>] mmc_rescan+0x240/0x2b0
<4>[ 15.214270] [<c12643ce>] process_one_work+0xfe/0x3f0
<4>[ 15.219311] [<c1242754>] ? wake_up_process+0x14/0x20
<4>[ 15.224357] [<c162f2e0>] ? mmc_detect_card_removed+0x80/0x80
<4>[ 15.230091] [<c12649c1>] worker_thread+0x121/0x2f0
<4>[ 15.234958] [<c12648a0>] ? rescuer_thread+0x1e0/0x1e0
<4>[ 15.240091] [<c12684cd>] kthread+0x6d/0x80
<4>[ 15.244264] [<c1268460>] ? __init_kthread_worker+0x30/0x30
<4>[ 15.245485] [<c186dc3a>] kernel_thread_helper+0x6/0x10
The reason is pm_runtime_set_active() is called before the device name
is set, and the dev name setting is done at mmc_add_card() laterly.
So when calling pm_runtime_set_active(), it will hit the strlen(devname==0)
which trigger the panic.
Here before calling pm_runtime_set_active(), set the dev name, although
it is duplicated with mmc_add_card(), but it do not break the original
design(commit 81968561b).
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/mmc/core/sdio.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2273ce6..73746af 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1104,6 +1104,15 @@ int mmc_attach_sdio(struct mmc_host *host)
*/
if (host->caps & MMC_CAP_POWER_OFF_CARD) {
/*
+ * pm_runtime_set_active will use strlen(dev_name),
+ * we must set it in advance to avoid crash,
+ * although it is the duplication in mmc_add_card
+ * laterly.
+ */
+ dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host),
+ card->rca);
+
+ /*
* Let runtime PM core know our card is active
*/
err = pm_runtime_set_active(&card->dev);
--
1.7.0.4
Hi Liu,
On Fri, Nov 16, 2012 at 2:54 PM, Chuansheng Liu
<[email protected]> wrote:
> So when calling pm_runtime_set_active(), it will hit the strlen(devname==0)
> which trigger the panic.
Can you please point to the exact line of code that triggers this panic ?
> Here before calling pm_runtime_set_active(), set the dev name, although
> it is duplicated with mmc_add_card(), but it do not break the original
> design(commit 81968561b).
Normally we'd like to avoid such a solution, so let's first make sure
we understand the problem.
Have you tried thinking how come this shows up only now - has any of
the relevant code been changed lately ? Are you using a vanilla Linux
tree ?
Thanks,
Ohad.
> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:[email protected]]
> Sent: Saturday, November 17, 2012 3:33 AM
> To: Liu, Chuansheng
> Cc: Chris Ball; [email protected]; [email protected]
> Subject: Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when
> calling pm_runtime_set_active()
>
> Hi Liu,
>
> On Fri, Nov 16, 2012 at 2:54 PM, Chuansheng Liu
> <[email protected]> wrote:
> > So when calling pm_runtime_set_active(), it will hit the strlen(devname==0)
> > which trigger the panic.
>
> Can you please point to the exact line of code that triggers this panic ?
The call trace is as below:
mmc_rescan
-> mmc_rescan_try_freq
-> mmc_attach_sdio
-> pm_runtime_set_active
-> __pm_runtime_set_status
-> __update_runtime_status
-> trace_runtime_pm_status
This function is corresponding to the below code in trace/events/power.h:
TRACE_EVENT(runtime_pm_status,
TP_PROTO(struct device *dev, int status),
TP_ARGS(dev, status),
TP_STRUCT__entry(
__string(devname, dev_name(dev))
Rechecked these codes, the trace event runtime_pm_status is added newly, this is different with vanilla
Linux. But it is just a new trace event.
So I still think that calling pm_runtime_set_active is not safe when dev_name is NULL.
If you agree this point, I can refine the code that moving "init the dev_name " from mmc_add_card
to mmc_sdio_init_card. Thanks.
>
> > Here before calling pm_runtime_set_active(), set the dev name, although
> > it is duplicated with mmc_add_card(), but it do not break the original
> > design(commit 81968561b).
>
> Normally we'd like to avoid such a solution, so let's first make sure
> we understand the problem.
>
> Have you tried thinking how come this shows up only now - has any of
> the relevant code been changed lately ? Are you using a vanilla Linux
> tree ?
>
> Thanks,
> Ohad.
On Mon, Nov 19, 2012 at 7:57 AM, Liu, Chuansheng
<[email protected]> wrote:
> Rechecked these codes, the trace event runtime_pm_status is added newly, this is different with vanilla
> Linux.
Not sure I'm following - can you point out which tree are you working with ?
> So I still think that calling pm_runtime_set_active is not safe when dev_name is NULL.
> If you agree this point, I can refine the code that moving "init the dev_name " from mmc_add_card
> to mmc_sdio_init_card.
This sounds more reasonable.
Thanks,
Ohad.
> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:[email protected]]
> Sent: Monday, November 19, 2012 3:47 PM
> To: Liu, Chuansheng
> Cc: Chris Ball; [email protected]; [email protected]
> Subject: Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when
> calling pm_runtime_set_active()
>
> On Mon, Nov 19, 2012 at 7:57 AM, Liu, Chuansheng
> <[email protected]> wrote:
> > Rechecked these codes, the trace event runtime_pm_status is added newly,
> this is different with vanilla
> > Linux.
>
> Not sure I'm following - can you point out which tree are you working with ?
Sorry, it is added internally for debugging purpose.
>
> > So I still think that calling pm_runtime_set_active is not safe when dev_name
> is NULL.
> > If you agree this point, I can refine the code that moving "init the dev_name "
> from mmc_add_card
> > to mmc_sdio_init_card.
>
> This sounds more reasonable.
I will try a V2 patch soon, thanks.
>
> Thanks,
> Ohad.
In below call trace:
mmc_rescan
-> mmc_rescan_try_freq()
-> mmc_attach_sdio()
-> mmc_sdio_init_card()
...
pm_runtime_set_active()
...
mmc_add_card()
The dev name is set until in mmc_add_card(), but before that, it is
possible the dev name is needed, for example in pm_runtime_set_active(),
we can call trace event to trace which dev is changing the runtime status.
So here advance it into mmc_sdio_init_card() to benefit others.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/mmc/core/bus.c | 5 +++--
drivers/mmc/core/sdio.c | 5 ++++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 9b68933..4884d6e 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -270,8 +270,9 @@ int mmc_add_card(struct mmc_card *card)
[UHS_DDR50_BUS_SPEED] = "DDR50 ",
};
-
- dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host), card->rca);
+ if (!dev_name(&card->dev))
+ dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host),
+ card->rca);
switch (card->type) {
case MMC_TYPE_MMC:
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2273ce6..a9f6f02 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -795,8 +795,11 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
goto remove;
}
finish:
- if (!oldcard)
+ if (!oldcard) {
host->card = card;
+ dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host),
+ card->rca);
+ }
return 0;
remove:
--
1.7.0.4
On Mon, Nov 19, 2012 at 10:01 AM, Liu, Chuansheng
<[email protected]> wrote:
>> > Rechecked these codes, the trace event runtime_pm_status is added newly,
>> this is different with vanilla
>> > Linux.
>>
>> Not sure I'm following - can you point out which tree are you working with ?
> Sorry, it is added internally for debugging purpose.
Maybe keep this patch internally too then ?
This is new version?
Maybe you should add prefix v2 in subject and the version history.
Best Regards
Jerry Huang
> -----Original Message-----
> From: [email protected] [mailto:linux-mmc-
> [email protected]] On Behalf Of Chuansheng Liu
> Sent: Tuesday, November 20, 2012 1:38 AM
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: [PATCH] mmc,sdio: advancing the setting of dev name in
> mmc_sdio_init_card()
>
>
> In below call trace:
> mmc_rescan
> -> mmc_rescan_try_freq()
> -> mmc_attach_sdio()
> -> mmc_sdio_init_card()
> ...
> pm_runtime_set_active()
> ...
> mmc_add_card()
>
> The dev name is set until in mmc_add_card(), but before that, it is
> possible the dev name is needed, for example in pm_runtime_set_active(),
> we can call trace event to trace which dev is changing the runtime status.
>
> So here advance it into mmc_sdio_init_card() to benefit others.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> drivers/mmc/core/bus.c | 5 +++--
> drivers/mmc/core/sdio.c | 5 ++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c index
> 9b68933..4884d6e 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -270,8 +270,9 @@ int mmc_add_card(struct mmc_card *card)
> [UHS_DDR50_BUS_SPEED] = "DDR50 ",
> };
>
> -
> - dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host), card-
> >rca);
> + if (!dev_name(&card->dev))
> + dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host),
> + card->rca);
>
> switch (card->type) {
> case MMC_TYPE_MMC:
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index
> 2273ce6..a9f6f02 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -795,8 +795,11 @@ static int mmc_sdio_init_card(struct mmc_host *host,
> u32 ocr,
> goto remove;
> }
> finish:
> - if (!oldcard)
> + if (!oldcard) {
> host->card = card;
> + dev_set_name(&card->dev, "%s:%04x", mmc_hostname(card->host),
> + card->rca);
> + }
> return 0;
>
> remove:
> --
> 1.7.0.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?