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

[Peter Griffin]

Hi Alexey,

Thanks for your analysis on this issue.

On Wed, 15 Jul 2026 at 22:35, Alexey Klimov <alexey.klimov@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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@xxxxxxxxxx?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 agree it seems inconsistent. Since hints are always programmed when
entering a power state, clearing the hint is probably not really
required.

>
> 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.

I agree, let's drop this patch temporarily and continue to follow up
with Samsung folks. Hopefully we can get clarification or confirmation
from someone who has access to the firmware code. If/when revisiting,
I think some of the analysis from this email could probably be added
to create a more verbose commit message or possibly a comment to
clearly explain why upstream deviates from the downstream
implementation. Of course, the best solution would be a fully
compliant PSCI firmware, which doesn't require any hints.

Thanks,

Peter.

>
> BR,
> Alexey.
>
> >> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> >> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@xxxxxxxxxx?part=3
> >> Fixes: 598995027b91 ("soc: samsung: exynos-pmu: enable CPU hotplug support for gs101")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Alexey Klimov <alexey.klimov@xxxxxxxxxx>
> >> ---
> >>  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
> >>
>