[PATCH v2 00/18] genirq: Remove setup_irq()

[PATCH v2 00/18] genirq: Remove setup_irq()

[afzal mohammed]

While trying to understand internals of irq handling, came across a
thread [1] in which tglx was referring to avoid usage of setup_irq().
The early boot setup_irq() invocations happen either via 'init_IRQ()'
or 'time_init()', while memory allocators are ready by 'mm_init()'.

Hence instances of setup_irq() are 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, modifications made in this series is being
exercised at runtime)

Much of the changes were created using Coccinelle with an intention
to learn it. 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.

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(), they were already
present w/ setup_irq(), status quo maintained.

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

Since changes from v1 are trivial as below, tags received has been
applied to the relevant patches, if any objections, please shout.

v2:
 * Replace pr_err("request_irq() on %s failed" by
           pr_err("%s: request_irq() failed"
 * m68k: remove now irrelevant comment separation comment lines
 * Commit message massage


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                      | 11 +---
 arch/m68k/coldfire/pit.c                      | 11 +---
 arch/m68k/coldfire/sltimers.c                 | 19 ++----
 arch/m68k/coldfire/timers.c                   | 21 ++-----
 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(+), 1117 deletions(-)


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

[PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

request_irq() is preferred over setup_irq(). The early boot setup_irq()
invocations happen either via 'init_IRQ()' or '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>
Tested-by: Greg Ungerer <gerg@linux-m68k.org> # ColdFire
---

v2:
 * Replace pr_err("request_irq() on %s failed" by
           pr_err("%s: request_irq() failed"
 * Commit message massage
 * remove now irrelevant comment lines at 3 places

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

diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
index 71ddb4c98726..55a76a2d3d58 100644
--- a/arch/m68k/68000/timers.c
+++ b/arch/m68k/68000/timers.c
@@ -68,14 +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)
 {
 	unsigned long flags;
@@ -106,7 +98,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("%s: request_irq() 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..604acd658dec 100644
--- a/arch/m68k/coldfire/pit.c
+++ b/arch/m68k/coldfire/pit.c
@@ -111,14 +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)
 {
 	unsigned long flags;
@@ -156,7 +148,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("%s: request_irq() 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..c5d5862e1d2b 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("%s: request_irq() 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("%s: request_irq() 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..52294c1f01f8 100644
--- a/arch/m68k/coldfire/timers.c
+++ b/arch/m68k/coldfire/timers.c
@@ -82,14 +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)
 {
 	unsigned long flags;
@@ -134,7 +126,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("%s: request_irq() failed\n", "timer");
 
 #ifdef CONFIG_HIGHPROFILE
 	coldfire_profile_init();
@@ -170,12 +163,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 +175,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("%s: request_irq() failed\n", "profile timer");
 }
 
 /***************************************************************************/
-- 
2.25.1

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

Hi Afzal,

On 24/2/20 10:50 am, afzal mohammed wrote:
> request_irq() is preferred over setup_irq(). The early boot setup_irq()
> invocations happen either via 'init_IRQ()' or '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>
> Tested-by: Greg Ungerer <gerg@linux-m68k.org> # ColdFire
> ---
> 
> v2:
>   * Replace pr_err("request_irq() on %s failed" by
>             pr_err("%s: request_irq() failed"
>   * Commit message massage
>   * remove now irrelevant comment lines at 3 places
> 
>   arch/m68k/68000/timers.c      | 11 ++---------
>   arch/m68k/coldfire/pit.c      | 11 ++---------
>   arch/m68k/coldfire/sltimers.c | 19 +++++--------------
>   arch/m68k/coldfire/timers.c   | 21 +++++----------------
>   4 files changed, 14 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
> index 71ddb4c98726..55a76a2d3d58 100644
> --- a/arch/m68k/68000/timers.c
> +++ b/arch/m68k/68000/timers.c
> @@ -68,14 +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)
>   {
>   	unsigned long flags;
> @@ -106,7 +98,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("%s: request_irq() failed\n", "timer");

Why not just:

                 pr_err("timer: request_irq() failed\n");

And maybe would it be useful to print out the error return code from a
failed request_irq()?  What about displaying the requested IRQ number as well?
Just a thought.

Regards
Greg


>   	/* 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..604acd658dec 100644
> --- a/arch/m68k/coldfire/pit.c
> +++ b/arch/m68k/coldfire/pit.c
> @@ -111,14 +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)
>   {
>   	unsigned long flags;
> @@ -156,7 +148,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("%s: request_irq() 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..c5d5862e1d2b 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("%s: request_irq() 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("%s: request_irq() 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..52294c1f01f8 100644
> --- a/arch/m68k/coldfire/timers.c
> +++ b/arch/m68k/coldfire/timers.c
> @@ -82,14 +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)
>   {
>   	unsigned long flags;
> @@ -134,7 +126,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("%s: request_irq() failed\n", "timer");
>   
>   #ifdef CONFIG_HIGHPROFILE
>   	coldfire_profile_init();
> @@ -170,12 +163,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 +175,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("%s: request_irq() failed\n", "profile timer");
>   }
>   
>   /***************************************************************************/
> 

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Wed, 26 Feb 2020, Greg Ungerer wrote:

> Hi Afzal,
> 
> On 24/2/20 10:50 am, afzal mohammed wrote:
> > request_irq() is preferred over setup_irq(). The early boot setup_irq()
> > invocations happen either via 'init_IRQ()' or '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>
> > Tested-by: Greg Ungerer <gerg@linux-m68k.org> # ColdFire
> > ---
> > 
> > v2:
> >   * Replace pr_err("request_irq() on %s failed" by
> >             pr_err("%s: request_irq() failed"
> >   * Commit message massage
> >   * remove now irrelevant comment lines at 3 places
> > 
> >   arch/m68k/68000/timers.c      | 11 ++---------
> >   arch/m68k/coldfire/pit.c      | 11 ++---------
> >   arch/m68k/coldfire/sltimers.c | 19 +++++--------------
> >   arch/m68k/coldfire/timers.c   | 21 +++++----------------
> >   4 files changed, 14 insertions(+), 48 deletions(-)
> > 
> > diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
> > index 71ddb4c98726..55a76a2d3d58 100644
> > --- a/arch/m68k/68000/timers.c
> > +++ b/arch/m68k/68000/timers.c
> > @@ -68,14 +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)
> >   {
> >   	unsigned long flags;
> > @@ -106,7 +98,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("%s: request_irq() failed\n", "timer");
> 
> Why not just:
> 
>                 pr_err("timer: request_irq() failed\n");
> 

I believe that the compiler would coalesce the two "timer" string 
constants in the patch from Afzal (as per my suggestion).

I suspect that your version costs a few extra bytes everywhere it appears 
(but I didn't check).

> And maybe would it be useful to print out the error return code from a 
> failed request_irq()?  What about displaying the requested IRQ number as 
> well? Just a thought.
> 

That error would almost always be -EBUSY, right?

Moreover, compare this change,

-	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
+	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);

with this change,

+	int err;

-	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
+	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
+	if (err)
+		return err;

Isn't the latter change the more common pattern? It prints nothing.

And arguably, the former example is actually the change that's described 
in the commit description.

This patch seems to be making two orthogonal changes but I'll leave that 
question to the maintainer. (I'm not really trying to NAK this patch.)

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

On 26/2/20 11:11 am, Finn Thain wrote:
> On Wed, 26 Feb 2020, Greg Ungerer wrote:
>> On 24/2/20 10:50 am, afzal mohammed wrote:
>>> request_irq() is preferred over setup_irq(). The early boot setup_irq()
>>> invocations happen either via 'init_IRQ()' or '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>
>>> Tested-by: Greg Ungerer <gerg@linux-m68k.org> # ColdFire
>>> ---
>>>
>>> v2:
>>>    * Replace pr_err("request_irq() on %s failed" by
>>>              pr_err("%s: request_irq() failed"
>>>    * Commit message massage
>>>    * remove now irrelevant comment lines at 3 places
>>>
>>>    arch/m68k/68000/timers.c      | 11 ++---------
>>>    arch/m68k/coldfire/pit.c      | 11 ++---------
>>>    arch/m68k/coldfire/sltimers.c | 19 +++++--------------
>>>    arch/m68k/coldfire/timers.c   | 21 +++++----------------
>>>    4 files changed, 14 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c
>>> index 71ddb4c98726..55a76a2d3d58 100644
>>> --- a/arch/m68k/68000/timers.c
>>> +++ b/arch/m68k/68000/timers.c
>>> @@ -68,14 +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)
>>>    {
>>>    	unsigned long flags;
>>> @@ -106,7 +98,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("%s: request_irq() failed\n", "timer");
>>
>> Why not just:
>>
>>                  pr_err("timer: request_irq() failed\n");
>>
> 
> I believe that the compiler would coalesce the two "timer" string
> constants in the patch from Afzal (as per my suggestion).>
> I suspect that your version costs a few extra bytes everywhere it appears
> (but I didn't check).

Maybe. It costs some extra code for another argument push and a bunch
of cycles to process the %s at run time though (if triggered).

The profile timer setup is not commonly used, so in most typical
builds there is no scope for coalescing the same string. So in the end
most builds will be a few bytes larger with the separated strings.

But really that is not the point. It just seems simpler and clearer to
me to put the string in place - all in one.


>> And maybe would it be useful to print out the error return code from a
>> failed request_irq()?  What about displaying the requested IRQ number as
>> well? Just a thought.
>>
> 
> That error would almost always be -EBUSY, right?

I expect it will never fail this early in boot.
But how will you know if it really is EBUSY if you don't print it out?


> Moreover, compare this change,
> 
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> 
> with this change,
> 
> +	int err;
> 
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> +	if (err)
> +		return err;
> 
> Isn't the latter change the more common pattern? It prints nothing.

Hmm, in my experience the much more common pattern is:

> +	int err;
> 
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> +	if (err) {
> +             pr_err("timer: request_irq() failed with err=%d\n", err);
> +		return err;
> +     }

Where the pr_err() could be one of pr_err, printk, dev_err, ...

Regards
Greg

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Wed, 26 Feb 2020, Greg Ungerer wrote:

> > That error would almost always be -EBUSY, right?
> 
> I expect it will never fail this early in boot. 

If so, it suggests to me that tweaking the error message string is just 
bikeshedding and that adding these error messages across the tree is just 
bloat.

> But how will you know if it really is EBUSY if you don't print it out?
> 
> > Moreover, compare this change,
> > 
> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > 
> > with this change,
> > 
> > +	int err;
> > 
> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > +	if (err)
> > +		return err;
> > 
> > Isn't the latter change the more common pattern? It prints nothing.
> 
> Hmm, in my experience the much more common pattern is:
> 
> > +	int err;
> > 
> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > +	if (err) {
> > +             pr_err("timer: request_irq() failed with err=%d\n", err);
> > +		return err;
> > +     }
> 
> Where the pr_err() could be one of pr_err, printk, dev_err, ...
> 

A rough poll using 'git grep' seems to agree with your assessment.

If -EBUSY means the end user has misconfigured something, printing 
"request_irq failed" would be helpful. But does that still happen?

Printing any error message for -ENOMEM is frowned upon, and printing -12 
is really unhelpful. So the most popular pattern isn't that great, though 
it is usually less verbose than the example you've given.

Besides, introducing local variables and altering control flow seems well 
out-of-scope for this kind of refactoring, right?

Anyway, if you're going to add an error message,
pr_err("%s: request_irq failed", foo) is unavoidable whenever foo isn't a 
string constant, so one can't expect to grep the source code for the 
literal error message from the log.

BTW, one of the benefits of "%s: request_irq failed" is that a compilation 
unit with multiple request_irq calls permits the compiler to coalesce all 
duplicated format strings. Whereas, that's not possible with
"foo: request_irq failed" and "bar: request_irq failed".

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

On 26/2/20 4:39 pm, Finn Thain wrote:
> On Wed, 26 Feb 2020, Greg Ungerer wrote:
> 
>>> That error would almost always be -EBUSY, right?
>>
>> I expect it will never fail this early in boot.
> 
> If so, it suggests to me that tweaking the error message string is just
> bikeshedding and that adding these error messages across the tree is just
> bloat.
> 
>> But how will you know if it really is EBUSY if you don't print it out?
>>
>>> Moreover, compare this change,
>>>
>>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>>> +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>>>
>>> with this change,
>>>
>>> +	int err;
>>>
>>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>>> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>>> +	if (err)
>>> +		return err;
>>>
>>> Isn't the latter change the more common pattern? It prints nothing.
>>
>> Hmm, in my experience the much more common pattern is:
>>
>>> +	int err;
>>>
>>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>>> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>>> +	if (err) {
>>> +             pr_err("timer: request_irq() failed with err=%d\n", err);
>>> +		return err;
>>> +     }
>>
>> Where the pr_err() could be one of pr_err, printk, dev_err, ...
>>
> 
> A rough poll using 'git grep' seems to agree with your assessment.
> 
> If -EBUSY means the end user has misconfigured something, printing
> "request_irq failed" would be helpful. But does that still happen?

I have seen it many times. Its not at all difficult to get interrupt
assignments wrong, duplicated, or otherwise mistaken when creating
device trees. Not so much m68k/coldfire platforms where they are
most commonly hard coded.


> Printing any error message for -ENOMEM is frowned upon, and printing -12
> is really unhelpful. So the most popular pattern isn't that great, though
> it is usually less verbose than the example you've given.
> 
> Besides, introducing local variables and altering control flow seems well
> out-of-scope for this kind of refactoring, right?

I don't agree with the local variable part. Adding a local variable to
keep track of the error return code doesn't seem out of scope for this change.
The patch as Afzal sent it doesn't change the control flow - and
that is the right thing to do here.


> Anyway, if you're going to add an error message,
> pr_err("%s: request_irq failed", foo) is unavoidable whenever foo isn't a
> string constant, so one can't expect to grep the source code for the
> literal error message from the log.
> 
> BTW, one of the benefits of "%s: request_irq failed" is that a compilation
> unit with multiple request_irq calls permits the compiler to coalesce all
> duplicated format strings. Whereas, that's not possible with
> "foo: request_irq failed" and "bar: request_irq failed".

Given the wide variety of message text used with failed request_irq() calls
it would be shear luck that this matched anything else. A quick grep shows
that "%s: request_irq() failed\n" has no other exact matches in the current
kernel source.

Regards
Greg

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Wed, 26 Feb 2020, Greg Ungerer wrote:

> On 26/2/20 4:39 pm, Finn Thain wrote:
> > 
> > If -EBUSY means the end user has misconfigured something, printing
> > "request_irq failed" would be helpful. But does that still happen?
> 
> I have seen it many times. Its not at all difficult to get interrupt 
> assignments wrong, duplicated, or otherwise mistaken when creating 
> device trees. Not so much m68k/coldfire platforms where they are most 
> commonly hard coded.
> 

I was thinking of end users and production builds. You seem to be 
concerned about developers. Catering to developers argues for pr_debug() 
here, if anything.

You say you've seen -16 errors "many times". Have you also seen -22? Did 
the ability to distinguish these values help you to fix your device tree?

> > ...
> > 
> > BTW, one of the benefits of "%s: request_irq failed" is that a 
> > compilation unit with multiple request_irq calls permits the compiler 
> > to coalesce all duplicated format strings. Whereas, that's not 
> > possible with "foo: request_irq failed" and "bar: request_irq failed".
> 
> Given the wide variety of message text used with failed request_irq() 
> calls it would be shear luck that this matched anything else. A quick 
> grep shows that "%s: request_irq() failed\n" has no other exact matches 
> in the current kernel source.
> 

You are overlooking the patches in this series that produce multiple 
identical format strings.

And the present lack of consistency isn't a great argument for more 
inconsistency IMO.

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

On 27/2/20 8:31 am, Finn Thain wrote:
> On Wed, 26 Feb 2020, Greg Ungerer wrote:
> 
>> On 26/2/20 4:39 pm, Finn Thain wrote:
>>>
>>> If -EBUSY means the end user has misconfigured something, printing
>>> "request_irq failed" would be helpful. But does that still happen?
>>
>> I have seen it many times. Its not at all difficult to get interrupt
>> assignments wrong, duplicated, or otherwise mistaken when creating
>> device trees. Not so much m68k/coldfire platforms where they are most
>> commonly hard coded.
>>
> 
> I was thinking of end users and production builds. You seem to be
> concerned about developers. Catering to developers argues for pr_debug()
> here, if anything.

Perhaps. But most of the kernel boot output as it stands today
is more debug (or maybe notice) than useful.


> You say you've seen -16 errors "many times". Have you also seen -22? Did
> the ability to distinguish these values help you to fix your device tree?

Probably not. But the real difficulty is trying to diagnose other
peoples problems with just console trace output. The more information
there the better.


>>> ...
>>>
>>> BTW, one of the benefits of "%s: request_irq failed" is that a
>>> compilation unit with multiple request_irq calls permits the compiler
>>> to coalesce all duplicated format strings. Whereas, that's not
>>> possible with "foo: request_irq failed" and "bar: request_irq failed".
>>
>> Given the wide variety of message text used with failed request_irq()
>> calls it would be shear luck that this matched anything else. A quick
>> grep shows that "%s: request_irq() failed\n" has no other exact matches
>> in the current kernel source.
>>
> 
> You are overlooking the patches in this series that produce multiple
> identical format strings.

No I didn't :-)  None of these will end up compiled in at the same time.
The various ColdFire SoC parts have a single timer hardware module -
and only the required one will be compiled in, not all of them.

Regards
Greg

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi Greg & Finn,

On Wed, Feb 26, 2020 at 10:42:00AM +1000, Greg Ungerer wrote:

> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
> > +		pr_err("%s: request_irq() failed\n", "timer");
> 
> Why not just:
> 
>                 pr_err("timer: request_irq() failed\n");

The reason to use %s was that it could be automated by cocci script &
the o/p didn't look bad. Second arg to pr_err is what cocci
presents me & there is wide variation in the name across the tree as
Finn noted.

Excerpts from v1 cover letter [1], 

- 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);

[ don't get mislead by string contents used, this was for v1, just to
 show how the result was obtained. To take care of Finn's suggesstion,
 instead of modifying cocci & then making changes other changes over
 that (i could not fully automate w/ cocci, and Julia said my script
 is fine as is), it was easier to run sed over the v1  patches ]

> And maybe would it be useful to print out the error return code from a
> failed request_irq()?

Since most of the existing setup_irq() didn't even check & handle
error return, my first thought was just s/setup_irq/request_irq, it
was easier from scripting pointing of view. i felt uncomfortable doing
nothing in case of error. Also noted that request_irq() definition has
a "__much_check", so decided to add it.

And there is a wide variation in the way return value is handled by
the caller, some already have a local variable, some don't. Moreover
in many cases caller cannot return any value, i.e. void, so what
to be done with return value was another issue, in some cases in
addition to printing error value, the error value can't be returned,
while some others it can.

> What about displaying the requested IRQ number as well?
> Just a thought.

i initially did the cocci to display IRQ number as the 3rd arg to
pr_err, but then it was observed that most of the those lines were
exceeding 80 chars, though cocci could align args properly in next
line, it could not put flower braces to the preceeding
'if request_irq()' & in next line (though it is a single C statement
inside 'if', per kernel coding style, flower brace had to be put for a
single statement that spans multiple lines ], else it had to be
manually added treewide.

On Wed, Feb 26, 2020 at 12:11:55PM +1100, Finn Thain wrote:

> Moreover, compare this change,
> 
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> 
> with this change,
> 
> +	int err;
> 
> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> +	if (err)
> +		return err;
> 
> Isn't the latter change the more common pattern? It prints nothing.
> 
> And arguably, the former example is actually the change that's described 
> in the commit description.
> 
> This patch seems to be making two orthogonal changes but I'll leave that 
> question to the maintainer. (I'm not really trying to NAK this patch.)

Instead of not checking the error value as in the existing cases &
mechanically replace w/ request_irq(), thought of at least giving user
the indication by way of pr_err (but i think most of the use is for
tick timer, if it fails, in most cases, system would not even boot)
due to the reasons mentioned above.

On Wed, Feb 26, 2020 at 12:11:38PM +1000, Greg Ungerer wrote:

> Hmm, in my experience the much more common pattern is:
> 
> > +	int err;
> > 
> > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > +	if (err) {
> > +             pr_err("timer: request_irq() failed with err=%d\n", err);
> > +		return err;
> > +     }

This is my preferred style, but note that returning error is not
possible in many of these cases as callers are void return type.

On Wed, Feb 26, 2020 at 05:39:57PM +1100, Finn Thain wrote:

> Besides, introducing local variables and altering control flow seems well 
> out-of-scope for this kind of refactoring, right?

To make changes perfect, it would be required to get into context of
each case (>150), and do it manually as there is wide variation like
whether caller can return error code, whether already a local integer
is defined to catch the error, whether it returns error after a goto,
whether we should allow it to proceed if it fails and so on.

And handling manually all the cases tree wide would be more error
prone & time consuming. i was relieved that there was only one build
error reported by test robot (i had done build & boot test only on ARM
& x86_64), given the amount of manual changes i had to do on top of
cocci generated ones.

At the same time, i didn't want to just mechanically replace & wanted
to add some value to the existing setup, which resulted in this patch.

My thought process was to do treewide removal of setup_irq() and
possibly low hanging cleanup's at the places where setup_irq() lives
to make sure that surrounding situation will be better than or at
least equal to the current.

But if the consensus is that all these situations have to be taken
care, let me know.

On Wed, Feb 26, 2020 at 10:26:55PM +1000, Greg Ungerer wrote:

> A quick grep shows
> that "%s: request_irq() failed\n" has no other exact matches in the current
> kernel source.

git grep -n '%s: request_irq' gives a few somewhat similar ones, i
remember it because searching this string after my changes to verify
gave more than that i added :)

Regards
afzal

[1] https://lkml.kernel.org/r/cover.1581478323.git.afzal.mohd.ma@gmail.com

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Geert Uytterhoeven]

Hi Afzal,

On Thu, Feb 27, 2020 at 9:18 AM afzal mohammed <afzal.mohd.ma@gmail.com> wrote:
> On Wed, Feb 26, 2020 at 10:42:00AM +1000, Greg Ungerer wrote:
> > > -   setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > > +   if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
> > > +           pr_err("%s: request_irq() failed\n", "timer");
> >
> > Why not just:
> >
> >                 pr_err("timer: request_irq() failed\n");
>
> The reason to use %s was that it could be automated by cocci script &
> the o/p didn't look bad. Second arg to pr_err is what cocci
> presents me & there is wide variation in the name across the tree as
> Finn noted.
>
> Excerpts from v1 cover letter [1],
>
> - 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);
>
> [ don't get mislead by string contents used, this was for v1, just to
>  show how the result was obtained. To take care of Finn's suggesstion,
>  instead of modifying cocci & then making changes other changes over
>  that (i could not fully automate w/ cocci, and Julia said my script
>  is fine as is), it was easier to run sed over the v1  patches ]
>
> > And maybe would it be useful to print out the error return code from a
> > failed request_irq()?
>
> Since most of the existing setup_irq() didn't even check & handle
> error return, my first thought was just s/setup_irq/request_irq, it
> was easier from scripting pointing of view. i felt uncomfortable doing
> nothing in case of error. Also noted that request_irq() definition has
> a "__much_check", so decided to add it.

Most (all?) of the code calling setup_irq() is very old, and most of the calls
happen very early, so any such failures are hard failures that prevent the
system from booting at all.  Hence printing a message may be futile, as it
may happen before the console has been initialized (modulo early-printk).

Just my 2 €c.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi Geert,

On Thu, Feb 27, 2020 at 09:32:46AM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 27, 2020 at 9:18 AM afzal mohammed <afzal.mohd.ma@gmail.com> wrote:

> > Since most of the existing setup_irq() didn't even check & handle
> > error return, my first thought was just s/setup_irq/request_irq, it
> > was easier from scripting pointing of view. i felt uncomfortable doing
> > nothing in case of error. Also noted that request_irq() definition has
> > a "__much_check", so decided to add it.
> 
> Most (all?) of the code calling setup_irq() is very old, and most of the calls
> happen very early, so any such failures are hard failures that prevent the
> system from booting at all.  Hence printing a message may be futile, as it
> may happen before the console has been initialized (modulo early-printk).

The main reason to at least acknowledge the return value was due to
__much_check in request_irq() definition, though w/ the compiler that
i used, there were no warnings, i feared that it might warn w/
some other compilers & in some cases (may be W=[1-3] ?).

Also as most are tick timers, if request_irq() fails, system would die
in that case. But i have seen (iirc in MIPS), in the same execution
sequence multiple setup_irq() invocations, so every instance might not
be unavoidable for system boot.

For tick timer cases, a BUG() might be suitable, but i didn't even
think of that option as that is a recipe for getting trashed from head
penguin (though he would not care trivial patches like this), same
scenario w/ adding warnings.

> Just my 2 €c.

That is my 2 paise, but per exchange rate it will be far less ;)

Regards
afzal

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Thu, 27 Feb 2020, Greg Ungerer wrote:

> On 27/2/20 8:31 am, Finn Thain wrote:
> 
> >>>
> >>> BTW, one of the benefits of "%s: request_irq failed" is that a 
> >>> compilation unit with multiple request_irq calls permits the 
> >>> compiler to coalesce all duplicated format strings. Whereas, that's 
> >>> not possible with "foo: request_irq failed" and "bar: request_irq 
> >>> failed".
> >>
> >> Given the wide variety of message text used with failed request_irq() 
> >> calls it would be shear luck that this matched anything else. A quick 
> >> grep shows that "%s: request_irq() failed\n" has no other exact 
> >> matches in the current kernel source.
> > 
> > You are overlooking the patches in this series that produce multiple 
> > identical format strings.
> 
> No I didn't :-)  None of these will end up compiled in at the same time. 
> The various ColdFire SoC parts have a single timer hardware module - and 
> only the required one will be compiled in, not all of them.
> 

I was referring to e.g. [PATCH v2 08/18] MIPS: Replace setup_irq() by 
request_irq(), in which you can find this:

@@ -116,8 +110,16 @@ static void __init ar7_irq_init(int base)
                                                 handle_level_irq);
        }
 
-       setup_irq(2, &ar7_cascade_action);
-       setup_irq(ar7_irq_base, &ar7_cascade_action);
+       if (request_irq(2, no_action, IRQF_NO_THREAD, "AR7 cascade interrupt",
+                       NULL)) {
+               pr_err("%s: request_irq() failed\n",
+                      "AR7 cascade interrupt");
+       }
+       if (request_irq(ar7_irq_base, no_action, IRQF_NO_THREAD,
+                       "AR7 cascade interrupt", NULL)) {
+               pr_err("%s: request_irq() failed\n",
+                      "AR7 cascade interrupt");
+       }
        set_c0_status(IE_IRQ0);
 }
 
BTW, I think that deduplication of string constants can happen during LTO, 
so the benefit of consistency need not be confined to a compilation unit. 
I don't think this is relevant to kernel builds.

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Thu, 27 Feb 2020, afzal mohammed wrote:

> On Thu, Feb 27, 2020 at 09:32:46AM +0100, Geert Uytterhoeven wrote:
> > On Thu, Feb 27, 2020 at 9:18 AM afzal mohammed <afzal.mohd.ma@gmail.com> wrote:

> > > Since most of the existing setup_irq() didn't even check & handle 
> > > error return, my first thought was just s/setup_irq/request_irq, it 
> > > was easier from scripting pointing of view. i felt uncomfortable 
> > > doing nothing in case of error. Also noted that request_irq() 
> > > definition has a "__much_check", so decided to add it.
> > 
> > Most (all?) of the code calling setup_irq() is very old, and most of 
> > the calls happen very early, so any such failures are hard failures 
> > that prevent the system from booting at all.  Hence printing a message 
> > may be futile, as it may happen before the console has been 
> > initialized (modulo early-printk).
> 
> The main reason to at least acknowledge the return value was due to 
> __much_check in request_irq() definition, though w/ the compiler that i 
> used, there were no warnings, i feared that it might warn w/ some other 
> compilers & in some cases (may be W=[1-3] ?).
> 

This isn't new code, so I'd assume it's been "checked" in the sense of 
"reviewed and tested".

So the lack of an error message could be taken to mean that there's no 
need for an error message.

If you want to stop the compiler complaining about an unchecked return 
value, assuming that it does so, please consider using

	if (request_irq(...))
		pr_debug(...);

That way there is no penalty paid for adding error messages that the 
original author apparently did not want.

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

Hi Afzal,

On 27/2/20 6:18 pm, afzal mohammed wrote:
> On Wed, Feb 26, 2020 at 10:42:00AM +1000, Greg Ungerer wrote:
> 
>>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>>> +	if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL))
>>> +		pr_err("%s: request_irq() failed\n", "timer");
>>
>> Why not just:
>>
>>                  pr_err("timer: request_irq() failed\n");
> 
> The reason to use %s was that it could be automated by cocci script &
> the o/p didn't look bad. Second arg to pr_err is what cocci
> presents me & there is wide variation in the name across the tree as
> Finn noted.
> 
> Excerpts from v1 cover letter [1],
> 
> - 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);
> 
> [ don't get mislead by string contents used, this was for v1, just to
>   show how the result was obtained. To take care of Finn's suggesstion,
>   instead of modifying cocci & then making changes other changes over
>   that (i could not fully automate w/ cocci, and Julia said my script
>   is fine as is), it was easier to run sed over the v1  patches ]
> 
>> And maybe would it be useful to print out the error return code from a
>> failed request_irq()?
> 
> Since most of the existing setup_irq() didn't even check & handle
> error return, my first thought was just s/setup_irq/request_irq, it
> was easier from scripting pointing of view. i felt uncomfortable doing
> nothing in case of error. Also noted that request_irq() definition has
> a "__much_check", so decided to add it.
> 
> And there is a wide variation in the way return value is handled by
> the caller, some already have a local variable, some don't. Moreover
> in many cases caller cannot return any value, i.e. void, so what
> to be done with return value was another issue, in some cases in
> addition to printing error value, the error value can't be returned,
> while some others it can.
>
>> What about displaying the requested IRQ number as well?
>> Just a thought.
> 
> i initially did the cocci to display IRQ number as the 3rd arg to
> pr_err, but then it was observed that most of the those lines were
> exceeding 80 chars, though cocci could align args properly in next
> line, it could not put flower braces to the preceeding
> 'if request_irq()' & in next line (though it is a single C statement
> inside 'if', per kernel coding style, flower brace had to be put for a
> single statement that spans multiple lines ], else it had to be
> manually added treewide.
> 
> On Wed, Feb 26, 2020 at 12:11:55PM +1100, Finn Thain wrote:
> 
>> Moreover, compare this change,
>>
>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>> +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>>
>> with this change,
>>
>> +	int err;
>>
>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>> +	if (err)
>> +		return err;
>>
>> Isn't the latter change the more common pattern? It prints nothing.
>>
>> And arguably, the former example is actually the change that's described
>> in the commit description.
>>
>> This patch seems to be making two orthogonal changes but I'll leave that
>> question to the maintainer. (I'm not really trying to NAK this patch.)
> 
> Instead of not checking the error value as in the existing cases &
> mechanically replace w/ request_irq(), thought of at least giving user
> the indication by way of pr_err (but i think most of the use is for
> tick timer, if it fails, in most cases, system would not even boot)
> due to the reasons mentioned above.
> 
> On Wed, Feb 26, 2020 at 12:11:38PM +1000, Greg Ungerer wrote:
> 
>> Hmm, in my experience the much more common pattern is:
>>
>>> +	int err;
>>>
>>> -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
>>> +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
>>> +	if (err) {
>>> +             pr_err("timer: request_irq() failed with err=%d\n", err);
>>> +		return err;
>>> +     }
> 
> This is my preferred style, but note that returning error is not
> possible in many of these cases as callers are void return type.
> 
> On Wed, Feb 26, 2020 at 05:39:57PM +1100, Finn Thain wrote:
> 
>> Besides, introducing local variables and altering control flow seems well
>> out-of-scope for this kind of refactoring, right?
> 
> To make changes perfect, it would be required to get into context of
> each case (>150), and do it manually as there is wide variation like
> whether caller can return error code, whether already a local integer
> is defined to catch the error, whether it returns error after a goto,
> whether we should allow it to proceed if it fails and so on.
> 
> And handling manually all the cases tree wide would be more error
> prone & time consuming. i was relieved that there was only one build
> error reported by test robot (i had done build & boot test only on ARM
> & x86_64), given the amount of manual changes i had to do on top of
> cocci generated ones.
> 
> At the same time, i didn't want to just mechanically replace & wanted
> to add some value to the existing setup, which resulted in this patch.
> 
> My thought process was to do treewide removal of setup_irq() and
> possibly low hanging cleanup's at the places where setup_irq() lives
> to make sure that surrounding situation will be better than or at
> least equal to the current.

Ok, makes sense. Given the very large tree-wide change I can see
why you wanted to completely automate it.


> But if the consensus is that all these situations have to be taken
> care, let me know.
> 
> On Wed, Feb 26, 2020 at 10:26:55PM +1000, Greg Ungerer wrote:
> 
>> A quick grep shows
>> that "%s: request_irq() failed\n" has no other exact matches in the current
>> kernel source.
> 
> git grep -n '%s: request_irq' gives a few somewhat similar ones, i
> remember it because searching this string after my changes to verify
> gave more than that i added :)

Unfortunately similar strings won't be coalesced...

Regards
Greg

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi Finn,

On Fri, Feb 28, 2020 at 09:38:20AM +1100, Finn Thain wrote:

> If you want to stop the compiler complaining about an unchecked return 
> value, assuming that it does so, please consider using
> 
> 	if (request_irq(...))
> 		pr_debug(...);
> 
> That way there is no penalty paid for adding error messages that the 
> original author apparently did not want.

Okay

Regards
afzal

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi Greg,

On Fri, Feb 28, 2020 at 05:05:36PM +1000, Greg Ungerer wrote:
> On 27/2/20 6:18 pm, afzal mohammed wrote:
> > On Wed, Feb 26, 2020 at 10:26:55PM +1000, Greg Ungerer wrote:

> > > A quick grep shows
> > > that "%s: request_irq() failed\n" has no other exact matches in the current
> > > kernel source.
> > 
> > git grep -n '%s: request_irq' gives a few somewhat similar ones, i
> > remember it because searching this string after my changes to verify
> > gave more than that i added :)
> 
> Unfortunately similar strings won't be coalesced...

Yes, sorry for that irrelevant comment, i didn't notice the context of
string coalescing w.r.t which you made that comment, my only excuse is
that i was drained by the end of writing that mail.

Regards
afzal

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi,

v3 has been posted w/ following changes,

1. Avoid unnecessary derefencing w.r.t irq name in pr_err
2. Modify pr_err() string so as to display in the form similar to,
        "Failed to request irq 2 (cascade)"
3. More commit message massage

Above are based on tglx feedback & would be applied to all patches in
addition to rearranging as required in other patches for readability.

Specific to m68k, following changes has been made based on m68 family
;) feedback,

1. Catch the errror code & display the symbolic error name usin "%pe"
format specifier
2. irq # in pr_err (already covered by following pt 2 pattern above)
3. s/pr_err/pr_debug

No altering of control flow based on error value has been done.

Regards
afzal

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Sat, 29 Feb 2020, afzal mohammed wrote:

> [...] 
> Specific to m68k, following changes has been made based on m68 family
> ;) feedback,
> 

None of my comments were specific to any architecture.

Also, I am not the maintainer for the affected m68k code. Greg is.

> 3. s/pr_err/pr_debug
> 

Please just ignore my opinion on that, since it contradicts the 
maintainer's guidance/preference.

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi,

On Sun, Mar 01, 2020 at 10:11:51AM +1100, Finn Thain wrote:
> On Sat, 29 Feb 2020, afzal mohammed wrote:

> > [...] 
> > Specific to m68k, following changes has been made based on m68 family
> > ;) feedback,
> > 
> 
> None of my comments were specific to any architecture.

One thing i had in my background, but realize now that didn't express
anywhere in my mails, in essence what Geert mentioned, i.e. being
legacy code, i did not give a treatment that would have been given to
adding new code.

But m68k subthread has been a very lively one and as not many changes,
felt it was not fair from my side not to handle almost as though it is
a new code addition.

There has been conflicting opinions, so i had to take a call one way
or other, including one against what i did not feel natural, mentioned
below, please let me know if further changes are required.

> > 3. s/pr_err/pr_debug
> 
> Please just ignore my opinion on that, since it contradicts the 
> maintainer's guidance/preference.

Yes, i will be remove this change.

Regards
afzal

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Sun, 1 Mar 2020, afzal mohammed wrote:

> On Sun, Mar 01, 2020 at 10:11:51AM +1100, Finn Thain wrote:
> > On Sat, 29 Feb 2020, afzal mohammed wrote:
> 
> > > [...] 
> > > Specific to m68k, following changes has been made based on m68 family
> > > ;) feedback,
> > > 
> > 
> > None of my comments were specific to any architecture.
> 
> One thing i had in my background, but realize now that didn't express 
> anywhere in my mails, in essence what Geert mentioned, i.e. being legacy 
> code, i did not give a treatment that would have been given to adding 
> new code.
> 
> But m68k subthread has been a very lively one and as not many changes, 
> felt it was not fair from my side not to handle almost as though it is a 
> new code addition.
> 

