linux-kernel - Re: [PATCH] sdhci: wakeup from runtime PM

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 7 Apr 2016 11:11:08 +0200
From:	Ulf Hansson <ulf.hansson@...aro.org>
To:	Ludovic Desroches <ludovic.desroches@...el.com>,
	Adrian Hunter <adrian.hunter@...el.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Nicolas Ferre <nicolas.ferre@...el.com>
Subject: Re: [PATCH] sdhci: wakeup from runtime PM

On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@...el.com> wrote:
> On 25/03/16 18:05, Ludovic Desroches wrote:
>> Hi,
>>
>> When not using the SDHCI controller, it is logical to save power by suspending
>> it. The issue is that SDHCI assumes that the controller is completely disabled.
>> It means the only way to wake up on a card event is to have a gpio for the card
>> detection (so two pins for the same signal). A possible workaround is to use
>> polling but the controller will be resumed/suspended between each attempts.
>>
>> We have already discussed a long time about this and it seems we don't agree.
>> In my opinion, even if I can't disable all clocks, I should use runtime PM
>> to save some power.
>>
>> I propose two patches, one which is a draft to try to solve it at sdhci level
>> and one at sdhci-of-at91 level.
>>
>> Concerning the first one, I don't understand why we need to reject irqs if
>> runtime_suspended is true.
>
> The interrupt handler might be called because the interrupt is shared i.e.
> the interrupt is for a different device.  In that case the host controller
> might be off and the registers inaccessible.  In that case we cannot even
> look at the interrupt register to determine if we were expecting the interrupt.
>
>>                            Only SDHCI_INT_CARD_INT irq is enabled so why we
>> should have other IRQs than this one?
>
> In the case of SDIO Card interrupt, it is delivered via the host controller,
> so we have to assume the registers are accessible if we are
> runtime-suspended with the SDIO IRQ enabled.
>
>>
>> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
>> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
>> solved my issue only by modifying my driver.
>
> I don't mind allowing card detect interrupts while runtime suspended, but we
> need a flag so that:
> - runtime suspend leaves the insert/remove interrupts enabled
> - irq handler knows it can access registers

To me, this seem like the wrong way of how to configure wake-ups for
these kind of devices.

I don't think the regular IRQs shall be enabled and the driver
shouldn't assume the registers are accessible without first runtime
resuming the device.

> - irq thread handler knows to runtime resume before doing anything else
>
> But it seems like you need to persuade Ulf first.

For a more thorough explanation to why I don't like, please have a
look at my comment for another related thread on the mmc list.
http://www.spinics.net/lists/linux-mmc/msg36132.html

Kind regards
Uffe