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

Marc Zyngier
2025-05-12T17:00:36Z

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.