[PATCH v7 8/9] arm64: pmu: Detect and enable multiple PMUs in an ACPI system

Hi Jeremy,

Jeremy Linton [off-list ref] writes:

Hi,

On 08/26/2016 10:04 AM, Punit Agrawal wrote:
(trimming)

quoted

quoted

+			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)

quoted

quoted

+/*
+ * 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.

quoted

quoted

+
+			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.

quoted

quoted

+
+			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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html