Re: [patch 22/66] bus: arm-ccn: convert to hotplug statemachine

[Pawel Moll]

On Mon, 2016-07-11 at 12:28 +0000, Anna-Maria Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> 
> 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 <bigeasy@xxxxxxxxxxxxx>
> Cc: Pawel Moll <pawel.moll@xxxxxxx>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@xxxxxxxxxxxxx>

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