Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/18] genirq: Remove setup_irq()
@ 2020-02-12  8:01 afzal mohammed
  2020-02-12  8:03 ` [PATCH 06/18] m68k: Replace setup_irq() by request_irq() afzal mohammed
  0 siblings, 1 reply; 6+ messages in thread
From: afzal mohammed @ 2020-02-12  8:01 UTC (permalink / raw)
  To: linux-kernel, linux-rpi-kernel, linux-arm-kernel,
	linux-samsung-soc, x86, linux-sh, linux-s390, linuxppc-dev,
	linux-parisc, linux-mips, linux-m68k, linux-ia64, linux-hexagon,
	linux-c6x-dev, linux-omap, linux-alpha
  Cc: Thomas Gleixner, Julia Lawall, Gilles Muller, Nicolas Palix,
	Michal Marek

While trying to understand internals of irq handling, came across a
thread [1] in which tglx was referring to avoid usage of setup_irq().
Existing callers of setup_irq() reached mostly via 'init_IRQ()' &
'time_init()', while memory allocators are ready by 'mm_init()'.

Hence instances of setup_irq() is replaced by request_irq() &
setup_irq() (along with remove_irq()) definition deleted in the last
patch.

Seldom remove_irq() usage has been observed coupled with setup_irq(),
wherever that has been found, it too has been replaced by free_irq().

Build & boot tested on ARM & x86_64 platforms (ensured that on the
machines used for testing there was an existing setup_irq()
invocation occuring at runtime)

Much of the changes were created using Coccinelle with an intention
to learn it. spatch command was directly run w/ semantic patch below.
But not everything could be automated.

Searching with 'git grep -n '\Wsetup_irq('' & avoiding the irrelevant
ones, 153 invocation's of setup_irq() were found. 112 could be replaced
w/ cocci, of which in a few files some desired hunks were missing or
not as expected, these were fixed up manually. Also the remaining 41
had to be done manually.

Although cocci could replace 112, because of line continue not
happening at paranthesis for request_irq(), around 80 had to be
manually aligned in the request_irq() statement. Problem was with my
below cocci snippet,

- setup_irq(E1,&act);
+ if (request_irq(E1,f_handler,f_flags,f_name,f_dev_id))
+ 	pr_err("request_irq() on %s failed\n", f_name);

Instead of the above, if below is used, line continue happens exactly
at paranthesis, but it lacks addition of printing on request_irq()
failure where existing setup_irq() failure was not doing it.

- setup_irq(E1,&act)
+ request_irq(E1,f_handler,f_flags,f_name,f_dev_id)

Above had an additional advantage of replacing instances of
if (setup_irq()) & BUG(setup_irq()), but luckily those instances were
very few.

So though many changes could be automated, there are a considerable
amount of manual changes, please review carefully especially mips &
alpha.

Usage of setup_percpu_irq() is untouched w/ this series.

There are 2 checkpatch warning about usage of BUG() [which was already
there w/ setup_irq()], they are left as is as it seems appropriate for
tick timer interrupt.

[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos

--->8---

@r1@
identifier ret;
@@

(
setup_irq(...);
|
ret = setup_irq(...);
)


@r2 depends on r1@
identifier act;
@@

static struct irqaction act = {
};

@r11 depends on r2@
identifier r2.act;
identifier f_handler;
@@

(
- act.handler = f_handler;
|
static struct irqaction act = {
 .handler = f_handler,
};
)

@r12 depends on r2@
identifier r2.act;
expression f_name;
@@

(
- act.name = f_name;
|
static struct irqaction act = {
 .name = f_name,
};
)

@r15 depends on r2@
identifier r2.act;
expression f_dev_id;
@@

(
- act.dev_id = f_dev_id;
|
static struct irqaction act = {
 .dev_id = f_dev_id,
};
)

@r16 depends on r2@
identifier r2.act;
expression f_flags;
@@

(
- act.flags = f_flags;
|
static struct irqaction act = {
 .flags = f_flags,
};
)

@r21 depends on r2@
identifier r2.act;
@@

- static struct irqaction act = {
- ...
- };

@r22 depends on r2 && r11 && r12 && r15 && r16@
identifier r2.act;
identifier r11.f_handler;
expression r12.f_name;
expression r15.f_dev_id;
expression r16.f_flags;
expression E1;
identifier ret;
@@

(
- setup_irq(E1,&act);
+ if (request_irq(E1,f_handler,f_flags,f_name,f_dev_id))
+ 	pr_err("request_irq() on %s failed\n", f_name);
|
- ret = setup_irq(E1,&act);
+ ret = request_irq(E1,f_handler,f_flags,f_name,f_dev_id);
)

@r23 depends on r2 && r11 && r12 && !r15 && r16@
identifier r2.act;
identifier r11.f_handler;
expression r12.f_name;
expression r16.f_flags;
expression E1;
identifier ret;
@@

(
- setup_irq(E1,&act);
+ if (request_irq(E1,f_handler,f_flags,f_name,NULL))
+ 	pr_err("request_irq() on %s failed\n", f_name);
|
- ret = setup_irq(E1,&act);
+ ret = request_irq(E1,f_handler,f_flags,f_name,NULL);
)

@r24 depends on r2 && r11 && r12 && r15 && !r16@
identifier r2.act;
identifier r11.f_handler;
expression r12.f_name;
expression r15.f_dev_id;
expression E1;
identifier ret;
@@

(
- setup_irq(E1,&act);
+ if (request_irq(E1,f_handler,0,f_name,f_dev_id))
+ 	pr_err("request_irq() on %s failed\n", f_name);
|
- ret = setup_irq(E1,&act);
+ ret = request_irq(E1,f_handler,0,f_name,f_dev_id);
)

@r25 depends on r2 && r11 && r12 && !r15 && !r16@
identifier r2.act;
identifier r11.f_handler;
expression r12.f_name;
expression E1;
identifier ret;
@@

(
- setup_irq(E1,&act);
+ if (request_irq(E1,f_handler,0,f_name,NULL))
+ 	pr_err("request_irq() on %s failed\n", f_name);
|
- ret = setup_irq(E1,&act);
+ ret = request_irq(E1,f_handler,0,f_name,NULL);
)

--->8---

afzal mohammed (18):
  alpha: replace setup_irq() by request_irq()
  ARM: replace setup_irq() by request_irq()
  c6x: replace setup_irq() by request_irq()
  hexagon: replace setup_irq() by request_irq()
  ia64: replace setup_irq() by request_irq()
  m68k: Replace setup_irq() by request_irq()
  microblaze: Replace setup_irq() by request_irq()
  MIPS: Replace setup_irq() by request_irq()
  parisc: Replace setup_irq() by request_irq()
  powerpc: Replace setup_irq() by request_irq()
  s390: replace setup_irq() by request_irq()
  sh: replace setup_irq() by request_irq()
  unicore32: replace setup_irq() by request_irq()
  x86: Replace setup_irq() by request_irq()
  xtensa: replace setup_irq() by request_irq()
  clocksource: Replace setup_irq() by request_irq()
  irqchip: Replace setup_irq() by request_irq()
  genirq: Remove setup_irq() and remove_irq()

 arch/alpha/kernel/irq_alpha.c                 | 29 ++-------
 arch/alpha/kernel/irq_i8259.c                 |  8 +--
 arch/alpha/kernel/irq_impl.h                  |  7 +--
 arch/alpha/kernel/irq_pyxis.c                 |  3 +-
 arch/alpha/kernel/sys_alcor.c                 |  3 +-
 arch/alpha/kernel/sys_cabriolet.c             |  3 +-
 arch/alpha/kernel/sys_eb64p.c                 |  3 +-
 arch/alpha/kernel/sys_marvel.c                |  2 +-
 arch/alpha/kernel/sys_miata.c                 |  6 +-
 arch/alpha/kernel/sys_ruffian.c               |  3 +-
 arch/alpha/kernel/sys_rx164.c                 |  3 +-
 arch/alpha/kernel/sys_sx164.c                 |  3 +-
 arch/alpha/kernel/sys_wildfire.c              |  7 +--
 arch/alpha/kernel/time.c                      |  6 +-
 arch/arm/mach-cns3xxx/core.c                  | 10 +---
 arch/arm/mach-ebsa110/core.c                  | 10 +---
 arch/arm/mach-ep93xx/timer-ep93xx.c           | 12 ++--
 arch/arm/mach-footbridge/dc21285-timer.c      | 11 +---
 arch/arm/mach-footbridge/isa-irq.c            |  8 +--
 arch/arm/mach-footbridge/isa-timer.c          | 11 +---
 arch/arm/mach-iop32x/time.c                   | 12 ++--
 arch/arm/mach-mmp/time.c                      | 11 +---
 arch/arm/mach-omap1/pm.c                      | 22 ++++---
 arch/arm/mach-omap1/time.c                    | 10 +---
 arch/arm/mach-omap1/timer32k.c                | 10 +---
 arch/arm/mach-omap2/timer.c                   | 11 +---
 arch/arm/mach-rpc/time.c                      |  8 +--
 arch/arm/mach-spear/time.c                    |  9 +--
 arch/arm/plat-orion/time.c                    | 10 +---
 arch/c6x/platforms/timer64.c                  | 11 +---
 arch/hexagon/kernel/smp.c                     | 17 +++---
 arch/hexagon/kernel/time.c                    | 11 +---
 arch/ia64/kernel/irq_ia64.c                   | 42 +++++--------
 arch/ia64/kernel/mca.c                        | 51 +++++-----------
 arch/m68k/68000/timers.c                      |  9 +--
 arch/m68k/coldfire/pit.c                      |  9 +--
 arch/m68k/coldfire/sltimers.c                 | 19 ++----
 arch/m68k/coldfire/timers.c                   | 19 ++----
 arch/microblaze/kernel/timer.c                | 10 +---
 arch/mips/alchemy/common/time.c               | 11 +---
 arch/mips/ar7/irq.c                           | 18 +++---
 arch/mips/ath25/ar2315.c                      |  9 +--
 arch/mips/ath25/ar5312.c                      |  9 +--
 arch/mips/bcm63xx/irq.c                       | 38 +++++-------
 arch/mips/cobalt/irq.c                        | 14 ++---
 arch/mips/dec/setup.c                         | 59 ++++++++-----------
 arch/mips/emma/markeins/irq.c                 | 20 +++----
 arch/mips/include/asm/sni.h                   |  2 +-
 arch/mips/jazz/irq.c                          | 12 +---
 arch/mips/kernel/cevt-bcm1480.c               | 11 +---
 arch/mips/kernel/cevt-ds1287.c                |  9 +--
 arch/mips/kernel/cevt-gt641xx.c               |  9 +--
 arch/mips/kernel/cevt-r4k.c                   |  4 +-
 arch/mips/kernel/cevt-sb1250.c                | 11 +---
 arch/mips/kernel/cevt-txx9.c                  | 11 +---
 arch/mips/kernel/i8253.c                      | 10 +---
 arch/mips/kernel/rtlx-mt.c                    |  8 +--
 arch/mips/kernel/smp.c                        | 33 ++++-------
 arch/mips/lasat/interrupt.c                   | 10 +---
 arch/mips/loongson2ef/common/bonito-irq.c     |  9 +--
 .../loongson2ef/common/cs5536/cs5536_mfgpt.c  | 10 +---
 arch/mips/loongson2ef/fuloong-2e/irq.c        | 14 ++---
 arch/mips/loongson2ef/lemote-2f/irq.c         | 20 ++-----
 arch/mips/loongson32/common/irq.c             | 21 ++++---
 arch/mips/loongson32/common/time.c            | 12 ++--
 arch/mips/loongson64/hpet.c                   | 10 +---
 arch/mips/mti-malta/malta-int.c               | 10 +---
 arch/mips/netlogic/xlr/fmn.c                  |  9 +--
 arch/mips/pmcs-msp71xx/msp_irq.c              | 28 ++++-----
 arch/mips/pmcs-msp71xx/msp_smp.c              | 22 ++-----
 arch/mips/pmcs-msp71xx/msp_time.c             |  7 ++-
 arch/mips/ralink/cevt-rt3352.c                | 17 +++---
 arch/mips/sgi-ip22/ip22-eisa.c                |  8 +--
 arch/mips/sgi-ip22/ip22-int.c                 | 49 +++++----------
 arch/mips/sgi-ip32/ip32-irq.c                 | 18 ++----
 arch/mips/sni/a20r.c                          |  4 +-
 arch/mips/sni/irq.c                           |  8 +--
 arch/mips/sni/pcit.c                          |  8 ++-
 arch/mips/sni/rm200.c                         | 23 +++-----
 arch/mips/sni/time.c                          | 10 +---
 arch/mips/vr41xx/common/irq.c                 |  9 +--
 arch/parisc/kernel/irq.c                      | 21 ++-----
 arch/powerpc/platforms/85xx/mpc85xx_cds.c     | 10 +---
 arch/powerpc/platforms/8xx/cpm1.c             |  9 +--
 arch/powerpc/platforms/8xx/m8xx_setup.c       |  9 +--
 arch/powerpc/platforms/chrp/setup.c           | 14 ++---
 arch/powerpc/platforms/powermac/pic.c         | 31 ++++------
 arch/powerpc/platforms/powermac/smp.c         |  9 +--
 arch/s390/kernel/irq.c                        |  8 +--
 arch/sh/boards/mach-cayman/irq.c              | 18 ++----
 arch/sh/drivers/dma/dma-pvr2.c                |  9 +--
 arch/unicore32/kernel/time.c                  | 11 +---
 arch/x86/kernel/irqinit.c                     | 18 +++---
 arch/x86/kernel/time.c                        | 10 +---
 arch/xtensa/kernel/smp.c                      |  8 +--
 arch/xtensa/kernel/time.c                     | 10 +---
 drivers/clocksource/bcm2835_timer.c           |  8 +--
 drivers/clocksource/bcm_kona_timer.c          | 10 +---
 drivers/clocksource/dw_apb_timer.c            | 11 +---
 drivers/clocksource/exynos_mct.c              | 12 ++--
 drivers/clocksource/mxs_timer.c               | 10 +---
 drivers/clocksource/nomadik-mtu.c             | 11 +---
 drivers/clocksource/samsung_pwm_timer.c       | 12 ++--
 drivers/clocksource/timer-atlas7.c            | 50 ++++++++--------
 drivers/clocksource/timer-cs5535.c            | 10 +---
 drivers/clocksource/timer-efm32.c             | 10 +---
 drivers/clocksource/timer-fsl-ftm.c           | 10 +---
 drivers/clocksource/timer-imx-gpt.c           | 10 +---
 drivers/clocksource/timer-integrator-ap.c     | 11 +---
 drivers/clocksource/timer-meson6.c            | 11 +---
 drivers/clocksource/timer-orion.c             |  9 +--
 drivers/clocksource/timer-prima2.c            | 11 +---
 drivers/clocksource/timer-pxa.c               | 10 +---
 drivers/clocksource/timer-sp804.c             | 11 +---
 drivers/clocksource/timer-u300.c              |  9 +--
 drivers/clocksource/timer-vf-pit.c            | 10 +---
 drivers/clocksource/timer-vt8500.c            | 11 +---
 drivers/clocksource/timer-zevio.c             | 13 ++--
 drivers/irqchip/irq-i8259.c                   |  9 +--
 drivers/irqchip/irq-ingenic.c                 | 11 ++--
 drivers/parisc/eisa.c                         |  8 +--
 drivers/s390/cio/airq.c                       |  8 +--
 drivers/s390/cio/cio.c                        |  8 +--
 include/linux/dw_apb_timer.h                  |  1 -
 include/linux/irq.h                           |  2 -
 kernel/irq/manage.c                           | 44 --------------
 126 files changed, 528 insertions(+), 1111 deletions(-)


base-commit: v5.6-rc1
-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 06/18] m68k: Replace setup_irq() by request_irq()
  2020-02-12  8:01 [PATCH 00/18] genirq: Remove setup_irq() afzal mohammed
@ 2020-02-12  8:03 ` afzal mohammed
  2020-02-12 22:25   ` Finn Thain
  2020-02-13  7:11   ` Greg Ungerer
  0 siblings, 2 replies; 6+ messages in thread
