Re: [PATCH 1/3] soc: samsung: exynos-pmu: use target cpu ID in hotplug callbacks

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Wed Jun 10, 2026 at 10:55 AM BST, Peter Griffin wrote:
> Hi Alexey,
>
> Thanks for your patch.
>
> On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>>
>> The CPU hotplug state callbacks __gs101_cpu_pmu_online() and
>> __gs101_cpu_pmu_offline() currently partially use smp_processor_id() to
>> determine the target register offset for the CPU inform hints. This may
>> be fine for cpuidle flow but broken for cpu hotplug where the target
>> cpu is passed as an argument and could be different from cpu where
>> that is executing (e.g. CPU 0 offlining CPU 1), meaning that
>> smp_processor_id() returns the id of local CPU but hotplug flow
>> deals with another CPU core undergoing the transition.
>
> This was intentional. The powermode hint is always programmed based on
> the currently executing CPU core in the gs101 downstream code (for
> both CPU Idle and CPU hotplug paths). See
> https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/drivers/soc/google/cal-if/pmucal_powermode.c#15
> and the pmu_intr_gen is done based on the actual CPU being
> enabled/disabled.

Beware of the Downstream bearing gifts.
Dowstream may or may not contain mistakes or maybe they have something
that workarounds potential problems. I agree that it is a good source
of reference but I doubt that we must use it as an ultimate GOAT reference
source.

Even Qualcomm downstream things have mistakes sometimes.

> It's possible Exynos850 requires something different. I suggest
> checking the equivalent function in the e850 downstream kernel.
>
>>
>> This causes the pmu driver to write power down and power on configuration
>> hints to the wrong hardware registers, messing up the power state of active
>> cores and failing to configure the target core. Fix this by removing the
>> cpuhint variable entirely and utilizing the target 'cpu' argument passed
>> to the callbacks by the hotplug core infrastructure.
>
> Unfortunately I think you're introducing the bug you describe with this patch.

I wouldn't be so quick judging this like this. I think this patch actually
does fixes a problem. IIRC I also tested this and there were no issues.

Considering that neither your team nor me have access to the implementation
details it would be easier to just drop it instead of endlessly debating
over this if there is no interest narrowing it down.
FWIW this was reported by Sashiko in the first place so I couldn't say what
justification can be provided.
https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3

Let's go over details. Idle callbacks are executed on target CPUs so
target CPU is the same as local CPU.
The hotplug callbacks are slightly different. Offline CPU callback executes
on target CPU so again the local CPU is equal to target CPU (CPU offlines
itself). The online hotplug callback stands out. It is executed on
some CPU and deal with target CPU, that some CPU is not the same as target CPU
as system assumes that CPU undergoing online transition is not ready yet.

All these callbacks write to CPUx_INFORM register (where x -- is CPU id).
The number of these regs are equal to the number of CPUs. I think these
are per-CPU registers.

Idle callbacks and hotplug offline callback write cpu power state hint
to the local CPU's cpu_inform register. On online callback, the random
CPUx_INFORM register is chosen and its state is cleared (because online
callback can be executed on random available CPU). Does it make any sense
to you? Why among 4 callbacks one of them updates random CPUx_INFORM
register?

I think these callbacks should deal with target CPU undergoing transition
and not with random CPU id.

If I consumed enough coffee and can still trust my eyes, I tested two
configurations:
-- this commit (and similar change for e850);
-- change where CPU_INFORM_CLEAR write is removed from online callback.

On e850 (with consecutive series) and gs101. All were fine.

My understanding from experiments is that:
-- clears of CPUx_INFORM are less important and maybe not needed at all;
-- important thing is the value in correct CPUx_INFORM when CPU goes
into a power state, upon executing psci call.

The potential race doesn't reproduce presumably because hotplug ops
are fully serialized.

The downstream probably uses smp_processor_id() for cluster power
down and up states. Perhaps, cluster offline callbacks don't have CPU id
for transition into CPD state and they are executed on the last CPU from
cluster undergoing power state transition.

Anyhow, I think it will less compilcated to remove this change rather
than speculate without a way to confirm the behaviour.

BR,
Alexey.

>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
>> Fixes: 598995027b91 ("soc: samsung: exynos-pmu: enable CPU hotplug support for gs101")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  drivers/soc/samsung/exynos-pmu.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>> index d58376c38179..6e635872247a 100644
>> --- a/drivers/soc/samsung/exynos-pmu.c
>> +++ b/drivers/soc/samsung/exynos-pmu.c
>> @@ -235,11 +235,10 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>>  static int __gs101_cpu_pmu_online(unsigned int cpu)
>>         __must_hold(&pmu_context->cpupm_lock)
>>  {
>> -       unsigned int cpuhint = smp_processor_id();
>>         u32 reg, mask;
>>
>>         /* clear cpu inform hint */
>> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
>> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
>>                      CPU_INFORM_CLEAR);
>>
>>         mask = BIT(cpu);
>> @@ -296,12 +295,10 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
>>  static int __gs101_cpu_pmu_offline(unsigned int cpu)
>>         __must_hold(&pmu_context->cpupm_lock)
>>  {
>> -       unsigned int cpuhint = smp_processor_id();
>>         u32 reg, mask;
>>
>>         /* set cpu inform hint */
>> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
>> -                    CPU_INFORM_C2);
>> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu), CPU_INFORM_C2);
>>
>>         mask = BIT(cpu);
>>         regmap_update_bits(pmu_context->pmuintrgen, GS101_GRP2_INTR_BID_ENABLE,
>>
>> --
>> 2.51.0
>>