| Message ID | 20211021180236.37428-6-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irq: remove handle_domain_{irq,nmi}() | expand |
On Thu, Oct 21, 2021 at 07:02:26PM +0100, Mark Rutland wrote: > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > handle_arch_irq() without performing any entry accounting. > > Add a generic wrapper to handle the commoon irqentry work when invoking > handle_arch_irq(). Where an architecture needs to perform some entry > accounting itself, it will need to invoke handle_arch_irq() itself. > > In subsequent patches it will become the responsibilty of the entry code > to set the irq regs when entering an IRQ (rather than deferring this to > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > regs now. This can be redundant in some cases, but is never harmful as > saving/restoring the old regs nests safely. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/irq/handle.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 221d80c31e94..27182003b879 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -14,6 +14,8 @@ > #include <linux/interrupt.h> > #include <linux/kernel_stat.h> > > +#include <asm/irq_regs.h> > + > #include <trace/events/irq.h> > > #include "internals.h" > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > handle_arch_irq = handle_irq; > return 0; > } > + > +/** > + * generic_handle_arch_irq - root irq handler for architectures which do no > + * entry accounting themselves > + * @regs: Register file coming from the low-level handling code > + */ > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + handle_arch_irq(regs); After all involved arches call generic_handle_arch_irq(), can handle_arch_irq be encapsulated by declaring as static? Two places need to be fixed for this purpose: -1. absorb the logic about handle_arch_irq in arm64/kernel/irq.c -2. In arm, setup_arch(), #ifdef CONFIG_GENERIC_IRQ_MULTI_HANDLER handle_arch_irq = mdesc->handle_irq; #endif Thanks, Pingfan
On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > handle_arch_irq() without performing any entry accounting. > > Add a generic wrapper to handle the commoon irqentry work when invoking > handle_arch_irq(). Where an architecture needs to perform some entry > accounting itself, it will need to invoke handle_arch_irq() itself. > > In subsequent patches it will become the responsibilty of the entry code > to set the irq regs when entering an IRQ (rather than deferring this to > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > regs now. This can be redundant in some cases, but is never harmful as > saving/restoring the old regs nests safely. Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/irq/handle.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 221d80c31e94..27182003b879 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -14,6 +14,8 @@ > #include <linux/interrupt.h> > #include <linux/kernel_stat.h> > > +#include <asm/irq_regs.h> > + > #include <trace/events/irq.h> > > #include "internals.h" > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > handle_arch_irq = handle_irq; > return 0; > } > + > +/** > + * generic_handle_arch_irq - root irq handler for architectures which do no > + * entry accounting themselves > + * @regs: Register file coming from the low-level handling code > + */ > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + > + irq_enter(); > + old_regs = set_irq_regs(regs); > + handle_arch_irq(regs); > + set_irq_regs(old_regs); > + irq_exit(); > +} > #endif > -- > 2.11.0 >
On Fri, Oct 22, 2021 at 10:33:53AM +0800, Guo Ren wrote: > On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > handle_arch_irq() without performing any entry accounting. > > > > Add a generic wrapper to handle the commoon irqentry work when invoking Ah, I'd typo'd 'common' there; I'll go fix that... > > handle_arch_irq(). Where an architecture needs to perform some entry > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > In subsequent patches it will become the responsibilty of the entry code > > to set the irq regs when entering an IRQ (rather than deferring this to > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > regs now. This can be redundant in some cases, but is never harmful as > > saving/restoring the old regs nests safely. > Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. We do, in patch 15 when handle_domain_irq() is removed entirely. :) Removing it immediately in this patch would require more ifdeffery and/or another copy of handle_domain_irq(), and since the duplication doesn't cause a functional problem, I think it's better to live with the temporary duplication for the next few patches than to try to optimize the intermediate steps at the cost of legibility. Thanks, Mark. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > index 221d80c31e94..27182003b879 100644 > > --- a/kernel/irq/handle.c > > +++ b/kernel/irq/handle.c > > @@ -14,6 +14,8 @@ > > #include <linux/interrupt.h> > > #include <linux/kernel_stat.h> > > > > +#include <asm/irq_regs.h> > > + > > #include <trace/events/irq.h> > > > > #include "internals.h" > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > handle_arch_irq = handle_irq; > > return 0; > > } > > + > > +/** > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > + * entry accounting themselves > > + * @regs: Register file coming from the low-level handling code > > + */ > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > +{ > > + struct pt_regs *old_regs; > > + > > + irq_enter(); > > + old_regs = set_irq_regs(regs); > > + handle_arch_irq(regs); > > + set_irq_regs(old_regs); > > + irq_exit(); > > +} > > #endif > > -- > > 2.11.0 > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
On Fri, Oct 22, 2021 at 10:10:26AM +0800, Pingfan Liu wrote: > On Thu, Oct 21, 2021 at 07:02:26PM +0100, Mark Rutland wrote: > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > handle_arch_irq() without performing any entry accounting. > > > > Add a generic wrapper to handle the commoon irqentry work when invoking > > handle_arch_irq(). Where an architecture needs to perform some entry > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > In subsequent patches it will become the responsibilty of the entry code > > to set the irq regs when entering an IRQ (rather than deferring this to > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > regs now. This can be redundant in some cases, but is never harmful as > > saving/restoring the old regs nests safely. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > index 221d80c31e94..27182003b879 100644 > > --- a/kernel/irq/handle.c > > +++ b/kernel/irq/handle.c > > @@ -14,6 +14,8 @@ > > #include <linux/interrupt.h> > > #include <linux/kernel_stat.h> > > > > +#include <asm/irq_regs.h> > > + > > #include <trace/events/irq.h> > > > > #include "internals.h" > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > handle_arch_irq = handle_irq; > > return 0; > > } > > + > > +/** > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > + * entry accounting themselves > > + * @regs: Register file coming from the low-level handling code > > + */ > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > +{ > > + struct pt_regs *old_regs; > > + > > + irq_enter(); > > + old_regs = set_irq_regs(regs); > > + handle_arch_irq(regs); > > After all involved arches call generic_handle_arch_irq(), can > handle_arch_irq be encapsulated by declaring as static? > > Two places need to be fixed for this purpose: > -1. absorb the logic about handle_arch_irq in arm64/kernel/irq.c > -2. In arm, setup_arch(), > #ifdef CONFIG_GENERIC_IRQ_MULTI_HANDLER > handle_arch_irq = mdesc->handle_irq; > #endif That arm bit would need to be set_handle_irq(mdesc->handle_irq); anywhere it uses handle_arch_irq it's depending on the CONFIG_GENERIC_IRQ_MULTI_HANDLER definition. While I agree it would seem nice to encapsulate this, in future we want architectures to move to the more correct entry sequencing described in the cover letter, and when that happens they should invoke handle_arch_irq() directly, so I think this is best to leave as-is. We have custom logic on arm64 because we want to handle IRQ and FIQ consistently, and there wasn't a neat way to bodge that into the generic code, but that issue doesn't apply to the other users of CONFIG_GENERIC_IRQ_MULTI_HANDLER. Thanks, Mark.
On Fri, Oct 22, 2021 at 4:52 PM Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Oct 22, 2021 at 10:33:53AM +0800, Guo Ren wrote: > > On Fri, Oct 22, 2021 at 2:03 AM Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to > > > handle_arch_irq() without performing any entry accounting. > > > > > > Add a generic wrapper to handle the commoon irqentry work when invoking > > Ah, I'd typo'd 'common' there; I'll go fix that... > > > > handle_arch_irq(). Where an architecture needs to perform some entry > > > accounting itself, it will need to invoke handle_arch_irq() itself. > > > > > > In subsequent patches it will become the responsibilty of the entry code > > > to set the irq regs when entering an IRQ (rather than deferring this to > > > an irqchip handler), so generic_handle_arch_irq() is made to set the irq > > > regs now. This can be redundant in some cases, but is never harmful as > > > saving/restoring the old regs nests safely. > > Shall we remove old_regs save/restore in handle_domain_irq? It's duplicated. > > We do, in patch 15 when handle_domain_irq() is removed entirely. :) I miss that patch. Yes, in generic_handle_domain_nmi. > > Removing it immediately in this patch would require more ifdeffery and/or > another copy of handle_domain_irq(), and since the duplication doesn't cause a > functional problem, I think it's better to live with the temporary duplication > for the next few patches than to try to optimize the intermediate steps at the > cost of legibility. Thx for explaining. Reviewed-by: Guo Ren <guoren@kernel.org> > > Thanks, > Mark. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Cc: Marc Zyngier <maz@kernel.org> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > --- > > > kernel/irq/handle.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > > > index 221d80c31e94..27182003b879 100644 > > > --- a/kernel/irq/handle.c > > > +++ b/kernel/irq/handle.c > > > @@ -14,6 +14,8 @@ > > > #include <linux/interrupt.h> > > > #include <linux/kernel_stat.h> > > > > > > +#include <asm/irq_regs.h> > > > + > > > #include <trace/events/irq.h> > > > > > > #include "internals.h" > > > @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > > > handle_arch_irq = handle_irq; > > > return 0; > > > } > > > + > > > +/** > > > + * generic_handle_arch_irq - root irq handler for architectures which do no > > > + * entry accounting themselves > > > + * @regs: Register file coming from the low-level handling code > > > + */ > > > +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) > > > +{ > > > + struct pt_regs *old_regs; > > > + > > > + irq_enter(); > > > + old_regs = set_irq_regs(regs); > > > + handle_arch_irq(regs); > > > + set_irq_regs(old_regs); > > > + irq_exit(); > > > +} > > > #endif > > > -- > > > 2.11.0 > > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c index 221d80c31e94..27182003b879 100644 --- a/kernel/irq/handle.c +++ b/kernel/irq/handle.c @@ -14,6 +14,8 @@ #include <linux/interrupt.h> #include <linux/kernel_stat.h> +#include <asm/irq_regs.h> + #include <trace/events/irq.h> #include "internals.h" @@ -226,4 +228,20 @@ int __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) handle_arch_irq = handle_irq; return 0; } + +/** + * generic_handle_arch_irq - root irq handler for architectures which do no + * entry accounting themselves + * @regs: Register file coming from the low-level handling code + */ +asmlinkage void noinstr generic_handle_arch_irq(struct pt_regs *regs) +{ + struct pt_regs *old_regs; + + irq_enter(); + old_regs = set_irq_regs(regs); + handle_arch_irq(regs); + set_irq_regs(old_regs); + irq_exit(); +} #endif
Several architectures select GENERIC_IRQ_MULTI_HANDLER and branch to handle_arch_irq() without performing any entry accounting. Add a generic wrapper to handle the commoon irqentry work when invoking handle_arch_irq(). Where an architecture needs to perform some entry accounting itself, it will need to invoke handle_arch_irq() itself. In subsequent patches it will become the responsibilty of the entry code to set the irq regs when entering an IRQ (rather than deferring this to an irqchip handler), so generic_handle_arch_irq() is made to set the irq regs now. This can be redundant in some cases, but is never harmful as saving/restoring the old regs nests safely. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> --- kernel/irq/handle.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)