Re: One bug of SDHCI driver

["Fu, Zhonghui" <zhonghui.fu@linux.intel.com>]

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/