[PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Ohad Ben-Cohen]

Hi Liu,

On Fri, Nov 16, 2012 at 2:54 PM, Chuansheng Liu
<chuansheng.liu@intel.com> 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.

RE: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Liu, Chuansheng]

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
> Sent: Saturday, November 17, 2012 3:33 AM
> To: Liu, Chuansheng
> Cc: Chris Ball; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> 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
> <chuansheng.liu@intel.com> 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.

Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Ohad Ben-Cohen]

On Mon, Nov 19, 2012 at 7:57 AM, Liu, Chuansheng
<chuansheng.liu@intel.com> 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.

RE: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Liu, Chuansheng]

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
> Sent: Monday, November 19, 2012 3:47 PM
> To: Liu, Chuansheng
> Cc: Chris Ball; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> 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
> <chuansheng.liu@intel.com> 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.

Re: [PATCH] mmc,sdio: Fix the panic due to devname NULL when calling pm_runtime_set_active()

[Ohad Ben-Cohen]

On Mon, Nov 19, 2012 at 10:01 AM, Liu, Chuansheng
<chuansheng.liu@intel.com> 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 ?

RE: [PATCH] mmc,sdio: advancing the setting of dev name in mmc_sdio_init_card()

[Huang Changming-R66093]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2603 bytes --]

This is new version?
Maybe you should add prefix v2 in subject and the version history.

Best Regards
Jerry Huang


> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Chuansheng Liu
> Sent: Tuesday, November 20, 2012 1:38 AM
> To: cjb@laptop.org; ohad@wizery.com
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org;
> chuansheng.liu@intel.com
> 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 <chuansheng.liu@intel.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] mmc,sdio: advancing the setting of dev name in mmc_sdio_init_card()

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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