lkml.org 
[lkml]   [2020]   [Feb]   [26]   [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()
From
Date

On 26/2/20 4:39 pm, Finn Thain wrote:
> On Wed, 26 Feb 2020, Greg Ungerer wrote:
>
>>> That error would almost always be -EBUSY, right?
>>
>> I expect it will never fail this early in boot.
>
> If so, it suggests to me that tweaking the error message string is just
> bikeshedding and that adding these error messages across the tree is just
> bloat.
>
>> 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, ...
>>
>
> A rough poll using 'git grep' seems to agree with your assessment.
>
> If -EBUSY means the end user has misconfigured something, printing
> "request_irq failed" would be helpful. But does that still happen?

I have seen it many times. Its not at all difficult to get interrupt
assignments wrong, duplicated, or otherwise mistaken when creating
device trees. Not so much m68k/coldfire platforms where they are
most commonly hard coded.


> Printing any error message for -ENOMEM is frowned upon, and printing -12
> is really unhelpful. So the most popular pattern isn't that great, though
> it is usually less verbose than the example you've given.
>
> Besides, introducing local variables and altering control flow seems well
> out-of-scope for this kind of refactoring, right?

I don't agree with the local variable part. Adding a local variable to
keep track of the error return code doesn't seem out of scope for this change.
The patch as Afzal sent it doesn't change the control flow - and
that is the right thing to do here.


> Anyway, if you're going to add an error message,
> pr_err("%s: request_irq failed", foo) is unavoidable whenever foo isn't a
> string constant, so one can't expect to grep the source code for the
> literal error message from the log.
>
> BTW, one of the benefits of "%s: request_irq failed" is that a compilation
> unit with multiple request_irq calls permits the compiler to coalesce all
> duplicated format strings. Whereas, that's not possible with
> "foo: request_irq failed" and "bar: request_irq failed".

Given the wide variety of message text used with failed request_irq() calls
it would be shear luck that this matched anything else. A quick grep shows
that "%s: request_irq() failed\n" has no other exact matches in the current
kernel source.

Regards
Greg


\
 
 \ /
  Last update: 2020-02-26 13:27    [W:0.154 / U:0.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site