I took Geert's comments to be architecture agnostic but perhaps I 
misunderstood.

BTW, how do you distinguish between "new code" and "legacy code"?

And why would you choose to do that when you are writing a tree-wide 
semantic patch?

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi,

On Sun, Mar 01, 2020 at 02:26:33PM +1100, Finn Thain wrote:

> BTW, how do you distinguish between "new code" and "legacy code"?

setup_irq() was used in olden days, nowadays request_irq(). Though
there are exceptions of trying to use setup_irq() even recently, but
there has been pushback when people notice it like Thomas had done
[1], and i saw recently one in mips smp support series & suggested not
to use it (that code iiuc they had it out of upstream for a long time).

So existence of setup_irq() in general i have considered to be legacy
code.

> And why would you choose to do that when you are writing a tree-wide 
> semantic patch?

The way i came up with this series is that while trying to understand
irq internals, came across [1], so then decided to do cleanup and i
thought scripting it would make it easy & also had been wanting to
get familiar w/ cocci, so decided to try it, but also realized that i
cannot fully automate it (Julia said my patch is okay, so i felt cocci
cannot fully automate w/o investing considerable effort in cocci), so
even w/ this v2, there are lot of manual changes, though cocci made it
easier.

> I took Geert's comments to be architecture agnostic but perhaps I 
> misunderstood.