From: afzal mohammed @ 2020-02-12  8:03 UTC (permalink / raw)
  To: linux-m68k, linux-kernel
  Cc: Greg Ungerer, Thomas Gleixner, Geert Uytterhoeven

request_irq() is preferred over setup_irq(). Existing callers of
setup_irq() reached mostly via 'init_IRQ()' & 'time_init()', while
memory allocators are ready by 'mm_init()'.

Per tglx[1], setup_irq() existed in olden days when allocators were not
ready by the time early interrupts were initialized.

Hence replace setup_irq() by request_irq().

Seldom remove_irq() usage has been observed coupled with setup_irq(),
wherever that has been found, it too has been replaced by free_irq().

[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos

Signed-off-by: afzal mohammed <afzal.mohd.ma@gmail.com>
---

Since cc'ing cover letter to all maintainers/reviewers would be too
many, refer for cover letter,
 https://lkml.kernel.org/r/cover.1581478323.git.afzal.mohd.ma@gmail.com

 arch/m68k/68000/timers.c      |  9 ++-------
 arch/m68k/coldfire/pit.c      |  9 ++-------
 arch/m68k/coldfire/sltimers.c | 19 +++++--------------
 arch/m68k/coldfire/timers.c   | 19 +++++--------------
 4 files changed, 14 insertions(+), 42 deletions(-)

diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
index 71ddb4c98726..7a55d664592e 100644
--- a/arch/m68k/68000/timers.c
+++ b/arch/m68k/68000/timers.c
@@ -68,12 +68,6 @@ static irqreturn_t hw_tick(int irq, void *dummy)
 
 /***************************************************************************/
 
-static struct irqaction m68328_timer_irq = {
-	.name	 = "timer",
-	.flags	 = IRQF_TIMER,
-	.handler = hw_tick,
-};
-
 /***************************************************************************/
 
 static u64 m68328_read_clk(struct clocksource *cs)
@@ -106,7 +100,8 @@ void hw_timer_init(irq_handler_t handler)
 	TCTL = 0;
 
 	/* set ISR */
-	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
+	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
+		pr_err("request_irq() on %s failed\n", "timer");
 
 	/* Restart mode, Enable int, Set clock source */
 	TCTL = TCTL_OM | TCTL_IRQEN | CLOCK_SOURCE;
diff --git a/arch/m68k/coldfire/pit.c b/arch/m68k/coldfire/pit.c
index eb6f16b0e2e6..d09e253abe5a 100644
--- a/arch/m68k/coldfire/pit.c
+++ b/arch/m68k/coldfire/pit.c
@@ -111,12 +111,6 @@ static irqreturn_t pit_tick(int irq, void *dummy)
 
 /***************************************************************************/
 
-static struct irqaction pit_irq = {
-	.name	 = "timer",
-	.flags	 = IRQF_TIMER,
-	.handler = pit_tick,
-};
-
 /***************************************************************************/
 
 static u64 pit_read_clk(struct clocksource *cs)
@@ -156,7 +150,8 @@ void hw_timer_init(irq_handler_t handler)
 	cf_pit_clockevent.min_delta_ticks = 0x3f;
 	clockevents_register_device(&cf_pit_clockevent);
 
-	setup_irq(MCF_IRQ_PIT1, &pit_irq);
+	if (request_irq(MCF_IRQ_PIT1, pit_tick, IRQF_TIMER, "timer", NULL))
+		pr_err("request_irq() on %s failed\n", "timer");
 
 	clocksource_register_hz(&pit_clk, FREQ);
 }
