linux-kernel - Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm status around suspend/resume

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:	Fri, 15 Nov 2013 07:30:13 -0600
From:	Nishanth Menon <nm@...com>
To:	Paul Walmsley <paul@...an.com>
CC:	Tony Lindgren <tony@...mide.com>, <rnayak@...com>,
	<khilman@...aro.org>, <balbi@...com>, <linux-omap@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3] ARM: OMAP2+: omap_device: maintain sane runtime pm
 status around suspend/resume

On 11/15/2013 02:07 AM, Paul Walmsley wrote:
> On Thu, 14 Nov 2013, Nishanth Menon wrote:
> 
>> OMAP device hooks around suspend|resume_noirq ensures that hwmod
>> devices are forced to idle using omap_device_idle/enable as part of
>> the last stage of suspend activity.
>>
>> For a device such as i2c who uses autosuspend, it is possible to enter
>> the suspend path with dev->power.runtime_status = RPM_ACTIVE.
>>
>> As part of the suspend flow, the generic runtime logic would increment
>> it's dev->power.disable_depth to 1. This should prevent further
>> pm_runtime_get_sync from succeeding once the runtime_status has been
>> set to RPM_SUSPENDED.
>>
>> Now, as part of the suspend_noirq handler in omap_device, we force the
>> following: if the device status is !suspended, we force the device
>> to idle using omap_device_idle (clocks are cut etc..). This ensures
>> that from a hardware perspective, the device is "suspended". However,
>> runtime_status is left to be active.
>>
>> *if* an operation is attempted after this point to
>> pm_runtime_get_sync, runtime framework depends on runtime_status to
>> indicate accurately the device status, and since it sees it to be
>> ACTIVE, it assumes the module is functional and returns a non-error
>> value. As a result the user will see pm_runtime_get succeed, however a
>> register access will crash due to the lack of clocks.
>>
>> To prevent this from happening, we should ensure that runtime_status
>> exactly indicates the device status. As a result of this change
>> any further calls to pm_runtime_get* would return -EACCES (since
>> disable_depth is 1). On resume, we restore the clocks and runtime
>> status exactly as we suspended with. These operations are not expected
>> to fail as we update the states after the core runtime framework has
>> suspended itself and restore before the core runtime framework has
>> resumed.
>>
>> Reported-by: J Keerthy <j-keerthy@...com>
>> Signed-off-by: Nishanth Menon <nm@...com>
>> Acked-by: Rajendra Nayak <rnayak@...com>
>> Acked-by: Kevin Hilman <khilman@...aro.org>
> 
> Looks reasonable to me.  Looks like this should be considered for -stable 
> - Nishanth, what do you think?

Every product kernel since 3.4 needed to be hacked (we have hacked in
different ways so far) to work around this (since we never spend time
digging deeper :( ), So, I do agree with your view that a -stable tag
will be most beneficial.

> 
> Tony or Kevin, do you want to take this one, or want me to?
> 
> 
> - Paul
> 


-- 
Regards,
Nishanth Menon
--
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/