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
