From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754143AbcGLKG4 (ORCPT ); Tue, 12 Jul 2016 06:06:56 -0400 Received: from foss.arm.com ([217.140.101.70]:57962 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753960AbcGLKFp (ORCPT ); Tue, 12 Jul 2016 06:05:45 -0400 Message-ID: <1468317930.10213.258.camel@arm.com> Subject: Re: [patch 22/66] bus: arm-ccn: convert to hotplug statemachine From: Pawel Moll To: Anna-Maria Gleixner , LKML Cc: Peter Zijlstra , Ingo Molnar , Sebastian Andrzej Siewior Date: Tue, 12 Jul 2016 11:05:30 +0100 In-Reply-To: <20160711122532.465002883@linutronix.de> References: <20160711122450.923603742@linutronix.de> <20160711122532.465002883@linutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5-1ubuntu3.1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2016-07-11 at 12:28 +0000, Anna-Maria Gleixner wrote: > From: Sebastian Andrzej Siewior > > Install the callbacks via the state machine and let the core invoke > the callbacks on the already online CPUs. > > Signed-off-by: Sebastian Andrzej Siewior > Cc: Pawel Moll > Signed-off-by: Anna-Maria Gleixner Although I won't shed a tear over the notifiers going, there's a problem with this patch... > --- > drivers/bus/arm-ccn.c | 47 ++++++++++++++++++++-------------- > ----------- > include/linux/cpuhotplug.h | 2 + > 2 files changed, 23 insertions(+), 26 deletions(-) > > --- a/drivers/bus/arm-ccn.c > +++ b/drivers/bus/arm-ccn.c > @@ -167,7 +167,6 @@ struct arm_ccn_dt { > struct hrtimer hrtimer; > > cpumask_t cpu; > - struct notifier_block cpu_nb; > > struct pmu pmu; > }; Notice that here each instance of CCN (unlikely as it is today, the code was written with the assumption that there's more than one interconnect ring in the system) get its own notifier block... > @@ -1171,30 +1170,23 @@ static enum hrtimer_restart arm_ccn_pmu_ > } > > > -static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb, > - unsigned long action, void *hcpu) > +static struct arm_ccn_dt *cpuhp_armccn_dt; > +static int arm_ccn_pmu_offline_cpu(unsigned int cpu) > { > - struct arm_ccn_dt *dt = container_of(nb, struct arm_ccn_dt, > cpu_nb); > + struct arm_ccn_dt *dt = cpuhp_armccn_dt; > struct arm_ccn *ccn = container_of(dt, struct arm_ccn, dt); > - unsigned int cpu = (long)hcpu; /* for (long) see ... but here (and in all the rest of this change) it's replaced by a static pointer to a single instance. > @@ -1270,9 +1262,10 @@ static int arm_ccn_pmu_init(struct arm_c > * ... and change the selection when it goes offline. > Priority is > * picked to have a chance to migrate events before perf is > notified. > */ > - ccn->dt.cpu_nb.notifier_call = arm_ccn_pmu_cpu_notifier; > - ccn->dt.cpu_nb.priority = CPU_PRI_PERF + 1, > - err = register_cpu_notifier(&ccn->dt.cpu_nb); > + cpuhp_armccn_dt = &ccn->dt; Even without checking if the pointer has been already set. > + err = cpuhp_setup_state(CPUHP_AP_PERF_ARM_CCN_ONLINE, > + "AP_PERF_ARM_CCN_ONLINE", NULL, > + arm_ccn_pmu_offline_cpu); > if (err) > goto error_cpu_notifier; > > @@ -1293,7 +1286,8 @@ static int arm_ccn_pmu_init(struct arm_c > > error_pmu_register: > error_set_affinity: > - unregister_cpu_notifier(&ccn->dt.cpu_nb); > + cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE); > + cpuhp_armccn_dt = NULL; error_cpu_notifier: > ida_simple_remove(&arm_ccn_pmu_ida, ccn->dt.id); > for (i = 0; i < ccn->num_xps; i++) > @@ -1308,7 +1302,8 @@ static void arm_ccn_pmu_cleanup(struct a > > if (ccn->irq) > irq_set_affinity_hint(ccn->irq, NULL); > - unregister_cpu_notifier(&ccn->dt.cpu_nb); > + cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE); > + cpuhp_armccn_dt = NULL; Same (only the other way round) here... > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -30,6 +30,7 @@ enum cpuhp_state { > CPUHP_AP_PERF_X86_AMD_IBS_STARTING, > CPUHP_AP_PERF_X86_CQM_STARTING, > CPUHP_AP_PERF_X86_CSTATE_STARTING, > + CPUHP_AP_PERF_XTENSA_STARTING, > CPUHP_AP_NOTIFY_STARTING, > CPUHP_AP_ONLINE, > CPUHP_TEARDOWN_CPU, That chunk does not belong here, does it? Regards Pawel