[PATCH V4 0/2] genirq: arm64: perf: support for percpu pmu interrupt

[PATCH V4 0/2] genirq: arm64: perf: support for percpu pmu interrupt

[Vinayak Kale]

This patch series adds support to handle interrupt registration/deregistration
in arm64 pmu driver when pmu interrupt type is percpu.

Changelog:
V4:
* In arm64 pmu driver: Avoid using irq_to_desc() to check validity of irq.

V3:
* Remove validity check for 'desc' from accessor function in irqdesc.h .
  Instead, check the irq 'desc' validity in arm64 pmu driver.

V2:
* To determine whether an IRQ is percpu or not, added an accessor function in
  irqdesc.h . This approach was used by Chris Smith here[1] for similar changes
  in arm pmu driver.
* In arm64 pmu driver: Got rid of unnecessary pointer typecastings.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/1207.3/02955.html

Vinayak Kale (2):
  genirq: Add an accessor for IRQ_PER_CPU flag
  arm64: perf: add support for percpu pmu interrupt

 arch/arm64/kernel/perf_event.c |  108 ++++++++++++++++++++++++++++++----------
 include/linux/irqdesc.h        |    8 +++
 2 files changed, 89 insertions(+), 27 deletions(-)

-- 
1.7.9.5

[PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Vinayak Kale]

This patch adds an accessor function for IRQ_PER_CPU flag.
The accessor function is useful to dertermine whether an IRQ is percpu or not.

Signed-off-by: Vinayak Kale <vkale@apm.com>
---
 include/linux/irqdesc.h |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 56fb646..26e2661 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -152,6 +152,14 @@ static inline int irq_balancing_disabled(unsigned int irq)
 	return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
 }
 
+static inline int irq_is_percpu(unsigned int irq)
+{
+	struct irq_desc *desc;
+
+	desc = irq_to_desc(irq);
+	return desc->status_use_accessors & IRQ_PER_CPU;
+}
+
 static inline void
 irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
 {
-- 
1.7.9.5

Re: [PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Stephen Boyd]

On 11/20/13 03:13, Vinayak Kale wrote:
> This patch adds an accessor function for IRQ_PER_CPU flag.
> The accessor function is useful to dertermine whether an IRQ is percpu or not.
>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---

This looks like a copy of Chris Smith's patch. Shouldn't Chris be the
author and the commit text be whatever Chris sent?

>  include/linux/irqdesc.h |    8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 56fb646..26e2661 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -152,6 +152,14 @@ static inline int irq_balancing_disabled(unsigned int irq)
>  	return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
>  }
>  
> +static inline int irq_is_percpu(unsigned int irq)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +	return desc->status_use_accessors & IRQ_PER_CPU;
> +}
> +
>  static inline void
>  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
>  {

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Vinayak Kale]

On Wed, Nov 20, 2013 at 11:46 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/20/13 03:13, Vinayak Kale wrote:
>> This patch adds an accessor function for IRQ_PER_CPU flag.
>> The accessor function is useful to dertermine whether an IRQ is percpu or not.
>>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> ---
>
> This looks like a copy of Chris Smith's patch. Shouldn't Chris be the
> author and the commit text be whatever Chris sent?

In the cover letter of this patch series I did mention about Chris's
earlier patch. I didn't know his email-id earlier, have found the
mail-id now. CCing the mail-id to check whether it's still valid.

