[RESEND PATCH 0/2] Distinguish percpu irq and percpu_devid irq

[RESEND PATCH 0/2] Distinguish percpu irq and percpu_devid irq

[Julien Thierry]

[Resending with cover letter + better commit message for patch 1]

Hi,

Looking at the arm_pmu driver, it seems we are using irq_is_percpu when we
want to know if an irq must be configured individually on each cpu. But
this is not what irq_is_percpu does. The arm_pmu marking all its irqs as
PERCPU, driver attempts to enable the irq on each CPU even when it is not
needed.

This has not caused issue so far because enable_percpu_irq returns
immediatly if the provided irq is not PERCPU_DEVID.

* Patch 1 adds the function to identify PERCPU_DEVID irqs.
* Patch 2 replaces the irq_is_percpu calls with the new function

Cheers,

Julien

Julien Thierry (2):
  irqdesc: Add function to identify percpu_devid irqs
  arm/arm64: pmu: Distinguish percpu irq and percpu_devid irq

 drivers/perf/arm_pmu.c          | 10 +++++-----
 drivers/perf/arm_pmu_platform.c |  4 ++--
 include/linux/irqdesc.h         |  8 ++++++++
 3 files changed, 15 insertions(+), 7 deletions(-)

--
1.9.1

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Julien Thierry]

irq_is_percpu indicates whether an irq should only target a single cpu.
PERCPU_DEVID flag indicates that an irq can be configured differently on
each cpu it can target.

Provide a function to check whether an irq is PERCPU_DEVID.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irqdesc.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 3e90a09..93960cf 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -244,6 +244,14 @@ static inline int irq_is_percpu(unsigned int irq)
 	return desc->status_use_accessors & IRQ_PER_CPU;
 }

+static inline int irq_is_percpu_devid(unsigned int irq)
+{
+	struct irq_desc *desc;
+
+	desc = irq_to_desc(irq);
+	return desc->status_use_accessors & IRQ_PER_CPU_DEVID;
+}
+
 static inline void
 irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
 {
--
1.9.1

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Marc Zyngier]

On 13/10/17 12:26, Julien Thierry wrote:
> irq_is_percpu indicates whether an irq should only target a single cpu.
> PERCPU_DEVID flag indicates that an irq can be configured differently on
> each cpu it can target.
> 
> Provide a function to check whether an irq is PERCPU_DEVID.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/irqdesc.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 3e90a09..93960cf 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -244,6 +244,14 @@ static inline int irq_is_percpu(unsigned int irq)
>  	return desc->status_use_accessors & IRQ_PER_CPU;
>  }
> 
> +static inline int irq_is_percpu_devid(unsigned int irq)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +	return desc->status_use_accessors & IRQ_PER_CPU_DEVID;
> +}
> +
>  static inline void
>  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
>  {
> --
> 1.9.1
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Mark Rutland]

Hi Thomas,

Would you be happy for this patch to be taken via the arm64 tree?

The next patch [1] makes use of this in the arm_pmu driver, and we're
likely to have some other changes there shortly.

Thanks,
Mark

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/536532.html

On Fri, Oct 13, 2017 at 12:26:44PM +0100, Julien Thierry wrote:
> irq_is_percpu indicates whether an irq should only target a single cpu.
> PERCPU_DEVID flag indicates that an irq can be configured differently on
> each cpu it can target.
> 
> Provide a function to check whether an irq is PERCPU_DEVID.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/irqdesc.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 3e90a09..93960cf 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -244,6 +244,14 @@ static inline int irq_is_percpu(unsigned int irq)
>  	return desc->status_use_accessors & IRQ_PER_CPU;
>  }
> 
> +static inline int irq_is_percpu_devid(unsigned int irq)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +	return desc->status_use_accessors & IRQ_PER_CPU_DEVID;
> +}
> +
>  static inline void
>  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
>  {
> --
> 1.9.1

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Will Deacon]

Hi Thomas,

I was hoping to merge this one via arm64 since it now has Marc's
Reviewed-by, but I'd really like your Ack on it too, if you don't mind.

Could you take a look, please?

Thanks,

Will

