2014-07-01 05:39:58

by Fu, Zhonghui

[permalink] [raw]
Subject: one doubt about mmc_sdio_init_card function


Hi, all

The mmc_sdio_init_card(drivers/mmc/core/sdio.c) function calls mmc_alloc_card(drivers/mmc/core/bus.c) function to allocate a card structure. card->dev.bus is assigned with mmc_bus_type in mmc_alloc_card function. Why not assign sdio_bus_type to card->dev.bus?


struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
{
struct mmc_card *card;

card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
if (!card)
return ERR_PTR(-ENOMEM);

card->host = host;

device_initialize(&card->dev);

card->dev.parent = mmc_classdev(host);
card->dev.bus = &mmc_bus_type;
card->dev.release = mmc_release_card;
card->dev.type = type;

return card;
}


Thanks,
Zhonghui


2014-07-02 02:52:09

by Aaron Lu

[permalink] [raw]
Subject: Re: one doubt about mmc_sdio_init_card function

On 07/01/2014 01:39 PM, Fu, Zhonghui wrote:
>
> Hi, all
>
> The mmc_sdio_init_card(drivers/mmc/core/sdio.c) function calls mmc_alloc_card(drivers/mmc/core/bus.c) function to allocate a card structure. card->dev.bus is assigned with mmc_bus_type in mmc_alloc_card function. Why not assign sdio_bus_type to card->dev.bus?

sdio card, mmc card, sd card are all devices on the mmc bus, hence their
bus type is set to mmc_bus_type.

sdio function device is a device on the sdio bus, hence its bus type is
sdio_bus_type.

Hope this helps,
Aaron

>
>
> struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
> {
> struct mmc_card *card;
>
> card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
> if (!card)
> return ERR_PTR(-ENOMEM);
>
> card->host = host;
>
> device_initialize(&card->dev);
>
> card->dev.parent = mmc_classdev(host);
> card->dev.bus = &mmc_bus_type;
> card->dev.release = mmc_release_card;
> card->dev.type = type;
>
> return card;
> }
>
>
> Thanks,
> Zhonghui
>

2014-07-03 15:47:52

by Fu, Zhonghui

[permalink] [raw]
Subject: One bug of SDHCI driver


Hi, all

The statement "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" is added in "sdhci_add_host" function in host/sdhci.c file. In some cases, this will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file and lead to resume failure. Could you please give me some advice how to fix this bug?



Thanks,
Zhonghui




2014-07-04 02:40:09

by Jaehoon Chung

[permalink] [raw]
Subject: Re: One bug of SDHCI driver

Hi,

just use the MMC_CAP2_SDIO_IRQ_NOTHREAD?

