Re: [PATCH 2/4] irqchip/gic-v3-its: Implement .msi_teardown() callback

[Marc Zyngier <maz@kernel.org>]

On Mon, 12 May 2025 17:30:58 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Sun, May 11, 2025 at 05:35:18PM +0100, Marc Zyngier wrote:
> > We currently nuke the structure representing an endpoint device
> > translating via an ITS on freeing the last LPI allocated for it.
> > 
> > That's an unfortunate state of affair, as it is pretty common for
> > a driver to allocate a single MSI, do something clever, teardown
> > this MSI, and reallocate a whole bunch of them. The nvme driver
> > does exactly that, amongst others.
> > 
> > What happens in that case is that the core code is buggy enough
> > to issue another .msi_prepare() call, even if it shouldn't.
> > This luckily cancels the above behaviour and hides the problem.
> > 
> > In order to fix the core code, let's start by implementing the new
> > .msi_teardown() callback. Nothing calls it yet, so a side effect
> > is that the its_dev structure will not be freed and that the DID
> > will stay mapped. Not a big deal, and this will be solved in the
> > following patch.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/irqchip/irq-gic-v3-its-msi-parent.c | 10 ++++
> >  drivers/irqchip/irq-gic-v3-its.c            | 56 +++++++++++++--------
> >  2 files changed, 45 insertions(+), 21 deletions(-)
> 
> First off, thanks a lot for putting this together, it makes an awful
> lot of sense to me.
> 
> > index 0115ad6c82593..3472b97477104 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -3620,8 +3620,43 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
> >  	return err;
> >  }
> >  
> > +static void its_msi_teardown(struct irq_domain *domain, msi_alloc_info_t *info)
> > +{
> > +	struct msi_domain_info *msi_info;
> > +	struct its_device *its_dev;
> > +	struct its_node *its;
> > +	u32 dev_id;
> > +
> > +	dev_id = info->scratchpad[0].ul;
> 
> I have just managed to get to a keyboard :), I don't think the dev_id
> makes it to this point, we overwrite it with the its_dev pointer in
> its_msi_prepare() (could use second scratchpad for the pointer maybe ?).
> 
> I was bitten by this while removing the old IWB code into the new one
> (unrelated to this code but that's how I noticed scratchpad is a union).

Gah, this is missing the fixup I had on top and that I didn't squash:

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index d8c4d3b8256f3..7e0e7f0160936 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3622,19 +3622,9 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
 
 static void its_msi_teardown(struct irq_domain *domain, msi_alloc_info_t *info)
 {
-	struct msi_domain_info *msi_info;
-	struct its_device *its_dev;
-	struct its_node *its;
-	u32 dev_id;
-
-	dev_id = info->scratchpad[0].ul;
-
-	msi_info = msi_get_domain_info(domain);
-	its = msi_info->data;
-
-	guard(mutex)(&its->dev_alloc_lock);
+	struct its_device *its_dev = info->scratchpad[0].ptr;
 
-	its_dev = its_find_device(its, dev_id);
+	guard(mutex)(&its_dev->its->dev_alloc_lock);
 
 	/* If the device is shared, keep everything around */
 	if (its_dev->shared)


So no need for another scratchpad entry -- the device structure has
everything we need. I'll repost things with tglx's comments addressed
and this fix correctly squashed in.

Sorry for the noise,

	M.

-- 
Without deviation from the norm, progress is not possible.