[PATCH v2 0/2] bus: CCN/CCI: Fix use of smp_processor_id()

[PATCH v2 0/2] bus: CCN/CCI: Fix use of smp_processor_id()

[Marc Zyngier]

I've just noticed that both the CCI and CCN drivers have a small
buglet in that they call smp_processor_id() from preemptible context,
which is frown upon (having just booted a 4.13 kernel with
DEBUG_PREEMPT on my Seattle, I was surprised to be greeted with a nice
backtrace...).

I've tested the CCN patch on the same Seatle box, but I've only
compile-tested the equivalent CCI patch (which is obviously correct --
famous last words...).

Thanks,

	M.

* From v1: Keep the current CPU refcount until we have registered the
  CPU notifiers, making sure we don't race against a surprise hotplug
  off.

Marc Zyngier (2):
  bus: arm-ccn: Fix use of smp_processor_id() in preemptible context
  bus: arm-cci: Fix use of smp_processor_id() in preemptible context

 drivers/bus/arm-cci.c | 7 +++++--
 drivers/bus/arm-ccn.c | 4 +++-
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] bus: arm-ccn: Fix use of smp_processor_id() in preemptible context

[Marc Zyngier]

Booting a DEBUG_PREEMPT enabled kernel on a CCN-based system
results in the following splat:

[...]
arm-ccn e8000000.ccn: No access to interrupts, using timer.
BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
caller is debug_smp_processor_id+0x1c/0x28
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0 #6111
Hardware name: AMD Seattle/Seattle, BIOS 17:08:23 Jun 26 2017
Call trace:
[<ffff000008089e78>] dump_backtrace+0x0/0x278
[<ffff00000808a22c>] show_stack+0x24/0x30
[<ffff000008bc3bc4>] dump_stack+0x8c/0xb0
[<ffff00000852b534>] check_preemption_disabled+0xfc/0x100
[<ffff00000852b554>] debug_smp_processor_id+0x1c/0x28
[<ffff000008551bd8>] arm_ccn_probe+0x358/0x4f0
[...]

as we use smp_processor_id() in the wrong context.

Turn this into a get_cpu()/put_cpu() that extends over the CPU hotplug
registration, making sure that we don't race against a CPU down operation.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/bus/arm-ccn.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
index e8c6946fed9d..8614d7fa457e 100644
--- a/drivers/bus/arm-ccn.c
+++ b/drivers/bus/arm-ccn.c
@@ -1297,7 +1297,7 @@ static int arm_ccn_pmu_init(struct arm_ccn *ccn)
 	}
 
 	/* Pick one CPU which we will use to collect data from CCN... */
-	cpumask_set_cpu(smp_processor_id(), &ccn->dt.cpu);
+	cpumask_set_cpu(get_cpu(), &ccn->dt.cpu);
 
 	/* Also make sure that the overflow interrupt is handled by this CPU */
 	if (ccn->irq) {
@@ -1314,10 +1314,12 @@ static int arm_ccn_pmu_init(struct arm_ccn *ccn)
 
 	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE,
 					 &ccn->dt.node);
+	put_cpu();
 	return 0;
 
 error_pmu_register:
 error_set_affinity:
+	put_cpu();
 	ida_simple_remove(&arm_ccn_pmu_ida, ccn->dt.id);
 	for (i = 0; i < ccn->num_xps; i++)
 		writel(0, ccn->xp[i].base + CCN_XP_DT_CONTROL);
-- 
2.11.0

Re: [PATCH v2 1/2] bus: arm-ccn: Fix use of smp_processor_id() in preemptible context

[Pawel Moll]

On Tue, 2017-10-03 at 18:14 +0100, Marc Zyngier wrote:
> Booting a DEBUG_PREEMPT enabled kernel on a CCN-based system
> results in the following splat:
>
> [...]
> arm-ccn e8000000.ccn: No access to interrupts, using timer.
> BUG: using smp_processor_id() in preemptible [00000000] code:
> swapper/0/1
> caller is debug_smp_processor_id+0x1c/0x28
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0 #6111
> Hardware name: AMD Seattle/Seattle, BIOS 17:08:23 Jun 26 2017
> Call trace:
> [<ffff000008089e78>] dump_backtrace+0x0/0x278
> [<ffff00000808a22c>] show_stack+0x24/0x30
> [<ffff000008bc3bc4>] dump_stack+0x8c/0xb0
> [<ffff00000852b534>] check_preemption_disabled+0xfc/0x100
> [<ffff00000852b554>] debug_smp_processor_id+0x1c/0x28
> [<ffff000008551bd8>] arm_ccn_probe+0x358/0x4f0
> [...]
>
> as we use smp_processor_id() in the wrong context.
>
> Turn this into a get_cpu()/put_cpu() that extends over the CPU
> hotplug
> registration, making sure that we don't race against a CPU down
> operation.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Acked-by: Pawel Moll <pawel.moll@arm.com>