if (!err && host->sdio_irq && !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
wake_up_process(host->sdio_irq_thread);

I didn't test this..but i believe that it will be fixed.

Best Regards,
Jaehoon Chung

On 07/04/2014 12:47 AM, Fu, Zhonghui wrote:
>
> Hi, all
>
> The statement "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" is added in "sdhci_add_host" function in host/sdhci.c file. In some cases, this will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file and lead to resume failure. Could you please give me some advice how to fix this bug?
>
>
>
> Thanks,
> Zhonghui
>
>
>
>
>
>

2014-07-06 15:19:15

by Fu, Zhonghui

[permalink] [raw]
Subject: Re: One bug of SDHCI driver


Yes, "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" of "sdhci_add_host" function in host/sdhci.c file make oops.

"sdio_card_irq_get" function in core/sdio_irq.c file:
if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) { /* the condition is false */
atomic_set(&host->sdio_irq_thread_abort, 0);
host->sdio_irq_thread =
kthread_run(sdio_irq_thread, host,
"ksdioirqd/%s", mmc_hostname(host));


This will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file.



Thanks,
Zhonghui



On 2014/7/4 10:40, Jaehoon Chung wrote:
> Hi,
>
> just use the MMC_CAP2_SDIO_IRQ_NOTHREAD?
>
> if (!err && host->sdio_irq && !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> wake_up_process(host->sdio_irq_thread);
>
> I didn't test this..but i believe that it will be fixed.
>
> Best Regards,
> Jaehoon Chung
>
> On 07/04/2014 12:47 AM, Fu, Zhonghui wrote:
>> Hi, all
>>
>> The statement "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" is added in "sdhci_add_host" function in host/sdhci.c file. In some cases, this will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file and lead to resume failure. Could you please give me some advice how to fix this bug?
>>
>>
>>
>> Thanks,
>> Zhonghui
>>
>>
>>
>>
>>
>>

2014-07-08 16:03:52

by Fu, Zhonghui

[permalink] [raw]
Subject: Re: One bug of SDHCI driver


Hi,

Why add "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" ? How to fix this bug?

Could you please give out some idea about this bug?


Thanks,
Zhonghui


On 2014/7/6 23:19, Fu, Zhonghui wrote:
> Yes, "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" of "sdhci_add_host" function in host/sdhci.c file make oops.
>
> "sdio_card_irq_get" function in core/sdio_irq.c file:
> if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) { /* the condition is false */
> atomic_set(&host->sdio_irq_thread_abort, 0);
> host->sdio_irq_thread =
> kthread_run(sdio_irq_thread, host,
> "ksdioirqd/%s", mmc_hostname(host));
>
>
> This will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file.
>
>
>
> Thanks,
> Zhonghui
>
>
>
> On 2014/7/4 10:40, Jaehoon Chung wrote:
>> Hi,
>>
>> just use the MMC_CAP2_SDIO_IRQ_NOTHREAD?
>>
>> if (!err && host->sdio_irq && !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
>> wake_up_process(host->sdio_irq_thread);
>>
>> I didn't test this..but i believe that it will be fixed.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>> On 07/04/2014 12:47 AM, Fu, Zhonghui wrote:
>>> Hi, all
>>>
>>> The statement "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" is added in "sdhci_add_host" function in host/sdhci.c file. In some cases, this will make "host->sdio_irq_thread" a NULL pointer in "mmc_sdio_resume" functon of core/sdio.c file and lead to resume failure. Could you please give me some advice how to fix this bug?
>>>
>>>
>>>
>>> Thanks,
>>> Zhonghui
>>>
>>>
>>>
>>>
>>>
>>>

2014-07-14 13:27:02

by Chris Ball

[permalink] [raw]
Subject: Re: One bug of SDHCI driver

Hi Zhonghui,

On Tue, Jul 08 2014, Fu, Zhonghui wrote:
> Why add "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" ? How to fix this bug?
>
> Could you please give out some idea about this bug?

Jaehoon already gave you a patch to fix this bug. Here it is again in
proper patch form. Please can you test it and let us know whether it
fixes the crash? Thanks.


From: Chris Ball <[email protected]>
Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread

781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
the use of our own custom threaded IRQ handler, but left in an
unconditional wake_up_process() on that handler at resume-time.

Reported-by: Fu, Zhonghui <[email protected]>
[Patch suggested by Jaehoon Chung]
Signed-off-by: Chris Ball <[email protected]>
---
drivers/mmc/core/sdio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index e636d9e..2a128e2 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
}
}

- if (!err && host->sdio_irqs)
+ if (!err && host->sdio_irqs &&
+ !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
wake_up_process(host->sdio_irq_thread);
mmc_release_host(host);

--
Chris Ball <http://printf.net/>

2014-07-15 02:54:59

by Fu, Zhonghui

[permalink] [raw]
Subject: Re: One bug of SDHCI driver


Hi,

The data type of "host" is "struct mmc_host", and there is not "quirks" member in this structure.


Thanks,
Zhonghui

On 2014/7/14 21:26, Chris Ball wrote:
> Hi Zhonghui,
>
> On Tue, Jul 08 2014, Fu, Zhonghui wrote:
>> Why add "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" ? How to fix this bug?
>>
>> Could you please give out some idea about this bug?
> Jaehoon already gave you a patch to fix this bug. Here it is again in
> proper patch form. Please can you test it and let us know whether it
> fixes the crash? Thanks.
>
>
> From: Chris Ball <[email protected]>
> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>
> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
> the use of our own custom threaded IRQ handler, but left in an
> unconditional wake_up_process() on that handler at resume-time.
>
> Reported-by: Fu, Zhonghui <[email protected]>
> [Patch suggested by Jaehoon Chung]
> Signed-off-by: Chris Ball <[email protected]>
> ---
> drivers/mmc/core/sdio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index e636d9e..2a128e2 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
> }
> }
>
> - if (!err && host->sdio_irqs)
> + if (!err && host->sdio_irqs &&
> + !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> wake_up_process(host->sdio_irq_thread);
> mmc_release_host(host);
>

2014-07-15 04:14:51

by Jaehoon Chung

[permalink] [raw]
Subject: Re: One bug of SDHCI driver

On 07/15/2014 11:54 AM, Fu, Zhonghui wrote:
>
> Hi,
>
> The data type of "host" is "struct mmc_host", and there is not "quirks" member in this structure.
Sorry for wrong typo.
You use the "host->caps2" instead of "host->quirks".


Best Regards,
Jaehoon Chung
>
>
> Thanks,
> Zhonghui
>
> On 2014/7/14 21:26, Chris Ball wrote:
>> Hi Zhonghui,
>>
>> On Tue, Jul 08 2014, Fu, Zhonghui wrote:
>>> Why add "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" ? How to fix this bug?
>>>
>>> Could you please give out some idea about this bug?
>> Jaehoon already gave you a patch to fix this bug. Here it is again in
>> proper patch form. Please can you test it and let us know whether it
>> fixes the crash? Thanks.
>>
>>
>> From: Chris Ball <[email protected]>
>> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>>
>> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
>> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
>> the use of our own custom threaded IRQ handler, but left in an
>> unconditional wake_up_process() on that handler at resume-time.
>>
>> Reported-by: Fu, Zhonghui <[email protected]>
>> [Patch suggested by Jaehoon Chung]
>> Signed-off-by: Chris Ball <[email protected]>
>> ---
>> drivers/mmc/core/sdio.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index e636d9e..2a128e2 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
>> }
>> }
>>
>> - if (!err && host->sdio_irqs)
>> + if (!err && host->sdio_irqs &&
>> + !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
>> wake_up_process(host->sdio_irq_thread);
>> mmc_release_host(host);
>>
>
> --
> 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
>

2014-07-15 04:40:50

by Jaehoon Chung

[permalink] [raw]
Subject: Re: One bug of SDHCI driver

From: Chris Ball <[email protected]>
Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread

781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
the use of our own custom threaded IRQ handler, but left in an
unconditional wake_up_process() on that handler at resume-time.

Reported-by: Fu, Zhonghui <[email protected]>
[Patch suggested by Jaehoon Chung]
Signed-off-by: Chris Ball <[email protected]>
---
drivers/mmc/core/sdio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index e636d9e..11cc4e0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
}
}