diff --git a/arch/m68k/coldfire/sltimers.c b/arch/m68k/coldfire/sltimers.c
index 1b11e7bacab3..2188a21e5413 100644
--- a/arch/m68k/coldfire/sltimers.c
+++ b/arch/m68k/coldfire/sltimers.c
@@ -50,18 +50,14 @@ irqreturn_t mcfslt_profile_tick(int irq, void *dummy)
 	return IRQ_HANDLED;
 }
 
-static struct irqaction mcfslt_profile_irq = {
-	.name	 = "profile timer",
-	.flags	 = IRQF_TIMER,
-	.handler = mcfslt_profile_tick,
-};
-
 void mcfslt_profile_init(void)
 {
 	printk(KERN_INFO "PROFILE: lodging TIMER 1 @ %dHz as profile timer\n",
 	       PROFILEHZ);
 
-	setup_irq(MCF_IRQ_PROFILER, &mcfslt_profile_irq);
+	if (request_irq(MCF_IRQ_PROFILER, mcfslt_profile_tick, IRQF_TIMER,
+			"profile timer", NULL))
+		pr_err("request_irq() on %s failed\n", "profile timer");
 
 	/* Set up TIMER 2 as high speed profile clock */
 	__raw_writel(MCF_BUSCLK / PROFILEHZ - 1, PA(MCFSLT_STCNT));
@@ -92,12 +88,6 @@ static irqreturn_t mcfslt_tick(int irq, void *dummy)
 	return timer_interrupt(irq, dummy);
 }
 
