On Thursday, October 14, 2021 8:23 PM,
Michael Kelley [off-list ref] 

At a micro-level, I've reviewed the patch set and could give my
Reviewed-By, though someone more expert on IRQ domains
than I am should definitely review as well.

But I've been thinking about the macro-level organization of
the code, and the handling of the x86 and ARM64 differences.
Short of creating two new .c files, one x86 specific and one
ARM64 specific (which seems like overkill), there's no getting
away from a few #ifdef's, and indeed pci-hyperv.c already
has a couple.  The problem space is just messy.

So if that's the case, then I'm not seeing much value in having
the code in pci-hyperv-irqchip.c broken out into a separate
source code file.  I did some playing around with organizing
the new functionality into the existing pci-hyperv.c with the
needed #ifdef's, and it seems a bit cleaner to me.   The new
.h file is also eliminated, and there are other small simplifications
that can be made by having everything in pci-hyperv.c.   With
this reorg, there are 50+ fewer lines added (though some of
the savings is admittedly just source code file headers).   I
can send you a .diff of the reorg'ed code if you want it.

Also, a good bit of the code under #ifdef ARM64 will compile
just fine on x86, though it wouldn't be used.  It's actually the
ARM64 side that more naturally fits the Linux IRQ domain model,
with the x86 side being the special case because of the quirks of
the x86 interrupt architecture.  When thinking about the code
from that standpoint, it's another reason to put the code all
into pci-hyperv.c.

The best overall structure to use is a judgment call because
there are tradeoffs for any of the choices.  There's no clear
"right" answer.  As such, my preference to combine all the
code into pci-hyperv.c is just a suggestion.  I won't try to hold
up accepting the code if you decide you want to keep the
current structure with separate pci-hyperv-irqchip.[ch] files.

Michael

Thanks for the feedback. Overall, I think it makes sense to keep
the irq chip implementation in a separate file because it will give
us more flexibility in the future to alter the irq chip implementation
or which irq chip we pick as parent (for example, we likely will
parent to the LPI irq chip in future) without having to touch the
core of the pci-hyperv. To me that separation sounds more logical
and beneficial than reducing a few lines of code immediately.

- Sunil