Re: [PATCHv3 2/6] arm64: don't use GENERIC_IRQ_MULTI_HANDLER

[Mark Rutland]

Hi Christoph,

On Mon, Mar 15, 2021 at 07:28:03PM +, Christoph Hellwig wrote:
> On Mon, Mar 15, 2021 at 11:56:25AM +, Mark Rutland wrote:
> > From: Marc Zyngier 
> > 
> > In subsequent patches we want to allow irqchip drivers to register as
> > FIQ handlers, with a set_handle_fiq() function. To keep the IRQ/FIQ
> > paths similar, we want arm64 to provide both set_handle_irq() and
> > set_handle_fiq(), rather than using GENERIC_IRQ_MULTI_HANDLER for the
> > former.
> 
> Having looked through the series I do not understand this rationale
> at all.  You've only added the default_handle_irq logic, which seems
> perfectly suitable and desirable for the generic version. 

The default_handle_irq thing isn't the point of the series, that part is
all preparatory work. I agree that probably makes sense for the generic
code, and I'm happy to update core code with this.

The big thing here is that (unlike most architectures), with arm64 a CPU
has two interrupt pins, IRQ and FIQ, and we need separate root handlers
for these. That's what this series aims to do, and patches 1-5 are all
preparatory work with that appearing in patch 6.

Our initial stab at this did try to add that support to core code, but
that was more painful to deal with, since you either add abstractions to
make this look generic that make the code more complex for bot hthe
genreic code and arch code, or you place arch-specific assumptions in
the core code. See Marc's eariler stab at this, where in effect we had
to duplicate the logic in the core code so that we didn't adversely
affect existing entry assembly on other architectures due to the way the
function pointers were stored.

> Please don't fork off generic code for no good reason.

I appreciate that this runs counter to the general goal of making things
generic wherever possible, but I do think in this case we have good
reasons, and the duplication is better than adding single-user
abstractions in the generic code that complicate the generic code and
arch code.

Thanks,
Mark.

Re: [PATCHv3 2/6] arm64: don't use GENERIC_IRQ_MULTI_HANDLER

[Christoph Hellwig]

On Mon, Mar 15, 2021 at 11:56:25AM +, Mark Rutland wrote:
> From: Marc Zyngier 
> 
> In subsequent patches we want to allow irqchip drivers to register as
> FIQ handlers, with a set_handle_fiq() function. To keep the IRQ/FIQ
> paths similar, we want arm64 to provide both set_handle_irq() and
> set_handle_fiq(), rather than using GENERIC_IRQ_MULTI_HANDLER for the
> former.

Having looked through the series I do not understand this rationale
at all.  You've only added the default_handle_irq logic, which seems
perfectly suitable and desirable for the generic version.  Please
don't fork off generic code for no good reason.

[PATCHv3 2/6] arm64: don't use GENERIC_IRQ_MULTI_HANDLER

[Mark Rutland]

From: Marc Zyngier 

In subsequent patches we want to allow irqchip drivers to register as
FIQ handlers, with a set_handle_fiq() function. To keep the IRQ/FIQ
paths similar, we want arm64 to provide both set_handle_irq() and
set_handle_fiq(), rather than using GENERIC_IRQ_MULTI_HANDLER for the
former.

This patch adds an arm64-specific implementation of set_handle_irq().
There should be no functional change as a result of this patch.

Signed-off-by: Marc Zyngier 
[Mark: use a single handler pointer]
Signed-off-by: Mark Rutland 
Tested-by: Hector Martin 
Cc: Catalin Marinas 
Cc: James Morse 
Cc: Thomas Gleixner 
Cc: Will Deacon 
---
 arch/arm64/Kconfig   |  1 -
 arch/arm64/include/asm/irq.h |  3 +++
 arch/arm64/kernel/irq.c  | 11 +++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5656e7aacd69..e7d2405be71f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -110,7 +110,6 @@ config ARM64
select GENERIC_EARLY_IOREMAP
select GENERIC_IDLE_POLL_SETUP
select GENERIC_IRQ_IPI
-   select GENERIC_IRQ_MULTI_HANDLER
select GENERIC_IRQ_PROBE
select GENERIC_IRQ_SHOW
select GENERIC_IRQ_SHOW_LEVEL
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index b2b0c6405eb0..8391c6f6f746 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -8,6 +8,9 @@
 
 struct pt_regs;
 
+int set_handle_irq(void (*handle_irq)(struct pt_regs *));
+#define set_handle_irq set_handle_irq
+
 static inline int nr_legacy_irqs(void)
 {
return 0;
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index dfb1feab867d..ad63bd50fa7b 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -71,6 +71,17 @@ static void init_irq_stacks(void)
 }
 #endif
 
+void (*handle_arch_irq)(struct pt_regs *) __ro_after_init;
+
+int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
+{
+   if (handle_arch_irq)
+   return -EBUSY;
+
+   handle_arch_irq = handle_irq;
+   return 0;
+}
+
 void __init init_IRQ(void)
 {
init_irq_stacks();
-- 
2.11.0