linux-kernel - Re: [PATCH 14/18] x86: 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]

Date:   Thu, 27 Feb 2020 17:06:48 +0530
From:   afzal mohammed <afzal.mohd.ma@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, Juergen Gross <jgross@...e.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 14/18] x86: Replace setup_irq() by request_irq()

Hi Thomas,

On Thu, Feb 27, 2020 at 11:49:20AM +0100, Thomas Gleixner wrote:
> afzal mohammed <afzal.mohd.ma@...il.com> writes:

> > Seldom remove_irq() usage has been observed coupled with setup_irq(),
> > wherever that has been found, it too has been replaced by free_irq().
> 
> Please do not copy the irrelevant parts of your boilerplate text into
> individual changelogs. Changelogs should have the information which is
> relevant to the patch they describe.

Sure, i will change.

> > +		if (request_irq(2, no_action, IRQF_NO_THREAD, "cascade", NULL))
> > +			pr_err("request_irq() on %s failed\n", "cascade");
> 
> What's the purpose of the %s indirection here?

Putting that indirection helped me automate making these kind of changes w/
cocci. As there were >150 instances of setup_irq and since "failed",
"request_irq()" were common, putting a %s indirection with the timer
name that changes in each instance, it was easy to take help of cocci
to automatically create it (though not fully).

Would you be okay to keep that indirection ?, if not , i will make the
changes manually.

> Also that error message is not really helpful:
> 
>      request_irq() on cascade failed
> 
> Something like:
> 
>      Failed to request irq 2 (cascade).

Yes, as i mentioned in m86k patch (6/18) [1], i was uncomfortable w/ that
string, based on Finn's feedback, in v2, it was modified to,

        cascade: request_irq() failed

though still using %s indirection.

> > -	if (setup_irq(0, &irq0))
> > +	if (request_irq(0, timer_interrupt,
> > +			IRQF_NOBALANCING | IRQF_IRQPOLL | IRQF_TIMER, "timer",
> > +			NULL))
> 
> This is realy ugly.
> 
> 	unsigned long flags = IRQF_NOBALANCING | IRQF_IRQPOLL | IRQF_TIMER;
> 
> 	....
>      	if (request_irq(0, timer_interrupt, flags, "timer", NULL))
>         	....
> 
> takes the same amount of lines, but is readable.

Yeah, i had felt it, the reason i went ahead that way was cocci could
automate that way for me for three-fourth of >150 instances making
things easy for me. Another reason was to reduce manual changes so as
to make it less error prone.

Seems you are against taking the easy route of taking cocci's help, in
that case, i will change as you suggest.

There was discussion on m68k (6/18) patch [2], but v2 series on
similar lines, not w.r.t flags, but mainly  w.r.t return value.

[1] http://lkml.kernel.org/r/20200213020330.GC2684@teres
[2] http://lkml.kernel.org/r/20200227081805.GA5746@afzalpc

Regards
afzal