Re: [GIT PULL] csky changes for v5.12-rc1

[Guo Ren <guoren@kernel.org>]

Hi all,

On Mon, Mar 1, 2021 at 4:36 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So this is entirely unrelated to the csky pull request, and is more of
> a generic "the perf CPU hotplug thing seems a complete mess".
>
> The csky thing is just the latest - of many - that have been bitten by
> the mess, and the one that added yet another hotplug state, and
> finally made me go "Let's at least talk about this"
>
> For csky, the problem is this:
>
> On Sat, Feb 27, 2021 at 7:43 PM <guoren@kernel.org> wrote:
> >
> > arch/csky patches for 5.12-rc1
> >
> >  - Fixup perf probe failed
>
> and in this case it is 398cb92495cc ("csky: Fixup perf probe failed")
> in my current -git tree.
>
> But it's also
>
>     cf6acb8bdb1d ("s390/cpumf: Add support for complete counter set extraction")
>     dcb5cdf60a1f ("powerpc/perf/hv-gpci: Add cpu hotplug support")
>     1a8f0886a600 ("powerpc/perf/hv-24x7: Add cpu hotplug support")
>     6b7ce8927b5a ("irqchip: RISC-V per-HART local interrupt controller driver")
>     e9b880581d55 ("coresight: cti: Add CPU Hotplug handling to CTI driver")
>     e0685fa228fd ("arm64: Retrieve stolen time as paravirtualized guest")
>     6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority
> over ARM arch timer")
>     78f4e932f776 ("x86/microcode, cpuhotplug: Add a microcode loader
> CPU hotplug callback")
>     72c69dcddce1 ("powerpc/perf: Trace imc events detection and cpuhotplug")
>     5861381d4866 ("PM / arch: x86: Rework the
> MSR_IA32_ENERGY_PERF_BIAS handling")
>     69c32972d593 ("drivers/perf: Add Cavium ThunderX2 SoC UNCORE PMU driver")
>     ...
>
> and that's not even the complete list.
>
> Does it really make sense to have this kind of silly enumeration of
> many (MANY!) different arch CPU hotplug state indexes, where most of
> them are relevant only to that particular architecture..
>
> No, I don't think this is a _problem_, but it's kind of ugly, wouldn't
> you agree?
>
> Wouldn't it be better to just reserve N different states for the
> architecture hotplug state, and then let each architecture decide how
> they want to order them?
>
> Or better yet, make at least some of them architecture-neutral.
> Because now there are drivers that clearly are very tied to one
> architecture - or SoCs (look at various timer things) - do they really
> want or need their own architecture- or SoC-specific hotplug state?
> IOW, do we really need all of these:
>
>         CPUHP_AP_ARM_ARCH_TIMER_STARTING,
>         CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
>         CPUHP_AP_JCORE_TIMER_STARTING,
>         CPUHP_AP_QCOM_TIMER_STARTING,
>         CPUHP_AP_TEGRA_TIMER_STARTING,
>         CPUHP_AP_ARMADA_TIMER_STARTING,
>         CPUHP_AP_MARCO_TIMER_STARTING,
>         CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>         CPUHP_AP_ARC_TIMER_STARTING,
>         CPUHP_AP_RISCV_TIMER_STARTING,
>         CPUHP_AP_CLINT_TIMER_STARTING,
>         CPUHP_AP_CSKY_TIMER_STARTING,
>         CPUHP_AP_HYPERV_TIMER_STARTING,
>         CPUHP_AP_KVM_ARM_TIMER_STARTING,
>         CPUHP_AP_DUMMY_TIMER_STARTING,
>
> as separate hotplug events?
>
> Whatever. I don't really care deeply, but this just smells a bit to me.
>
> Comments?

We could use CPUHP_AP_ONLINE_DYN to reduce most of the above.

Here is the example of csky:

diff --git a/arch/csky/kernel/perf_event.c b/arch/csky/kernel/perf_event.c
index e5f1842..ccc27c3 100644
--- a/arch/csky/kernel/perf_event.c
+++ b/arch/csky/kernel/perf_event.c
@@ -1319,10 +1319,10 @@ int csky_pmu_device_probe(struct platform_device *pdev,
                pr_notice("[perf] PMU request irq fail!\n");
        }

-       ret = cpuhp_setup_state(CPUHP_AP_PERF_CSKY_ONLINE, "AP_PERF_ONLINE",
+       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"arch/csky/perf_event:starting",
                                csky_pmu_starting_cpu,
                                csky_pmu_dying_cpu);
-       if (ret) {
+       if (ret < 0) {
                csky_pmu_free_irq();
                free_percpu(csky_pmu.hw_events);
                return ret;
diff --git a/drivers/clocksource/timer-mp-csky.c
b/drivers/clocksource/timer-mp-csky.c
index 183a995..fc17d77 100644
--- a/drivers/clocksource/timer-mp-csky.c
+++ b/drivers/clocksource/timer-mp-csky.c
@@ -151,11 +151,11 @@ static int __init csky_mptimer_init(struct
device_node *np)
        clocksource_register_hz(&csky_clocksource, timer_of_rate(to));
        sched_clock_register(sched_clock_read, 32, timer_of_rate(to));

-       ret = cpuhp_setup_state(CPUHP_AP_CSKY_TIMER_STARTING,
+       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                                "clockevents/csky/timer:starting",
                                csky_mptimer_starting_cpu,
                                csky_mptimer_dying_cpu);
-       if (ret)
+       if (ret < 0)
                return -EINVAL;

        return 0;
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index f14adb8..5abcfda 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -134,7 +134,6 @@ enum cpuhp_state {
        CPUHP_AP_ARC_TIMER_STARTING,
        CPUHP_AP_RISCV_TIMER_STARTING,
        CPUHP_AP_CLINT_TIMER_STARTING,
-       CPUHP_AP_CSKY_TIMER_STARTING,
        CPUHP_AP_HYPERV_TIMER_STARTING,
        CPUHP_AP_KVM_STARTING,
        CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
@@ -186,7 +185,6 @@ enum cpuhp_state {
        CPUHP_AP_PERF_POWERPC_TRACE_IMC_ONLINE,
        CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
        CPUHP_AP_PERF_POWERPC_HV_GPCI_ONLINE,
-       CPUHP_AP_PERF_CSKY_ONLINE,
        CPUHP_AP_WATCHDOG_ONLINE,
        CPUHP_AP_WORKQUEUE_ONLINE,
        CPUHP_AP_RCUTREE_ONLINE,


--
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/