From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752284AbdGDUUU (ORCPT ); Tue, 4 Jul 2017 16:20:20 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:33177 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752206AbdGDUUS (ORCPT ); Tue, 4 Jul 2017 16:20:18 -0400 Date: Tue, 4 Jul 2017 22:20:14 +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: <20170704202014.fkpeahwyfddepens@kozik-lap> References: <20170628145623.20716-1-krzk@kernel.org> <20170628145623.20716-3-krzk@kernel.org> <20170704181011.zcialijxnyucwzry@kozik-lap> <20170704183653.5o5wllbdbloirvr2@kozik-lap> <20170704200557.h5yrsoega5ai46mt@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 10:12:13PM +0200, Rafael J. Wysocki wrote: > On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski 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