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@xxxxxxxxx>
Tested-by: Greg Ungerer <gerg@xxxxxxxxxxxxxx> # 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.
+ 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;
+ }