Skip to contentSkip to conversation

Remove setup_percpu_irq() & remove_percpu_irq

PATCH In review afzal mohammed 2 participants 6 messages Apr 11, 2020 – Apr 20, 2020

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover Remove setup_percpu_irq() & remove_percpu_irq

Hi,

While doing the removal of setup_irq(), it was observed that setup_percpu_irq() also can be removed similarly by replacing it w/ request_percpu_irq(), which does allocate memory. In the initial setup_irq() removal cover letters [1], it was mentioned that setup_percpu_irq() is untouched.

After removing setup_irq(), it does not look good to let live setup_percpu_irq(), especially since it being a low hanging fruit. Hence replace setup_percpu_irq() by it's allocator equivalent. request_percpu_irq() cannot be used since all the users need to pass IRQF_TIMER flag, which it would not allow. Thus it's variant, __request_percpu_irq() is used.

In addition to removing setup_percpu_irq() definition, remove_percpu_irq(), unused, is also removed.

It seems setup_percpu_irq() is used only by MIPS (even the one outside MIPS arch dir, i.e. the clocksource driver). The series has been build tested, though i would have loved to do a qemu test as well, i do not see a qemu machine type that can test the changes done here (in malta machine, in which clocksource changes are getting built, the changes done here are not being runtime tested).

Regards
afzal

[1] https://lkml.kernel.org/r/[email protected]

afzal mohammed (3):
MIPS: Replace setup_percpu_irq() by request_percpu_irq() variant clocksource/drivers/mips-gic-timer: Replace setup_percpu_irq() by
request_percpu_irq() variant
genirq: Remove setup_percpu_irq() and remove_percpu_irq()

 arch/mips/include/asm/cevt-r4k.h     |  1 -
 arch/mips/kernel/cevt-r4k.c          | 11 --------
 arch/mips/sgi-ip27/ip27-timer.c      | 13 ++++-----
 arch/mips/sgi-ip30/ip30-timer.c      |  6 ++--
 drivers/clocksource/mips-gic-timer.c | 10 ++-----
 include/linux/irq.h                  |  2 --
 kernel/irq/manage.c                  | 42 ----------------------------
 7 files changed, 10 insertions(+), 75 deletions(-)
Signature · 1 line
2.18.0
#1 lore raw

PATCH 1/3 MIPS: Replace setup_percpu_irq() by request_percpu_irq() variant

Recently all usages of setup_irq() was replaced by request_irq() as allocators are ready by the time setup_irq() was invoked. Similarly setup_percpu_irq() can be replaced by request_percpu_irq(). But in the callers handled here __request_percpu_irq() has to be used as request_percpu_irq() does not give the user a chance to pass the flags, and IRQF_TIMER has to be passed here, so the variant __request_percpu_irq() is used.

---
 arch/mips/include/asm/cevt-r4k.h |  1 -
 arch/mips/kernel/cevt-r4k.c      | 11 -----------
 arch/mips/sgi-ip27/ip27-timer.c  | 13 +++++--------
 arch/mips/sgi-ip30/ip30-timer.c  |  6 +++---
 4 files changed, 8 insertions(+), 23 deletions(-)
arch/mips/include/asm/cevt-r4k.h +0 -1
index 2e13a038d260..5229eb34f28a 100644
@@ -23,7 +23,6 @@ void mips_event_handler(struct clock_event_device *dev);
 int c0_compare_int_usable(void);
 irqreturn_t c0_compare_interrupt(int, void *);
 
-extern struct irqaction c0_compare_irqaction;
 extern int cp0_timer_irq_installed;
 
 #endif /* __ASM_CEVT_R4K_H */
