Re: [PATCH v4 02/10] arm_mpam: propagate MSC access errors for hw_probe functions
From: Jonathan Cameron <jic23@kernel.org>
Date: 2026-07-27 22:21:31
Also in:
linux-acpi, lkml
On Thu, 23 Jul 2026 17:54:46 +0200 Andre Przywara [off-list ref] wrote:
Allow the functions probing for MSC hardware and features to return an error, and propagate read and write errors from the lower level up. This uses some "scoped cleanup" functions like scoped_guard() and ACQUIRE() to avoid the complexity of error handling when a lock has been taken. Since the mon_sel_lock is a bit special (even more so in an upcoming patch), we define a new GUARD type for it. Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Hi Andre A couple of whites space comments inline. Reviewed-by: Jonathan Cameron <redacted>
quoted hunk ↗ jump to hunk
--- drivers/resctrl/mpam_devices.c | 127 +++++++++++++++++++++----------- drivers/resctrl/mpam_internal.h | 8 ++ 2 files changed, 91 insertions(+), 44 deletions(-)diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c index 912c21ce7f9f..ecd45a585942 100644 --- a/drivers/resctrl/mpam_devices.c +++ b/drivers/resctrl/mpam_devices.c
quoted hunk ↗ jump to hunk
static int mpam_msc_hw_probe(struct mpam_msc *msc) { + int ret; u64 idr; u16 partid_max; u8 ris_idx, pmg_max;@@ -1021,11 +1047,14 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc) } /* Grab an IDR value to find out how many RIS there are */ - mutex_lock(&msc->part_sel_lock); - mpam_msc_read_idr(msc, &idr); - mpam_read_partsel_reg(msc, IIDR, &msc->iidr); - - mutex_unlock(&msc->part_sel_lock); + scoped_guard(mutex, &msc->part_sel_lock) { + ret = mpam_msc_read_idr(msc, &idr); + if (ret) + return ret;
Aim for formatting consistency. I'd put a blank line here and in similar locations to keep each read / error check block separate...
quoted hunk ↗ jump to hunk
+ ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr); + if (ret) + return ret; + } mpam_enable_quirks(msc);@@ -1036,10 +1065,15 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc) msc->pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr); for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) { - mutex_lock(&msc->part_sel_lock); - __mpam_part_sel(ris_idx, 0, msc); - mpam_msc_read_idr(msc, &idr); - mutex_unlock(&msc->part_sel_lock); + scoped_guard(mutex, &msc->part_sel_lock) { + ret = __mpam_part_sel(ris_idx, 0, msc); + if (ret) + return ret; +
Just like you have done here.
quoted hunk ↗ jump to hunk
+ ret = mpam_msc_read_idr(msc, &idr); + if (ret) + return ret; + }diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h index 04d1a59f02af..0c3f6a040b20 100644 --- a/drivers/resctrl/mpam_internal.h +++ b/drivers/resctrl/mpam_internal.h@@ -162,6 +162,14 @@ static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc) raw_spin_lock_init(&msc->_mon_sel_lock); } +DEFINE_GUARD(mon_sel, + struct mpam_msc *, + mpam_mon_sel_lock(_T), + mpam_mon_sel_unlock(_T));
Why wrap so much. Something like: DEFINE_GUARD(mon_sel, struct mpam_msc *, mpam_mon_sel_lock(_T), mpam_mon_sel_unlock(_T)); or all on one line seems fine to me.
+DEFINE_GUARD_COND(mon_sel, _lock, + mpam_mon_sel_lock(_T), + _RET); +
Similar.
/* Bits for mpam features bitmaps */
enum mpam_device_features {
mpam_feat_cpor_part,