RE: [PATCH v2 8/8] irqchip/al-fic: add irq_set_affinity callback

[Farber, Eliav]

cat /home/farbere/work/reply-radu-v2-8of8-redirect.txt
On Sat, 2026-10-10 at 16:19 -0400, Radu Rendec wrote:
> On Mon, 2026-10-05 at 11:20 +0000, Farber, Eliav wrote:
> > On Sun, 2026-10-05 at 01:25 +0000, Radu Rendec wrote:
> > > This is a common problem to all multiplexing irq drivers; not
> > > because the parent interrupt is shared but because changing the
> > > affinity of one child interrupt (by changing the parent
> > > interrupt's affinity under the
> > > hood) would have the side effect of changing the affinity of all
> > > its siblings.
> > >
> > > I think the correct behavior is to fail with -EINVAL in this case.
> >
> > -EINVAL is not usable here because it restores the failure this
> > patch fixes. The consumer is arm-cmn. We have a SoC where its
> > interrupt is routed through a FIC group instead of a direct GIC SPI,
> > and drivers/perf/arm-cmn.c does
> >
> >         err = irq_set_affinity(irq, cpumask_of(cmn->cpu));
> >         if (err)
> >                 return err;
> >
> > at probe, so with no .irq_set_affinity callback the PMU does not
> > probe at all.
>
> All right, now I see what the problem is. I will be fully honest and
> transparent and admit I don't know much about PMU drivers in general
> or arm-cmn in particular. But I believe that irq_set_affinity() (with
> a hard fail) is there for a reason. Of course, because we are in 2026,
> I did ask a LLM what that reason could be, but I will refrain from
> taking the answer as correct/definitive or speculating; I will just
> say it probably needs careful consideration.
>
> The requested affinity mask includes a single CPU; by contrast, unless
> explicitly configured, the FIC parent IRQ will probably be affine to
> all available CPUs, so the cpumask_equal() in your patch will likely
> be always false. The concern is not the warning itself (you're using
> pr_warn_once); it's that the arm-cmn IRQ handler will run on a
> different CPU than it expects/has requested.
>
> > > If you insist on having the ability to change the child interrupts'
> > > affinity, you may want to look at this patch and do something similar:
> > > https://lore.kernel.org/all/20251128212055.1409093-4-rrendec@redha
> > > t.com/
> >
> > I implemented that for al-fic and tested it on hardware. It seems to
> > work: with the parent SPI pinned to one CPU and the child's affinity
> > set to another, the child dispatch is accounted on the target CPU,
> > one dispatch per parent interrupt, no re-fire storm.
> >
> > What stops me from just sending it is that it changes the interrupt
> > path for every child of every FIC instance, not only for a child
> > that asks for affinity. .irq_pre_redirect is called before the
> > affinity check, so it runs on every dispatch. The ack therefore has
> > to leave the flow handler and be done in the parent context always,
> > and .irq_ack has to become a noop so it is not done twice. A level
> > configured group has to be masked in .irq_pre_redirect as well,
> > because the FIC re-asserts the cause bit while the source is still
> > asserted, so for those groups the mask moves into the parent context
> > too. Today all of this happens inside
> > handle_level_irq() and handle_edge_irq(), under desc->lock.
>
> The dw_pci driver is not the best example because it uses a stacked
> layout with the multiplexing irqchip at the bottom and a hierarchy
> irqchip on top (which is why the .irq_pre_redirect was necessary).
> Unfortunately it's the only example I am aware of (and also what
> prompted the redirect framework implementation). I apologize for the
> confusion it may have created.
>
> I think in the case of al-fic, it's sufficient to set
> irq_chip_redirect_set_affinity as the .irq_set_affinity handler and
> replace generic_handle_domain_irq with generic_handle_demux_domain_irq.
> That will not change the ack/mask path.
>
> Sorry! Scratch that. Human hallucination :)
>
> Yes, your interpretation is correct. The ack and mask need to move to
> .irq_pre_redirect because they must happen in hardware interrupt
> context; if the demultiplexed interrupt is redirected, its flow handler
> (handle_level_irq() or handle_edge_irq()) runs asynchronously in
> irq_work context, so ack/mask can't happen there.

In v5 I implemented it this way, based on your suggestion above and on
the dw_pci reference you and Thomas pointed me to earlier:
.irq_set_affinity is irq_chip_redirect_set_affinity, the dispatch loop
calls generic_handle_demux_domain_irq() instead of
generic_handle_domain_irq(), .irq_ack is a noop, and the ack (plus the
mask, for a level configured group) now happens in a new
.irq_pre_redirect. Tested on our hardware: with a child's affinity
pinned away from the CPU taking the parent SPI, the ack still happens on
the parent's CPU and the handler body runs on the target CPU, no
re-fire storm.

> But note that even though the call path changes for any interrupt, the
> handler is still called synchronously (at the end of
> generic_handle_demux_domain_irq()) if the affinity hasn't been changed
> and redirection is not necessary. So there is no overhead in that
> case.

Confirmed that too - the inline path is unchanged when no redirect is
needed.

> I think all this would be less horrible for a driver that wants to
> implement redirection if we changed changed handle_level_irq() and
> handle_edge_irq() to skip the ack/mask for irqchips that implement
> redirection (e.g. based on an irqchip flag), and provided a default
> implementation of .irq_pre_redirect that called the ack/mask handlers
> instead. That way, all an irqchip driver would have to do to implement
> redirection would be:
>  * set the flag that skips ack/mask in the flow handlers;
>  * set .irq_set_affinity = irq_chip_redirect_set_affinity;
>  * set .irq_pre_redirect = default_implementation_to_be_added.
>
> I'll try to draft something along those lines next week and send an
> RFC patch. We'll see what Thomas thinks :)

> By the way, did you mention the FIC chips could be stacked or am I
> imagining things again? That could be an extra wrinkle in the context
> of redirection.

Not imagining it - al-fic groups can be stacked, an FIC's parent can be
another FIC's output instead of a direct GIC SPI. Tested that case too
and redirection worked the same way through the extra level.

> > Do you still consider that justified for this driver, given that the
> > only caller needing anything here is one that just needs the call not
> > to fail?
>
> As I explained above, I really doubt that arm-cmn just needs the call
> not to fail. I am inclined to think that it actually *needs* the IRQ
> to run on the CPU it wants, hence the hard fail.

Agreed.

Will send v5 shortly.

Thanks,
Eliav