On Tue, Jul 4, 2017 at 10:20 PM, Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxxx> 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
