[lkml]   [2020]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()
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...


 \ /
  Last update: 2020-02-28 08:06    [W:0.102 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site