I assume you'll get this merged yourself? Or do you want me to relay
the CCN one (I've got a couple of other small changes to the driver in
the queue).

Paweł
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v2 1/2] bus: arm-ccn: Fix use of smp_processor_id() in preemptible context

[Marc Zyngier]

On 04/10/17 12:26, Pawel Moll wrote:
> On Tue, 2017-10-03 at 18:14 +0100, Marc Zyngier wrote:
>> Booting a DEBUG_PREEMPT enabled kernel on a CCN-based system
>> results in the following splat:
>>
>> [...]
>> arm-ccn e8000000.ccn: No access to interrupts, using timer.
>> BUG: using smp_processor_id() in preemptible [00000000] code:
>> swapper/0/1
>> caller is debug_smp_processor_id+0x1c/0x28
>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0 #6111
>> Hardware name: AMD Seattle/Seattle, BIOS 17:08:23 Jun 26 2017
>> Call trace:
>> [<ffff000008089e78>] dump_backtrace+0x0/0x278
>> [<ffff00000808a22c>] show_stack+0x24/0x30
>> [<ffff000008bc3bc4>] dump_stack+0x8c/0xb0
>> [<ffff00000852b534>] check_preemption_disabled+0xfc/0x100
>> [<ffff00000852b554>] debug_smp_processor_id+0x1c/0x28
>> [<ffff000008551bd8>] arm_ccn_probe+0x358/0x4f0
>> [...]
>>
>> as we use smp_processor_id() in the wrong context.
>>
>> Turn this into a get_cpu()/put_cpu() that extends over the CPU
>> hotplug
>> registration, making sure that we don't race against a CPU down
>> operation.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Acked-by: Pawel Moll <pawel.moll@arm.com>
> 
> I assume you'll get this merged yourself? Or do you want me to relay
> the CCN one (I've got a couple of other small changes to the driver in
> the queue).

I'd rather you take care of it if you already have a queue of patches.
Otherwise, I'll bundle the two patches and ask the armsoc folks to do
something with them.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/2] bus: arm-ccn: Fix use of smp_processor_id() in preemptible context

[Pawel Moll]

On Wed, 2017-10-04 at 13:32 +0100, Marc Zyngier wrote:
> 
> > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > 
> > I assume you'll get this merged yourself? Or do you want me to
> relay
> > the CCN one (I've got a couple of other small changes to the driver
> in
> > the queue).
>
> I'd rather you take care of it if you already have a queue of
> patches.
> Otherwise, I'll bundle the two patches and ask the armsoc folks to do
> something with them.

Ok. I guess you can include CCI as well. Not today, not tomorrow, but
Friday or early next week.

Cheers!

Paweł
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 2/2] bus: arm-cci: Fix use of smp_processor_id() in preemptible context

[Marc Zyngier]

The ARM CCI driver seem to be using smp_processor_id() in a
preemptible context, which is likely to make a DEBUG_PREMPT
kernel scream at boot time.

Turn this into a get_cpu()/put_cpu() that extends over the CPU
hotplug registration, making sure that we don't race against
a CPU down operation.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/bus/arm-cci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index c49da15d9790..d1aa11c083f9 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -1755,14 +1755,17 @@ 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);
-	cpumask_set_cpu(smp_processor_id(), &cci_pmu->cpus);
+	cpumask_set_cpu(get_cpu(), &cci_pmu->cpus);
 
 	ret = cci_pmu_init(cci_pmu, pdev);
-	if (ret)
+	if (ret) {
+		put_cpu();
 		return ret;
+	}
 
 	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CCI_ONLINE,
 					 &cci_pmu->node);
+	put_cpu();
 	pr_info("ARM %s PMU driver probed", cci_pmu->model->name);
 	return 0;
 }
-- 
2.11.0

Re: [PATCH v2 0/2] bus: CCN/CCI: Fix use of smp_processor_id()

[Mark Rutland]

On Tue, Oct 03, 2017 at 06:14:11PM +0100, Marc Zyngier wrote:
> I've just noticed that both the CCI and CCN drivers have a small
> buglet in that they call smp_processor_id() from preemptible context,
> which is frown upon (having just booted a 4.13 kernel with
> DEBUG_PREEMPT on my Seattle, I was surprised to be greeted with a nice
> backtrace...).
> 
> I've tested the CCN patch on the same Seatle box, but I've only
> compile-tested the equivalent CCI patch (which is obviously correct --
> famous last words...).
> 
> Thanks,
> 
> 	M.
> 
> * From v1: Keep the current CPU refcount until we have registered the
>   CPU notifiers, making sure we don't race against a surprise hotplug
>   off.
> 
> Marc Zyngier (2):
>   bus: arm-ccn: Fix use of smp_processor_id() in preemptible context
>   bus: arm-cci: Fix use of smp_processor_id() in preemptible context

For both:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.