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

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?

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".

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