Fix Arm system PMU hotplug issues

The arm-cci probe logic faces a cyclic dependency wherein it has to pick a valid CPU to associate with before registering the PMU device, has to have the PMU state initialised before handling hotplug events in case it must be migrated, but has to have the hotplug notifier registered before the chosen CPU may go offline lest things get out of sync. The present code has tried to solve the races by using get_cpu() to pick the current CPU and prevent it from disappearing while the other two registrations are performed, but that results in taking mutexes with preemption disabled, which makes certain configurations very unhappy:

[ 1.983337] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:2004
[ 1.983340] in_atomic(): 1, irqs_disabled(): 0, pid: 1, name: swapper/0
[ 1.983342] Preemption disabled at:
[ 1.983353] [<ffffff80089801f4>] cci_pmu_probe+0x1dc/0x488
[ 1.983360] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.20-rt8-yocto-preempt-rt #1
[ 1.983362] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
[ 1.983364] Call trace:
[ 1.983369] dump_backtrace+0x0/0x158
[ 1.983372] show_stack+0x24/0x30
[ 1.983378] dump_stack+0x80/0xa4
[ 1.983383] ___might_sleep+0x138/0x160
[ 1.983386] __might_sleep+0x58/0x90
[ 1.983391] __rt_mutex_lock_state+0x30/0xc0
[ 1.983395] _mutex_lock+0x24/0x30
[ 1.983400] perf_pmu_register+0x2c/0x388
[ 1.983404] cci_pmu_probe+0x2bc/0x488
[ 1.983409] platform_drv_probe+0x58/0xa8

However, we don't actually mind being preempted or migrated at this point; all that really matters is that whichever CPU we pick does not get offlined before we're done. Thus, do the robust thing and instead take the lock to inhibit CPU hotplug for the duration. This also revealed an additional race in assigning the global pointer too late relative to the hotplug notifier, so that gets fixed in the process.

Reported-by "Li, Meng" <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-cci.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

drivers/perf/arm-cci.c +12 -10

index 1bfeb160c5b1..f6d9df07ec9b 100644
@@ -1692,21 +1692,23 @@ static int cci_pmu_probe(struct platform_device *pdev)
 	raw_spin_lock_init(&cci_pmu->hw_events.pmu_lock);
 	mutex_init(&cci_pmu->reserve_mutex);
 	atomic_set(&cci_pmu->active_events, 0);
-	cci_pmu->cpu = get_cpu();
+
+	cpus_read_lock();
+	cci_pmu->cpu = smp_processor_id();
 
 	ret = cci_pmu_init(cci_pmu, pdev);
-	if (ret) {
-		put_cpu();
-		return ret;
-	}
+	if (ret)
+		goto out;
 
-	cpuhp_setup_state_nocalls(CPUHP_AP_PERF_ARM_CCI_ONLINE,
-				  "perf/arm/cci:online", NULL,
-				  cci_pmu_offline_cpu);
-	put_cpu();
 	g_cci_pmu = cci_pmu;
+	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_PERF_ARM_CCI_ONLINE,
+					     "perf/arm/cci:online", NULL,
+					     cci_pmu_offline_cpu);
+
 	pr_info("ARM %s PMU driver probed", cci_pmu->model->name);
-	return 0;
+out:
+	cpus_read_unlock();
+	return ret;
 }
 
 static int cci_pmu_remove(struct platform_device *pdev)

Signature · 1 line

2.20.1.dirty

The arm-cci probe logic faces a cyclic dependency wherein it has to pick a valid CPU to associate with before registering the PMU device, has to have the PMU state initialised before handling hotplug events in case it must be migrated, but has to have the hotplug notifier registered before the chosen CPU may go offline lest things get out of sync. The present code has tried to solve the races by using get_cpu() to pick the current CPU and prevent it from disappearing while the other two registrations are performed, but that results in taking mutexes with preemption disabled, which makes certain configurations very unhappy:

[ 1.983337] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:2004
[ 1.983340] in_atomic(): 1, irqs_disabled(): 0, pid: 1, name: swapper/0
[ 1.983342] Preemption disabled at:
[ 1.983353] [<ffffff80089801f4>] cci_pmu_probe+0x1dc/0x488
[ 1.983360] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.20-rt8-yocto-preempt-rt #1
[ 1.983362] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
[ 1.983364] Call trace:
[ 1.983369] dump_backtrace+0x0/0x158
[ 1.983372] show_stack+0x24/0x30
[ 1.983378] dump_stack+0x80/0xa4
[ 1.983383] ___might_sleep+0x138/0x160
[ 1.983386] __might_sleep+0x58/0x90
[ 1.983391] __rt_mutex_lock_state+0x30/0xc0
[ 1.983395] _mutex_lock+0x24/0x30
[ 1.983400] perf_pmu_register+0x2c/0x388
[ 1.983404] cci_pmu_probe+0x2bc/0x488
[ 1.983409] platform_drv_probe+0x58/0xa8

However, we don't actually mind being preempted or migrated at this point; all that really matters is that whichever CPU we pick does not get offlined before we're done. Thus, do the robust thing and instead take the lock to inhibit CPU hotplug for the duration. This also revealed an additional race in assigning the global pointer too late relative to the hotplug notifier, so that gets fixed in the process.

Reported-by "Li, Meng" <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

Like arm-cci, arm-ccn has the same issue where disabling preemption to avoid races between registering the PMU device and the hotplug notifier can lead to those operations taking mutexes in an invalid context. Fix it the same way by disabling hotplug instead of preemption. Since we only ever associate the PMU instance with a single CPU, we can also take the opportunity to slightly simplify the hotplug handling to track just that CPU number instead of a full cpumask.

Signed-off-by Robin Murphy <[email protected]>

Like other system PMUs which associate themselves with an arbitrary CPU for housekeeping purposes, arm_dsu has a race between registering the hotplug notifier and registering the PMU device, such that the hotplug niotifier can potentially fire and attempt to migrate the PMU context before the latter is valid. This is easily resolved by inhibiting hotplug until both the notifier and PMU device are successfully set up.

For the same reason, also suppress any synchronous notifier calls in the cleanup path if PMU registration fails.

Signed-off-by Robin Murphy <[email protected]>

Quoted 11 lines · Robin Murphy

Like other system PMUs which associate themselves with an arbitrary CPU for housekeeping purposes, arm_dsu has a race between registering the hotplug notifier and registering the PMU device, such that the hotplug niotifier can potentially fire and attempt to migrate the PMU context before the latter is valid. This is easily resolved by inhibiting hotplug until both the notifier and PMU device are successfully set up.

For the same reason, also suppress any synchronous notifier calls in the cleanup path if PMU registration fails.

Signed-off-by Robin Murphy <[email protected]>

Should we add :

Fixes commit 7520fa99246dade7ab6 ("perf: ARM DynamIQ Shared Unit PMU support")

Either way:

Reviewed-by Suzuki K Poulose <[email protected]>

Hi all,

Following the report of a preemption-related bug in arm-cci, it turns out there's a fair bit of cleaning up to do in this area. I've started here with the Arm drivers that I'm fairly familiar with - I suspect the hisi/qcom/xgene ones suffer from similar issues, but it's going to take me a while longer to figure them out in detail.

Robin.