[PATCH v2 17/29] arm_mpam: Extend reset logic to allow devices to be reset any time

[James Morse]

Hi Fenghua,

On 25/09/2025 08:16, Fenghua Yu wrote:
> On 9/10/25 13:42, James Morse wrote:
>> cpuhp callbacks aren't the only time the MSC configuration may need to
>> be reset. Resctrl has an API call to reset a class.
>> If an MPAM error interrupt arrives it indicates the driver has
>> misprogrammed an MSC. The safest thing to do is reset all the MSCs
>> and disable MPAM.
>>
>> Add a helper to reset RIS via their class. Call this from mpam_disable(),
>> which can be scheduled from the error interrupt handler.


>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index e7faf453b5d7..a9d3c4b09976 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>>   @@ -1340,8 +1338,56 @@ static void mpam_enable_once(void)
>>              mpam_partid_max + 1, mpam_pmg_max + 1);
>>   }
>>   +static void mpam_reset_component_locked(struct mpam_component *comp)
>> +{
>> +    struct mpam_msc *msc;
>> +    struct mpam_vmsc *vmsc;
>> +    struct mpam_msc_ris *ris;
>> +
>> +    lockdep_assert_cpus_held();
>> +
>> +    guard(srcu)(&mpam_srcu);
>> +   
> 
> Nested locks on mpam_srcu in this call chain:
> 
> mpam_disable() -> mpam_reset_class() -> mpam_reset_class_locked() -> mpam_component_locked()

These are allowed to nest like this.


> There are redundant locks on mpam_srcu in mpam_disabled(), mpam_reset_class_locked(), and
> mpam_reset_component_locked().
> 
> It's better to guard mpam_srcu only in the top function mpam_disable() for simpler logic
> and lower overhead.

These things don't block, so there no real overhead. Shuffling them around to avoid the
harmless nesting would likely complicate the flow, not simplify it.

In the reset case you point at here, the resctrl code would need to take the srcu lock
before calling it - which is exposing the innards of what that function does.



>> list_for_each_entry_srcu(vmsc, &comp->vmsc, comp_list,
>> +                 srcu_read_lock_held(&mpam_srcu)) {
>> +        msc = vmsc->msc;
>> +
>> +        list_for_each_entry_srcu(ris, &vmsc->ris, vmsc_list,
>> +                     srcu_read_lock_held(&mpam_srcu)) {
>> +            if (!ris->in_reset_state)
>> +                mpam_touch_msc(msc, mpam_reset_ris, ris);
>> +            ris->in_reset_state = true;
>> +        }
>> +    }
>> +}


>> +/*
>> + * Called in response to an error IRQ.
>> + * All of MPAMs errors indicate a software bug, restore any modified
>> + * controls to their reset values.
>> + */
>>   void mpam_disable(struct work_struct *ignored)
>>   {
>> +    int idx;
>> +    struct mpam_class *class;
>>       struct mpam_msc *msc, *tmp;
>>         mutex_lock(&mpam_cpuhp_state_lock);
>> @@ -1351,6 +1397,12 @@ void mpam_disable(struct work_struct *ignored)
>>       }
>>       mutex_unlock(&mpam_cpuhp_state_lock);
>>   +    idx = srcu_read_lock(&mpam_srcu);

> It's better to change to guard(srcu)(&mpam_srcu);

For this one - absolutely not.
The guard() thing allows the toolchain to decide when to drop the lock. Further down in
this same function is an attempt to free the memory that got deferred. Guess what happens
if you call synchronize_srcu() while still in an srcu read side section....


>> +    list_for_each_entry_srcu(class, &mpam_classes, classes_list,
>> +                 srcu_read_lock_held(&mpam_srcu))
>> +        mpam_reset_class(class);
>> +    srcu_read_unlock(&mpam_srcu, idx);
>> +
>>       mutex_lock(&mpam_list_lock);
>>       list_for_each_entry_safe(msc, tmp, &mpam_all_msc, all_msc_list)
>>           mpam_msc_destroy(msc);


Thanks,

James