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

  ACPI TRBE does not have a HID for identification which could create and add
a platform device into the platform bus. Also without a platform device, it
cannot be probed and bound to a platform driver.

This creates a dummy platform device for TRBE after ascertaining that ACPI
provides required interrupts uniformly across all cpus on the system. This
device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
being built as a module.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/acpi.h |  3 ++
 drivers/perf/arm_pmu_acpi.c   | 63 +++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h  |  1 +
 3 files changed, 67 insertions(+)
  

On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote:
> ACPI TRBE does not have a HID for identification which could create and add
> a platform device into the platform bus. Also without a platform device, it
> cannot be probed and bound to a platform driver.
> 
> This creates a dummy platform device for TRBE after ascertaining that ACPI
> provides required interrupts uniformly across all cpus on the system. This
> device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
> being built as a module.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

--->8

> 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
> @@ -139,6 +139,68 @@ static inline void arm_spe_acpi_register_device(void)
>  }
>  #endif /* CONFIG_ARM_SPE_PMU */
>  
> +#ifdef CONFIG_CORESIGHT_TRBE
> +static struct resource trbe_acpi_resources[] = {
> +	{
> +		/* irq */
> +		.flags          = IORESOURCE_IRQ,
> +	}
> +};
> +
> +static struct platform_device trbe_acpi_dev = {
> +	.name = ARMV8_TRBE_PDEV_NAME,
> +	.id = -1,
> +	.resource = trbe_acpi_resources,
> +	.num_resources = ARRAY_SIZE(trbe_acpi_resources)
> +};
> +
> +static void arm_trbe_acpi_register_device(void)
> +{
> +	int cpu, hetid, irq, ret;
> +	bool first = true;
> +	u16 gsi = 0;
> +
> +	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_TRBE)
> +			return;
> +
> +		if (first) {
> +			gsi = gicc->trbe_interrupt;
> +			if (!gsi)
> +				return;
> +
> +			hetid = find_acpi_cpu_topology_hetero_id(cpu);
> +			first = false;
> +		} else if ((gsi != gicc->trbe_interrupt) ||
> +			   (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
> +			pr_warn("ACPI: TRBE must be homogeneous\n");
> +			return;
> +		}
> +	}
> +
> +	irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
> +	if (irq < 0) {
> +		pr_warn("ACPI: TRBE Unable to register interrupt: %d\n", gsi);
> +		return;
> +	}
> +	trbe_acpi_resources[0].start = irq;
> +
> +	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 :)

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.

Cheers,

Will
  

On 7/28/23 20:10, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote:
>> ACPI TRBE does not have a HID for identification which could create and add
>> a platform device into the platform bus. Also without a platform device, it
>> cannot be probed and bound to a platform driver.
>>
>> This creates a dummy platform device for TRBE after ascertaining that ACPI
>> provides required interrupts uniformly across all cpus on the system. This
>> device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
>> being built as a module.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> --->8
> 
>> 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
>> @@ -139,6 +139,68 @@ static inline void arm_spe_acpi_register_device(void)
>>  }
>>  #endif /* CONFIG_ARM_SPE_PMU */
>>  
>> +#ifdef CONFIG_CORESIGHT_TRBE
>> +static struct resource trbe_acpi_resources[] = {
>> +	{
>> +		/* irq */
>> +		.flags          = IORESOURCE_IRQ,
>> +	}
>> +};
>> +
>> +static struct platform_device trbe_acpi_dev = {
>> +	.name = ARMV8_TRBE_PDEV_NAME,
>> +	.id = -1,
>> +	.resource = trbe_acpi_resources,
>> +	.num_resources = ARRAY_SIZE(trbe_acpi_resources)
>> +};
>> +
>> +static void arm_trbe_acpi_register_device(void)
>> +{
>> +	int cpu, hetid, irq, ret;
>> +	bool first = true;
>> +	u16 gsi = 0;
>> +
>> +	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_TRBE)
>> +			return;
>> +
>> +		if (first) {
>> +			gsi = gicc->trbe_interrupt;
>> +			if (!gsi)
>> +				return;
>> +
>> +			hetid = find_acpi_cpu_topology_hetero_id(cpu);
>> +			first = false;
>> +		} else if ((gsi != gicc->trbe_interrupt) ||
>> +			   (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
>> +			pr_warn("ACPI: TRBE must be homogeneous\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
>> +	if (irq < 0) {
>> +		pr_warn("ACPI: TRBE Unable to register interrupt: %d\n", gsi);
>> +		return;
>> +	}
>> +	trbe_acpi_resources[0].start = irq;
>> +
>> +	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.
  

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

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

On Tue, Aug 01, 2023 at 09:05:54AM +0530, Anshuman Khandual wrote:
> 
> 
> 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 ?

__maybe_unused?

Like I said, I didn't test that thing at all, I was just trying to
illustrate the sort of refactoring I had in mind.

Will
  

On 8/1/23 13:08, Will Deacon wrote:
> On Tue, Aug 01, 2023 at 09:05:54AM +0530, Anshuman Khandual wrote:
>>
>>
>> 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 ?
> 
> __maybe_unused?
> 
> Like I said, I didn't test that thing at all, I was just trying to
> illustrate the sort of refactoring I had in mind.

Sure. If it's okay, will use your Co-developed-by/Signed-off-by tags
for this refactoring patch.