Re: [PATCH v11 08/13] arm_mpam: propagate MSC access errors for interrupt control

[Andre Przywara]

Hi Jonathan,

On 9/25/26 00:20, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 17:27:34 +0200
> Andre Przywara <andre.przywara@xxxxxxx> 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@xxxxxxx>
> > Reviewed-by: Srivathsa L Rao <srivathsa.rao@xxxxxxxxxxxxxxxx>
>
> A few trivial things below.  Anyhow, I don't really care about them so

Your comments make sense, and since they shrink the diff, I am all for 
them. Thanks for the tag!

Cheers,
Andre
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
>
> > ---
> >   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;
> > +		}
>
> Scope ends here so unless something else gets added in later patches, you
> could just use a guard() and reduce the noise.
>
> >   	}
> >   
> >   	return 0;
> > @@ -2800,10 +2800,10 @@ static void mpam_unregister_irqs(void)
> >   		if (irq <= 0)
> >   			continue;
> >   
> > -		mutex_lock(&msc->error_irq_lock);
>
> Given you don't return early as a result this is an unrelated change. I don't
> really mind but in ideal world would be a separate patch.
>
> > +		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);
> >   	}
> >   }
> >