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@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.

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