On Fri, Oct 13, 2017 at 06:44:23PM +0100, Mark Rutland wrote:
> Would you be happy for this patch to be taken via the arm64 tree?
> 
> The next patch [1] makes use of this in the arm_pmu driver, and we're
> likely to have some other changes there shortly.
> 
> Thanks,
> Mark
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/536532.html
> 
> On Fri, Oct 13, 2017 at 12:26:44PM +0100, Julien Thierry wrote:
> > irq_is_percpu indicates whether an irq should only target a single cpu.
> > PERCPU_DEVID flag indicates that an irq can be configured differently on
> > each cpu it can target.
> > 
> > Provide a function to check whether an irq is PERCPU_DEVID.
> > 
> > Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  include/linux/irqdesc.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> > index 3e90a09..93960cf 100644
> > --- a/include/linux/irqdesc.h
> > +++ b/include/linux/irqdesc.h
> > @@ -244,6 +244,14 @@ static inline int irq_is_percpu(unsigned int irq)
> >  	return desc->status_use_accessors & IRQ_PER_CPU;
> >  }
> > 
> > +static inline int irq_is_percpu_devid(unsigned int irq)
> > +{
> > +	struct irq_desc *desc;
> > +
> > +	desc = irq_to_desc(irq);
> > +	return desc->status_use_accessors & IRQ_PER_CPU_DEVID;
> > +}
> > +
> >  static inline void
> >  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
> >  {
> > --
> > 1.9.1

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Thomas Gleixner]

On Tue, 24 Oct 2017, Will Deacon wrote:

> Hi Thomas,
> 
> I was hoping to merge this one via arm64 since it now has Marc's
> Reviewed-by, but I'd really like your Ack on it too, if you don't mind.
> 
> Could you take a look, please?

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[RESEND PATCH 1/2] irqdesc: Add function to identify percpu_devid irqs

[Will Deacon]

On Tue, Oct 24, 2017 at 04:50:19PM +0200, Thomas Gleixner wrote:
> On Tue, 24 Oct 2017, Will Deacon wrote:
> 
> > Hi Thomas,
> > 
> > I was hoping to merge this one via arm64 since it now has Marc's
> > Reviewed-by, but I'd really like your Ack on it too, if you don't mind.
> > 
> > Could you take a look, please?
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Brill, cheers.

Will

[RESEND PATCH 2/2] arm/arm64: pmu: Distinguish percpu irq and percpu_devid irq

[Julien Thierry]

arm_pmu interrupts are maked as PERCPU even when these are not local
physical interrupts to a single CPU. When using non-local interrupts,
interrupts marked as PERCPU will not get freed not disabled properly
by the PMU driver.

Check if interrupts are local to a single CPU with PERCPU_DEVID since
this is what the PMU driver really needs to know.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Will Deacon <will.deacon@arm.com>
Mark Rutland <mark.rutland@arm.com>
---
 drivers/perf/arm_pmu.c          | 10 +++++-----
 drivers/perf/arm_pmu_platform.c |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index d14fc2e..7bc5eee 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -539,7 +539,7 @@ void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
 	if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
 		return;

