[PATCH 16/33] arm_mpam: Add helpers for managing the locking around the mon_sel registers

[James Morse]

Hi Fenghua,

On 28/08/2025 18:07, Fenghua Yu wrote:
> On 8/22/25 08:29, James Morse wrote:
>> The MSC MON_SEL register needs to be accessed from hardirq context by the
>> PMU drivers, making an irqsave spinlock the obvious lock to protect these
>> registers. On systems with SCMI mailboxes it must be able to sleep, meaning
>> a mutex must be used.
>>
>> Clearly these two can't exist at the same time.
>>
>> Add helpers for the MON_SEL locking. The outer lock must be taken in a
>> pre-emptible context before the inner lock can be taken. On systems with
>> SCMI mailboxes where the MON_SEL accesses must sleep - the inner lock
>> will fail to be 'taken' if the caller is unable to sleep. This will allow
>> the PMU driver to fail without having to check the interface type of
>> each MSC.


>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>> index a623f405ddd8..c6f087f9fa7d 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -68,10 +68,19 @@ struct mpam_msc {
>>         /*
>>        * mon_sel_lock protects access to the MSC hardware registers that are
>> -     * affeted by MPAMCFG_MON_SEL.
>> +     * affected by MPAMCFG_MON_SEL, and the mbwu_state.
>> +     * Both the 'inner' and 'outer' must be taken.
>> +     * For real MMIO MSC, the outer lock is unnecessary - but keeps the
>> +     * code common with:
>> +     * Firmware backed MSC need to sleep when accessing the MSC, which
>> +     * means some code-paths will always fail. For these MSC the outer
>> +     * lock is providing the protection, and the inner lock fails to
>> +     * be taken if the task is unable to sleep.
>> +     *
>>        * If needed, take msc->probe_lock first.
>>        */
>>       struct mutex        outer_mon_sel_lock;
>> +    bool            outer_lock_held;

> Is it better to define outer_lock_held at atomic_t?

Writes a protected by the outer lock, its just something to generate a warning for debug.
I can make it a READ_ONCE() if you're worried about torn values in the failure case.
(as its just for debug, I'm not worried about false-negatives)


>>       raw_spinlock_t        inner_mon_sel_lock;
>>       unsigned long        inner_mon_sel_flags;
>>   @@ -81,6 +90,52 @@ struct mpam_msc {
>>       struct mpam_garbage    garbage;
>>   };
>>   +static inline bool __must_check mpam_mon_sel_inner_lock(struct mpam_msc *msc)
>> +{
>> +    /*
>> +     * The outer lock may be taken by a CPU that then issues an IPI to run
>> +     * a helper that takes the inner lock. lockdep can't help us here.
>> +     */
>> +    WARN_ON_ONCE(!msc->outer_lock_held);
> 
> At this point, msc->outer_lock_held might not be true yet due to no memory barrier on it
> on this CPU.

The IPI machinery has this covered:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic.c#n838


> If it's atomic_t and it's set as true on another CPU by smp_store_release(),
> it's guaranteed to be visible as true on this CPU. Without atomic setting, we may see a
> false warning here and cause debug difficult.


Thanks,

James