Re: [PATCH v10 08/14] arm_mpam: propagate MSC access errors for interrupt control

[Ben Horgan <ben.horgan@arm.com>]

Hi Andre,

On 11/09/2026 17:19, Andre Przywara wrote:
> Hi Ben,
> 
> On 9/11/26 16:26, Ben Horgan wrote:
>> Hi Andre,
>>
>> On 11/09/2026 12:28, Andre Przywara wrote:
>>> Allow the functions dealing with interrupt registration and enablement
>>> to check for and return errors, and propagate MSC read and write errors
>>> from the lower level up.
>>> This does not cover the IRQ handler yet, as this needs some more
>>> attention.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> Reviewed-by: Srivathsa L Rao <srivathsa.rao@oss.qualcomm.com>
>>
>> Commenting here as this is the last error propagation patch. I think we need a clean boundary for
>> the error propagation. As you said a while back I think we can propagate error for all non-static
>> functions from mpam_devices.c and the callers can discard the errors if appropriate. For instance,
>> mpam_reset_class_locked() should propagate any error.
> 
> So I checked all non-static functions in mpam_devices.c: most either don't deal with MSCs at all, or
> already propagate errors.
> The two outliers are mpam_disable(), which must be "void", due to it being called via DECLARE_WORK,
> and mpam_reset_class_locked(), as you write above. The latter is a bit sad, because the caller is
> void, so any errors would be discarded there anyway, but for the sake of completeness we should
> indeed propagate here.
> 
> And I guess it doesn't make sense to continue the vMSC or RIS list iterations after detecting the
> first error, but we just bail out immediately? Because any error would trigger mpam_disable() anyway?

Yeah, bailing early makes sense to me.

Thanks,

Ben

> 
> Cheers,
> Andre
> 
> 
>>
>> Thanks,
>>
>> Ben
>>
>>> ---
>>>   drivers/resctrl/mpam_devices.c | 31 +++++++++++++++----------------
>>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index 558efab75f911..eb26c25b9f970 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -1105,7 +1105,9 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>>       }
>>>         /* Clear any stale errors */
>>> -    mpam_msc_clear_esr(msc);
>>> +    ret = mpam_msc_clear_esr(msc);
>>> +    if (ret)
>>> +        return ret;
>>>         spin_lock(&partid_max_lock);
>>>       mpam_partid_max = min(mpam_partid_max, msc->partid_max);
>>> @@ -2668,9 +2670,7 @@ static int mpam_enable_msc_ecr(void *_msc)
>>>   {
>>>       struct mpam_msc *msc = _msc;
>>>   -    __mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
>>> -
>>> -    return 0;
>>> +    return __mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
>>>   }
>>>     /* This can run in mpam_disable(), and the interrupt handler on the same CPU */
>>> @@ -2678,9 +2678,7 @@ static int mpam_disable_msc_ecr(void *_msc)
>>>   {
>>>       struct mpam_msc *msc = _msc;
>>>   -    __mpam_write_reg(msc, MPAMF_ECR, 0);
>>> -
>>> -    return 0;
>>> +    return __mpam_write_reg(msc, MPAMF_ECR, 0);
>>>   }
>>>     static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>>> @@ -2777,11 +2775,13 @@ static int mpam_register_irqs(void)
>>>                   return err;
>>>           }
>>>   -        mutex_lock(&msc->error_irq_lock);
>>> -        msc->error_irq_req = true;
>>> -        mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>>> -        msc->error_irq_hw_enabled = true;
>>> -        mutex_unlock(&msc->error_irq_lock);
>>> +        scoped_guard(mutex, &msc->error_irq_lock) {
>>> +            msc->error_irq_req = true;
>>> +            err = mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>>> +            if (err)
>>> +                return err;
>>> +            msc->error_irq_hw_enabled = true;
>>> +        }
>>>       }
>>>         return 0;
>>> @@ -2800,10 +2800,10 @@ static void mpam_unregister_irqs(void)
>>>           if (irq <= 0)
>>>               continue;
>>>   -        mutex_lock(&msc->error_irq_lock);
>>> +        guard(mutex)(&msc->error_irq_lock);
>>>           if (msc->error_irq_hw_enabled) {
>>> -            mpam_touch_msc(msc, mpam_disable_msc_ecr, msc);
>>> -            msc->error_irq_hw_enabled = false;
>>> +            if (!mpam_touch_msc(msc, mpam_disable_msc_ecr, msc))
>>> +                msc->error_irq_hw_enabled = false;
>>>           }
>>>             if (msc->error_irq_req) {
>>> @@ -2815,7 +2815,6 @@ static void mpam_unregister_irqs(void)
>>>               }
>>>               msc->error_irq_req = false;
>>>           }
>>> -        mutex_unlock(&msc->error_irq_lock);
>>>       }
>>>   }
>>>   
>>
>