arch/mips/kernel/cevt-r4k.c +0 -11
index 17a9cbb8b3df..4ffa9f485d07 100644
@@ -158,17 +158,6 @@ irqreturn_t c0_compare_interrupt(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-struct irqaction c0_compare_irqaction = {
-	.handler = c0_compare_interrupt,
-	/*
-	 * IRQF_SHARED: The timer interrupt may be shared with other interrupts
-	 * such as perf counter and FDC interrupts.
-	 */
-	.flags = IRQF_PERCPU | IRQF_TIMER | IRQF_SHARED,
-	.name = "timer",
-};
-
-
 void mips_event_handler(struct clock_event_device *dev)
 {
 }
arch/mips/sgi-ip27/ip27-timer.c +5 -8
index 61f3565f3645..6e2b58b47580 100644
@@ -68,13 +68,6 @@ static irqreturn_t hub_rt_counter_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-struct irqaction hub_rt_irqaction = {
-	.handler	= hub_rt_counter_handler,
-	.percpu_dev_id	= &hub_rt_clockevent,
-	.flags		= IRQF_PERCPU | IRQF_TIMER,
-	.name		= "hub-rt",
-};
-
 /*
  * This is a hack; we really need to figure these values out dynamically
  *
@@ -111,9 +104,13 @@ void hub_rt_clock_event_init(void)
 
 static void __init hub_rt_clock_event_global_init(void)
 {
+	int irq = IP27_RT_TIMER_IRQ;
+
 	irq_set_handler(IP27_RT_TIMER_IRQ, handle_percpu_devid_irq);
 	irq_set_percpu_devid(IP27_RT_TIMER_IRQ);
-	setup_percpu_irq(IP27_RT_TIMER_IRQ, &hub_rt_irqaction);
+	if (__request_percpu_irq(irq, hub_rt_counter_handler, IRQF_TIMER,
+				 "hub-rt", &hub_rt_clockevent) < 0)
+		pr_err("Failed to request percpu irq %d (hub-rt)\n", irq);
 }
 
 static u64 hub_rt_read(struct clocksource *cs)
arch/mips/sgi-ip30/ip30-timer.c +3 -3
index d13e105478ae..dcc22eaddcda 100644
@@ -52,11 +52,11 @@ void __init plat_time_init(void)
 	int irq = get_c0_compare_int();
 
 	cp0_timer_irq_installed = 1;
-	c0_compare_irqaction.percpu_dev_id = &mips_clockevent_device;
-	c0_compare_irqaction.flags &= ~IRQF_SHARED;
 	irq_set_handler(irq, handle_percpu_devid_irq);
 	irq_set_percpu_devid(irq);
-	setup_percpu_irq(irq, &c0_compare_irqaction);
+	if (__request_percpu_irq(irq, c0_compare_interrupt, IRQF_TIMER, "timer",
+				 &mips_clockevent_device) < 0)
+		pr_err("Failed to request percpu irq %d (timer)\n", irq);
 	enable_percpu_irq(irq, IRQ_TYPE_NONE);
 
 	ip30_heart_clocksource_init();
Signature · 1 line
2.18.0
#2 lore raw

PATCH 2/3 clocksource/drivers/mips-gic-timer: Replace setup_percpu_irq() by request_percpu_irq() variant

In reply to afzal mohammed

Recently all usages of setup_irq() was replaced by request_irq() as allocators are ready by the time setup_irq() was invoked. Similarly setup_percpu_irq() can be replaced by request_percpu_irq(). But in the callers handled here __request_percpu_irq() has to be used as request_percpu_irq() does not give the user a chance to pass the flags, and IRQF_TIMER has to be passed here, so the variant __request_percpu_irq() is used.

---
 drivers/clocksource/mips-gic-timer.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/clocksource/mips-gic-timer.c +2 -8
index 37671a5d4ed9..e8e6bb6159f5 100644
@@ -67,13 +67,6 @@ static irqreturn_t gic_compare_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static struct irqaction gic_compare_irqaction = {
-	.handler = gic_compare_interrupt,
-	.percpu_dev_id = &gic_clockevent_device,
-	.flags = IRQF_PERCPU | IRQF_TIMER,
-	.name = "timer",
-};
-
 static void gic_clockevent_cpu_init(unsigned int cpu,
 				    struct clock_event_device *cd)
 {
@@ -137,7 +130,8 @@ static int gic_clockevent_init(void)
 	if (!gic_frequency)
 		return -ENXIO;
 
-	ret = setup_percpu_irq(gic_timer_irq, &gic_compare_irqaction);
+	ret = __request_percpu_irq(gic_timer_irq, gic_compare_interrupt,
+				   IRQF_TIMER, "timer", &gic_clockevent_device);
 	if (ret < 0) {
 		pr_err("IRQ %d setup failed (%d)\n", gic_timer_irq, ret);
 		return ret;
Signature · 1 line
2.18.0
#3 lore raw

PATCH 3/3 genirq: Remove setup_percpu_irq() and remove_percpu_irq()

In reply to afzal mohammed

Only users of setup_percpu_irq() were in MIPS, they have been converted to use request_percpu_irq() variants. Also remove_percpu_irq() have no users.

Hence remove setup_percpu_irq() as well as remove_percpu_irq().

---
 include/linux/irq.h |  2 --
 kernel/irq/manage.c | 42 ------------------------------------------
 2 files changed, 44 deletions(-)
include/linux/irq.h +0 -2
index 29f5bad87eb3..156995ab5d6e 100644
@@ -560,8 +560,6 @@ enum {
 #define IRQ_DEFAULT_INIT_FLAGS	ARCH_IRQ_INIT_FLAGS
 
 struct irqaction;
-extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
-extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
 
 extern void irq_cpu_online(void);
 extern void irq_cpu_offline(void);
kernel/irq/manage.c +0 -42

54 lines not on this page · raw message

Signature · 1 line
2.18.0
#4 lore raw
In reply to afzal mohammed

Hi Thomas Gleixner,

Quoted 13 lines · afzal mohammed

While doing the removal of setup_irq(), it was observed that setup_percpu_irq() also can be removed similarly by replacing it w/ request_percpu_irq(), which does allocate memory. In the initial setup_irq() removal cover letters [1], it was mentioned that setup_percpu_irq() is untouched.

After removing setup_irq(), it does not look good to let live setup_percpu_irq(), especially since it being a low hanging fruit. Hence replace setup_percpu_irq() by it's allocator equivalent. request_percpu_irq() cannot be used since all the users need to pass IRQF_TIMER flag, which it would not allow. Thus it's variant, __request_percpu_irq() is used.

Quoted 2 lines

In addition to removing setup_percpu_irq() definition, remove_percpu_irq(), unused, is also removed.

Do you feel that this series adds value ?, if not, i will abandon this series.

Thanks for your guidance w.r.t setup_irq() removal.

Regards
afzal

#5 lore raw

Afzal,

Quoted 15 lines · afzal mohammed
Quoted 13 lines · afzal mohammed

While doing the removal of setup_irq(), it was observed that setup_percpu_irq() also can be removed similarly by replacing it w/ request_percpu_irq(), which does allocate memory. In the initial setup_irq() removal cover letters [1], it was mentioned that setup_percpu_irq() is untouched.

After removing setup_irq(), it does not look good to let live setup_percpu_irq(), especially since it being a low hanging fruit. Hence replace setup_percpu_irq() by it's allocator equivalent. request_percpu_irq() cannot be used since all the users need to pass IRQF_TIMER flag, which it would not allow. Thus it's variant, __request_percpu_irq() is used.

Quoted 5 lines
Quoted 2 lines

In addition to removing setup_percpu_irq() definition, remove_percpu_irq(), unused, is also removed.

Do you feel that this series adds value ?, if not, i will abandon this series.

 7 files changed, 10 insertions(+), 75 deletions(-)

is definitely worth it. There is no point in having two interfaces. I'll have a look at the changes later today.

Thanks,

tglx