>
>>  include/linux/irqdesc.h |    8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
>> index 56fb646..26e2661 100644
>> --- a/include/linux/irqdesc.h
>> +++ b/include/linux/irqdesc.h
>> @@ -152,6 +152,14 @@ static inline int irq_balancing_disabled(unsigned int irq)
>>       return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
>>  }
>>
>> +static inline int irq_is_percpu(unsigned int irq)
>> +{
>> +     struct irq_desc *desc;
>> +
>> +     desc = irq_to_desc(irq);
>> +     return desc->status_use_accessors & IRQ_PER_CPU;
>> +}
>> +
>>  static inline void
>>  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
>>  {
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>

Re: [PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Vinayak Kale]

[removing chris.smith@st.com]

On Thu, Nov 21, 2013 at 11:36 AM, Vinayak Kale <vkale@apm.com> wrote:
> On Wed, Nov 20, 2013 at 11:46 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 11/20/13 03:13, Vinayak Kale wrote:
>>> This patch adds an accessor function for IRQ_PER_CPU flag.
>>> The accessor function is useful to dertermine whether an IRQ is percpu or not.
>>>
>>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>>> ---
>>
>> This looks like a copy of Chris Smith's patch. Shouldn't Chris be the
>> author and the commit text be whatever Chris sent?
>
> In the cover letter of this patch series I did mention about Chris's
> earlier patch. I didn't know his email-id earlier, have found the
> mail-id now. CCing the mail-id to check whether it's still valid.
>

Chris's mail-id doesn't seem to be valid, the earlier mail to his
mail-id [chris.smith@st.com] bounced.
Please let me know in such case how to mention original author's credits.

>>
>>>  include/linux/irqdesc.h |    8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
>>> index 56fb646..26e2661 100644
>>> --- a/include/linux/irqdesc.h
>>> +++ b/include/linux/irqdesc.h
>>> @@ -152,6 +152,14 @@ static inline int irq_balancing_disabled(unsigned int irq)
>>>       return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
>>>  }
>>>
>>> +static inline int irq_is_percpu(unsigned int irq)
>>> +{
>>> +     struct irq_desc *desc;
>>> +
>>> +     desc = irq_to_desc(irq);
>>> +     return desc->status_use_accessors & IRQ_PER_CPU;
>>> +}
>>> +
>>>  static inline void
>>>  irq_set_lockdep_class(unsigned int irq, struct lock_class_key *class)
>>>  {
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> hosted by The Linux Foundation
>>

Re: [PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Stephen Boyd]

On 11/20/13 22:10, Vinayak Kale wrote:
> [removing chris.smith@st.com]
>
> On Thu, Nov 21, 2013 at 11:36 AM, Vinayak Kale <vkale@apm.com> wrote:
>> On Wed, Nov 20, 2013 at 11:46 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 11/20/13 03:13, Vinayak Kale wrote:
>>>> This patch adds an accessor function for IRQ_PER_CPU flag.
>>>> The accessor function is useful to dertermine whether an IRQ is percpu or not.
>>>>
>>>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>>>> ---
>>> This looks like a copy of Chris Smith's patch. Shouldn't Chris be the
>>> author and the commit text be whatever Chris sent?
>> In the cover letter of this patch series I did mention about Chris's
>> earlier patch. I didn't know his email-id earlier, have found the
>> mail-id now. CCing the mail-id to check whether it's still valid.
>>
> Chris's mail-id doesn't seem to be valid, the earlier mail to his
> mail-id [chris.smith@st.com] bounced.
> Please let me know in such case how to mention original author's credits.

It's up to the maintainer accepting the patch. If I was picking up the
patch I would say it doesn't really matter if the mail-id is valid
anymore. Leave the original patch intact and just add your sign-off. If
you took the patch and significantly changed it it's good to put
"Based-on-a-patch-by:" and then take over authorship.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH V4 1/2] genirq: Add an accessor for IRQ_PER_CPU flag

[Vinayak Kale]

On Fri, Nov 22, 2013 at 6:24 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/20/13 22:10, Vinayak Kale wrote:
>> [removing chris.smith@st.com]
>>
>> On Thu, Nov 21, 2013 at 11:36 AM, Vinayak Kale <vkale@apm.com> wrote:
>>> On Wed, Nov 20, 2013 at 11:46 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>> On 11/20/13 03:13, Vinayak Kale wrote:
>>>>> This patch adds an accessor function for IRQ_PER_CPU flag.
>>>>> The accessor function is useful to dertermine whether an IRQ is percpu or not.
>>>>>
>>>>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>>>>> ---
>>>> This looks like a copy of Chris Smith's patch. Shouldn't Chris be the
>>>> author and the commit text be whatever Chris sent?
>>> In the cover letter of this patch series I did mention about Chris's
>>> earlier patch. I didn't know his email-id earlier, have found the
>>> mail-id now. CCing the mail-id to check whether it's still valid.
>>>
>> Chris's mail-id doesn't seem to be valid, the earlier mail to his
>> mail-id [chris.smith@st.com] bounced.
>> Please let me know in such case how to mention original author's credits.
>
> It's up to the maintainer accepting the patch. If I was picking up the
> patch I would say it doesn't really matter if the mail-id is valid
> anymore. Leave the original patch intact and just add your sign-off. If
> you took the patch and significantly changed it it's good to put
> "Based-on-a-patch-by:" and then take over authorship.
>
Thanks for the info.