-static struct irqaction mcfslt_timer_irq = {
-	.name	 = "timer",
-	.flags	 = IRQF_TIMER,
-	.handler = mcfslt_tick,
-};
-
 static u64 mcfslt_read_clk(struct clocksource *cs)
 {
 	unsigned long flags;
@@ -140,7 +130,8 @@ void hw_timer_init(irq_handler_t handler)
 	mcfslt_cnt = mcfslt_cycles_per_jiffy;
 
 	timer_interrupt = handler;
-	setup_irq(MCF_IRQ_TIMER, &mcfslt_timer_irq);
+	if (request_irq(MCF_IRQ_TIMER, mcfslt_tick, IRQF_TIMER, "timer", NULL))
+		pr_err("request_irq() on %s failed\n", "timer");
 
 	clocksource_register_hz(&mcfslt_clk, MCF_BUSCLK);
 
diff --git a/arch/m68k/coldfire/timers.c b/arch/m68k/coldfire/timers.c
index 227aa5d13709..f384e92d8b1c 100644
--- a/arch/m68k/coldfire/timers.c
+++ b/arch/m68k/coldfire/timers.c
@@ -82,12 +82,6 @@ static irqreturn_t mcftmr_tick(int irq, void *dummy)
 
 /***************************************************************************/
 
-static struct irqaction mcftmr_timer_irq = {
-	.name	 = "timer",
-	.flags	 = IRQF_TIMER,
-	.handler = mcftmr_tick,
-};
-
 /***************************************************************************/
 
 static u64 mcftmr_read_clk(struct clocksource *cs)
@@ -134,7 +128,8 @@ void hw_timer_init(irq_handler_t handler)
 
 	timer_interrupt = handler;
 	init_timer_irq();
-	setup_irq(MCF_IRQ_TIMER, &mcftmr_timer_irq);
+	if (request_irq(MCF_IRQ_TIMER, mcftmr_tick, IRQF_TIMER, "timer", NULL))
+		pr_err("request_irq() on %s failed\n", "timer");
 
 #ifdef CONFIG_HIGHPROFILE
 	coldfire_profile_init();
@@ -170,12 +165,6 @@ irqreturn_t coldfire_profile_tick(int irq, void *dummy)
 
 /***************************************************************************/
 
-static struct irqaction coldfire_profile_irq = {
-	.name	 = "profile timer",
-	.flags	 = IRQF_TIMER,
-	.handler = coldfire_profile_tick,
-};
-
 void coldfire_profile_init(void)
 {
 	printk(KERN_INFO "PROFILE: lodging TIMER2 @ %dHz as profile timer\n",
@@ -188,7 +177,9 @@ void coldfire_profile_init(void)
 	__raw_writew(MCFTIMER_TMR_ENORI | MCFTIMER_TMR_CLK16 |
 		MCFTIMER_TMR_RESTART | MCFTIMER_TMR_ENABLE, PA(MCFTIMER_TMR));
 
-	setup_irq(MCF_IRQ_PROFILER, &coldfire_profile_irq);
+	if (request_irq(MCF_IRQ_PROFILER, coldfire_profile_tick, IRQF_TIMER,
+			"profile timer", NULL))
+		pr_err("request_irq() on %s failed\n", "profile timer");
 }
 
 /***************************************************************************/
-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 06/18] m68k: Replace setup_irq() by request_irq()
  2020-02-12  8:03 ` [PATCH 06/18] m68k: Replace setup_irq() by request_irq() afzal mohammed
@ 2020-02-12 22:25   ` Finn Thain
  2020-02-13  2:03     ` afzal mohammed
  2020-02-13  7:11   ` Greg Ungerer
  1 sibling, 1 reply; 6+ messages in thread
From: Finn Thain @ 2020-02-12 22:25 UTC (permalink / raw)
  To: afzal mohammed
  Cc: linux-m68k, linux-kernel, Greg Ungerer, Thomas Gleixner,
	Geert Uytterhoeven

On Wed, 12 Feb 2020, afzal mohammed wrote:

> request_irq() is preferred over setup_irq(). Existing callers of
> setup_irq() reached mostly via 'init_IRQ()' & 'time_init()', while
> memory allocators are ready by 'mm_init()'.
> 
> Per tglx[1], setup_irq() existed in olden days when allocators were not
> ready by the time early interrupts were initialized.
> 
> Hence replace setup_irq() by request_irq().
> 
> Seldom remove_irq() usage has been observed coupled with setup_irq(),
> wherever that has been found, it too has been replaced by free_irq().
> 
> [1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
> 
> Signed-off-by: afzal mohammed <afzal.mohd.ma@gmail.com>
> ---
> 
> Since cc'ing cover letter to all maintainers/reviewers would be too
> many, refer for cover letter,
>  https://lkml.kernel.org/r/cover.1581478323.git.afzal.mohd.ma@gmail.com
> 
>  arch/m68k/68000/timers.c      |  9 ++-------
>  arch/m68k/coldfire/pit.c      |  9 ++-------
>  arch/m68k/coldfire/sltimers.c | 19 +++++--------------
>  arch/m68k/coldfire/timers.c   | 19 +++++--------------
>  4 files changed, 14 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
> index 71ddb4c98726..7a55d664592e 100644
> --- a/arch/m68k/68000/timers.c
> +++ b/arch/m68k/68000/timers.c
> @@ -68,12 +68,6 @@ static irqreturn_t hw_tick(int irq, void *dummy)
>  
>  /***************************************************************************/
>  
> -static struct irqaction m68328_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = hw_tick,
> -};
> -
>  /***************************************************************************/
>  
>  static u64 m68328_read_clk(struct clocksource *cs)
> @@ -106,7 +100,8 @@ void hw_timer_init(irq_handler_t handler)
>  	TCTL = 0;
>  
>  	/* set ISR */
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");

"request_irq() on timer failed" is bad grammar and doesn't convey what 
went wrong. It could be taken to mean that request_irq() was called 
because a timer went off.

Have you considered,

		pr_err("%s: request_irq() failed\n", "timer");

I think that would be more typical of error messages on UNIX. E.g.

$ grep x -- /tmp/foo
grep: /tmp/foo: No such file or directory

>  
>  	/* Restart mode, Enable int, Set clock source */
>  	TCTL = TCTL_OM | TCTL_IRQEN | CLOCK_SOURCE;
> diff --git a/arch/m68k/coldfire/pit.c b/arch/m68k/coldfire/pit.c
> index eb6f16b0e2e6..d09e253abe5a 100644
> --- a/arch/m68k/coldfire/pit.c
> +++ b/arch/m68k/coldfire/pit.c
> @@ -111,12 +111,6 @@ static irqreturn_t pit_tick(int irq, void *dummy)
>  
>  /***************************************************************************/
>  
> -static struct irqaction pit_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = pit_tick,
> -};
> -
>  /***************************************************************************/
>  
>  static u64 pit_read_clk(struct clocksource *cs)
> @@ -156,7 +150,8 @@ void hw_timer_init(irq_handler_t handler)
>  	cf_pit_clockevent.min_delta_ticks = 0x3f;
>  	clockevents_register_device(&cf_pit_clockevent);
>  
> -	setup_irq(MCF_IRQ_PIT1, &pit_irq);
> +	if (request_irq(MCF_IRQ_PIT1, pit_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>  

Same here.

>  	clocksource_register_hz(&pit_clk, FREQ);
>  }
> diff --git a/arch/m68k/coldfire/sltimers.c b/arch/m68k/coldfire/sltimers.c
> index 1b11e7bacab3..2188a21e5413 100644
> --- a/arch/m68k/coldfire/sltimers.c
> +++ b/arch/m68k/coldfire/sltimers.c
> @@ -50,18 +50,14 @@ irqreturn_t mcfslt_profile_tick(int irq, void *dummy)
>  	return IRQ_HANDLED;
>  }
>  
> -static struct irqaction mcfslt_profile_irq = {
> -	.name	 = "profile timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcfslt_profile_tick,
> -};
> -
>  void mcfslt_profile_init(void)
>  {
>  	printk(KERN_INFO "PROFILE: lodging TIMER 1 @ %dHz as profile timer\n",
>  	       PROFILEHZ);
>  
> -	setup_irq(MCF_IRQ_PROFILER, &mcfslt_profile_irq);
> +	if (request_irq(MCF_IRQ_PROFILER, mcfslt_profile_tick, IRQF_TIMER,
> +			"profile timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "profile timer");
>  

And here.

>  	/* Set up TIMER 2 as high speed profile clock */
>  	__raw_writel(MCF_BUSCLK / PROFILEHZ - 1, PA(MCFSLT_STCNT));
> @@ -92,12 +88,6 @@ static irqreturn_t mcfslt_tick(int irq, void *dummy)
>  	return timer_interrupt(irq, dummy);
>  }
>  
> -static struct irqaction mcfslt_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcfslt_tick,
> -};
> -
>  static u64 mcfslt_read_clk(struct clocksource *cs)
>  {
>  	unsigned long flags;
> @@ -140,7 +130,8 @@ void hw_timer_init(irq_handler_t handler)
>  	mcfslt_cnt = mcfslt_cycles_per_jiffy;
>  
>  	timer_interrupt = handler;
> -	setup_irq(MCF_IRQ_TIMER, &mcfslt_timer_irq);
> +	if (request_irq(MCF_IRQ_TIMER, mcfslt_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>  

And so on.

>  	clocksource_register_hz(&mcfslt_clk, MCF_BUSCLK);
>  
> diff --git a/arch/m68k/coldfire/timers.c b/arch/m68k/coldfire/timers.c
> index 227aa5d13709..f384e92d8b1c 100644
> --- a/arch/m68k/coldfire/timers.c
> +++ b/arch/m68k/coldfire/timers.c
> @@ -82,12 +82,6 @@ static irqreturn_t mcftmr_tick(int irq, void *dummy)
>  
>  /***************************************************************************/
>  
> -static struct irqaction mcftmr_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcftmr_tick,
> -};
> -
>  /***************************************************************************/
>  
>  static u64 mcftmr_read_clk(struct clocksource *cs)
> @@ -134,7 +128,8 @@ void hw_timer_init(irq_handler_t handler)
>  
>  	timer_interrupt = handler;
>  	init_timer_irq();
> -	setup_irq(MCF_IRQ_TIMER, &mcftmr_timer_irq);
> +	if (request_irq(MCF_IRQ_TIMER, mcftmr_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>  
>  #ifdef CONFIG_HIGHPROFILE
>  	coldfire_profile_init();
> @@ -170,12 +165,6 @@ irqreturn_t coldfire_profile_tick(int irq, void *dummy)
>  
>  /***************************************************************************/
>  
> -static struct irqaction coldfire_profile_irq = {
> -	.name	 = "profile timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = coldfire_profile_tick,
> -};
> -
>  void coldfire_profile_init(void)
>  {
>  	printk(KERN_INFO "PROFILE: lodging TIMER2 @ %dHz as profile timer\n",
> @@ -188,7 +177,9 @@ void coldfire_profile_init(void)
>  	__raw_writew(MCFTIMER_TMR_ENORI | MCFTIMER_TMR_CLK16 |
>  		MCFTIMER_TMR_RESTART | MCFTIMER_TMR_ENABLE, PA(MCFTIMER_TMR));
>  
> -	setup_irq(MCF_IRQ_PROFILER, &coldfire_profile_irq);
> +	if (request_irq(MCF_IRQ_PROFILER, coldfire_profile_tick, IRQF_TIMER,
> +			"profile timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "profile timer");
>  }
>  
>  /***************************************************************************/
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 06/18] m68k: Replace setup_irq() by request_irq()
  2020-02-12 22:25   ` Finn Thain
@ 2020-02-13  2:03     ` afzal mohammed
  0 siblings, 0 replies; 6+ messages in thread
From: afzal mohammed @ 2020-02-13  2:03 UTC (permalink / raw)
  To: Finn Thain
  Cc: linux-m68k, linux-kernel, Greg Ungerer, Thomas Gleixner,
	Geert Uytterhoeven

Hi,

On Thu, Feb 13, 2020 at 09:25:19AM +1100, Finn Thain wrote:

> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
> > +		pr_err("request_irq() on %s failed\n", "timer");
> 
> "request_irq() on timer failed" is bad grammar and doesn't convey what 
> went wrong. It could be taken to mean that request_irq() was called 
> because a timer went off.
> 
> Have you considered,
> 
> 		pr_err("%s: request_irq() failed\n", "timer");

i was uncomfortable with the string contents, since that didn't seem
nonsense and to avoid pondering time over it, it was used.

Your suggestion is definitely better, will use that instead.

Regards
afzal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 06/18] m68k: Replace setup_irq() by request_irq()
  2020-02-12  8:03 ` [PATCH 06/18] m68k: Replace setup_irq() by request_irq() afzal mohammed
  2020-02-12 22:25   ` Finn Thain
@ 2020-02-13  7:11   ` Greg Ungerer
  2020-02-14 13:07     ` afzal mohammed
  1 sibling, 1 reply; 6+ messages in thread
From: Greg Ungerer @ 2020-02-13  7:11 UTC (permalink / raw)
  To: afzal mohammed, linux-m68k, linux-kernel
  Cc: Thomas Gleixner, Geert Uytterhoeven

Hi Afzal,

On 12/2/20 6:03 pm, afzal mohammed wrote:
> request_irq() is preferred over setup_irq(). Existing callers of
> setup_irq() reached mostly via 'init_IRQ()' & 'time_init()', while
> memory allocators are ready by 'mm_init()'.
> 
> Per tglx[1], setup_irq() existed in olden days when allocators were not
> ready by the time early interrupts were initialized.
> 
> Hence replace setup_irq() by request_irq().
> 
> Seldom remove_irq() usage has been observed coupled with setup_irq(),
> wherever that has been found, it too has been replaced by free_irq().
> 
> [1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
> 
> Signed-off-by: afzal mohammed <afzal.mohd.ma@gmail.com>
> ---
> 
> Since cc'ing cover letter to all maintainers/reviewers would be too
> many, refer for cover letter,
>   https://lkml.kernel.org/r/cover.1581478323.git.afzal.mohd.ma@gmail.com
> 
>   arch/m68k/68000/timers.c      |  9 ++-------
>   arch/m68k/coldfire/pit.c      |  9 ++-------
>   arch/m68k/coldfire/sltimers.c | 19 +++++--------------
>   arch/m68k/coldfire/timers.c   | 19 +++++--------------
>   4 files changed, 14 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
> index 71ddb4c98726..7a55d664592e 100644
> --- a/arch/m68k/68000/timers.c
> +++ b/arch/m68k/68000/timers.c
> @@ -68,12 +68,6 @@ static irqreturn_t hw_tick(int irq, void *dummy)
>   
>   /***************************************************************************/
>   
> -static struct irqaction m68328_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = hw_tick,
> -};
> -
>   /***************************************************************************/

Remove this comment line as well. Nothing left to separate
between those comment lines with the struct initialization removed.


>   static u64 m68328_read_clk(struct clocksource *cs)
> @@ -106,7 +100,8 @@ void hw_timer_init(irq_handler_t handler)
>   	TCTL = 0;
>   
>   	/* set ISR */
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>   
>   	/* Restart mode, Enable int, Set clock source */
>   	TCTL = TCTL_OM | TCTL_IRQEN | CLOCK_SOURCE;
> diff --git a/arch/m68k/coldfire/pit.c b/arch/m68k/coldfire/pit.c
> index eb6f16b0e2e6..d09e253abe5a 100644
> --- a/arch/m68k/coldfire/pit.c
> +++ b/arch/m68k/coldfire/pit.c
> @@ -111,12 +111,6 @@ static irqreturn_t pit_tick(int irq, void *dummy)
>   
>   /***************************************************************************/
>   
> -static struct irqaction pit_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = pit_tick,
> -};
> -
>   /***************************************************************************/

Remove this comment line as well.


>   static u64 pit_read_clk(struct clocksource *cs)
> @@ -156,7 +150,8 @@ void hw_timer_init(irq_handler_t handler)
>   	cf_pit_clockevent.min_delta_ticks = 0x3f;
>   	clockevents_register_device(&cf_pit_clockevent);
>   
> -	setup_irq(MCF_IRQ_PIT1, &pit_irq);
> +	if (request_irq(MCF_IRQ_PIT1, pit_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>   
>   	clocksource_register_hz(&pit_clk, FREQ);
>   }
> diff --git a/arch/m68k/coldfire/sltimers.c b/arch/m68k/coldfire/sltimers.c
> index 1b11e7bacab3..2188a21e5413 100644
> --- a/arch/m68k/coldfire/sltimers.c
> +++ b/arch/m68k/coldfire/sltimers.c
> @@ -50,18 +50,14 @@ irqreturn_t mcfslt_profile_tick(int irq, void *dummy)
>   	return IRQ_HANDLED;
>   }
>   
> -static struct irqaction mcfslt_profile_irq = {
> -	.name	 = "profile timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcfslt_profile_tick,
> -};
> -
>   void mcfslt_profile_init(void)
>   {
>   	printk(KERN_INFO "PROFILE: lodging TIMER 1 @ %dHz as profile timer\n",
>   	       PROFILEHZ);
>   
> -	setup_irq(MCF_IRQ_PROFILER, &mcfslt_profile_irq);
> +	if (request_irq(MCF_IRQ_PROFILER, mcfslt_profile_tick, IRQF_TIMER,
> +			"profile timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "profile timer");
>   
>   	/* Set up TIMER 2 as high speed profile clock */
>   	__raw_writel(MCF_BUSCLK / PROFILEHZ - 1, PA(MCFSLT_STCNT));
> @@ -92,12 +88,6 @@ static irqreturn_t mcfslt_tick(int irq, void *dummy)
>   	return timer_interrupt(irq, dummy);
>   }
>   
> -static struct irqaction mcfslt_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcfslt_tick,
> -};
> -
>   static u64 mcfslt_read_clk(struct clocksource *cs)
>   {
>   	unsigned long flags;
> @@ -140,7 +130,8 @@ void hw_timer_init(irq_handler_t handler)
>   	mcfslt_cnt = mcfslt_cycles_per_jiffy;
>   
>   	timer_interrupt = handler;
> -	setup_irq(MCF_IRQ_TIMER, &mcfslt_timer_irq);
> +	if (request_irq(MCF_IRQ_TIMER, mcfslt_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>   
>   	clocksource_register_hz(&mcfslt_clk, MCF_BUSCLK);
>   
> diff --git a/arch/m68k/coldfire/timers.c b/arch/m68k/coldfire/timers.c
> index 227aa5d13709..f384e92d8b1c 100644
> --- a/arch/m68k/coldfire/timers.c
> +++ b/arch/m68k/coldfire/timers.c
> @@ -82,12 +82,6 @@ static irqreturn_t mcftmr_tick(int irq, void *dummy)
>   
>   /***************************************************************************/
>   
> -static struct irqaction mcftmr_timer_irq = {
> -	.name	 = "timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = mcftmr_tick,
> -};
> -
>   /***************************************************************************/

Remove comment line here too.


>   static u64 mcftmr_read_clk(struct clocksource *cs)
> @@ -134,7 +128,8 @@ void hw_timer_init(irq_handler_t handler)
>   
>   	timer_interrupt = handler;
>   	init_timer_irq();
> -	setup_irq(MCF_IRQ_TIMER, &mcftmr_timer_irq);
> +	if (request_irq(MCF_IRQ_TIMER, mcftmr_tick, IRQF_TIMER, "timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "timer");
>   
>   #ifdef CONFIG_HIGHPROFILE
>   	coldfire_profile_init();
> @@ -170,12 +165,6 @@ irqreturn_t coldfire_profile_tick(int irq, void *dummy)
>   
>   /***************************************************************************/
>   
> -static struct irqaction coldfire_profile_irq = {
> -	.name	 = "profile timer",
> -	.flags	 = IRQF_TIMER,
> -	.handler = coldfire_profile_tick,
> -};
> -
>   void coldfire_profile_init(void)
>   {
>   	printk(KERN_INFO "PROFILE: lodging TIMER2 @ %dHz as profile timer\n",
> @@ -188,7 +177,9 @@ void coldfire_profile_init(void)
>   	__raw_writew(MCFTIMER_TMR_ENORI | MCFTIMER_TMR_CLK16 |
>   		MCFTIMER_TMR_RESTART | MCFTIMER_TMR_ENABLE, PA(MCFTIMER_TMR));
>   
> -	setup_irq(MCF_IRQ_PROFILER, &coldfire_profile_irq);
> +	if (request_irq(MCF_IRQ_PROFILER, coldfire_profile_tick, IRQF_TIMER,
> +			"profile timer", NULL))
> +		pr_err("request_irq() on %s failed\n", "profile timer");
>   }
>   
>   /***************************************************************************/

I tested this out on ColdFire hardware I have, worked fine.
All defconfigs still compiled too.

Regards
Greg



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 06/18] m68k: Replace setup_irq() by request_irq()
  2020-02-13  7:11   ` Greg Ungerer
@ 2020-02-14 13:07     ` afzal mohammed
  0 siblings, 0 replies; 6+ messages in thread
From: afzal mohammed @ 2020-02-14 13:07 UTC (permalink / raw)
  To: Greg Ungerer
  Cc: linux-m68k, linux-kernel, Thomas Gleixner, Geert Uytterhoeven,
	Julia Lawall

Hi Greg,

On Thu, Feb 13, 2020 at 05:11:17PM +1000, Greg Ungerer wrote:
> On 12/2/20 6:03 pm, afzal mohammed wrote:

> > diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
> > index 71ddb4c98726..7a55d664592e 100644
> > --- a/arch/m68k/68000/timers.c
> > +++ b/arch/m68k/68000/timers.c
> > @@ -68,12 +68,6 @@ static irqreturn_t hw_tick(int irq, void *dummy)
> >   /***************************************************************************/
> > -static struct irqaction m68328_timer_irq = {
> > -	.name	 = "timer",
> > -	.flags	 = IRQF_TIMER,
> > -	.handler = hw_tick,
> > -};
> > -
> >   /***************************************************************************/

> Remove this comment line as well. Nothing left to separate
> between those comment lines with the struct initialization removed.

i will remove above as well as the similar ones.

Because  you mentioned, i checked cocci o/p (change above was provided
as is by running cocci). Cocci by default removes the comment line you
mentioned. Initially that was the way cocci was run, but then it was
observed that in another file in addition to removing associated
comments, it was swallowing other unrelated comments that were living
together. Also sometimes associated comments had to be retained, just
that it had to be relocated near added code (that is not applicable in
this case). So i ran cocci w/ "--keep-comments" option & decided to
manually handle comment related, in this file i failed noticing the
unneeded comment line during self review.

> I tested this out on ColdFire hardware I have, worked fine.
> All defconfigs still compiled too.

Thanks for testing

Regards
afzal

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12  8:01 [PATCH 00/18] genirq: Remove setup_irq() afzal mohammed
2020-02-12  8:03 ` [PATCH 06/18] m68k: Replace setup_irq() by request_irq() afzal mohammed
2020-02-12 22:25   ` Finn Thain
2020-02-13  2:03     ` afzal mohammed
2020-02-13  7:11   ` Greg Ungerer
2020-02-14 13:07     ` afzal mohammed

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git