Re: [PATCH v5 15/34] arm_mpam: Add helpers for managing the locking around the mon_sel registers
From: Ben Horgan <ben.horgan@arm.com>
Date: 2025-11-19 10:12:32
Also in:
linux-acpi, lkml
Hi Fenghua, On 11/19/25 04:13, Fenghua Yu wrote:
Hi, Ben, On 11/17/25 08:59, Ben Horgan wrote:quoted
From: James Morse <james.morse@arm.com> The MSC MON_SEL register needs to be accessed from hardirq for the overflow interrupt, and when taking an IPI to access these registers on platforms where MSC are not accessible from every CPU. This makes an irqsave spinlock the obvious lock to protect these registers. On systems with SCMI or PCC mailboxes it must be able to sleep, meaning a mutex must be used. The SCMI or PCC platforms can't support an overflow interrupt, and can't access the registers from hardirq context. Clearly these two can't exist for one MSC at the same time. Add helpers for the MON_SEL locking. For now, use a irqsave spinlock and only support 'real' MMIO platforms. In the future this lock will be split in two allowing SCMI/PCC platforms to take a mutex. Because there are contexts where the SCMI/PCC platforms can't make an access, mpam_mon_sel_lock() needs to be able to fail. Do this now, so that all the error handling on these paths is present. This allows the relevant paths to fail if they are needed on a platform where this isn't possible, instead of having to make explicit checks of the interface type. Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com> Reviewed-by: Gavin Shan <redacted> Reviewed-by: Shaopeng Tan <redacted> Reviewed-by: Fenghua Yu <fenghuay@nvidia.com> Tested-by: Fenghua Yu <fenghuay@nvidia.com> Tested-by: Shaopeng Tan <redacted> Tested-by: Peter Newman <redacted> Tested-by: Carl Worth <redacted> Tested-by: Gavin Shan <redacted> Tested-by: Zeng Heng <redacted> Signed-off-by: James Morse <james.morse@arm.com> Signed-off-by: Ben Horgan <ben.horgan@arm.com[SNIP]quoted
diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h index 768a58a3ab27..b62ee55e1ed5 100644--- a/drivers/resctrl/mpam_internal.h +++ b/drivers/resctrl/mpam_internal.h[SNIP]quoted
+/* Returning false here means accesses to mon_sel must fail and report an error. */ +static inline bool __must_check mpam_mon_sel_lock(struct mpam_msc *msc) +{ + WARN_ON_ONCE(msc->iface != MPAM_IFACE_MMIO); + + raw_spin_lock_irqsave(&msc->_mon_sel_lock, msc->_mon_sel_flags); + return true; +}This helper always returns true, never false. And this may cause issue later. On the bottom line, this causes confusion in the comment and when later its return value is always checked by callers. It's better to improve this helper? Option 1: warn and return false when ris->iface is not MMIO. No changes in other patches which call the helper. Seems this is a better fix.
Ok. The return value checking is intentional so that the locking for the firmware backed interface can be easily re-instated to the driver later. I'll return false when ris->iface is not MMIO but we won't get there as the probe will have already failed.
Option 2: warn on non MMIO iface but no return value. Other patches need to be changed when calling the helper. Thanks. -Fenghua
-- Thanks, Ben