| Message ID | 1471985280-2243-9-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeremy, Jeremy Linton <jeremy.linton@arm.com> writes: > Its possible that an ACPI system has multiple CPU types in it > with differing PMU counters. Iterate the CPU's and make a determination > about how many of each type exist in the system. Then take and create > a PMU platform device for each type, and assign it the interrupts parsed > from the MADT. Creating a platform device is necessary because the PMUs > are not described as devices in the DSDT table. > > This code is loosely based on earlier work by Mark Salter. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Thanks for squashing changes to arm_pmu_acpi.c from different patches in v6 into one patch. Except for the a function definition in Patch 5 that can be moved here I think you've got everything. The combined patch is a lot easier to review. Some comments below. > --- > drivers/perf/arm_pmu.c | 7 +- > drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 170 insertions(+), 1 deletion(-) > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > index ee9e301..98a037a 100644 > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev, > if (!ret) > ret = init_fn(pmu); > } else if (probe_table) { > - ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id()); > + if (acpi_disabled) { > + /* use the current cpu. */ > + ret = probe_plat_pmu(pmu, probe_table, > + read_cpuid_id()); > + } else > + ret = probe_plat_pmu(pmu, probe_table, pdev->id); Please add matching braces on both sides of the else. > } > > if (ret) { > diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c > index e784714..c0d6888 100644 > --- a/drivers/perf/arm_pmu_acpi.c > +++ b/drivers/perf/arm_pmu_acpi.c [...] > @@ -39,13 +49,167 @@ 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. */ > +static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus) > +{ > + int i; > + bool alloc_failure = false; > + > + 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) && (!alloc_failure)) { The parenthesis around the conditions can be dropped. > + pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL); > + if (!pmu) { > + pr_warn("Unable to allocate pmu_types\n"); > + /* > + * continue to count cpus for any pmu_types > + * already allocated, but don't allocate any > + * more pmu_types. This avoids undercounting. > + */ > + alloc_failure = true; Why not just fail probe and return an error? What is the benefit of having some of the PMUs available? > + } 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'. > + */ > +static int __init arm_pmu_acpi_register_pmu(int count, struct resource *res, > + int last_cpu_id) Please drop the prefix "last_". AFAICS, it doesn't provide any information. > +{ > + 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; > + } > + } This entire "if" block is quite hard to review. Quoting Documentation/CodingStyle, "if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program." > + > + /* 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; > + } > + } Moving the for_each_possible_cpu block out of this function should help makes things simpler. It doesn't have any connection to registering the platform device and you could then do if (!count) return count; which should help reduce a level of indentation. But you can further use the same approach with other conditions in the block as well. > + > + 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. > + */ > +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus, > + struct resource *res, int *last_cpu_id) > +{ > + int i, count; > + int irq; > + > + /* 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; Please don't silently continue if the irq is missing. It deserves a user visible message. We don't want users complaining about kernel issues when the firmware fails to provide the required information. > + > + irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi, > + pmu_irqs[i].trigger, > + ACPI_ACTIVE_HIGH); Check for the return value of acpi_register_gsi as it can return an error. > + > + 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; What is the benefit of using the entire MIDR for cpu_id when the grouping is done on the basis of a subset, i.e., part number. > + } > + } > + return count; > +} > + > static int __init pmu_acpi_init(void) > { > + struct resource *res; > int err = -ENOMEM; > + int count, cpu_id; > + struct pmu_types *pmu, *safe_temp; > + LIST_HEAD(pmus); > > if (acpi_disabled) > return 0; > > + arm_pmu_acpi_determine_cpu_types(&pmus); > + > + list_for_each_entry_safe(pmu, safe_temp, &pmus, list) { > + res = kcalloc(pmu->cpu_count, > + sizeof(struct resource), GFP_KERNEL); > + > + /* for a given PMU type collect all the GSIs. */ > + if (res) { > + count = arm_pmu_acpi_gsi_res(pmu, res, > + &cpu_id); > + /* > + * register this set of interrupts > + * with a new PMU device > + */ > + err = arm_pmu_acpi_register_pmu(count, res, cpu_id); > + if (!err) > + pr_info("Registered %d devices for %X\n", > + count, pmu->cpu_type); > + kfree(res); > + } else > + pr_warn("PMU unable to allocate interrupt resource space\n"); Same comment about partial registration as above. It's better to error out IMO. Also if this stays, please use matching parenthesis on both sides of the else block. Thanks, Punit > + > + list_del(&pmu->list); > + kfree(pmu); > + } > + > return err; > } > + > arch_initcall(pmu_acpi_init);
Hi, On 08/26/2016 10:04 AM, Punit Agrawal wrote: (trimming) >> + pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL); >> + if (!pmu) { >> + pr_warn("Unable to allocate pmu_types\n"); >> + /* >> + * continue to count cpus for any pmu_types >> + * already allocated, but don't allocate any >> + * more pmu_types. This avoids undercounting. >> + */ >> + alloc_failure = true; > > Why not just fail probe and return an error? What is the benefit of > having some of the PMUs available? AFAIC, there isn't a good reason for penalizing PMU's which we can get working if a subset of the system PMUs can't be created. But this is per PMU type, so with current systems the kzalloc will be called a max of 2 times (there is the potential of a 3rd time, due to some other error handling, but that doesn't change the argument much). AKA, this doesn't result in "partial registration" of a PMU. So, really the only question in my mind is does it work if one of the allocations fails and the other succeeds, and the answer is yes, the remaining interrupts/etc get associated with the correct PMU, and it gets created and should work as well as perf currently works in systems with heterogeneous PMUs (cue discussion about CPU process migration). So, since this is early boot, and we are taking a tiny allocation, if this fails i suspect that the machine will probably die anyway, not due to the choice of whether the PMU is counted properly or not. I would guess the platform allocation or similar will die.. (trimming) >> +/* >> + * 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. >> + */ >> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus, >> + struct resource *res, int *last_cpu_id) >> +{ >> + int i, count; >> + int irq; >> + >> + /* 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; > > Please don't silently continue if the irq is missing. It deserves a user > visible message. We don't want users complaining about kernel issues > when the firmware fails to provide the required information. See below. > >> + >> + irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi, >> + pmu_irqs[i].trigger, >> + ACPI_ACTIVE_HIGH); > > Check for the return value of acpi_register_gsi as it can return an > error. Ok, so this is probably a little subtle, but IIRC from a few months back when I was testing this/reworking it, duplicate GSI registrations for exclusive PPI's result in errors (see any random discussion about PPI's not being ACPI GSI's). As the code to handle SPI vs PPI exists in the platform code, I decided to ignore registration errors until such a determination can be made. AKA, i'm potentially tossing invalid irq's into the irq list for PPIs, but it doesn't matter because they are temporary ignored. The core ARM PMU code, has a much better handle on what is a correct interrupt binding, so the decision about whether these failures need to be worried about are delayed until then. This results in a large simplification because we handle the irq deregistration necessarily for any further errors together, AKA you will notice a complete lack of differing code paths for PPI vs SPI in this module. As far as gsi=0, in cases where there are placeholder GICC entries in the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that is incorrect, so again it gets skipped, the pmu mask doesn't have it listed, and everything "works". So if i'm going to add a message here i'm, going to wrap it in a reg_midr!=0 check. > >> + >> + 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; > > What is the benefit of using the entire MIDR for cpu_id when the > grouping is done on the basis of a subset, i.e., part number. Because the platform code isn't partnum specific, unless the the ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about what part of the MIDR happens to be used (if any) to the part probe table is probably a good idea. Especially if someone happens to think that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro.. If anything It might be worth removing the partnum checks in this module as well. That way should someone ship a machine with the same CPU only differing by revision (or pick your favorite !partnum field) they get different PMUs definitions. Why anyone would do that I cannot guess, but I do see that apparently the xscale version was important at one point.
Hi Jeremy, Jeremy Linton <jeremy.linton@arm.com> writes: > Hi, > > On 08/26/2016 10:04 AM, Punit Agrawal wrote: > (trimming) >>> + pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL); >>> + if (!pmu) { >>> + pr_warn("Unable to allocate pmu_types\n"); >>> + /* >>> + * continue to count cpus for any pmu_types >>> + * already allocated, but don't allocate any >>> + * more pmu_types. This avoids undercounting. >>> + */ >>> + alloc_failure = true; >> >> Why not just fail probe and return an error? What is the benefit of >> having some of the PMUs available? > > AFAIC, there isn't a good reason for penalizing PMU's which we can get > working if a subset of the system PMUs can't be created. But this is > per PMU type, so with current systems the kzalloc will be called a max > of 2 times (there is the potential of a 3rd time, due to some other > error handling, but that doesn't change the argument much). AKA, this > doesn't result in "partial registration" of a PMU. If it wasn't clear, by partial registration I was referring to a strict subset of core PMUs being registered. > > So, really the only question in my mind is does it work if one of the > allocations fails and the other succeeds, and the answer is yes, the > remaining interrupts/etc get associated with the correct PMU, and it > gets created and should work as well as perf currently works in > systems with heterogeneous PMUs (cue discussion about CPU process > migration). > > So, since this is early boot, and we are taking a tiny allocation, if > this fails i suspect that the machine will probably die anyway, not > due to the choice of whether the PMU is counted properly or not. I > would guess the platform allocation or similar will die.. As you have pointed out earlier, if the allocation fails something is seriously wrong with the system. In such a situation, I don't see the justification of additional code complexity (as evidenced in the patch) - when having a subset of PMUs available isn't even going to be high on the users' priority. > > (trimming) > >>> +/* >>> + * 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. >>> + */ >>> +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus, >>> + struct resource *res, int *last_cpu_id) >>> +{ >>> + int i, count; >>> + int irq; >>> + >>> + /* 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; >> >> Please don't silently continue if the irq is missing. It deserves a user >> visible message. We don't want users complaining about kernel issues >> when the firmware fails to provide the required information. > > See below. > >> >>> + >>> + irq = acpi_register_gsi(NULL, pmu_irqs[i].gsi, >>> + pmu_irqs[i].trigger, >>> + ACPI_ACTIVE_HIGH); >> >> Check for the return value of acpi_register_gsi as it can return an >> error. > > Ok, so this is probably a little subtle, but IIRC from a few months > back when I was testing this/reworking it, duplicate GSI registrations > for exclusive PPI's result in errors (see any random discussion about > PPI's not being ACPI GSI's). Is this - improper handling of PPIs in ACPI - something we should be looking at improving? The current behaviour seems like a bodge we are working around. I don't think this work should be part of this series. > As the code to handle SPI vs PPI exists > in the platform code, I decided to ignore registration errors until > such a determination can be made. AKA, i'm potentially tossing invalid > irq's into the irq list for PPIs, but it doesn't matter because they > are temporary ignored. The core ARM PMU code, has a much better handle > on what is a correct interrupt binding, so the decision about whether > these failures need to be worried about are delayed until then. This > results in a large simplification because we handle the irq > deregistration necessarily for any further errors together, AKA you > will notice a complete lack of differing code paths for PPI vs SPI in > this module. > > As far as gsi=0, in cases where there are placeholder GICC entries in > the MADT (think disabled CPU) then i've seen gsi=0. I'm not sure that > is incorrect, so again it gets skipped, the pmu mask doesn't have it > listed, and everything "works". So if i'm going to add a message here > i'm, going to wrap it in a reg_midr!=0 check. > >> >>> + >>> + 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; >> >> What is the benefit of using the entire MIDR for cpu_id when the >> grouping is done on the basis of a subset, i.e., part number. > > Because the platform code isn't partnum specific, unless the the > ARMV8_PMU_PART_PROBE() macro is utilized. Delaying any decisions about > what part of the MIDR happens to be used (if any) to the part probe > table is probably a good idea. Especially if someone happens to think > that they want a ARMV8_PMU_PART_REVISION_PROBE() or similar macro.. > > > If anything It might be worth removing the partnum checks in this > module as well. That way should someone ship a machine with the same > CPU only differing by revision (or pick your favorite !partnum field) > they get different PMUs definitions. Why anyone would do that I cannot > guess, but I do see that apparently the xscale version was important > at one point. It is generally a hard problem to guess what implementations might do in the future. ;) Punit > > > -- > 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
On Fri, Aug 26, 2016 at 05:44:59PM -0500, Jeremy Linton wrote: > Hi, > > On 08/26/2016 10:04 AM, Punit Agrawal wrote: > (trimming) > >>+ pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL); > >>+ if (!pmu) { > >>+ pr_warn("Unable to allocate pmu_types\n"); > >>+ /* > >>+ * continue to count cpus for any pmu_types > >>+ * already allocated, but don't allocate any > >>+ * more pmu_types. This avoids undercounting. > >>+ */ > >>+ alloc_failure = true; > > > >Why not just fail probe and return an error? What is the benefit of > >having some of the PMUs available? > > AFAIC, there isn't a good reason for penalizing PMU's which we can get > working if a subset of the system PMUs can't be created. But this is per PMU > type, so with current systems the kzalloc will be called a max of 2 times > (there is the potential of a 3rd time, due to some other error handling, but > that doesn't change the argument much). AKA, this doesn't result in "partial > registration" of a PMU. ... but this will look mighty confusing to userspace, where things will appear to "half-work", if for some reason the machine makes it that far at all. I think we should stick with the KISS approach and just fail the probe as Punit is suggesting. Will
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index ee9e301..98a037a 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -1063,7 +1063,12 @@ int arm_pmu_device_probe(struct platform_device *pdev, if (!ret) ret = init_fn(pmu); } else if (probe_table) { - ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id()); + if (acpi_disabled) { + /* use the current cpu. */ + ret = probe_plat_pmu(pmu, probe_table, + read_cpuid_id()); + } else + ret = probe_plat_pmu(pmu, probe_table, pdev->id); } if (ret) { diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c index e784714..c0d6888 100644 --- a/drivers/perf/arm_pmu_acpi.c +++ b/drivers/perf/arm_pmu_acpi.c @@ -2,13 +2,17 @@ * 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/acpi.h> #include <linux/irq.h> @@ -23,6 +27,12 @@ struct pmu_irq { bool registered; }; +struct pmu_types { + struct list_head list; + int cpu_type; + int cpu_count; +}; + static struct pmu_irq pmu_irqs[NR_CPUS] __initdata; /* @@ -39,13 +49,167 @@ 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. */ +static void __init arm_pmu_acpi_determine_cpu_types(struct list_head *pmus) +{ + int i; + bool alloc_failure = false; + + 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) && (!alloc_failure)) { + pmu = kzalloc(sizeof(struct pmu_types), GFP_KERNEL); + if (!pmu) { + pr_warn("Unable to allocate pmu_types\n"); + /* + * continue to count cpus for any pmu_types + * already allocated, but don't allocate any + * more pmu_types. This avoids undercounting. + */ + alloc_failure = true; + } 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'. + */ +static 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. + */ +static int __init arm_pmu_acpi_gsi_res(struct pmu_types *pmus, + struct resource *res, int *last_cpu_id) +{ + int i, count; + int irq; + + /* 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 resource *res; int err = -ENOMEM; + int count, cpu_id; + struct pmu_types *pmu, *safe_temp; + LIST_HEAD(pmus); if (acpi_disabled) return 0; + arm_pmu_acpi_determine_cpu_types(&pmus); + + list_for_each_entry_safe(pmu, safe_temp, &pmus, list) { + res = kcalloc(pmu->cpu_count, + sizeof(struct resource), GFP_KERNEL); + + /* for a given PMU type collect all the GSIs. */ + if (res) { + count = arm_pmu_acpi_gsi_res(pmu, res, + &cpu_id); + /* + * register this set of interrupts + * with a new PMU device + */ + err = arm_pmu_acpi_register_pmu(count, res, cpu_id); + if (!err) + pr_info("Registered %d devices for %X\n", + count, pmu->cpu_type); + kfree(res); + } else + pr_warn("PMU unable to allocate interrupt resource space\n"); + + list_del(&pmu->list); + kfree(pmu); + } + return err; } + arch_initcall(pmu_acpi_init);
Its possible that an ACPI system has multiple CPU types in it with differing PMU counters. Iterate the CPU's and make a determination about how many of each type exist in the system. Then take and create a PMU platform device for each type, and assign it the interrupts parsed from the MADT. Creating a platform device is necessary because the PMUs are not described as devices in the DSDT table. This code is loosely based on earlier work by Mark Salter. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/perf/arm_pmu.c | 7 +- drivers/perf/arm_pmu_acpi.c | 164 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 1 deletion(-)