I have made a minor change w.r.t. Chris's original patch: changed the
accessor function name from irq_is_per_cpu() to irq_is_percpu() since
I think irq_is_percpu() name is more inline with other *percpu* kernel
functions.

I will put Chris's sign-off as well as mine. I hope tglx will be fine with this.

[PATCH V4 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

Add support for irq registration when pmu interrupt is percpu.

Signed-off-by: Vinayak Kale <vkale@apm.com>
Signed-off-by: Tuan Phan <tphan@apm.com>
---
 arch/arm64/kernel/perf_event.c |  108 ++++++++++++++++++++++++++++++----------
 1 file changed, 81 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index cea1594..de12ba8 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -22,6 +22,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/perf_event.h>
@@ -363,22 +364,55 @@ validate_group(struct perf_event *event)
 }
 
 static void
+armpmu_disable_percpu_irq(void *data)
+{
+	struct arm_pmu *armpmu = data;
+	struct platform_device *pmu_device = armpmu->plat_device;
+	int irq = platform_get_irq(pmu_device, 0);
+
+	cpumask_test_and_clear_cpu(smp_processor_id(), &armpmu->active_irqs);
+	disable_percpu_irq(irq);
+}
+
+static void
 armpmu_release_hardware(struct arm_pmu *armpmu)
 {
 	int i, irq, irqs;
 	struct platform_device *pmu_device = armpmu->plat_device;
 
 	irqs = min(pmu_device->num_resources, num_possible_cpus());
+	if (irqs < 1)
+		return;
 
-	for (i = 0; i < irqs; ++i) {
-		if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
-			continue;
-		irq = platform_get_irq(pmu_device, i);
-		if (irq >= 0)
-			free_irq(irq, armpmu);
+	irq = platform_get_irq(pmu_device, 0);
+	if (irq < 0)
+		return;
+
+	if (irq_is_percpu(irq)) {
+		on_each_cpu(armpmu_disable_percpu_irq, armpmu, 1);
+		free_percpu_irq(irq, &cpu_hw_events);
+	} else {
+		for (i = 0; i < irqs; ++i) {
+			if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
+				continue;
+			irq = platform_get_irq(pmu_device, i);
+			if (irq >= 0)
+				free_irq(irq, armpmu);
+		}
 	}
 }
 
+static void
+armpmu_enable_percpu_irq(void *data)
+{
+	struct arm_pmu *armpmu = data;
+	struct platform_device *pmu_device = armpmu->plat_device;
+	int irq = platform_get_irq(pmu_device, 0);
+
+	enable_percpu_irq(irq, 0);
+	cpumask_set_cpu(smp_processor_id(), &armpmu->active_irqs);
+}
+
 static int
 armpmu_reserve_hardware(struct arm_pmu *armpmu)
 {
@@ -396,34 +430,54 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 		return -ENODEV;
 	}
 
-	for (i = 0; i < irqs; ++i) {
-		err = 0;
-		irq = platform_get_irq(pmu_device, i);
-		if (irq < 0)
-			continue;
+	irq = platform_get_irq(pmu_device, 0);
+	if (irq < 0) {
+		pr_err("failed to get an irq for PMU device\n");
+		return -ENODEV;
+	}
 
-		/*
-		 * If we have a single PMU interrupt that we can't shift,
-		 * assume that we're running on a uniprocessor machine and
-		 * continue. Otherwise, continue without this interrupt.
-		 */
-		if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
-			pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
-				    irq, i);
-			continue;
-		}
+	if (irq_is_percpu(irq)) {
+		err = request_percpu_irq(irq, armpmu->handle_irq,
+				"arm-pmu", &cpu_hw_events);
 
-		err = request_irq(irq, armpmu->handle_irq,
-				  IRQF_NOBALANCING,
-				  "arm-pmu", armpmu);
 		if (err) {
-			pr_err("unable to request IRQ%d for ARM PMU counters\n",
-				irq);
+			pr_err("unable to request percpu IRQ%d for ARM PMU counters\n",
+					irq);
 			armpmu_release_hardware(armpmu);
 			return err;
 		}
 
-		cpumask_set_cpu(i, &armpmu->active_irqs);
+		on_each_cpu(armpmu_enable_percpu_irq, armpmu, 1);
+	} else {
+		for (i = 0; i < irqs; ++i) {
+			err = 0;
+			irq = platform_get_irq(pmu_device, i);
+			if (irq < 0)
+				continue;
+
+			/*
+			 * If we have a single PMU interrupt that we can't shift,
+			 * assume that we're running on a uniprocessor machine and
+			 * continue. Otherwise, continue without this interrupt.
+			 */
+			if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+				pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
+						irq, i);
+				continue;
+			}
+
+			err = request_irq(irq, armpmu->handle_irq,
+					IRQF_NOBALANCING,
+					"arm-pmu", armpmu);
+			if (err) {
+				pr_err("unable to request IRQ%d for ARM PMU counters\n",
+						irq);
+				armpmu_release_hardware(armpmu);
+				return err;
+			}
+
+			cpumask_set_cpu(i, &armpmu->active_irqs);
+		}
 	}
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH V4 2/2] arm64: perf: add support for percpu pmu interrupt

