linux-kernel - Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ff09082f3f43d4b358ec59d7ed8f216@agner.ch>
Date:	Wed, 03 Dec 2014 10:57:10 +0100
From:	Stefan Agner <stefan@...er.ch>
To:	Lucas Stach <l.stach@...gutronix.de>
Cc:	chris@...ntf.net, ulf.hansson@...aro.org, anton@...msg.org,
	linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
	rmk+kernel@....linux.org.uk, shawn.guo@...aro.org,
	b29396@...escale.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio
 interrupts

On 2014-12-03 10:25, Lucas Stach wrote:
> Am Dienstag, den 02.12.2014, 17:47 +0100 schrieb Stefan Agner:
>> Enable IPG clock for sdio interrupts while runtime PM, since this
>> clock is needed for register access. The need of this clock has been
>> verified on Vybrid, but this is probably true for i.MX53 and maybe
>> others. The need for bus access during runtime suspend has been
>> introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
>> while sdhci runtime suspended").
>>
> Am I reading this wrong, or is this commit actually doing something
> different than what the commit message says?
> 
> This does _not_ enable the IPG clock during runtime PM, in fact it is
> disabling it while suspending even with SDIO ints enabled, which is
> wrong, as no SDIO interrupts will be delivered with this clock disabled.

You are completely right. What I wanted was what is written in the
commit message, but I misinterpreted the code!

=> This patch is invalid.

> One thing that seems to be wrong with the current code is that the state
> of sdhci_sdio_irq_enabled could change between a runtime suspend and
> resume, so to keep the clocks balanced the driver has to remember if it
> disabled those two clocks in the suspend path and always enable them in
> that case in the resume path.

As far as I can see, the only place where that flag changes is in
sdhci_enable_sdio_irq. But this function makes sure runtime PM is off
while changing that, so I guess it's ok....

--
Stefan

> 
>> Signed-off-by: Stefan Agner <stefan@...er.ch>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 587ee0e..b7e9ad1 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
>>
>>  	ret = sdhci_runtime_suspend_host(host);
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_disable_unprepare(imx_data->clk_per);
>> -		clk_disable_unprepare(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_disable_unprepare(imx_data->clk_ipg);
>>  	clk_disable_unprepare(imx_data->clk_ahb);
>>
>>  	return ret;
>> @@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_prepare_enable(imx_data->clk_per);
>> -		clk_prepare_enable(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_prepare_enable(imx_data->clk_ipg);
>>  	clk_prepare_enable(imx_data->clk_ahb);
>>
>>  	return sdhci_runtime_resume_host(host);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/