Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

Hi Alexey,

Apologies, I've been on vacation 2 of the last 3 weeks, but I thought
we had this all sorted.

On Thu, 16 Jul 2026 at 13:57, Alexey Klimov <[email protected]> wrote:
>
> Hi Peter,
>
> On Mon Jun 22, 2026 at 9:43 PM BST, Peter Griffin wrote:
> > Hi Alexey,
> >
> > On Mon, 22 Jun 2026 at 19:57, Alexey Klimov <[email protected]> wrote:
> > [..]
> >> >> >
> >> >> > I originally made the failure non-fatal because the system still boots
> >> >> > without the notifiers registered (and all other Arm64 Exynos SoCs
> >> >> > upstream don't register notifiers and AFAICT have broken cpu hotplug
> >> >> > and cpu idle).
> >> >> >
> >> >> > In hindsight, that seems like a mistake. I think your patch to fully
> >> >> > unwind everything in case of failure makes more sense.  See small
> >> >> > comment below about destroy_cpuhp_and_cpuidle()
> >> >>
> >> >> Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
> >> >> return any errors?
> >> >
> >> > I suggest you re-read my above comment above ^^
> >>
> >> Could you please clarify what specifically addresses my question about
> >> notifiers?
> >
> > Sure, I was referring to this part of my previous reply:
> >
> >> >> > In hindsight, that seems like a mistake. I think your patch to fully
> >> >> > unwind everything in case of failure makes more sense.
> >
> > [..]
> >>
> >> If c2 idles are used during reboot/shutdown then they fail or what?
> >
> > This followed similar logic to the Samsung downstream kernel drivers.
> > I have no extra information about it beyond the downstream kernel
> > source. It seemed reasonable though that CPU's will be hotplugged
> > during suspend and reboot so you may wish to ignore these.
> >
> > The proper solution of course is a fully PSCI compliant firmware,
> > which doesn't require these side channel hints.
>
> It seems there is some misunderstanding, where we are not getting to the
> point.

From my point of view your proposed patch looks good, as I mentioned
in my earlier replies. The only reason for not sending a reviewed-by
tag was the outstanding piece of feedback regarding only calling
destroy_cpuhp_and_cpuidle() for platforms that called
setup_cpuhp_and_cpuidle(). Currently it is only gs101 that calls
setup_cpuhp_and_cpuidle() but in your patch you call
destroy_cpuhp_and_cpuidle() unconditionally. Will that cause issues
for other SoCs?

> I think I still do not see any comment about failed notifiers
> and question about c2 idle states is kinda misdirected.
> I also don't see any answers that clarify this from previous emails:

As I understand it your proposed patch fixes all the issues you are
asking questions about? Is there some outstanding point that your
patch hasn't already addressed?

>
> >Why do we need to have notifiers (say cpu_pm_register_notifier())
> >registered if, for instance, cpuhp_setup_state() fails?
>
> Let's jump to the details then to move it forward somewhere.
>
> If one of the cpuhp_setup_state() fails and in the current sources
> (without this patch) we don't check if it succeeds or not we may not
> have properly working hotplug notifiers =>
> {set,clear}_bit(cpu, pmu_context->in_cpuhp) may not be updated
> correctly.
>
> The if-check logic in cpuidle callback:
>
>         if (test_bit(cpu, pmu_context->in_cpuhp)) {
>                 raw_spin_unlock(&pmu_context->cpupm_lock);
>                 return NOTIFY_BAD;
>         }
>
> relies on that and may not work reliably. As a result c2 idle states
> may stop working or offlined CPU may be put into c2 idle state.
>
> Going further, if register_reboot_notifier() fails and we don't
> check its return value then
>
>         /* Ignore CPU_PM_ENTER event in reboot or suspend sequence. */
>         if (pmu_context->sys_insuspend || pmu_context->sys_inreboot) {
>                 raw_spin_unlock(&pmu_context->cpupm_lock);
>                 return NOTIFY_OK;
>         }
>
> a CPU can be put or at least attempted to be put into c2 idle state.
> That's why there was a question:
>
> >> If c2 idles are used during reboot/shutdown then they fail or what?
>
> If it is fine to use c2 idle states during power off or reboot,
> then why the code blocks them during power off/reboot?
>
> >> I am not saying that patch is correct and some rework is needed but I don't
> >> get why we should completely ignore errors from hotplug states registration
> >> and should not check registration of notifiers. At least warning should be
> >> shown to user that pm functionality might be unreliable.
> >
> > As mentioned above, and in my previous reply, I think your proposed
> > patch is a good idea.
>
> You mentioned, so far as I understood your replies, that _only_
> destroy_cpuhp_and_cpuidle() is useful and setup_cpuhp_and_cpuidle()
> should be non-fatal.

I don't know how you reached that conclusion from my replies, as I
never said either of those things. The only outstanding point for me
in your patch was about calling destroy_cpuhp_and_cpuidle()
unconditionally for platforms that never called
setup_cpuhp_and_cpuidle(). I was expecting either a new version or
some justification as to why that's OK.

Peter