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

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

>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 still think there are some needed error paths for
setup_cpuhp_and_cpuidle() as described above.

Best regards,
Alexey.