- if (!err && host->sdio_irqs)
+ if (!err && host->sdio_irqs &&
+ !(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
wake_up_process(host->sdio_irq_thread);
mmc_release_host(host);

--
1.7.9.5

On 07/15/2014 01:14 PM, Jaehoon Chung wrote:
> On 07/15/2014 11:54 AM, Fu, Zhonghui wrote:
>>
>> Hi,
>>
>> The data type of "host" is "struct mmc_host", and there is not "quirks" member in this structure.
> Sorry for wrong typo.
> You use the "host->caps2" instead of "host->quirks".
>
>
> Best Regards,
> Jaehoon Chung
>>
>>
>> Thanks,
>> Zhonghui
>>
>> On 2014/7/14 21:26, Chris Ball wrote:
>>> Hi Zhonghui,
>>>
>>> On Tue, Jul 08 2014, Fu, Zhonghui wrote:
>>>> Why add "mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;" ? How to fix this bug?
>>>>
>>>> Could you please give out some idea about this bug?
>>> Jaehoon already gave you a patch to fix this bug. Here it is again in
>>> proper patch form. Please can you test it and let us know whether it
>>> fixes the crash? Thanks.
>>>
>>>
>>> From: Chris Ball <[email protected]>
>>> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>>>
>>> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
>>> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
>>> the use of our own custom threaded IRQ handler, but left in an
>>> unconditional wake_up_process() on that handler at resume-time.
>>>
>>> Reported-by: Fu, Zhonghui <[email protected]>
>>> [Patch suggested by Jaehoon Chung]
>>> Signed-off-by: Chris Ball <[email protected]>
>>> ---
>>> drivers/mmc/core/sdio.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index e636d9e..2a128e2 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>> }
>>> }
>>>
>>> - if (!err && host->sdio_irqs)
>>> + if (!err && host->sdio_irqs &&
>>> + !(host->quirks & MMC_CAP2_SDIO_IRQ_NOTHREAD))
>>> wake_up_process(host->sdio_irq_thread);
>>> mmc_release_host(host);
>>>
>>
>> --
>> 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
>>
>
> --
> 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
>

