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

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

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