Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680964 > unrolled thread

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch()  call on non-genpd device Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-04 15:10 +0200
    Re: [PATCH v3 2/8] PM / Domains: Handle safely  genpd_syscore_switch() call on non-genpd device Krzysztof Kozlowski <krzk@kernel.org> - 2017-07-04 20:20 +0200
      Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch()  call on non-genpd device Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-04 20:30 +0200
        Re: [PATCH v3 2/8] PM / Domains: Handle safely  genpd_syscore_switch() call on non-genpd device Krzysztof Kozlowski <krzk@kernel.org> - 2017-07-04 20:40 +0200
          Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch()  call on non-genpd device "Rafael J. Wysocki" <rafael@kernel.org> - 2017-07-04 22:00 +0200
            Re: [PATCH v3 2/8] PM / Domains: Handle safely  genpd_syscore_switch() call on non-genpd device Krzysztof Kozlowski <krzk@kernel.org> - 2017-07-04 22:10 +0200
              Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch()  call on non-genpd device "Rafael J. Wysocki" <rafael@kernel.org> - 2017-07-04 22:20 +0200
                Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch()  call on non-genpd device "Rafael J. Wysocki" <rafael@kernel.org> - 2017-07-04 22:30 +0200
                Re: [PATCH v3 2/8] PM / Domains: Handle safely  genpd_syscore_switch() call on non-genpd device Krzysztof Kozlowski <krzk@kernel.org> - 2017-07-04 22:30 +0200

#1680964 — Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

Hi Krzysztof, Rafael,

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
in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
CPU: 0 PID: 1730 Comm: s2ram Not tainted
4.12.0-koelsch-07061-g810fee9afeba15ef #3592
Hardware name: Generic R8A7791 (Flattened Device Tree)
[<c020e9f4>] (unwind_backtrace) from [<c020a484>] (show_stack+0x10/0x14)
[<c020a484>] (show_stack) from [<c04017e8>] (dump_stack+0x7c/0x9c)
[<c04017e8>] (dump_stack) from [<c0240284>] (___might_sleep+0x124/0x160)
[<c0240284>] (___might_sleep) from [<c0717cfc>] (mutex_lock+0x18/0x60)
[<c0717cfc>] (mutex_lock) from [<c04de11c>] (genpd_lookup_dev+0x38/0x94)
[<c04de11c>] (genpd_lookup_dev) from [<c04dfd34>]
(pm_genpd_syscore_poweroff+0x8/0x2c)
[<c04dfd34>] (pm_genpd_syscore_poweroff) from [<c05fcb70>]
(sh_cmt_clock_event_suspend+0x18/0x28)
[<c05fcb70>] (sh_cmt_clock_event_suspend) from [<c027f174>]
(clockevents_suspend+0x40/0x54)
[<c027f174>] (clockevents_suspend) from [<c02762d8>]
(timekeeping_suspend+0x23c/0x278)
[<c02762d8>] (timekeeping_suspend) from [<c04ce028>]
(syscore_suspend+0x88/0x138)
[<c04ce028>] (syscore_suspend) from [<c025d740>]
(suspend_devices_and_enter+0x308/0x4fc)
[<c025d740>] (suspend_devices_and_enter) from [<c025db5c>]
(pm_suspend+0x228/0x280)
[<c025db5c>] (pm_suspend) from [<c025c6b8>] (state_store+0xac/0xcc)
[<c025c6b8>] (state_store) from [<c0342f9c>] (kernfs_fop_write+0x170/0x1b0)
[<c0342f9c>] (kernfs_fop_write) from [<c02e47dc>] (__vfs_write+0x20/0x108)
[<c02e47dc>] (__vfs_write) from [<c02e5b80>] (vfs_write+0xb8/0x144)
[<c02e5b80>] (vfs_write) from [<c02e6804>] (SyS_write+0x40/0x80)
[<c02e6804>] (SyS_write) from [<c0206c40>] (ret_fast_syscall+0x0/0x34)

Reverting the commit fixes the issue.