2014-07-20 14:51:44

by Fu, Zhonghui

[permalink] [raw]
Subject: Re: One bug of SDHCI driver


Hi,

Chris' patch is not enough to fix this bug. I made a patch as follows and verified it can work. Could you please give out some comments about this patch?


Thanks,
Zhonghui

>From 72d6f5b56fa04290fd3a055a3333de1d89e7c8d4 Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
Date: Sun, 20 Jul 2014 22:29:53 +0800
Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread

781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
the use of our own custom threaded IRQ handler, but left in an
unconditional wake_up_process() on that handler at resume-time.

Signed-off-by: Fu Zhonghui <[email protected]>
---
drivers/mmc/core/sdio.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index e636d9e..8369e56 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -992,8 +992,18 @@ static int mmc_sdio_resume(struct mmc_host *host)
}
}

- if (!err && host->sdio_irqs)
- wake_up_process(host->sdio_irq_thread);
+ if (!err && host->sdio_irqs) {
+ if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
+ wake_up_process(host->sdio_irq_thread);
+ } else {
+ mmc_release_host(host);
+ mmc_host_clk_hold(host);
+ host->ops->enable_sdio_irq(host, 1);
+ mmc_host_clk_release(host);
+ mmc_claim_host(host);
+ }
+ }
+
mmc_release_host(host);

host->pm_flags &= ~MMC_PM_KEEP_POWER;
-- 1.7.1



On 2014/7/15 12:40, Jaehoon Chung wrote:
> From: Chris Ball <[email protected]>
> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>
> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
> the use of our own custom threaded IRQ handler, but left in an
> unconditional wake_up_process() on that handler at resume-time.
>
> Reported-by: Fu, Zhonghui <[email protected]>
> [Patch suggested by Jaehoon Chung]
> Signed-off-by: Chris Ball <[email protected]>
> ---
> drivers/mmc/core/sdio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index e636d9e..11cc4e0 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
> }
> }
>
> - if (!err && host->sdio_irqs)
> + if (!err && host->sdio_irqs &&
> + !(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> wake_up_process(host->sdio_irq_thread);
> mmc_release_host(host);
>

2014-07-24 15:27:34

by Fu, Zhonghui

[permalink] [raw]
Subject: Re: One bug of SDHCI driver


Hi,

Any comments for this new patch?

Thanks,
Zhonghui
On 2014/7/20 22:51, Fu, Zhonghui wrote:
> Hi,
>
> Chris' patch is not enough to fix this bug. I made a patch as follows and verified it can work. Could you please give out some comments about this patch?
>
>
> Thanks,
> Zhonghui
>
> From 72d6f5b56fa04290fd3a055a3333de1d89e7c8d4 Mon Sep 17 00:00:00 2001
> From: Fu Zhonghui <[email protected]>
> Date: Sun, 20 Jul 2014 22:29:53 +0800
> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>
> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
> the use of our own custom threaded IRQ handler, but left in an
> unconditional wake_up_process() on that handler at resume-time.
>
> Signed-off-by: Fu Zhonghui <[email protected]>
> ---
> drivers/mmc/core/sdio.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index e636d9e..8369e56 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -992,8 +992,18 @@ static int mmc_sdio_resume(struct mmc_host *host)
> }
> }
>
> - if (!err && host->sdio_irqs)
> - wake_up_process(host->sdio_irq_thread);
> + if (!err && host->sdio_irqs) {
> + if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
> + wake_up_process(host->sdio_irq_thread);
> + } else {
> + mmc_release_host(host);
> + mmc_host_clk_hold(host);
> + host->ops->enable_sdio_irq(host, 1);
> + mmc_host_clk_release(host);
> + mmc_claim_host(host);
> + }
> + }
> +
> mmc_release_host(host);
>
> host->pm_flags &= ~MMC_PM_KEEP_POWER;
> -- 1.7.1
>
>
>
> On 2014/7/15 12:40, Jaehoon Chung wrote:
>> From: Chris Ball <[email protected]>
>> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>>
>> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
>> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
>> the use of our own custom threaded IRQ handler, but left in an
>> unconditional wake_up_process() on that handler at resume-time.
>>
>> Reported-by: Fu, Zhonghui <[email protected]>
>> [Patch suggested by Jaehoon Chung]
>> Signed-off-by: Chris Ball <[email protected]>
>> ---
>> drivers/mmc/core/sdio.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index e636d9e..11cc4e0 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
>> }
>> }
>>
>> - if (!err && host->sdio_irqs)
>> + if (!err && host->sdio_irqs &&
>> + !(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
>> wake_up_process(host->sdio_irq_thread);
>> mmc_release_host(host);
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2014-07-30 03:40:25

by Jaehoon Chung

[permalink] [raw]
Subject: Re: One bug of SDHCI driver

Hi, Zhonghui.

On 07/30/2014 11:47 AM, Fu, Zhonghui wrote:
>
> Hi,
>
> In the resume function, SDIO irq must be enabled, or the interrupts from devices on SDIO bus can't be acknowledged. I also uploaded this new patch to https://bugzilla.kernel.org/show_bug.cgi?id=80151.
> Could you please help to review it?
>
>
>
>
> Thanks,
> Zhonghui
>
> On 2014/7/24 23:27, Fu, Zhonghui wrote:
>> Hi,
>>
>> Any comments for this new patch?
>>
>> Thanks,
>> Zhonghui
>> On 2014/7/20 22:51, Fu, Zhonghui wrote:
>>> Hi,
>>>
>>> Chris' patch is not enough to fix this bug. I made a patch as follows and verified it can work. Could you please give out some comments about this patch?
>>>
>>>
>>> Thanks,
>>> Zhonghui
>>>
>>> >From 72d6f5b56fa04290fd3a055a3333de1d89e7c8d4 Mon Sep 17 00:00:00 2001
>>> From: Fu Zhonghui <[email protected]>
>>> Date: Sun, 20 Jul 2014 22:29:53 +0800
>>> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>>>
>>> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
>>> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
>>> the use of our own custom threaded IRQ handler, but left in an
>>> unconditional wake_up_process() on that handler at resume-time.
>>>
>>> Signed-off-by: Fu Zhonghui <[email protected]>
>>> ---
>>> drivers/mmc/core/sdio.c | 14 ++++++++++++--
>>> 1 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index e636d9e..8369e56 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -992,8 +992,18 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>> }
>>> }
>>>
>>> - if (!err && host->sdio_irqs)
>>> - wake_up_process(host->sdio_irq_thread);
>>> + if (!err && host->sdio_irqs) {
>>> + if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
>>> + wake_up_process(host->sdio_irq_thread);
>>> + } else {
>>> + mmc_release_host(host);
>>> + mmc_host_clk_hold(host);
>>> + host->ops->enable_sdio_irq(host, 1);
>>> + mmc_host_clk_release(host);
>>> + mmc_claim_host(host);
>>> + }
>>> + }
If you enable the sdio_irq, I think it needs to check whether MMC_CAP_SDIO_IRQ is set or not.


