Re: [PATCH V2 1/4] arm_pmu: acpi: Refactor arm_spe_acpi_register_device()

[Will Deacon]

On Tue, Aug 01, 2023 at 03:10:49PM +0530, Anshuman Khandual wrote:
> Sanity checking all the GICC tables for same interrupt number, and ensuring
> a homogeneous ACPI based machine, could be used for other platform devices
> as well. Hence this refactors arm_spe_acpi_register_device() into a common
> helper arm_acpi_register_pmu_device().
> 
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Co-developed-by: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
>  drivers/perf/arm_pmu_acpi.c | 110 +++++++++++++++++++++++-------------
>  1 file changed, 70 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index 90815ad762eb..d9d5a7bbb92f 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -70,6 +70,68 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>  }
>  
>  #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
> +static int
> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
> +			     u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
> +{
> +	int cpu, hetid, irq, ret;
> +	bool matched = false;
> +	u16 gsi = 0;
> +
> +	/*
> +	 * Ensure that platform device must have IORESOURCE_IRQ
> +	 * resource to hold gsi interrupt.
> +	 */
> +	if (pdev->num_resources != 1)
> +		return -ENXIO;
> +
> +	if (pdev->resource[0].flags != IORESOURCE_IRQ)
> +		return -ENXIO;
> +
> +	/*
> +	 * Sanity check all the GICC tables for the same interrupt
> +	 * number. For now, only support homogeneous ACPI machines.
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		struct acpi_madt_generic_interrupt *gicc;
> +		u16 this_gsi;
> +
> +		gicc = acpi_cpu_get_madt_gicc(cpu);
> +		if (gicc->header.length < len)
> +			return matched ? -ENXIO : 0;
> +
> +		this_gsi = parse_gsi(gicc);
> +		if (!this_gsi)
> +			return matched ? -ENXIO : 0;

I think you can push this check into the conditional below...

> +
> +		if (!matched) {
> +			hetid = find_acpi_cpu_topology_hetero_id(cpu);

... i.e. add a:

			if (!this_gsi)
				return -ENXIO;

here.

> +			gsi = this_gsi;
> +			matched = true;

And then, come to think of it, can we get rid of 'matched' altogether?
Since a gsi of 0 is treated as invalid, we could just check that instead,
no? So this becomes:


		gicc = acpi_cpu_get_madt_gicc(cpu);
		if (gicc->header.length < len)
			return gsi ? -ENXIO : 0;

		this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
		this_gsi = parse_gsi(gicc);
		if (!gsi) {
			if (!this_gsi)
				return -ENXIO;
			gsi = this_gsi;
			hetid = this_hetid;
		} else if (hetid != this_hetid || gsi != this_gsi) {
			pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
			return -ENXIO;
		}


What do you reckon?

Will