From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752534AbdGDUG2 (ORCPT ); Tue, 4 Jul 2017 16:06:28 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34144 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752302AbdGDUGB (ORCPT ); Tue, 4 Jul 2017 16:06:01 -0400 Date: Tue, 4 Jul 2017 22:05:57 +0200 From: Krzysztof Kozlowski To: "Rafael J. Wysocki" Cc: Geert Uytterhoeven , "Rafael J. Wysocki" , Kevin Hilman , Ulf Hansson , Len Brown , Pavel Machek , Greg Kroah-Hartman , Linux PM list , "linux-kernel@vger.kernel.org" , Linux-Renesas Subject: Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device Message-ID: <20170704200557.h5yrsoega5ai46mt@kozik-lap> References: <20170628145623.20716-1-krzk@kernel.org> <20170628145623.20716-3-krzk@kernel.org> <20170704181011.zcialijxnyucwzry@kozik-lap> <20170704183653.5o5wllbdbloirvr2@kozik-lap> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 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 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 > >> >> > Signed-off-by: Krzysztof Kozlowski > >> >> > Acked-by: Ulf Hansson > >> >> > >> >> 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