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

[Marc Zyngier]

On Mon, 12 May 2025 15:34:59 +0100,
Thomas Gleixner <tglx at linutronix.de> wrote:
> 
> On Sun, May 11 2025 at 17:35, Marc Zyngier wrote:
> > We currently nuke the structure representing an endpoint device
> 
> How is we? We means nothing as you know :)

We means a lot to *me*.

> 
> > 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
> 
> s/let's//
> 
> I really have to understand why everyone is so fond of "let's". It only
> makes limited sense when the patch is proposed, but in a change log it
> does not make sense at all.
> 
> > .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.
> 
> Now I see why you added this incomprehensible condition into the core
> code. Bah.
> 
> Why don't you add this callback once you changed the prepare muck, which
> introduces info::alloc_data?

Changing prepare first breaks nvme and igbxe, amongst other things,
for the reasons explained just above, so you need to implement
teardown first, plug the MSI controller into it, and only then you can
switch prepare.

What I can do is:

(1) introduce teardown without the call site in msi_remove_device_irq_domain()

(2) add its implementation in the ITS driver

(3) move the prepare crap

(4) add the teardown call to msi_remove_device_irq_domain(), without
    the check on info->alloc_data

With that order, everything will keep working, with a temporary leak
of ITTs in the ITS driver between patches (2) and (4).

Is that something you can live with?

	M.

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