-	if (irq_is_percpu(irq)) {
+	if (irq_is_percpu_devid(irq)) {
 		free_percpu_irq(irq, &hw_events->percpu_pmu);
 		cpumask_clear(&armpmu->active_irqs);
 		return;
@@ -565,10 +565,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
 	if (!irq)
 		return 0;

-	if (irq_is_percpu(irq) && cpumask_empty(&armpmu->active_irqs)) {
+	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
 		err = request_percpu_irq(irq, handler, "arm-pmu",
 					 &hw_events->percpu_pmu);
-	} else if (irq_is_percpu(irq)) {
+	} else if (irq_is_percpu_devid(irq)) {
 		int other_cpu = cpumask_first(&armpmu->active_irqs);
 		int other_irq = per_cpu(hw_events->irq, other_cpu);

@@ -649,7 +649,7 @@ static int arm_perf_starting_cpu(unsigned int cpu, struct hlist_node *node)

 	irq = armpmu_get_cpu_irq(pmu, cpu);
 	if (irq) {
-		if (irq_is_percpu(irq)) {
+		if (irq_is_percpu_devid(irq)) {
 			enable_percpu_irq(irq, IRQ_TYPE_NONE);
 			return 0;
 		}
@@ -667,7 +667,7 @@ static int arm_perf_teardown_cpu(unsigned int cpu, struct hlist_node *node)
 		return 0;

 	irq = armpmu_get_cpu_irq(pmu, cpu);
-	if (irq && irq_is_percpu(irq))
+	if (irq && irq_is_percpu_devid(irq))
 		disable_percpu_irq(irq);

 	return 0;
diff --git a/drivers/perf/arm_pmu_platform.c b/drivers/perf/arm_pmu_platform.c
index 4eafa7a..bbc64ee 100644
--- a/drivers/perf/arm_pmu_platform.c
+++ b/drivers/perf/arm_pmu_platform.c
@@ -126,7 +126,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)

 	if (num_irqs == 1) {
 		int irq = platform_get_irq(pdev, 0);
-		if (irq && irq_is_percpu(irq))
+		if (irq && irq_is_percpu_devid(irq))
 			return pmu_parse_percpu_irq(pmu, irq);
 	}

@@ -149,7 +149,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
 		if (WARN_ON(irq <= 0))
 			continue;

-		if (irq_is_percpu(irq)) {
+		if (irq_is_percpu_devid(irq)) {
 			pr_warn("multiple PPIs or mismatched SPI/PPI detected\n");
 			return -EINVAL;
 		}
--
1.9.1

[RESEND PATCH 2/2] arm/arm64: pmu: Distinguish percpu irq and percpu_devid irq

[Mark Rutland]

On Fri, Oct 13, 2017 at 12:26:45PM +0100, Julien Thierry wrote:
> arm_pmu interrupts are maked as PERCPU even when these are not local
> physical interrupts to a single CPU. When using non-local interrupts,
> interrupts marked as PERCPU will not get freed not disabled properly
> by the PMU driver.
> 
> Check if interrupts are local to a single CPU with PERCPU_DEVID since
> this is what the PMU driver really needs to know.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Will Deacon <will.deacon@arm.com>
> Mark Rutland <mark.rutland@arm.com>

I guess these should have been with prefixed 'Cc: '.

The patch itself looks good to me:

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

I suspect we'll need to take both patches via the usual arm pmu tree, so
it'd be good if we could get an ack on patch 1.

Thanks,
Mark.

> ---
>  drivers/perf/arm_pmu.c          | 10 +++++-----
>  drivers/perf/arm_pmu_platform.c |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index d14fc2e..7bc5eee 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -539,7 +539,7 @@ void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
>  	if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
>  		return;
> 
> -	if (irq_is_percpu(irq)) {
> +	if (irq_is_percpu_devid(irq)) {
>  		free_percpu_irq(irq, &hw_events->percpu_pmu);
>  		cpumask_clear(&armpmu->active_irqs);
>  		return;
> @@ -565,10 +565,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
>  	if (!irq)
>  		return 0;
> 
> -	if (irq_is_percpu(irq) && cpumask_empty(&armpmu->active_irqs)) {
> +	if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
>  		err = request_percpu_irq(irq, handler, "arm-pmu",
>  					 &hw_events->percpu_pmu);
> -	} else if (irq_is_percpu(irq)) {
> +	} else if (irq_is_percpu_devid(irq)) {
>  		int other_cpu = cpumask_first(&armpmu->active_irqs);
>  		int other_irq = per_cpu(hw_events->irq, other_cpu);
> 
> @@ -649,7 +649,7 @@ static int arm_perf_starting_cpu(unsigned int cpu, struct hlist_node *node)
> 
>  	irq = armpmu_get_cpu_irq(pmu, cpu);
>  	if (irq) {
> -		if (irq_is_percpu(irq)) {
> +		if (irq_is_percpu_devid(irq)) {
>  			enable_percpu_irq(irq, IRQ_TYPE_NONE);
>  			return 0;
>  		}
> @@ -667,7 +667,7 @@ static int arm_perf_teardown_cpu(unsigned int cpu, struct hlist_node *node)
>  		return 0;
> 
>  	irq = armpmu_get_cpu_irq(pmu, cpu);
> -	if (irq && irq_is_percpu(irq))
> +	if (irq && irq_is_percpu_devid(irq))
>  		disable_percpu_irq(irq);
> 
>  	return 0;
> diff --git a/drivers/perf/arm_pmu_platform.c b/drivers/perf/arm_pmu_platform.c
> index 4eafa7a..bbc64ee 100644
> --- a/drivers/perf/arm_pmu_platform.c
> +++ b/drivers/perf/arm_pmu_platform.c
> @@ -126,7 +126,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
> 
>  	if (num_irqs == 1) {
>  		int irq = platform_get_irq(pdev, 0);
> -		if (irq && irq_is_percpu(irq))
> +		if (irq && irq_is_percpu_devid(irq))
>  			return pmu_parse_percpu_irq(pmu, irq);
>  	}
> 
> @@ -149,7 +149,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
>  		if (WARN_ON(irq <= 0))
>  			continue;
> 
> -		if (irq_is_percpu(irq)) {
> +		if (irq_is_percpu_devid(irq)) {
>  			pr_warn("multiple PPIs or mismatched SPI/PPI detected\n");
>  			return -EINVAL;
>  		}
> --
> 1.9.1

[RESEND PATCH 2/2] arm/arm64: pmu: Distinguish percpu irq and percpu_devid irq

[Julien Thierry]

On 13/10/17 17:47, Mark Rutland wrote:
> On Fri, Oct 13, 2017 at 12:26:45PM +0100, Julien Thierry wrote:
>> arm_pmu interrupts are maked as PERCPU even when these are not local
>> physical interrupts to a single CPU. When using non-local interrupts,
>> interrupts marked as PERCPU will not get freed not disabled properly
>> by the PMU driver.
>>
>> Check if interrupts are local to a single CPU with PERCPU_DEVID since
>> this is what the PMU driver really needs to know.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Will Deacon <will.deacon@arm.com>
>> Mark Rutland <mark.rutland@arm.com>
>
> I guess these should have been with prefixed 'Cc: '.

Oops, yes they should've.

>
> The patch itself looks good to me:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> I suspect we'll need to take both patches via the usual arm pmu tree, so
> it'd be good if we could get an ack on patch 1.
>

Thanks!

> Thanks,
> Mark.
>
>> ---
>>   drivers/perf/arm_pmu.c          | 10 +++++-----
>>   drivers/perf/arm_pmu_platform.c |  4 ++--
>>   2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index d14fc2e..7bc5eee 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -539,7 +539,7 @@ void armpmu_free_irq(struct arm_pmu *armpmu, int cpu)
>>      if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
>>              return;
>>
>> -    if (irq_is_percpu(irq)) {
>> +    if (irq_is_percpu_devid(irq)) {
>>              free_percpu_irq(irq, &hw_events->percpu_pmu);
>>              cpumask_clear(&armpmu->active_irqs);
>>              return;
>> @@ -565,10 +565,10 @@ int armpmu_request_irq(struct arm_pmu *armpmu, int cpu)
>>      if (!irq)
>>              return 0;
>>
>> -    if (irq_is_percpu(irq) && cpumask_empty(&armpmu->active_irqs)) {
>> +    if (irq_is_percpu_devid(irq) && cpumask_empty(&armpmu->active_irqs)) {
>>              err = request_percpu_irq(irq, handler, "arm-pmu",
>>                                       &hw_events->percpu_pmu);
>> -    } else if (irq_is_percpu(irq)) {
>> +    } else if (irq_is_percpu_devid(irq)) {
>>              int other_cpu = cpumask_first(&armpmu->active_irqs);
>>              int other_irq = per_cpu(hw_events->irq, other_cpu);
>>
>> @@ -649,7 +649,7 @@ static int arm_perf_starting_cpu(unsigned int cpu, struct hlist_node *node)
>>
>>      irq = armpmu_get_cpu_irq(pmu, cpu);
>>      if (irq) {
>> -            if (irq_is_percpu(irq)) {
>> +            if (irq_is_percpu_devid(irq)) {
>>                      enable_percpu_irq(irq, IRQ_TYPE_NONE);
>>                      return 0;
>>              }
>> @@ -667,7 +667,7 @@ static int arm_perf_teardown_cpu(unsigned int cpu, struct hlist_node *node)
>>              return 0;
>>
>>      irq = armpmu_get_cpu_irq(pmu, cpu);
>> -    if (irq && irq_is_percpu(irq))
>> +    if (irq && irq_is_percpu_devid(irq))
>>              disable_percpu_irq(irq);
>>
>>      return 0;
>> diff --git a/drivers/perf/arm_pmu_platform.c b/drivers/perf/arm_pmu_platform.c
>> index 4eafa7a..bbc64ee 100644
>> --- a/drivers/perf/arm_pmu_platform.c
>> +++ b/drivers/perf/arm_pmu_platform.c
>> @@ -126,7 +126,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
>>
>>      if (num_irqs == 1) {
>>              int irq = platform_get_irq(pdev, 0);
>> -            if (irq && irq_is_percpu(irq))
>> +            if (irq && irq_is_percpu_devid(irq))
>>                      return pmu_parse_percpu_irq(pmu, irq);
>>      }
>>
>> @@ -149,7 +149,7 @@ static int pmu_parse_irqs(struct arm_pmu *pmu)
>>              if (WARN_ON(irq <= 0))
>>                      continue;
>>
>> -            if (irq_is_percpu(irq)) {
>> +            if (irq_is_percpu_devid(irq)) {
>>                      pr_warn("multiple PPIs or mismatched SPI/PPI detected\n");
>>                      return -EINVAL;
>>              }
>> --
>> 1.9.1

--
Julien Thierry
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.