Best Regards,
Jaehoon Chung
>>> +
>>> mmc_release_host(host);
>>>
>>> host->pm_flags &= ~MMC_PM_KEEP_POWER;
>>> -- 1.7.1
>>>
>>>
>>>
>>> On 2014/7/15 12:40, Jaehoon Chung wrote:
>>>> From: Chris Ball <[email protected]>
>>>> Subject: [PATCH] mmc: core: sdio: Fix unconditional wake_up_process() on sdio thread
>>>>
>>>> 781e989cf59 ("mmc: sdhci: convert to new SDIO IRQ handling") and
>>>> bf3b5ec66bd ("mmc: sdio_irq: rework sdio irq handling") disabled
>>>> the use of our own custom threaded IRQ handler, but left in an
>>>> unconditional wake_up_process() on that handler at resume-time.
>>>>
>>>> Reported-by: Fu, Zhonghui <[email protected]>
>>>> [Patch suggested by Jaehoon Chung]
>>>> Signed-off-by: Chris Ball <[email protected]>
>>>> ---
>>>> drivers/mmc/core/sdio.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>> index e636d9e..11cc4e0 100644
>>>> --- a/drivers/mmc/core/sdio.c
>>>> +++ b/drivers/mmc/core/sdio.c
>>>> @@ -992,7 +992,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>> }
>>>> }
>>>>
>>>> - if (!err && host->sdio_irqs)
>>>> + if (!err && host->sdio_irqs &&
>>>> + !(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
>>>> wake_up_process(host->sdio_irq_thread);
>>>> mmc_release_host(host);
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>