And Thomas suggested to make improvements over script generated o/p [2]
and only consider scripting as an initial first step. So the way i am
making changes now is to take suggestions from Thomas to be applied
treewide, at the same time also take care of suggestions from
arch/subsytem maintainer/mailing list in the relevant patches, since
arch maintainers are the ones owning it.

Sometimes had a feeling as though the changes in this series is akin
to cutting the foot to fit the shoe ;), but still went ahead as it was
legacy code, easier & less error prone. But now based on the overall
feedback, to proceed, i had to change.

Regards
afzal

[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
[2] https://lore.kernel.org/lkml/87sgiwma3x.fsf@nanos.tec.linutronix.de/

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Finn Thain]

On Sun, 1 Mar 2020, afzal mohammed wrote:

> Hi,
> 
> On Sun, Mar 01, 2020 at 02:26:33PM +1100, Finn Thain wrote:
> 
> > BTW, how do you distinguish between "new code" and "legacy code"?
> 
> setup_irq() was used in olden days, nowadays request_irq(). Though there 
> are exceptions of trying to use setup_irq() even recently, but there has 
> been pushback when people notice it like Thomas had done [1], and i saw 
> recently one in mips smp support series & suggested not to use it (that 
> code iiuc they had it out of upstream for a long time).
> 
> So existence of setup_irq() in general i have considered to be legacy 
> code.
> 

