lkml.org 
[lkml]   [2017]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
>> Hi Krzysztof,
>>
>> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
>> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> >> > genpd_syscore_switch() had two problems:
>> >> > 1. It silently assumed that device, it is being called for, belongs to
>> >> > generic power domain and used container_of() on its power domain
>> >> > pointer. Such assumption might not be true always.
>> >> >
>> >> > 2. It iterated over list of generic power domains without holding
>> >> > gpd_list_lock mutex thus list could have been modified in the same
>> >> > time.
>> >> >
>> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
>> >> > for non-generic power domains and uses mutex when iterating.
>> >> >
>> >> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> >> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >>
>> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
>> >> request "[GIT PULL] Power management updates for v4.13-rc1".
>> >>
>> >> > Not tested on real hardware.
>> >>
>> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
>> >> where the system timer is an IRQ safe device:
>> >>
>> >> PM: Syncing filesystems ... done.
>> >> PM: Preparing system for sleep (mem)
>> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> >> OOM killer disabled.
>> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> >> PM: Suspending system (mem)
>> >> PM: suspend of devices complete after 122.841 msecs
>> >> PM: suspend devices took 0.130 seconds
>> >> PM: late suspend of devices complete after 2.682 msecs
>> >> PM: noirq suspend of devices complete after 29.951 msecs
>> >> Disabling non-boot CPUs ...
>> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
>> >
>> > Thanks for report!
>> >
>> > Damn it, although I couldn't find this in the code, but I was fearing
>> > that this ends up in atomic section. That would kind of explain why
>> > mutex was not there [1].
>> >
>> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
>> > section" there was no locking at all... In this context this was
>> > probably safe because it was executed *after* disabling non-boot CPUs
>> > but then the function cannot be called in other contexts.
>> >
>> > I am not sure I will be capable of developing the proper fix as I do not
>> > have the hardware and I do not know all stuff happening in sh suspend.
>> > Probably reverting this and living with non-locked path would be the
>> > safest choice.
>> >
>> > [1] https://patchwork.kernel.org/patch/9778903/
>>
>> AFAIU, all syscore stuff runs in atomic context.
>
> Indeed... The confusing part is that this code is syscore only from
> the name, it is not hooked in to syscore_ops. Although going by call
> chain (through sh clocksource drivers) we end up in
> timekeeping_suspend() which is a syscore op.
>
> I wonder whether it would be useful - after reverting my commit - to add
> an assert (which is a stronger API requirement than only documentation "may
> only be called during the system core (syscore) suspend") like:
> WARN_ON(num_online_cpus() > 1));
> as without mutexes this should not be executed with more than one online
> CPU.

Or maybe WARN_ON_ONCE(!in_atomic())?

I'm queuing up a revert of the $subject commit.

Thanks,
Rafael

\
 
 \ /
  Last update: 2017-07-04 21:55    [W:0.087 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site