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

[Anshuman Khandual]

On 8/11/23 15:49, Will Deacon wrote:
> On Wed, Aug 09, 2023 at 01:54:55PM +0100, Suzuki K Poulose wrote:
>> On 08/08/2023 14:16, Will Deacon wrote:
>>> On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote:
>>>> On 08/08/2023 09:22, 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 | 105 ++++++++++++++++++++++--------------
>>>>>    1 file changed, 65 insertions(+), 40 deletions(-)
>>>>>
>>>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>>>>> index 90815ad762eb..72454bef2a70 100644
>>>>> --- a/drivers/perf/arm_pmu_acpi.c
>>>>> +++ b/drivers/perf/arm_pmu_acpi.c
>>>>> +	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;
>>>>
>>>> A postivie return value here could confuse the caller. Also, with my comment
>>>> below, we don't really need to return something from here.
>>>
>>> How does this return a positive value?
>>
>> Right now, there aren't. My point is this function returns a "return value"
>> of another function. And the caller of this function doesn't
>> really follow the "check" it needs.  e.g.:
>>
>> ret = foo();
>> if (ret < 0)
>> 	error;
>> return ret;
>>
>>
>>
>> And the caller only checks for
>>
>> if (ret)
>> 	error;
>>
>> This seems fragile.
> 
> Yeah, the '< 0' check is weird. I'd be inclined to drop that entirely
> from the helper function tbh...

static int __maybe_unused
arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
                             u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
{
	.....
	.....
        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;
}

We would still need to call acpi_unregister_gsi() in the helper itself in case previous
platform_device_register() did not work correctly. AFAICS, acpi_unregister_gsi() cannot
be moved to the caller. We could change the error check as 'if (ret)' which is the case
in many other places in the tree. Also drop the above generic pr_warn() error message.

static int __maybe_unused
arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
                             u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
{
	.....
	.....
        ret = platform_device_register(pdev);
        if (ret)
		acpi_unregister_gsi(gsi);
        return ret;
}

>
>>>>> +	int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
>>>>> +					       arm_spe_parse_gsi);
>>>>> +	if (ret)
>>>>>    		pr_warn("ACPI: SPE: Unable to register device\n");
>>>>
>>>> With this change, a system without SPE interrupt description always
>>>> generates the above message. Is this intended ?
>>>
>>> If there are no irqs, why doesn't this return 0?
>>
>> Apologies, I missed that.
>>
>>> arm_acpi_register_pmu_device() should only fail if either:
>>>
>>>    - The static resources passed in are broken
>>>    - The tables are not homogeneous
>>>    - We fail to register the interrupt
>>>
>>> so something is amiss.
>>
>> Agreed. We don't need duplicate messages about an error ?
>> i.e., one in arm_acpi_register_pmu_device() and another
>> one in the caller ? (Of course adding any missing error msgs).
> 
> ... and then just print the registration failure message in the caller.

But we already have such messages in respective callers.

static void arm_spe_acpi_register_device(void)
{
        int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
                                               arm_spe_parse_gsi);
        if (ret)
                pr_warn("ACPI: SPE: Unable to register device\n");
}

static void arm_trbe_acpi_register_device(void)
{
        int ret = arm_acpi_register_pmu_device(&trbe_dev, ACPI_MADT_GICC_TRBE,
                                               arm_trbe_parse_gsi);
        if (ret)
                pr_warn("ACPI: TRBE: Unable to register device\n");
}