linux-kernel - Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.22.394.2002261637400.8@nippy.intranet>
Date:   Wed, 26 Feb 2020 17:39:57 +1100 (AEDT)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     Greg Ungerer <gerg@...ux-m68k.org>
cc:     afzal mohammed <afzal.mohd.ma@...il.com>,
        linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [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".