From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 5C2BF4E4317; Wed, 30 Sep 2026 14:01:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790776937; cv=none; b=VTwHmDVLBNC5Lft6sYKoS1EBNQPXbxQPwrKYuMYhXzT/PN8yYSda1eV7G9Zt9QXONnQIQIwiNlwZfZ8rA+TC66fhzXWdwV5K9aIXYpAbV0aXp4+h5kJt2wb4TNASkTzHFPiCODHQhdzRwPScC6ooJpGe8DV6YZG16ZsIqxc/7o0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790776937; c=relaxed/simple; bh=EZ89Fm9plmALyyCq9f/yNwwTgbNrl+9IoYkTg1Ivj78=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=iQP61JnNDCMsK+IKXh4S9CSRtcLont+X75MnV9cr4fUjnhUru/orTQNyOy86uiOgHmjttpWDiiq5veffoztTAAE2UehA9r6lVBHtMM5BlyLVAy3CfuMeqBxtAUpISlkfnmtqk4NP6HTiMLji5NrJ+OXsPfrgiqBgELOUVTbw0ls= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=U3KPkqtj; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="U3KPkqtj" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AB117497; Wed, 30 Sep 2026 07:01:47 -0700 (PDT) Received: from [10.2.212.8] (e134344.arm.com [10.2.212.8]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4D9203F86F; Wed, 30 Sep 2026 07:01:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1790776911; bh=EZ89Fm9plmALyyCq9f/yNwwTgbNrl+9IoYkTg1Ivj78=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=U3KPkqtjupeYAimDJeyZphk2u3aAye/K2ii9jOV93izfEWqQMwCygB4V500l/jmvV ZK6AzF8PxCidMSv7V2CFW9zT0QTzCX8O/WLrn6KeDNCqoDzbS2vatSrL1p0cUC8wps HCwViFRog+leL8VSHFTE5GmUb6HinbKCB7Uc5gT8= Message-ID: <4cc35ee7-c4d3-40c0-b1f9-24ef5a2f5844@arm.com> Date: Wed, 30 Sep 2026 15:01:46 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v11 02/13] arm_mpam: propagate MSC access errors for hw_probe functions To: Andre Przywara , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Catalin Marinas , Will Deacon , "Rafael J . Wysocki" , Len Brown , James Morse , Reinette Chatre , Fenghua Yu Cc: Jonathan Cameron , Srivathsa L Rao , Ganapatrao Kulkarni , Trilok Soni , Srinivas Ramana , Niyas Sait , Lee Trager , Ritwick Sharma , Gavin Shan , linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20260924152739.2865510-1-andre.przywara@arm.com> <20260924152739.2865510-3-andre.przywara@arm.com> Content-Language: en-US From: Ben Horgan In-Reply-To: <20260924152739.2865510-3-andre.przywara@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Andre, On 24/09/2026 16:27, Andre Przywara 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 > Reviewed-by: Jonathan Cameron > Reviewed-by: Ben Horgan > Reviewed-by: Gavin Shan > Tested-by: Gavin Shan # on NVIDIA Grace Hopper > --- > drivers/resctrl/mpam_devices.c | 126 +++++++++++++++++++++----------- > drivers/resctrl/mpam_internal.h | 4 + > 2 files changed, 87 insertions(+), 43 deletions(-) > > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c > index c7686d3799653..22069e7f7bb05 100644 > --- a/drivers/resctrl/mpam_devices.c > +++ b/drivers/resctrl/mpam_devices.c > @@ -798,38 +798,46 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris) > bool can_set, can_clear; > struct mpam_msc *msc = ris->vmsc->msc; > > - if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc))) > + ACQUIRE(mon_sel_lock, guard)(msc); > + if (ACQUIRE_ERR(mon_sel_lock, &guard)) > return false; > > mon_sel = FIELD_PREP(MSMON_CFG_MON_SEL_MON_SEL, 0) | > FIELD_PREP(MSMON_CFG_MON_SEL_RIS, ris->ris_idx); > - mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel); > + if (mpam_write_monsel_reg(msc, CFG_MON_SEL, mon_sel)) > + return false; > > /* Hardware might ignore nrdy if it's not enabled */ > ctl_val = MSMON_CFG_CSU_CTL_TYPE_CSU; > ctl_val |= MSMON_CFG_x_CTL_MATCH_PARTID; > ctl_val |= MSMON_CFG_x_CTL_MATCH_PMG; > ctl_val |= MSMON_CFG_x_CTL_EN; > - mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0); > - mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val); > + if (mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0)) > + return false; > + if (mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val)) > + return false; > > - _mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY); > - _mpam_read_monsel_reg(msc, MSMON_CSU, &now); > + if (_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY)) > + return false; > + if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now)) > + return false; > can_set = now & MSMON___NRDY; > > - _mpam_write_monsel_reg(msc, MSMON_CSU, 0); > + if (_mpam_write_monsel_reg(msc, MSMON_CSU, 0)) > + return false; > /* Configuration change to try and coax hardware into setting nrdy */ > - mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1); > - _mpam_read_monsel_reg(msc, MSMON_CSU, &now); > + if (mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1)) > + return false; > + if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now)) > + return false; > can_clear = !(now & MSMON___NRDY); > - mpam_mon_sel_unlock(msc); > > return (!can_set || !can_clear); > } I've had a look at the Sashiko reports [1] for these patches. I'll call out the things that I feel need answering. As sashiko says, mpam_ris_hw_probe_csu_nrdy(), this swallows the error rather than propagating. It would be better if this returned success/error and the answer to whether or not the h/w supports nrdy separately. As we make other accesses to the msc later it is unlikely this will ever cause a failing probe to report success though. Thanks, Ben [1] https://sashiko.dev/#/patchset/20260924152739.2865510-1-andre.przywara%40arm.com > > -static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > +static int mpam_ris_hw_probe(struct mpam_msc_ris *ris) > { > - int err; > + int fw_has_nrdy_err, err; > struct mpam_msc *msc = ris->vmsc->msc; > struct device *dev = &msc->pdev->dev; > struct mpam_props *props = &ris->props; > @@ -842,7 +850,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) { > u32 ccap_features; > > - mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features); > + err = mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features); > + if (err) > + return err; > > props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features); > if (props->cmax_wd && > @@ -867,7 +877,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) { > u32 cpor_features; > > - mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features); > + err = mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features); > + if (err) > + return err; > > props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features); > if (props->cpbm_wd) > @@ -877,8 +889,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > /* Memory bandwidth partitioning */ > if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) { > u32 mbw_features; > - > - mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features); > + err = mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features); > + if (err) > + return err; > > /* portion bitmap resolution */ > props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features); > @@ -907,8 +920,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > /* Priority partitioning */ > if (FIELD_GET(MPAMF_IDR_HAS_PRI_PART, ris->idr)) { > u32 pri_features; > - > - mpam_read_partsel_reg(msc, PRI_IDR, &pri_features); > + err = mpam_read_partsel_reg(msc, PRI_IDR, &pri_features); > + if (err) > + return err; > > props->intpri_wd = FIELD_GET(MPAMF_PRI_IDR_INTPRI_WD, pri_features); > if (props->intpri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_INTPRI, pri_features)) { > @@ -929,20 +943,24 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) { > u32 msmon_features; > > - mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features); > + err = mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features); > + if (err) > + return err; > > /* > * If the firmware max-nrdy-us property is missing, the > * CSU counters can't be used. Should we wait forever? > */ > - err = device_property_read_u32(&msc->pdev->dev, > - "arm,not-ready-us", > - &msc->nrdy_usec); > + fw_has_nrdy_err = device_property_read_u32(&msc->pdev->dev, > + "arm,not-ready-us", > + &msc->nrdy_usec); > > if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) { > u32 csumonidr; > > - mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr); > + err = mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr); > + if (err) > + return err; > > props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr); > if (props->num_csu_mon) { > @@ -960,7 +978,7 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > * Accept the missing firmware property if NRDY appears > * un-implemented. > */ > - if (err && hw_managed) > + if (fw_has_nrdy_err && hw_managed) > dev_err_once(dev, "Counters are not usable because not-ready timeout was not provided by firmware."); > } > } > @@ -968,7 +986,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > bool has_long; > u32 mbwumon_idr; > > - mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr); > + err = mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr); > + if (err) > + return err; > > props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumon_idr); > if (props->num_mbwu_mon) { > @@ -1001,16 +1021,22 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris) > u16 partid_max; > u32 nrwidr; > > - mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr); > + err = mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr); > + if (err) > + return err; > + > partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr); > > mpam_set_feature(mpam_feat_partid_nrw, props); > msc->partid_max = min(msc->partid_max, partid_max); > } > + > + return 0; > } > > static int mpam_msc_hw_probe(struct mpam_msc *msc) > { > + int ret; > u64 idr; > u16 partid_max; > u8 ris_idx, pmg_max; > @@ -1025,11 +1051,15 @@ 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); > + scoped_guard(mutex, &msc->part_sel_lock) { > + ret = mpam_msc_read_idr(msc, &idr); > + if (ret) > + return ret; > > - mutex_unlock(&msc->part_sel_lock); > + ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr); > + if (ret) > + return ret; > + } > > mpam_enable_quirks(msc); > > @@ -1040,10 +1070,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; > + > + ret = mpam_msc_read_idr(msc, &idr); > + if (ret) > + return ret; > + } > > partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr); > pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr); > @@ -1051,17 +1086,22 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc) > msc->pmg_max = min(msc->pmg_max, pmg_max); > msc->has_extd_esr = FIELD_GET(MPAMF_IDR_HAS_EXTD_ESR, idr); > > - mutex_lock(&mpam_list_lock); > - ris = mpam_get_or_create_ris(msc, ris_idx); > - mutex_unlock(&mpam_list_lock); > - if (IS_ERR(ris)) > - return PTR_ERR(ris); > + scoped_guard(mutex, &mpam_list_lock) { > + ris = mpam_get_or_create_ris(msc, ris_idx); > + if (IS_ERR(ris)) > + return PTR_ERR(ris); > + } > ris->idr = idr; > > - mutex_lock(&msc->part_sel_lock); > - __mpam_part_sel(ris_idx, 0, msc); > - mpam_ris_hw_probe(ris); > - 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; > + > + ret = mpam_ris_hw_probe(ris); > + if (ret) > + return ret; > + } > } > > /* Clear any stale errors */ > diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h > index def0e3a65c230..68a6cf2b9cc73 100644 > --- a/drivers/resctrl/mpam_internal.h > +++ b/drivers/resctrl/mpam_internal.h > @@ -162,6 +162,10 @@ 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)); > +DEFINE_GUARD_COND(mon_sel, _lock, mpam_mon_sel_lock(_T), _RET); > + > /* Bits for mpam features bitmaps */ > enum mpam_device_features { > mpam_feat_cpor_part,