[Marc Zyngier]

[dropped patches@apm.com]

Vinayak,

Please keep reviewers on CC, as it makes easier to track the changes.

On 20/11/13 11:13, Vinayak Kale wrote:
> Add support for irq registration when pmu interrupt is percpu.
> 
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> Signed-off-by: Tuan Phan <tphan@apm.com>
> ---
>  arch/arm64/kernel/perf_event.c |  108 ++++++++++++++++++++++++++++++----------
>  1 file changed, 81 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cea1594..de12ba8 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/bitmap.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/perf_event.h>
> @@ -363,22 +364,55 @@ validate_group(struct perf_event *event)
>  }
>  
>  static void
> +armpmu_disable_percpu_irq(void *data)
> +{
> +	struct arm_pmu *armpmu = data;
> +	struct platform_device *pmu_device = armpmu->plat_device;
> +	int irq = platform_get_irq(pmu_device, 0);
> +
> +	cpumask_test_and_clear_cpu(smp_processor_id(), &armpmu->active_irqs);
> +	disable_percpu_irq(irq);
> +}
> +
> +static void
>  armpmu_release_hardware(struct arm_pmu *armpmu)
>  {
>  	int i, irq, irqs;
>  	struct platform_device *pmu_device = armpmu->plat_device;
>  
>  	irqs = min(pmu_device->num_resources, num_possible_cpus());
> +	if (irqs < 1)
> +		return;
>  
> -	for (i = 0; i < irqs; ++i) {
> -		if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
> -			continue;
> -		irq = platform_get_irq(pmu_device, i);
> -		if (irq >= 0)
> -			free_irq(irq, armpmu);
> +	irq = platform_get_irq(pmu_device, 0);
> +	if (irq < 0)

So I'm going to sound like a stuck record: irq == 0 is not a valid IRQ
number, full stop. This is how we express the fact that there is no IRQ.

If you're touching that code, you might as well fix this buglet.

> +		return;
> +
> +	if (irq_is_percpu(irq)) {
> +		on_each_cpu(armpmu_disable_percpu_irq, armpmu, 1);
> +		free_percpu_irq(irq, &cpu_hw_events);
> +	} else {
> +		for (i = 0; i < irqs; ++i) {
> +			if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
> +				continue;
> +			irq = platform_get_irq(pmu_device, i);
> +			if (irq >= 0)

Same here.

> +				free_irq(irq, armpmu);
> +		}
>  	}
>  }
>  
> +static void
> +armpmu_enable_percpu_irq(void *data)
> +{
> +	struct arm_pmu *armpmu = data;
> +	struct platform_device *pmu_device = armpmu->plat_device;
> +	int irq = platform_get_irq(pmu_device, 0);
> +
> +	enable_percpu_irq(irq, 0);
> +	cpumask_set_cpu(smp_processor_id(), &armpmu->active_irqs);
> +}
> +
>  static int
>  armpmu_reserve_hardware(struct arm_pmu *armpmu)
>  {
> @@ -396,34 +430,54 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
>  		return -ENODEV;
>  	}
>  
> -	for (i = 0; i < irqs; ++i) {
> -		err = 0;
> -		irq = platform_get_irq(pmu_device, i);
> -		if (irq < 0)
> -			continue;
> +	irq = platform_get_irq(pmu_device, 0);
> +	if (irq < 0) {

And here.

> +		pr_err("failed to get an irq for PMU device\n");
> +		return -ENODEV;
> +	}
>  
> -		/*
> -		 * If we have a single PMU interrupt that we can't shift,
> -		 * assume that we're running on a uniprocessor machine and
> -		 * continue. Otherwise, continue without this interrupt.
> -		 */
> -		if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
> -			pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
> -				    irq, i);
> -			continue;
> -		}
> +	if (irq_is_percpu(irq)) {
> +		err = request_percpu_irq(irq, armpmu->handle_irq,
> +				"arm-pmu", &cpu_hw_events);
>  
> -		err = request_irq(irq, armpmu->handle_irq,
> -				  IRQF_NOBALANCING,
> -				  "arm-pmu", armpmu);
>  		if (err) {
> -			pr_err("unable to request IRQ%d for ARM PMU counters\n",
> -				irq);
> +			pr_err("unable to request percpu IRQ%d for ARM PMU counters\n",
> +					irq);
>  			armpmu_release_hardware(armpmu);
>  			return err;
>  		}
>  
> -		cpumask_set_cpu(i, &armpmu->active_irqs);
> +		on_each_cpu(armpmu_enable_percpu_irq, armpmu, 1);
> +	} else {
> +		for (i = 0; i < irqs; ++i) {
> +			err = 0;
> +			irq = platform_get_irq(pmu_device, i);
> +			if (irq < 0)

And here.

> +				continue;
> +
> +			/*
> +			 * If we have a single PMU interrupt that we can't shift,
> +			 * assume that we're running on a uniprocessor machine and
> +			 * continue. Otherwise, continue without this interrupt.
> +			 */
> +			if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
> +				pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
> +						irq, i);
> +				continue;
> +			}
> +
> +			err = request_irq(irq, armpmu->handle_irq,
> +					IRQF_NOBALANCING,
> +					"arm-pmu", armpmu);
> +			if (err) {
> +				pr_err("unable to request IRQ%d for ARM PMU counters\n",
> +						irq);
> +				armpmu_release_hardware(armpmu);
> +				return err;
> +			}
> +
> +			cpumask_set_cpu(i, &armpmu->active_irqs);
> +		}
>  	}
>  
>  	return 0;
> 

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

