diff mbox

[09/11] arm64: pmu: Add routines for detecting differing PMU types in the system

Message ID 1466529109-21715-10-git-send-email-jeremy.linton@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeremy Linton June 21, 2016, 5:11 p.m. UTC
In preparation for enabling heterogeneous PMUs on ACPI systems
add routines that detect this and group the resulting PMUs and
interrupts.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/perf/arm_pmu_acpi.c | 137 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 3 deletions(-)

Comments

Punit Agrawal July 1, 2016, 1:58 p.m. UTC | #1
Jeremy Linton <jeremy.linton@arm.com> writes:

> In preparation for enabling heterogeneous PMUs on ACPI systems
> add routines that detect this and group the resulting PMUs and
> interrupts.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/perf/arm_pmu_acpi.c | 137 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index a24cdd0..482a54d 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -1,23 +1,36 @@
>  /*
> - * PMU support
> + * ARM ACPI PMU support
>   *
>   * Copyright (C) 2015 Red Hat Inc.
> + * Copyright (C) 2016 ARM Ltd.
>   * Author: Mark Salter <msalter@redhat.com>
> + *         Jeremy Linton <jeremy.linton@arm.com>
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2.  See
>   * the COPYING file in the top-level directory.
>   *
>   */
>  
> +#define pr_fmt(fmt) "ACPI-PMU: " fmt
> +
> +#include <asm/cpu.h>
>  #include <linux/perf/arm_pmu.h>
>  #include <linux/platform_device.h>
>  #include <linux/acpi.h>
>  #include <linux/irq.h>
>  #include <linux/irqdesc.h>
> +#include <linux/list.h>
>  
>  struct pmu_irq {
> -	int gsi;
> -	int trigger;
> +	int  gsi;
> +	int  trigger;
> +	bool registered;
> +};
> +
> +struct pmu_types {
> +	struct list_head list;
> +	int		 cpu_type;
> +	int		 cpu_count;
>  };

You can stash the associated resources in the above structure. That
should simplify some code below.

