RE: [PATCH v2 6/8] irqchip/al-fic: support error and fatal outputs and FIC v2

[Farber, Eliav]

On Sun, 2026-10-05 at 01:01 +0000, Radu Rendec wrote:
> On Sun, 2026-09-27 at 08:06 +0000, Eliav Farber wrote:
> > +   if (version == AL_FIC_VERSION_V2 &&
> > +       (fic_id == AL_FIC_ID_ERROR || fic_id == AL_FIC_ID_FATAL))
> > +           gc_flags = 0;
> > +   else
> > +           gc_flags = IRQ_GC_INIT_MASK_CACHE;
>
> This is correct but gc_flags can be set to IRQ_GC_INIT_MASK_CACHE at
> the declaration, and then the "else" branch is not needed. Or it can be
> initialized to 0 and set to IRQ_GC_INIT_MASK_CACHE for the version/id
> that support it; the condition would have to be flipped of course e.g.
>     if (version != AL_FIC_VERSION_V2 || fic_id == AL_FIC_ID_INFO)
>         gc_flags = IRQ_GC_INIT_MASK_CACHE;
>
> That's just a suggestion, and it's totally fine with me if you prefer
> to keep it like that.

Took the first form - initialise it at the declaration and only clear it
on the v2 error/fatal path.

> > +   if (!(gc_flags & IRQ_GC_INIT_MASK_CACHE))
> > +           gc->mask_cache = 0xFFFFFFFF;
>
> This is correct but I prefer to write it as ~0U. It's easy to miss one
> 'F', both when writing and reading it. Again, it's just a suggestion
> and a matter of style.

Done, here and in the three mask-register writes below.

> > +   /* make sure the controller works in non msi_x mode */
> > +   control |= CONTROL_MASK_MSI_X;
>
> The side effect of this is that all the other bits previously set in
> the AL_FIC_CONTROL register are preserved, whereas before this patch
> they were reset by initializing "control" to CONTROL_MASK_MSI_X. Is
> this intentional? If it is, then perhaps it deserves a comment because
> it looks like a behavior change.

It is intentional, but it is not actually a behaviour change. Every
writable bit in this register resets to 0; the only non-zero reset field
is the revision, which is read-only. So at probe the read-modify-write
produces the same value as building it from CONTROL_MASK_MSI_X alone.

I kept the read-modify-write because it is the better practice - it costs
nothing and does not rely on the reset value staying 0 - and because the
version field now has to be read from this register anyway. I did not add
a code comment since there is no surprising behaviour to flag, but I added
a paragraph to the commit message explaining the equivalence. Let me know
if you would still rather see a comment at the write site.

All of this is in v3, which I will post shortly.

Thanks,
Eliav