one doubt about mmc_sdio_init_card function

one doubt about mmc_sdio_init_card function

[Fu, Zhonghui]

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

Re: one doubt about mmc_sdio_init_card function

[Aaron Lu]

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
> 

One bug of SDHCI driver

[Fu, Zhonghui]

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

Re: One bug of SDHCI driver

[Jaehoon Chung]

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
> 
> 
> 
> 
> 
> 

Re: One bug of SDHCI driver

[Fu, Zhonghui]

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
>>
>>
>>
>>
>>
>>

Re: One bug of SDHCI driver

[Fu, Zhonghui]

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
>>>
>>>
>>>
>>>
>>>
>>>

Re: One bug of SDHCI driver

[Chris Ball]

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 <chris@printf.net>
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 <zhonghui.fu@linux.intel.com>
[Patch suggested by Jaehoon Chung]
Signed-off-by: Chris Ball <chris@printf.net>
---
 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/>

Re: One bug of SDHCI driver

[Fu, Zhonghui]

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 <chris@printf.net>
> 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 <zhonghui.fu@linux.intel.com>
> [Patch suggested by Jaehoon Chung]
> Signed-off-by: Chris Ball <chris@printf.net>
> ---
>  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);
>  

Re: One bug of SDHCI driver

[Jaehoon Chung]

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 <chris@printf.net>
>> 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 <zhonghui.fu@linux.intel.com>
>> [Patch suggested by Jaehoon Chung]
>> Signed-off-by: Chris Ball <chris@printf.net>
>> ---
>>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: One bug of SDHCI driver

[Jaehoon Chung]

From: Chris Ball <chris@printf.net>
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 <zhonghui.fu@linux.intel.com>
[Patch suggested by Jaehoon Chung]
Signed-off-by: Chris Ball <chris@printf.net>
---
 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 <chris@printf.net>
>>> 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 <zhonghui.fu@linux.intel.com>
>>> [Patch suggested by Jaehoon Chung]
>>> Signed-off-by: Chris Ball <chris@printf.net>
>>> ---
>>>  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 majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: One bug of SDHCI driver

[Fu, Zhonghui]

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 <zhonghui.fu@linux.intel.com>
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 <zhonghui.fu@linux.intel.com>
---
 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 <chris@printf.net>
> 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 <zhonghui.fu@linux.intel.com>
> [Patch suggested by Jaehoon Chung]
> Signed-off-by: Chris Ball <chris@printf.net>
> ---
>  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);
>  

Re: One bug of SDHCI driver

[Fu, Zhonghui]

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 <zhonghui.fu@linux.intel.com>
> 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 <zhonghui.fu@linux.intel.com>
> ---
>  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 <chris@printf.net>
>> 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 <zhonghui.fu@linux.intel.com>
>> [Patch suggested by Jaehoon Chung]
>> Signed-off-by: Chris Ball <chris@printf.net>
>> ---
>>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: One bug of SDHCI driver

[Jaehoon Chung]

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 <zhonghui.fu@linux.intel.com>
>>> 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 <zhonghui.fu@linux.intel.com>
>>> ---
>>>  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 <chris@printf.net>
>>>> 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 <zhonghui.fu@linux.intel.com>
>>>> [Patch suggested by Jaehoon Chung]
>>>> Signed-off-by: Chris Ball <chris@printf.net>
>>>> ---
>>>>  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 majordomo@vger.kernel.org
>>> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: One bug of SDHCI driver

[Fu, Zhonghui]

Hi, Jaehoon

According to your comments, I created a new patch for this issue as follows:


Thanks,
Zhonghui


>From 6cee984e1d76ba0a3320430f8cf4318ab65fcf06 Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
Date: Tue, 5 Aug 2014 12:44:38 +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.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=80151

In addition, the check for MMC_CAP_SDIO_IRQ capability is added
before enable sdio IRQ.

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Chris Ball <chris@printf.net>
Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
---
 drivers/mmc/core/sdio.c     |   14 ++++++++++++--
 drivers/mmc/core/sdio_irq.c |    4 ++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index e636d9e..e04a540 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 if (host->caps & MMC_CAP_SDIO_IRQ) {
+			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;
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 5cc13c8..696eca4 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -208,7 +208,7 @@ static int sdio_card_irq_get(struct mmc_card *card)
 				host->sdio_irqs--;
 				return err;
 			}
