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

[Radu Rendec]

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@xxxxxxxxxx/
> > 
> > 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.

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.

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.

> > 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.