Re: [RFC PATCH 07/38] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation

Ben Horgan
2025-12-19T12:17:21Z

Hi Jonathan,

On 12/18/25 11:30, Jonathan Cameron wrote:
> On Fri, 5 Dec 2025 21:58:30 +0000
> James Morse <james.morse@arm.com> wrote:
>
>> resctrl has its own data structures to describe its resources. We
>> can't use these directly as we play tricks with the 'MBA' resource,
>> picking the MPAM controls or monitors that best apply. We may export
>> the same component as both L3 and MBA.
>>
>> Add mpam_resctrl_exports[] as the array of class->resctrl mappings we
>> are exporting, and add the cpuhp hooks that allocated and free the
>> resctrl domain structures.
>>
>> While we're here, plumb in a few other obvious things.
>>
>> CONFIG_ARM_CPU_RESCTRL is used to allow this code to be built
>> even though it can't yet be linked against resctrl.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
> Hi,
>
> A few code flow related comments. Fairly trivial stuff but I think
> some parts of this can be made more readable / maintainable with
> minor reorganization.
>
> Jonathan
>
>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 2996ad93fc3e..efaf7633bc35 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
> ...
>
>> @@ -2516,6 +2522,12 @@ static void mpam_enable_once(void)
>> mutex_unlock(&mpam_list_lock);
>> cpus_read_unlock();
>>
>> + if (!err) {
>> + err = mpam_resctrl_setup();
>> + if (err)
>> + pr_err("Failed to initialise resctrl: %d\n", err);
>> + }
>> +
>> if (err) {
>> mpam_disable_reason = "Failed to enable.";
>> schedule_work(&mpam_broken_work);
>
> I'd be tempted to move this to an error handling block via a goto
> making this bit
> if (err)
> goto err_disable_mpam;
>
> err = mpam_resctrl_setup();
> if (err) {
> pr_err();
> goto err_dsiable_mpam;
> }
>
> Up to you though. Personally I like all my good paths as straight line
> code with the errors handled in if (err) as that consistency really helps
> readability.
>

I'll leave this one as is. It looks like James tried hard to avoid a goto.

Thanks,

Ben