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

[Greg Ungerer <gerg@linux-m68k.org>]

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