>  
>  static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
> @@ -36,6 +49,124 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>  		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>  }
>  
> +/* Count number and type of CPU cores in the system. */
> +void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
> +{
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
> +		struct pmu_types *pmu;
> +
> +		list_for_each_entry(pmu, pmus, list) {
> +			if (pmu->cpu_type == partnum) {
> +				pmu->cpu_count++;
> +				break;
> +			}
> +		}
> +
> +		/* we didn't find the CPU type, add an entry to identify it */
> +		if (&pmu->list == pmus) {
> +			pmu = kcalloc(1, sizeof(struct pmu_types), GFP_KERNEL);

Use kzalloc here.

> +			if (!pmu) {
> +				pr_warn("Unable to allocate pmu_types\n");

Bail out with error if the memory can't be allocated. Otherwise, we risk
silently failing to register a PMU type.

> +			} else {
> +				pmu->cpu_type = partnum;
> +				pmu->cpu_count++;
> +				list_add_tail(&pmu->list, pmus);
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
> + * This group utilizes 'count' resources in the 'res'.
> + */
> +int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
> +					    int last_cpu_id)
> +{

With the addition of the irq resources to struct pmu_types, you can just pass
the pmu structure here.

> +	int i;
> +	int err = -ENOMEM;
> +	bool free_gsi = false;
> +	struct platform_device *pdev;
> +
> +	if (count) {

        if (!count)
           goto out;

That should help reduce the nesting below. Others might have a different
opinion, but I think it's ok to use goto when it helps make the code
more readable.

Similarly, some of the code below can be simplified as well.

> +		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
> +		if (pdev) {
> +			err = platform_device_add_resources(pdev, res, count);
> +			if (!err) {
> +				err = platform_device_add(pdev);
> +				if (err) {
> +					pr_warn("Unable to register PMU device\n");
> +					free_gsi = true;
> +				}
> +			} else {
> +				pr_warn("Unable to add resources to device\n");
> +				free_gsi = true;
> +				platform_device_put(pdev);
> +			}
> +		} else {
> +			pr_warn("Unable to allocate platform device\n");
> +			free_gsi = true;
> +		}
> +	}
> +
> +	/* unmark (and possibly unregister) registered GSIs */
> +	for_each_possible_cpu(i) {
> +		if (pmu_irqs[i].registered) {
> +			if (free_gsi)
> +				acpi_unregister_gsi(pmu_irqs[i].gsi);
> +			pmu_irqs[i].registered = false;
> +		}
> +	}
> +

out:

> +	return err;
> +}
> +
> +/*
> + * For the given cpu/pmu type, walk all known GSIs, register them, and add
> + * them to the resource structure. Return the number of GSI's contained
> + * in the res structure, and the id of the last CPU/PMU we added.
> + */
> +int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
> +				       struct resource *res, int *last_cpu_id)

With struct resource as part of the pmu_types structure you can drop the
last two arguments and allocate the resources in this function.

> +{
> +	int i, count;
> +	int irq;
> +
> +	pr_info("Setting up %d PMUs for CPU type %X\n", pmus->cpu_count,
> +							pmus->cpu_type);

Please drop this pr_info.

> +	/* lets group all the PMU's from similar CPU's together */
> +	count = 0;
> +	for_each_possible_cpu(i) {
> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
> +
> +		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {

You can invert the condition check here and reduce nesting.

> +			if (pmu_irqs[i].gsi == 0)
> +				continue;
> +
> +			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
> +						pmu_irqs[i].trigger,
> +						ACPI_ACTIVE_HIGH);
> +
> +			res[count].start = res[count].end = irq;
> +			res[count].flags = IORESOURCE_IRQ;
> +
> +			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
> +				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
> +			else
> +				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
> +
> +			pmu_irqs[i].registered = true;
> +			count++;
> +			(*last_cpu_id) = cinfo->reg_midr;
> +		}
> +	}
> +	return count;
> +}
> +
>  static int __init pmu_acpi_init(void)
>  {
>  	struct platform_device *pdev;
Jeremy Linton July 1, 2016, 2:54 p.m. UTC | #2
On 07/01/2016 08:58 AM, Punit Agrawal wrote:
> Jeremy Linton <jeremy.linton@arm.com> writes:
>
>> In preparation for enabling heterogeneous PMUs on ACPI systems
>> add routines that detect this and group the resulting PMUs and
>> interrupts.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   drivers/perf/arm_pmu_acpi.c | 137 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 134 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>> index a24cdd0..482a54d 100644
>> --- a/drivers/perf/arm_pmu_acpi.c
>> +++ b/drivers/perf/arm_pmu_acpi.c
>> @@ -1,23 +1,36 @@
>>   /*
>> - * PMU support
>> + * ARM ACPI PMU support
>>    *
>>    * Copyright (C) 2015 Red Hat Inc.
>> + * Copyright (C) 2016 ARM Ltd.
>>    * Author: Mark Salter <msalter@redhat.com>
>> + *         Jeremy Linton <jeremy.linton@arm.com>
>>    *
>>    * This work is licensed under the terms of the GNU GPL, version 2.  See
>>    * the COPYING file in the top-level directory.
>>    *
>>    */
>>
>> +#define pr_fmt(fmt) "ACPI-PMU: " fmt
>> +
>> +#include <asm/cpu.h>
>>   #include <linux/perf/arm_pmu.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/acpi.h>
>>   #include <linux/irq.h>
>>   #include <linux/irqdesc.h>
>> +#include <linux/list.h>
>>
>>   struct pmu_irq {
>> -	int gsi;
>> -	int trigger;
>> +	int  gsi;
>> +	int  trigger;
>> +	bool registered;
>> +};
>> +
>> +struct pmu_types {
>> +	struct list_head list;
>> +	int		 cpu_type;
>> +	int		 cpu_count;
>>   };
>
> You can stash the associated resources in the above structure. That
> should simplify some code below.

	How is that? One structure is per cpu, the other is per pmu type in the 
system, they are actually completely independent and intertwining them 
will only server to obfuscate the code.


>
>>
>>   static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
>> @@ -36,6 +49,124 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>>   		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>>   }
>>
>> +/* Count number and type of CPU cores in the system. */
>> +void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
>> +{
>> +	int i;
>> +
>> +	for_each_possible_cpu(i) {
>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
>> +		struct pmu_types *pmu;
>> +
>> +		list_for_each_entry(pmu, pmus, list) {
>> +			if (pmu->cpu_type == partnum) {
>> +				pmu->cpu_count++;
>> +				break;
>> +			}
>> +		}
>> +
>> +		/* we didn't find the CPU type, add an entry to identify it */
>> +		if (&pmu->list == pmus) {
>> +			pmu = kcalloc(1, sizeof(struct pmu_types), GFP_KERNEL);
>
> Use kzalloc here.
Ok fair point.

>
>> +			if (!pmu) {
>> +				pr_warn("Unable to allocate pmu_types\n");
>
> Bail out with error if the memory can't be allocated. Otherwise, we risk
> silently failing to register a PMU type.

? Its not silent, it fails to allocate the space complains about it, and 
therefor this pmu type is not created. In a system with a single CPU 
this basically cancels the whole operation. If there is more than one 
pmu, the remaining PMUs continue to have a chance of being created, 
although if the memory allocation fails (this is pretty early boot code) 
there is a high probability there is something seriously wrong with the 
system.


>
>> +			} else {
>> +				pmu->cpu_type = partnum;
>> +				pmu->cpu_count++;
>> +				list_add_tail(&pmu->list, pmus);
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +/*
>> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
>> + * This group utilizes 'count' resources in the 'res'.
>> + */
>> +int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
>> +					    int last_cpu_id)
>> +{
>
> With the addition of the irq resources to struct pmu_types, you can just pass
> the pmu structure here.

	Thats a point, but the lifetimes of the structures are different and 
outside of their shared use in this single function never really 
interact. I prefer not unnecessarily intertwine independent data 
structures simply to reduce parameter counts for a single function. 
Especially since it complicates cleanup because the validity of the 
resource structure will have to be tracked relative to its successful 
registration.
Jeremy Linton July 1, 2016, 3:28 p.m. UTC | #3
On 07/01/2016 08:58 AM, Punit Agrawal wrote:
(trimming)
>> +			if (!pmu) {
>> +				pr_warn("Unable to allocate pmu_types\n");
>
> Bail out with error if the memory can't be allocated. Otherwise, we risk
> silently failing to register a PMU type.

Reading this again, I think I misunderstood what you were saying.. Aka, 
this should be pr_error() not that the error handing is failing to catch 
the failure.

Anyway, I will promote this.
Punit Agrawal July 1, 2016, 3:43 p.m. UTC | #4
Jeremy Linton <jeremy.linton@arm.com> writes:

> On 07/01/2016 08:58 AM, Punit Agrawal wrote:
>> Jeremy Linton <jeremy.linton@arm.com> writes:
>>
>>> In preparation for enabling heterogeneous PMUs on ACPI systems
>>> add routines that detect this and group the resulting PMUs and
>>> interrupts.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>   drivers/perf/arm_pmu_acpi.c | 137 +++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 134 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>>> index a24cdd0..482a54d 100644
>>> --- a/drivers/perf/arm_pmu_acpi.c
>>> +++ b/drivers/perf/arm_pmu_acpi.c
>>> @@ -1,23 +1,36 @@
>>>   /*
>>> - * PMU support
>>> + * ARM ACPI PMU support
>>>    *
>>>    * Copyright (C) 2015 Red Hat Inc.
>>> + * Copyright (C) 2016 ARM Ltd.
>>>    * Author: Mark Salter <msalter@redhat.com>
>>> + *         Jeremy Linton <jeremy.linton@arm.com>
>>>    *
>>>    * This work is licensed under the terms of the GNU GPL, version 2.  See
>>>    * the COPYING file in the top-level directory.
>>>    *
>>>    */
>>>
>>> +#define pr_fmt(fmt) "ACPI-PMU: " fmt
>>> +
>>> +#include <asm/cpu.h>
>>>   #include <linux/perf/arm_pmu.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/acpi.h>
>>>   #include <linux/irq.h>
>>>   #include <linux/irqdesc.h>
>>> +#include <linux/list.h>
>>>
>>>   struct pmu_irq {
>>> -	int gsi;
>>> -	int trigger;
>>> +	int  gsi;
>>> +	int  trigger;
>>> +	bool registered;
>>> +};
>>> +
>>> +struct pmu_types {
>>> +	struct list_head list;
>>> +	int		 cpu_type;
>>> +	int		 cpu_count;
>>>   };
>>
>> You can stash the associated resources in the above structure. That
>> should simplify some code below.
>
> 	How is that? One structure is per cpu, the other is per pmu
> type in the system, they are actually completely independent and
> intertwining them will only server to obfuscate the code.

Just to clarify, I am referring to "struct resources" you allocate for
use with the pmu platform device in the next patch.

>
>
>>
>>>
>>>   static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
>>> @@ -36,6 +49,124 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>>>   		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>>>   }
>>>
>>> +/* Count number and type of CPU cores in the system. */
>>> +void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
>>> +{
>>> +	int i;
>>> +
>>> +	for_each_possible_cpu(i) {
>>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>>> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
>>> +		struct pmu_types *pmu;
>>> +
>>> +		list_for_each_entry(pmu, pmus, list) {
>>> +			if (pmu->cpu_type == partnum) {
>>> +				pmu->cpu_count++;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		/* we didn't find the CPU type, add an entry to identify it */
>>> +		if (&pmu->list == pmus) {
>>> +			pmu = kcalloc(1, sizeof(struct pmu_types), GFP_KERNEL);
>>
>> Use kzalloc here.
> Ok fair point.
>
>>
>>> +			if (!pmu) {
>>> +				pr_warn("Unable to allocate pmu_types\n");
>>
>> Bail out with error if the memory can't be allocated. Otherwise, we risk
>> silently failing to register a PMU type.
>
> ? Its not silent, it fails to allocate the space complains about it,
> and therefor this pmu type is not created.

Sorry for the confusion, I didn't mean PMU type in the above comment. I
was thinking of the missed cpu in cpu_count.

If the allocation succeeds in the next iteration (as we don't break out
of the loop), we'll end up undercounting cpus that have this type of
pmu. In turn, this will lead to accessing beyond the allocated "struct
resource" in arm_pmu_acpi_gsi_res.

Punit

> In a system with a single
> CPU this basically cancels the whole operation. If there is more than
> one pmu, the remaining PMUs continue to have a chance of being
> created, although if the memory allocation fails (this is pretty early
> boot code) there is a high probability there is something seriously
> wrong with the system.
>
>
>>
>>> +			} else {
>>> +				pmu->cpu_type = partnum;
>>> +				pmu->cpu_count++;
>>> +				list_add_tail(&pmu->list, pmus);
>>> +			}
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
>>> + * This group utilizes 'count' resources in the 'res'.
>>> + */
>>> +int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
>>> +					    int last_cpu_id)
>>> +{
>>
>> With the addition of the irq resources to struct pmu_types, you can just pass
>> the pmu structure here.
>
> 	Thats a point, but the lifetimes of the structures are
> different and outside of their shared use in this single function
> never really interact. I prefer not unnecessarily intertwine
> independent data structures simply to reduce parameter counts for a
> single function. Especially since it complicates cleanup because the
> validity of the resource structure will have to be tracked relative to
> its successful registration.
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeremy Linton July 1, 2016, 4:21 p.m. UTC | #5
On 07/01/2016 10:43 AM, Punit Agrawal wrote:
> Jeremy Linton <jeremy.linton@arm.com> writes:
>
>> On 07/01/2016 08:58 AM, Punit Agrawal wrote:
>>> Jeremy Linton <jeremy.linton@arm.com> writes:
>>>
>>>> In preparation for enabling heterogeneous PMUs on ACPI systems
>>>> add routines that detect this and group the resulting PMUs and
>>>> interrupts.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> ---
>>>>    drivers/perf/arm_pmu_acpi.c | 137 +++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 134 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>>>> index a24cdd0..482a54d 100644
>>>> --- a/drivers/perf/arm_pmu_acpi.c
>>>> +++ b/drivers/perf/arm_pmu_acpi.c
>>>> @@ -1,23 +1,36 @@
>>>>    /*
>>>> - * PMU support
>>>> + * ARM ACPI PMU support
>>>>     *
>>>>     * Copyright (C) 2015 Red Hat Inc.
>>>> + * Copyright (C) 2016 ARM Ltd.
>>>>     * Author: Mark Salter <msalter@redhat.com>
>>>> + *         Jeremy Linton <jeremy.linton@arm.com>
>>>>     *
>>>>     * This work is licensed under the terms of the GNU GPL, version 2.  See
>>>>     * the COPYING file in the top-level directory.
>>>>     *
>>>>     */
>>>>
>>>> +#define pr_fmt(fmt) "ACPI-PMU: " fmt
>>>> +
>>>> +#include <asm/cpu.h>
>>>>    #include <linux/perf/arm_pmu.h>
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/acpi.h>
>>>>    #include <linux/irq.h>
>>>>    #include <linux/irqdesc.h>
>>>> +#include <linux/list.h>
>>>>
>>>>    struct pmu_irq {
>>>> -	int gsi;
>>>> -	int trigger;
>>>> +	int  gsi;
>>>> +	int  trigger;
>>>> +	bool registered;
>>>> +};
>>>> +
>>>> +struct pmu_types {
>>>> +	struct list_head list;
>>>> +	int		 cpu_type;
>>>> +	int		 cpu_count;
>>>>    };
>>>
>>> You can stash the associated resources in the above structure. That
>>> should simplify some code below.
>>
>> 	How is that? One structure is per cpu, the other is per pmu
>> type in the system, they are actually completely independent and
>> intertwining them will only server to obfuscate the code.
>
> Just to clarify, I am referring to "struct resources" you allocate for
> use with the pmu platform device in the next patch.

Ah yes, I understood that for the later comment but for this one I 
thought you were suggesting nesting these too. Thanks for clarifying.

>
>>
>>
>>>
>>>>
>>>>    static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
>>>> @@ -36,6 +49,124 @@ void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
>>>>    		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
>>>>    }
>>>>
>>>> +/* Count number and type of CPU cores in the system. */
>>>> +void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for_each_possible_cpu(i) {
>>>> +		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
>>>> +		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
>>>> +		struct pmu_types *pmu;
>>>> +
>>>> +		list_for_each_entry(pmu, pmus, list) {
>>>> +			if (pmu->cpu_type == partnum) {
>>>> +				pmu->cpu_count++;
>>>> +				break;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		/* we didn't find the CPU type, add an entry to identify it */
>>>> +		if (&pmu->list == pmus) {
>>>> +			pmu = kcalloc(1, sizeof(struct pmu_types), GFP_KERNEL);
>>>
>>> Use kzalloc here.
>> Ok fair point.
>>
>>>
>>>> +			if (!pmu) {
>>>> +				pr_warn("Unable to allocate pmu_types\n");
>>>
>>> Bail out with error if the memory can't be allocated. Otherwise, we risk
>>> silently failing to register a PMU type.
>>
>> ? Its not silent, it fails to allocate the space complains about it,
>> and therefor this pmu type is not created.
>
> Sorry for the confusion, I didn't mean PMU type in the above comment. I
> was thinking of the missed cpu in cpu_count.
>
> If the allocation succeeds in the next iteration (as we don't break out
> of the loop), we'll end up undercounting cpus that have this type of
> pmu. In turn, this will lead to accessing beyond the allocated "struct
> resource" in arm_pmu_acpi_gsi_res.

Oh! Yah, that is a good point. I guess I missed that when i converted 
from the fixed buffer (which couldn't have this problem).

Actually I think i'm going to add a bad_alloc flag to stop further 
allocation attempts in this function. That way in theory the other side 
of the problem doesn't occur either (aka we get one allocation that 
succeeds, then one that fails, and the cpu's get under counted for the 
one that succeeded).




>
> Punit
>
>> In a system with a single
>> CPU this basically cancels the whole operation. If there is more than
>> one pmu, the remaining PMUs continue to have a chance of being
>> created, although if the memory allocation fails (this is pretty early
>> boot code) there is a high probability there is something seriously
>> wrong with the system.
>>
>>
>>>
>>>> +			} else {
>>>> +				pmu->cpu_type = partnum;
>>>> +				pmu->cpu_count++;
>>>> +				list_add_tail(&pmu->list, pmus);
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +}
>>>> +
>>>> +/*
>>>> + * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
>>>> + * This group utilizes 'count' resources in the 'res'.
>>>> + */
>>>> +int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
>>>> +					    int last_cpu_id)
>>>> +{
>>>
>>> With the addition of the irq resources to struct pmu_types, you can just pass
>>> the pmu structure here.
>>
>> 	Thats a point, but the lifetimes of the structures are
>> different and outside of their shared use in this single function
>> never really interact. I prefer not unnecessarily intertwine
>> independent data structures simply to reduce parameter counts for a
>> single function. Especially since it complicates cleanup because the
>> validity of the resource structure will have to be tracked relative to
>> its successful registration.
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index a24cdd0..482a54d 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -1,23 +1,36 @@ 
 /*
- * PMU support
+ * ARM ACPI PMU support
  *
  * Copyright (C) 2015 Red Hat Inc.
+ * Copyright (C) 2016 ARM Ltd.
  * Author: Mark Salter <msalter@redhat.com>
+ *         Jeremy Linton <jeremy.linton@arm.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
  *
  */
 
+#define pr_fmt(fmt) "ACPI-PMU: " fmt
+
+#include <asm/cpu.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
 #include <linux/acpi.h>
 #include <linux/irq.h>
 #include <linux/irqdesc.h>
+#include <linux/list.h>
 
 struct pmu_irq {
-	int gsi;
-	int trigger;
+	int  gsi;
+	int  trigger;
+	bool registered;
+};
+
+struct pmu_types {
+	struct list_head list;
+	int		 cpu_type;
+	int		 cpu_count;
 };
 
 static struct pmu_irq pmu_irqs[NR_CPUS] __initdata;
@@ -36,6 +49,124 @@  void __init arm_pmu_parse_acpi(int cpu, struct acpi_madt_generic_interrupt *gic)
 		pmu_irqs[cpu].trigger = ACPI_LEVEL_SENSITIVE;
 }
 
+/* Count number and type of CPU cores in the system. */
+void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus)
+{
+	int i;
+
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+		u32 partnum = MIDR_PARTNUM(cinfo->reg_midr);
+		struct pmu_types *pmu;
+
+		list_for_each_entry(pmu, pmus, list) {
+			if (pmu->cpu_type == partnum) {
+				pmu->cpu_count++;
+				break;
+			}
+		}
+
+		/* we didn't find the CPU type, add an entry to identify it */
+		if (&pmu->list == pmus) {
+			pmu = kcalloc(1, sizeof(struct pmu_types), GFP_KERNEL);
+			if (!pmu) {
+				pr_warn("Unable to allocate pmu_types\n");
+			} else {
+				pmu->cpu_type = partnum;
+				pmu->cpu_count++;
+				list_add_tail(&pmu->list, pmus);
+			}
+		}
+	}
+}
+
+/*
+ * Registers the group of PMU interfaces which correspond to the 'last_cpu_id'.
+ * This group utilizes 'count' resources in the 'res'.
+ */
+int __init arm_pmu_acpi_register_pmu(int count, struct resource *res,
+					    int last_cpu_id)
+{
+	int i;
+	int err = -ENOMEM;
+	bool free_gsi = false;
+	struct platform_device *pdev;
+
+	if (count) {
+		pdev = platform_device_alloc(ARMV8_PMU_PDEV_NAME, last_cpu_id);
+		if (pdev) {
+			err = platform_device_add_resources(pdev, res, count);
+			if (!err) {
+				err = platform_device_add(pdev);
+				if (err) {
+					pr_warn("Unable to register PMU device\n");
+					free_gsi = true;
+				}
+			} else {
+				pr_warn("Unable to add resources to device\n");
+				free_gsi = true;
+				platform_device_put(pdev);
+			}
+		} else {
+			pr_warn("Unable to allocate platform device\n");
+			free_gsi = true;
+		}
+	}
+
+	/* unmark (and possibly unregister) registered GSIs */
+	for_each_possible_cpu(i) {
+		if (pmu_irqs[i].registered) {
+			if (free_gsi)
+				acpi_unregister_gsi(pmu_irqs[i].gsi);
+			pmu_irqs[i].registered = false;
+		}
+	}
+
+	return err;
+}
+
+/*
+ * For the given cpu/pmu type, walk all known GSIs, register them, and add
+ * them to the resource structure. Return the number of GSI's contained
+ * in the res structure, and the id of the last CPU/PMU we added.
+ */
+int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus,
+				       struct resource *res, int *last_cpu_id)
+{
+	int i, count;
+	int irq;
+
+	pr_info("Setting up %d PMUs for CPU type %X\n", pmus->cpu_count,
+							pmus->cpu_type);
+	/* lets group all the PMU's from similar CPU's together */
+	count = 0;
+	for_each_possible_cpu(i) {
+		struct cpuinfo_arm64 *cinfo = per_cpu_ptr(&cpu_data, i);
+
+		if (pmus->cpu_type == MIDR_PARTNUM(cinfo->reg_midr)) {
+			if (pmu_irqs[i].gsi == 0)
+				continue;
+
+			irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi,
+						pmu_irqs[i].trigger,
+						ACPI_ACTIVE_HIGH);
+
+			res[count].start = res[count].end = irq;
+			res[count].flags = IORESOURCE_IRQ;
+
+			if (pmu_irqs[i].trigger == ACPI_EDGE_SENSITIVE)
+				res[count].flags |= IORESOURCE_IRQ_HIGHEDGE;
+			else
+				res[count].flags |= IORESOURCE_IRQ_HIGHLEVEL;
+
+			pmu_irqs[i].registered = true;
+			count++;
+			(*last_cpu_id) = cinfo->reg_midr;
+		}
+	}
+	return count;
+}
+
 static int __init pmu_acpi_init(void)
 {
 	struct platform_device *pdev;