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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230801073857.GB25854@willie-the-truck>
Date:   Tue, 1 Aug 2023 08:38:58 +0100
From:   Will Deacon <will@...nel.org>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, suzuki.poulose@....com,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] arm_pmu: acpi: Add a representative platform device
 for TRBE

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