Re: [PATCH V4 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

Hi Marc,

On Wed, Nov 20, 2013 at 6:44 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> [dropped patches@apm.com]
>
> Vinayak,
>
> Please keep reviewers on CC, as it makes easier to track the changes.
Sure, will do.
>
> On 20/11/13 11:13, Vinayak Kale wrote:
>> Add support for irq registration when pmu interrupt is percpu.
>>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> Signed-off-by: Tuan Phan <tphan@apm.com>
>> ---
>>  arch/arm64/kernel/perf_event.c |  108 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 81 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index cea1594..de12ba8 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -22,6 +22,7 @@
>>
>>  #include <linux/bitmap.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/irq.h>
>>  #include <linux/kernel.h>
>>  #include <linux/export.h>
>>  #include <linux/perf_event.h>
>> @@ -363,22 +364,55 @@ validate_group(struct perf_event *event)
>>  }
>>
>>  static void
>> +armpmu_disable_percpu_irq(void *data)
>> +{
>> +     struct arm_pmu *armpmu = data;
>> +     struct platform_device *pmu_device = armpmu->plat_device;
>> +     int irq = platform_get_irq(pmu_device, 0);
>> +
>> +     cpumask_test_and_clear_cpu(smp_processor_id(), &armpmu->active_irqs);
>> +     disable_percpu_irq(irq);
>> +}
>> +
>> +static void
>>  armpmu_release_hardware(struct arm_pmu *armpmu)
>>  {
>>       int i, irq, irqs;
>>       struct platform_device *pmu_device = armpmu->plat_device;
>>
>>       irqs = min(pmu_device->num_resources, num_possible_cpus());
>> +     if (irqs < 1)
>> +             return;
>>
>> -     for (i = 0; i < irqs; ++i) {
>> -             if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
>> -                     continue;
>> -             irq = platform_get_irq(pmu_device, i);
>> -             if (irq >= 0)
>> -                     free_irq(irq, armpmu);
>> +     irq = platform_get_irq(pmu_device, 0);
>> +     if (irq < 0)
>
> So I'm going to sound like a stuck record: irq == 0 is not a valid IRQ
> number, full stop. This is how we express the fact that there is no IRQ.
>
> If you're touching that code, you might as well fix this buglet.
In Will's existing code, I think he was taking care of 'no IRQ' case
by comparing pmu_device->num_resources. Do you think this is not
enough and we must enforce the check after each platform_get_irq()?
Existing driver code snippet as below for quick reference.

[snip]
static int
armpmu_reserve_hardware(struct arm_pmu *armpmu)
{
        int i, err, irq, irqs;
        struct platform_device *pmu_device = armpmu->plat_device;

        if (!pmu_device) {
                pr_err("no PMU device registered\n");
                return -ENODEV;
        }

        irqs = min(pmu_device->num_resources, num_possible_cpus());
        if (irqs < 1) {
                pr_err("no irqs for PMUs defined\n");
                return -ENODEV;
        }
        for (i = 0; i < irqs; ++i) {
                err = 0;
                irq = platform_get_irq(pmu_device, i);
                if (irq < 0)
                         continue;

[snip]
>
>> +             return;
>> +
>> +     if (irq_is_percpu(irq)) {
>> +             on_each_cpu(armpmu_disable_percpu_irq, armpmu, 1);
>> +             free_percpu_irq(irq, &cpu_hw_events);
>> +     } else {
>> +             for (i = 0; i < irqs; ++i) {
>> +                     if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
>> +                             continue;
>> +                     irq = platform_get_irq(pmu_device, i);
>> +                     if (irq >= 0)
>
> Same here.
>
>> +                             free_irq(irq, armpmu);
>> +             }
>>       }
>>  }
>>
>> +static void
>> +armpmu_enable_percpu_irq(void *data)
>> +{
>> +     struct arm_pmu *armpmu = data;
>> +     struct platform_device *pmu_device = armpmu->plat_device;
>> +     int irq = platform_get_irq(pmu_device, 0);
>> +
>> +     enable_percpu_irq(irq, 0);
>> +     cpumask_set_cpu(smp_processor_id(), &armpmu->active_irqs);
>> +}
>> +
>>  static int
>>  armpmu_reserve_hardware(struct arm_pmu *armpmu)
>>  {
>> @@ -396,34 +430,54 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
>>               return -ENODEV;
>>       }
>>
>> -     for (i = 0; i < irqs; ++i) {
>> -             err = 0;
>> -             irq = platform_get_irq(pmu_device, i);
>> -             if (irq < 0)
>> -                     continue;
>> +     irq = platform_get_irq(pmu_device, 0);
>> +     if (irq < 0) {
>
> And here.
>
>> +             pr_err("failed to get an irq for PMU device\n");
>> +             return -ENODEV;
>> +     }
>>
>> -             /*
>> -              * If we have a single PMU interrupt that we can't shift,
>> -              * assume that we're running on a uniprocessor machine and
>> -              * continue. Otherwise, continue without this interrupt.
>> -              */
>> -             if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
>> -                     pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
>> -                                 irq, i);
>> -                     continue;
>> -             }
>> +     if (irq_is_percpu(irq)) {
>> +             err = request_percpu_irq(irq, armpmu->handle_irq,
>> +                             "arm-pmu", &cpu_hw_events);
>>
>> -             err = request_irq(irq, armpmu->handle_irq,
>> -                               IRQF_NOBALANCING,
>> -                               "arm-pmu", armpmu);
>>               if (err) {
>> -                     pr_err("unable to request IRQ%d for ARM PMU counters\n",
>> -                             irq);
>> +                     pr_err("unable to request percpu IRQ%d for ARM PMU counters\n",
>> +                                     irq);
>>                       armpmu_release_hardware(armpmu);
>>                       return err;
>>               }
>>
>> -             cpumask_set_cpu(i, &armpmu->active_irqs);
>> +             on_each_cpu(armpmu_enable_percpu_irq, armpmu, 1);
>> +     } else {
>> +             for (i = 0; i < irqs; ++i) {
>> +                     err = 0;
>> +                     irq = platform_get_irq(pmu_device, i);
>> +                     if (irq < 0)
>
> And here.
>
>> +                             continue;
>> +
>> +                     /*
>> +                      * If we have a single PMU interrupt that we can't shift,
>> +                      * assume that we're running on a uniprocessor machine and
>> +                      * continue. Otherwise, continue without this interrupt.
>> +                      */
>> +                     if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
>> +                             pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
>> +                                             irq, i);
>> +                             continue;
>> +                     }
>> +
>> +                     err = request_irq(irq, armpmu->handle_irq,
>> +                                     IRQF_NOBALANCING,
>> +                                     "arm-pmu", armpmu);
>> +                     if (err) {
>> +                             pr_err("unable to request IRQ%d for ARM PMU counters\n",
>> +                                             irq);
>> +                             armpmu_release_hardware(armpmu);
>> +                             return err;
>> +                     }
>> +
>> +                     cpumask_set_cpu(i, &armpmu->active_irqs);
>> +             }
>>       }
>>
>>       return 0;
>>
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

