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

[Radu Rendec <radu@rendec.net>]

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

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

-- 
Regards,
Radu