> ---
>  drivers/base/power/domain.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 01e31d9f6c94..d31a4434b8b3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
>  {
>         struct generic_pm_domain *genpd;
>
> -       genpd = dev_to_genpd(dev);
> -       if (!pm_genpd_present(genpd))
> +       genpd = genpd_lookup_dev(dev);
> +       if (!genpd)
>                 return;
>
>         if (suspend) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [next] | [standalone]

#1681113 — Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof, Rafael,
> 
> 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

Hi,

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/

Best regards,
Krzysztof


> in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
> CPU: 0 PID: 1730 Comm: s2ram Not tainted
> 4.12.0-koelsch-07061-g810fee9afeba15ef #3592
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> [<c020e9f4>] (unwind_backtrace) from [<c020a484>] (show_stack+0x10/0x14)
> [<c020a484>] (show_stack) from [<c04017e8>] (dump_stack+0x7c/0x9c)
> [<c04017e8>] (dump_stack) from [<c0240284>] (___might_sleep+0x124/0x160)
> [<c0240284>] (___might_sleep) from [<c0717cfc>] (mutex_lock+0x18/0x60)
> [<c0717cfc>] (mutex_lock) from [<c04de11c>] (genpd_lookup_dev+0x38/0x94)
> [<c04de11c>] (genpd_lookup_dev) from [<c04dfd34>]
> (pm_genpd_syscore_poweroff+0x8/0x2c)
> [<c04dfd34>] (pm_genpd_syscore_poweroff) from [<c05fcb70>]
> (sh_cmt_clock_event_suspend+0x18/0x28)
> [<c05fcb70>] (sh_cmt_clock_event_suspend) from [<c027f174>]
> (clockevents_suspend+0x40/0x54)
> [<c027f174>] (clockevents_suspend) from [<c02762d8>]
> (timekeeping_suspend+0x23c/0x278)
> [<c02762d8>] (timekeeping_suspend) from [<c04ce028>]
> (syscore_suspend+0x88/0x138)
> [<c04ce028>] (syscore_suspend) from [<c025d740>]
> (suspend_devices_and_enter+0x308/0x4fc)
> [<c025d740>] (suspend_devices_and_enter) from [<c025db5c>]
> (pm_suspend+0x228/0x280)
> [<c025db5c>] (pm_suspend) from [<c025c6b8>] (state_store+0xac/0xcc)
> [<c025c6b8>] (state_store) from [<c0342f9c>] (kernfs_fop_write+0x170/0x1b0)
> [<c0342f9c>] (kernfs_fop_write) from [<c02e47dc>] (__vfs_write+0x20/0x108)
> [<c02e47dc>] (__vfs_write) from [<c02e5b80>] (vfs_write+0xb8/0x144)
> [<c02e5b80>] (vfs_write) from [<c02e6804>] (SyS_write+0x40/0x80)
> [<c02e6804>] (SyS_write) from [<c0206c40>] (ret_fast_syscall+0x0/0x34)
> 
> Reverting the commit fixes the issue.
> 
> > ---
> >  drivers/base/power/domain.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 01e31d9f6c94..d31a4434b8b3 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
> >  {
> >         struct generic_pm_domain *genpd;
> >
> > -       genpd = dev_to_genpd(dev);
> > -       if (!pm_genpd_present(genpd))
> > +       genpd = genpd_lookup_dev(dev);
> > +       if (!genpd)
> >                 return;
> >
> >         if (suspend) {
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1681119

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.

Don't worry, you're not the only one.
This bug report was almost 100% the same as an earlier one for a patch
from Ulf ;-)
(cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
*noirq() callbacks")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1681121 — Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

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.

Best regards,
Krzysztof

> 
> Don't worry, you're not the only one.
> This bug report was almost 100% the same as an earlier one for a patch
> from Ulf ;-)
> (cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
> *noirq() callbacks")
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1681156

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

[toc] | [prev] | [next] | [standalone]

#1681164 — Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote:
> 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())?

You could be in atomic section on this CPU and still have other CPUs
online playing with gpd_list (without any protection from locking).
This function is safe only on non-SMP case.

Best regards,
Krzysztof
 
> I'm queuing up a revert of the $subject commit.
> 
> Thanks,
> Rafael

[toc] | [prev] | [next] | [standalone]

#1681169

On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote:
>> 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())?
>
> You could be in atomic section on this CPU and still have other CPUs
> online playing with gpd_list (without any protection from locking).
> This function is safe only on non-SMP case.

Well, not quite.

It is safe if you can guarantee that no other CPUs will touch the data
structure in question concurrently, which pretty much is the case for
timekeeping_suspend() even though it may be invoked without taking the
other CPUs offline (from the suspend-to-idle core path).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1681171

On Tue, Jul 4, 2017 at 10:20 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>  >> >> > 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())?
>> >
>> > You could be in atomic section on this CPU and still have other CPUs
>> > online playing with gpd_list (without any protection from locking).
>> > This function is safe only on non-SMP case.
>>
>> Well, not quite.
>>
>> It is safe if you can guarantee that no other CPUs will touch the data
>> structure in question concurrently, which pretty much is the case for
>> timekeeping_suspend() even though it may be invoked without taking the
>> other CPUs offline (from the suspend-to-idle core path).
>
> Right, that would work fine for that case.
>
> However I was rather thinking that we have an in-kernel API (exported)
> so someone might by mistake try to use it in different contexts. For
> example in some atomic section but on a platform which offlines CPUs
> later. Thus it would be called in some imaginary suspend path but with
> CPUs still being online. Partially it is already mentioned in documentation
> although I am not sure that on every possible architecture syscore ops
> are called after disabling non-boot CPUs...

Yes, they are.  Nonboot CPUs are disabled by the core.

Anyway, while I see your point, it would be rather hard to find an assertion
that would also work for the suspend-to-idle timekeeping_suspend() invocation
case.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1681174 — Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
 >> >> > 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())?
> >
> > You could be in atomic section on this CPU and still have other CPUs
> > online playing with gpd_list (without any protection from locking).
> > This function is safe only on non-SMP case.
> 
> Well, not quite.
> 
> It is safe if you can guarantee that no other CPUs will touch the data
> structure in question concurrently, which pretty much is the case for
> timekeeping_suspend() even though it may be invoked without taking the
> other CPUs offline (from the suspend-to-idle core path).

Right, that would work fine for that case.

However I was rather thinking that we have an in-kernel API (exported)
so someone might by mistake try to use it in different contexts. For
example in some atomic section but on a platform which offlines CPUs
later. Thus it would be called in some imaginary suspend path but with
CPUs still being online. Partially it is already mentioned in documentation
although I am not sure that on every possible architecture syscore ops
are called after disabling non-boot CPUs...

And anyway for in-kernel API having a explicit assert is a stronger way
of documenting requirement than a comment.

Best regards,
Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web