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

[afzal mohammed <afzal.mohd.ma@gmail.com>]

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