Re: [PATCH V4 2/2] arm64: perf: add support for percpu pmu interrupt

[Will Deacon]

On Wed, Nov 20, 2013 at 05:28:50PM +0000, Vinayak Kale wrote:
> In Will's existing code, I think he was taking care of 'no IRQ' case
> by comparing pmu_device->num_resources. Do you think this is not
> enough and we must enforce the check after each platform_get_irq()?
> Existing driver code snippet as below for quick reference.
> 
> [snip]
> static int
> armpmu_reserve_hardware(struct arm_pmu *armpmu)
> {
>         int i, err, irq, irqs;
>         struct platform_device *pmu_device = armpmu->plat_device;
> 
>         if (!pmu_device) {
>                 pr_err("no PMU device registered\n");
>                 return -ENODEV;
>         }
> 
>         irqs = min(pmu_device->num_resources, num_possible_cpus());
>         if (irqs < 1) {
>                 pr_err("no irqs for PMUs defined\n");
>                 return -ENODEV;
>         }

This bit is fine.

>         for (i = 0; i < irqs; ++i) {
>                 err = 0;
>                 irq = platform_get_irq(pmu_device, i);
>                 if (irq < 0)
>                          continue;

This is a bug, which you can fix in your patch. IRQ0 isn't valid.

Will

Re: [PATCH V4 2/2] arm64: perf: add support for percpu pmu interrupt

[Vinayak Kale]

On Wed, Nov 20, 2013 at 11:30 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Nov 20, 2013 at 05:28:50PM +0000, Vinayak Kale wrote:
>> In Will's existing code, I think he was taking care of 'no IRQ' case
>> by comparing pmu_device->num_resources. Do you think this is not
>> enough and we must enforce the check after each platform_get_irq()?
>> Existing driver code snippet as below for quick reference.
>>
>> [snip]
>> static int
>> armpmu_reserve_hardware(struct arm_pmu *armpmu)
>> {
>>         int i, err, irq, irqs;
>>         struct platform_device *pmu_device = armpmu->plat_device;
>>
>>         if (!pmu_device) {
>>                 pr_err("no PMU device registered\n");
>>                 return -ENODEV;
>>         }
>>
>>         irqs = min(pmu_device->num_resources, num_possible_cpus());
>>         if (irqs < 1) {
>>                 pr_err("no irqs for PMUs defined\n");
>>                 return -ENODEV;
>>         }
>
> This bit is fine.
>
>>         for (i = 0; i < irqs; ++i) {
>>                 err = 0;
>>                 irq = platform_get_irq(pmu_device, i);
>>                 if (irq < 0)
>>                          continue;
>
> This is a bug, which you can fix in your patch. IRQ0 isn't valid.

Okay, will fix this.