Re: [PATCH 1/3] arm_pmu: acpi: Add a representative platform device for TRBE

[Anshuman Khandual]

On 7/31/23 20:29, Will Deacon wrote:
> On Mon, Jul 31, 2023 at 05:38:38PM +0530, Anshuman Khandual wrote:
>> On 7/28/23 20:10, Will Deacon wrote:
>>> On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote:
>>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>>>> index 90815ad762eb..dd3df6729808 100644
>>>> --- a/drivers/perf/arm_pmu_acpi.c
>>>> +++ b/drivers/perf/arm_pmu_acpi.c
> 
> [...]
> 
>>>> +	ret = platform_device_register(&trbe_acpi_dev);
>>>> +	if (ret < 0) {
>>>> +		pr_warn("ACPI: TRBE: Unable to register device\n");
>>>> +		acpi_unregister_gsi(gsi);
>>>> +	}
>>>> +}
>>>> +#else
>>>> +static inline void arm_trbe_acpi_register_device(void)
>>>> +{
>>>> +
>>>> +}
>>>> +#endif /* CONFIG_CORESIGHT_TRBE */
>>>
>>> This looks like you ran s/spe/trbe/ over the SPE device registration
>>> code :)
>>
>> Yeah, almost :) 
>>
>>> Please can you refactor things so we don't have all the duplication? I
>>> suspect this won't be the last device which needs the same treatement.
>>
>> Should the refactoring just accommodate SPE, and TRBE or make it more generic to
>> accommodate future devices as well. Something like the following enumeration.
>>
>> enum arm_platform_device {
>>        ARM_PLATFORM_DEVICE_SPE,
>>        ARM_PLATFORM_DEVICE_TRBE,
>>        ARM_PLATFORM_DEVICE_MAX,
>> };
>>
>> But that would require adding some helper functions to select these following
>> elements based on the above enumeration via a common function
>>
>> - gicc->XXX_interrupt
>> - ACPI_MADT_GICC_SPE/TRBE for header length comparison
>> - static struct platform_device/resources (static objects in the file)
>>
>> Seems like will add much more code for a refactor. Did you have something else
>> in mind for the refactor.
> 
> All I'm saying is that we shouldn't have identical copies of the code to
> walk the MADT, pull out the irqs and register the device.
> 
> So something like the totally untested hack below. I probably broke
> something, but hopefully you see what I mean.
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index 90815ad762eb..7f1cf36c6e69 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -69,6 +69,62 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>                 acpi_unregister_gsi(gsi);
>  }
>  
> +static int
> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
> +                            u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))

This factored out helper should be wrapped inside CONFIG_ARM_SPE_PMU
and CONFIG_CORESIGHT_TRBE ? Otherwise, there will be no callers left
for this helper triggering warning.

drivers/perf/arm_pmu_acpi.c:73:1: warning: ‘arm_acpi_register_pmu_device’ defined but not used [-Wunused-function]
   73 | arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

But in that case, we have to keep adding new configs when new devices
require platform devices to be registered. Is there a better way ?

> +{
> +       int cpu, hetid, irq, ret;
> +       bool matched = false;
> +       u16 gsi = 0;
> +
> +       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, we 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 (!matched) {
> +                       hetid = find_acpi_cpu_topology_hetero_id(cpu);
> +                       gsi = this_gsi;
> +                       matched = true;
> +               } else if (hetid != find_acpi_cpu_topology_hetero_id(cpu) ||
> +                          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;
> +}
> +
>  #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
>  static struct resource spe_resources[] = {
>         {
> @@ -89,49 +145,18 @@ static struct platform_device spe_dev = {
>   * and create a SPE device if we detect a recent MADT with
>   * a homogeneous PPI mapping.
>   */
> +static u16 arm_spe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
> +{
> +       return gicc->spe_interrupt;
> +}
> +
>  static void arm_spe_acpi_register_device(void)
>  {
> -       int cpu, hetid, irq, ret;
> -       bool first = true;
> -       u16 gsi = 0;
> +       int err = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
> +                                              arm_spe_parse_gsi);
>  
> -       /*
> -        * Sanity check all the GICC tables for the same interrupt number.
> -        * For now, we only support homogeneous ACPI/SPE machines.
> -        */
> -       for_each_possible_cpu(cpu) {
> -               struct acpi_madt_generic_interrupt *gicc;
> -
> -               gicc = acpi_cpu_get_madt_gicc(cpu);
> -               if (gicc->header.length < ACPI_MADT_GICC_SPE)
> -                       return;
> -
> -               if (first) {
> -                       gsi = gicc->spe_interrupt;
> -                       if (!gsi)
> -                               return;
> -                       hetid = find_acpi_cpu_topology_hetero_id(cpu);
> -                       first = false;
> -               } else if ((gsi != gicc->spe_interrupt) ||
> -                          (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
> -                       pr_warn("ACPI: SPE must be homogeneous\n");
> -                       return;
> -               }
> -       }
> -
> -       irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE,
> -                               ACPI_ACTIVE_HIGH);
> -       if (irq < 0) {
> -               pr_warn("ACPI: SPE Unable to register interrupt: %d\n", gsi);
> -               return;
> -       }
> -
> -       spe_resources[0].start = irq;
> -       ret = platform_device_register(&spe_dev);
> -       if (ret < 0) {
> -               pr_warn("ACPI: SPE: Unable to register device\n");
> -               acpi_unregister_gsi(gsi);
> -       }
> +       if (err)
> +               pr_warn("ACPI: Failed to register SPE device\n");
>  }
>  #else
>  static inline void arm_spe_acpi_register_device(void)
>