Re: [PATCH v3 2/2] perf: arm_pmu_acpi: Support heterogeneous ACPI systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: arm_pmu_acpi: unbalanced acpi_unregister_gsi() on group error
--

commit 160cc5903c9a42ca04c98cc8b994a9fea642d22f
Author: Johnny.Fan <Johnny.Fan@cixtech.com>

perf: arm_pmu_acpi: Support heterogeneous ACPI systems

This commit updates arm_pmu_acpi to support heterogeneous ACPI systems by
grouping CPUs by their PPTT hetero ID and MADT GICC overflow interrupt GSI,
and registering one SPE or TRBE platform device per group with its CPU mask
passed as platform data.

> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index ca6bf8f86cfe9..bffeeb0638004 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
[ ... ]
> @@ -68,91 +70,196 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>  		acpi_unregister_gsi(gsi);
>  }
>  
> +struct arm_pmu_acpi_group {
> +	int hetid;
> +	u16 gsi;
> +	bool registered;
> +	struct cpumask cpus;
> +};
[ ... ]
> +static bool arm_acpi_gsi_in_use(struct arm_pmu_acpi_group **groups,
> +				int nr_groups, int idx)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_groups; i++) {
> +		if (i == idx)
> +			continue;
> +		if (groups[i]->registered && groups[i]->gsi == groups[idx]->gsi)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int arm_acpi_register_group(struct arm_pmu_acpi_group *group,
> +				   const char *name)
> +{
> +	struct arm_pmu_acpi_dev_data data = { };
> +	struct platform_device *pdev;
> +	struct resource res;
> +	int irq, ret;
> +
> +	irq = acpi_register_gsi(NULL, group->gsi, ACPI_LEVEL_SENSITIVE,
> +				ACPI_ACTIVE_HIGH);
> +	if (irq < 0)
> +		return irq;
[ ... ]
> +	for (i = 0; i < nr_groups; i++) {
> +		ret = arm_acpi_register_group(groups[i], name);
> +		if (ret) {
> +			pr_warn("ACPI: %s: %*pbl: add device failed (%d)\n",
> +				name, cpumask_pr_args(&groups[i]->cpus), ret);
> +			last_err = ret;
> +			continue;
> +		}
> +
> +		groups[i]->registered = true;
> +		nr_devices++;
> +	}
> +
> +	/*
> +	 * acpi_register_gsi() does not reference count the GSI mapping, so
> +	 * release a shared GSI only once no successfully registered device
> +	 * uses it any more.
> +	 */
> +	for (i = 0; i < nr_groups; i++) {
> +		if (groups[i]->registered)
> +			continue;
> +		if (arm_acpi_gsi_in_use(groups, nr_groups, i))
> +			continue;
> +		acpi_unregister_gsi(groups[i]->gsi);
> +	}

[Severity: Medium]
Can this cleanup loop in arm_acpi_register_pmu_devices() make unbalanced
or duplicate calls to acpi_unregister_gsi()?

In arm_acpi_register_group(), if acpi_register_gsi() fails and returns a
negative error code, the function returns early and leaves
groups[i]->registered as false. The cleanup loop in
arm_acpi_register_pmu_devices() then calls acpi_unregister_gsi() for that
group even though acpi_register_gsi() never succeeded.

Looking at drivers/acpi/irq.c:acpi_unregister_gsi():

	if (WARN_ON(acpi_irq_model == ACPI_IRQ_MODEL_GIC && gsi < 16))
		return;

	d = irq_find_matching_fwnode(acpi_get_gsi_domain_id(gsi),
				     DOMAIN_BUS_ANY);
	irq = irq_find_mapping(d, gsi);
	irq_dispose_mapping(irq);

If a malformed MADT GICC entry specifies an SGI (0 < gsi < 16) that
acpi_register_gsi() rejected, calling acpi_unregister_gsi() will hit the
WARN_ON(). Or if acpi_register_gsi() failed in irq_create_fwspec_mapping()
due to a trigger type mismatch with an existing mapping, calling
acpi_unregister_gsi() will dispose of that pre-existing IRQ mapping.

In addition, arm_acpi_gsi_in_use() only checks whether another group with
the same GSI has registered == true. If two or more groups share the same
GSI and all of them fail platform device registration after
acpi_register_gsi() succeeds, arm_acpi_gsi_in_use() returns false for each
of them, causing the loop to call acpi_unregister_gsi() multiple times for
the same GSI.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102084153.3975282-1-johnny.fan@cixtech.com?part=2