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

[Anshuman Khandual]

On 8/1/23 20:19, Will Deacon wrote:
> 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;

Unlike before, this returns -ENXIO on the very first instance itself without
a match. Previously this returned 0 in such cases. But I guess that is okay.

> 			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?

This makes sense, and the modified function will look something like

arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
                             u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
{
        int cpu, irq, ret;
        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;
                int this_hetid, hetid;
                u16 this_gsi;

                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;

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

        irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
        if (irq < 0) {
                pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
                return -ENXIO;
        }

        pdev->resource[0].start = irq;
        ret = platform_device_register(pdev);
        if (ret < 0) {
                pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
                acpi_unregister_gsi(gsi);
        }
        return ret;
}