| Message ID | d376f6bbd5804616b38145780789785b19a37543.1549299188.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix Arm system PMU hotplug issues | expand |
On 04/02/2019 17:09, Robin Murphy wrote: > 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 <robin.murphy@arm.com> Should we add : Fixes: commit 7520fa99246dade7ab6 ("perf: ARM DynamIQ Shared Unit PMU support") Either way: Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Hi Suzuki, On 05/02/2019 11:40, Suzuki K Poulose wrote: > On 04/02/2019 17:09, Robin Murphy wrote: >> 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 <robin.murphy@arm.com> > > Should we add : > > Fixes: commit 7520fa99246dade7ab6 ("perf: ARM DynamIQ Shared Unit PMU > support") > > Either way: > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Thanks for the reviews! I think this patch is worthwhile for cleanliness and consistency, but since it's neither self-contained (with the dependency on #4) nor "fix[ing] a real bug that bothers people" I'm not convinced it really deserves backporting - unlike the preemption issue, actually getting a crash from this race in practice is unlikely enough that it would probably require some determined, deliberate effort to trigger it with just the right conditions. Cheers, Robin.
diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c index 660cb8ac886a..cfaca06b964a 100644 --- a/drivers/perf/arm_dsu_pmu.c +++ b/drivers/perf/arm_dsu_pmu.c @@ -717,7 +717,8 @@ static int dsu_pmu_device_probe(struct platform_device *pdev) dsu_pmu->irq = irq; platform_set_drvdata(pdev, dsu_pmu); - rc = cpuhp_state_add_instance(dsu_pmu_cpuhp_state, + cpus_read_lock(); + rc = cpuhp_state_add_instance_cpuslocked(dsu_pmu_cpuhp_state, &dsu_pmu->cpuhp_node); if (rc) return rc; @@ -738,9 +739,10 @@ static int dsu_pmu_device_probe(struct platform_device *pdev) }; rc = perf_pmu_register(&dsu_pmu->pmu, name, -1); + cpus_read_unlock(); if (rc) { - cpuhp_state_remove_instance(dsu_pmu_cpuhp_state, - &dsu_pmu->cpuhp_node); + cpuhp_state_remove_instance_nocalls(dsu_pmu_cpuhp_state, + &dsu_pmu->cpuhp_node); irq_set_affinity_hint(dsu_pmu->irq, NULL); }
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 <robin.murphy@arm.com> --- drivers/perf/arm_dsu_pmu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)