I see. You're defining "legacy code" in this case to mean code that uses a 
deprecated API, that needs to be modernized.

> > And why would you choose to do that when you are writing a tree-wide 
> > semantic patch?
> 
> The way i came up with this series is that while trying to understand 
> irq internals, came across [1], so then decided to do cleanup and i 
> thought scripting it would make it easy & also had been wanting to get 
> familiar w/ cocci, so decided to try it, but also realized that i cannot 
> fully automate it (Julia said my patch is okay, so i felt cocci cannot 
> fully automate w/o investing considerable effort in cocci), so even w/ 
> this v2, there are lot of manual changes, though cocci made it easier.
> 
> > I took Geert's comments to be architecture agnostic but perhaps I 
> > misunderstood.
> 
> And Thomas suggested to make improvements over script generated o/p [2] 
> and only consider scripting as an initial first step. So the way i am 
> making changes now is to take suggestions from Thomas to be applied 
> treewide, at the same time also take care of suggestions from 
> arch/subsytem maintainer/mailing list in the relevant patches, since 
> arch maintainers are the ones owning it.
> 

Thanks for the detailed explanation.

I had assumed that your intention was to find a consensus so that the 
whole tree could be consistently and automatically improved. My mistake.

> Sometimes had a feeling as though the changes in this series is akin to 
> cutting the foot to fit the shoe ;), but still went ahead as it was 
> legacy code, easier & less error prone. But now based on the overall 
> feedback, to proceed, i had to change.
> 

Not based on feedback from me I hope -- I have no veto in this case, as 
you can see from MAINTAINERS.

> Regards
> afzal
> 
> [1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos
> [2] https://lore.kernel.org/lkml/87sgiwma3x.fsf@nanos.tec.linutronix.de/
> 

Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[afzal mohammed]

Hi Finn,

On Mon, Mar 02, 2020 at 05:26:17PM +1100, Finn Thain wrote:

> I had assumed that your intention was to find a consensus so that the 
> whole tree could be consistently and automatically improved

Yeah, my main goal was to get rid of setup_irq(), other things were
secondary and proceeded to achieve the removal by hook or crook, but was
caught red handed :)

> > Sometimes had a feeling as though the changes in this series is akin to 
> > cutting the foot to fit the shoe ;), but still went ahead as it was 
> > legacy code, easier & less error prone. But now based on the overall 
> > feedback, to proceed, i had to change.
> > 
> 
> Not based on feedback from me I hope -- I have no veto in this case, as 
> you can see from MAINTAINERS.

i don't know what to say, i attempted to accomodate the reviews as
much as possible, some times when opinions are conflicting i had to
take a call one way or the other, with more importance to maintainer's
view.

Regards
afzal