Re: [PATCH] arm_mpam: make the mon_sel guard conditional only

[Andre Przywara <andre.przywara@arm.com>]

Hi Sasha,

many thanks for doing this!

On 10/3/26 14:12, Sasha Levin wrote:
> Building arm64 allmodconfig with clang fails:
> 
>    drivers/resctrl/mpam_internal.h:207:7: error: ignoring return value
>    of function declared with 'warn_unused_result' attribute
>    [-Werror,-Wunused-result]
>      207 |              mpam_mon_sel_lock(_T), mpam_mon_sel_unlock(_T));

Ooops, I indeed tested this only with GCC.

> mpam_mon_sel_lock() is __must_check and can fail: for an MSC accessed
> through the MPAM-Fb firmware interface it returns false when called
> from a context that cannot sleep. The mon_sel guard was defined with
> DEFINE_GUARD(), which calls it unconditionally and discards the
> result, so a guard(mon_sel) user would carry on without the lock and
> release it on scope exit. The unconditional guard only exists as the
> base for the conditional mon_sel_lock variant, which is the one
> actually used.

Yes, that was the idea I had to model this particular lock, though I 
wasn't entirely happy with it, but didn't dare to create a whole new 
class. Turns out my solution was not only slightly less elegant, but 
also broken ;-) So many thanks for pointing this out and providing the 
proper solution.

> Define mon_sel_lock directly as a conditional guard class instead, as
> posix-timers does for lock_timer: the constructor returns NULL when
> the lock cannot be taken, and the destructor only releases a lock
> that was taken. ACQUIRE(mon_sel_lock, ...) works as before, including
> ACQUIRE_ERR() returning -EBUSY on failure, and there is no
> unconditional variant left to misuse.
> 
> Reported-by: kernelci.org bot <bot@kernelci.org>
> Closes: https://d.kernelci.org/i/maestro:66f869ecbbb5b8592562ffd89f049c5ddf682547
> Closes: https://d.kernelci.org/i/maestro:647269216758644837afcaac8e67d8dbe713a0e2
> Fixes: 0db1715e4c9c ("arm_mpam: propagate MSC access errors for hw_probe functions")
> Assisted-by: LLM
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   drivers/resctrl/mpam_internal.h | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index ee7c3953a9b6c..be1795b29aca2 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -203,9 +203,10 @@ static inline int mpam_mon_sel_lock_init(struct device *dev,
>   	return devm_mutex_init(dev, &msc->mon_sel_mutex);
>   }
>   
> -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);
> +DEFINE_CLASS(mon_sel_lock, struct mpam_msc *,
> +	     _T ? mpam_mon_sel_unlock(_T) : (void)0,
> +	     mpam_mon_sel_lock(msc) ? msc : NULL, struct mpam_msc *msc);
> +DEFINE_CLASS_IS_COND_GUARD(mon_sel_lock);

Ah, that's ... nice, I guess, though I share that headache argument with 
Nathan ;-)
So that looks superficially right, but the details go a bit above my 
head, although I tested it to both still work and to fix the LLVM build 
error, so:

Tested-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre

>   
>   /* Bits for mpam features bitmaps */
>   enum mpam_device_features {