-		} else {
+		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
 			mmc_host_clk_hold(host);
 			host->ops->enable_sdio_irq(host, 1);
 			mmc_host_clk_release(host);
@@ -229,7 +229,7 @@ static int sdio_card_irq_put(struct mmc_card *card)
 		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
 			atomic_set(&host->sdio_irq_thread_abort, 1);
 			kthread_stop(host->sdio_irq_thread);
-		} else {
+		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
 			mmc_host_clk_hold(host);
 			host->ops->enable_sdio_irq(host, 0);
 			mmc_host_clk_release(host);
-- 1.7.1






On 2014/7/30 11:40, Jaehoon Chung wrote:
> 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 <zhonghui.fu@linux.intel.com>
>>>> 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 <zhonghui.fu@linux.intel.com>
>>>> ---
>>>>  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 <chris@printf.net>
>>>>> 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 <zhonghui.fu@linux.intel.com>
>>>>> [Patch suggested by Jaehoon Chung]
>>>>> Signed-off-by: Chris Ball <chris@printf.net>
>>>>> ---
>>>>>  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 majordomo@vger.kernel.org
>>>> 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 majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/

Re: One bug of SDHCI driver

[Fu, Zhonghui]

kindly reminder.

Thanks,
Zhonghui

On 2014/8/5 12:56, Fu, Zhonghui wrote:
> Hi, Jaehoon
>
> According to your comments, I created a new patch for this issue as follows:
>
>
> Thanks,
> Zhonghui
>
>
> From 6cee984e1d76ba0a3320430f8cf4318ab65fcf06 Mon Sep 17 00:00:00 2001
> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> Date: Tue, 5 Aug 2014 12:44:38 +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.
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=80151
>
> In addition, the check for MMC_CAP_SDIO_IRQ capability is added
> before enable sdio IRQ.
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Chris Ball <chris@printf.net>
> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> ---
>  drivers/mmc/core/sdio.c     |   14 ++++++++++++--
>  drivers/mmc/core/sdio_irq.c |    4 ++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index e636d9e..e04a540 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 if (host->caps & MMC_CAP_SDIO_IRQ) {
> +			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;
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index 5cc13c8..696eca4 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -208,7 +208,7 @@ static int sdio_card_irq_get(struct mmc_card *card)
>  				host->sdio_irqs--;
>  				return err;
>  			}
> -		} else {
> +		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
>  			mmc_host_clk_hold(host);
>  			host->ops->enable_sdio_irq(host, 1);
>  			mmc_host_clk_release(host);
> @@ -229,7 +229,7 @@ static int sdio_card_irq_put(struct mmc_card *card)
>  		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
>  			atomic_set(&host->sdio_irq_thread_abort, 1);
>  			kthread_stop(host->sdio_irq_thread);
> -		} else {
> +		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
>  			mmc_host_clk_hold(host);
>  			host->ops->enable_sdio_irq(host, 0);
>  			mmc_host_clk_release(host);
> -- 1.7.1
>
>
>
>
>
>
> On 2014/7/30 11:40, Jaehoon Chung wrote:
>> 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 <zhonghui.fu@linux.intel.com>
>>>>> 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 <zhonghui.fu@linux.intel.com>
>>>>> ---
>>>>>  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 <chris@printf.net>
>>>>>> 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 <zhonghui.fu@linux.intel.com>
>>>>>> [Patch suggested by Jaehoon Chung]
>>>>>> Signed-off-by: Chris Ball <chris@printf.net>
>>>>>> ---
>>>>>>  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 majordomo@vger.kernel.org
>>>>> 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 majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at  http://www.tux.org/lkml/

Re: One bug of SDHCI driver

[Fu, Zhonghui]

Hi, everyone

I have posted this new patch in a separate mail. Many thanks for your comments.


Thanks,
Zhonghui

On 2014/8/7 14:58, Fu, Zhonghui wrote:
> kindly reminder.
>
> Thanks,
> Zhonghui
>
> On 2014/8/5 12:56, Fu, Zhonghui wrote:
>> Hi, Jaehoon
>>
>> According to your comments, I created a new patch for this issue as follows:
>>
>>
>> Thanks,
>> Zhonghui
>>
>>
>> From 6cee984e1d76ba0a3320430f8cf4318ab65fcf06 Mon Sep 17 00:00:00 2001
>> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> Date: Tue, 5 Aug 2014 12:44:38 +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.
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=80151
>>
>> In addition, the check for MMC_CAP_SDIO_IRQ capability is added
>> before enable sdio IRQ.
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Chris Ball <chris@printf.net>
>> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> ---
>>  drivers/mmc/core/sdio.c     |   14 ++++++++++++--
>>  drivers/mmc/core/sdio_irq.c |    4 ++--
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index e636d9e..e04a540 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 if (host->caps & MMC_CAP_SDIO_IRQ) {
>> +			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;
>> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
>> index 5cc13c8..696eca4 100644
>> --- a/drivers/mmc/core/sdio_irq.c
>> +++ b/drivers/mmc/core/sdio_irq.c
>> @@ -208,7 +208,7 @@ static int sdio_card_irq_get(struct mmc_card *card)
>>  				host->sdio_irqs--;
>>  				return err;
>>  			}
>> -		} else {
>> +		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
>>  			mmc_host_clk_hold(host);
>>  			host->ops->enable_sdio_irq(host, 1);
>>  			mmc_host_clk_release(host);
>> @@ -229,7 +229,7 @@ static int sdio_card_irq_put(struct mmc_card *card)
>>  		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
>>  			atomic_set(&host->sdio_irq_thread_abort, 1);
>>  			kthread_stop(host->sdio_irq_thread);
>> -		} else {
>> +		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
>>  			mmc_host_clk_hold(host);
>>  			host->ops->enable_sdio_irq(host, 0);
>>  			mmc_host_clk_release(host);
>> -- 1.7.1
>>
>>
>>
>>
>>
>>
>> On 2014/7/30 11:40, Jaehoon Chung wrote:
>>> 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 <zhonghui.fu@linux.intel.com>
>>>>>> 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 <zhonghui.fu@linux.intel.com>
>>>>>> ---
>>>>>>  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 <chris@printf.net>
>>>>>>> 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 <zhonghui.fu@linux.intel.com>
>>>>>>> [Patch suggested by Jaehoon Chung]
>>>>>>> Signed-off-by: Chris Ball <chris@printf.net>
>>>>>>> ---
>>>>>>>  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 majordomo@vger.kernel.org
>>>>>> 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 majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>> Please read the FAQ at  http://www.tux.org/lkml/