Re: [PATCH 14/18] x86: Replace setup_irq() by request_irq()

From: afzal mohammed <afzal.mohd.ma@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Juergen Gross <jgross@suse.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH 14/18] x86: Replace setup_irq() by request_irq()
Date: Thu, 27 Feb 2020 17:06:48 +0530	[thread overview]
Message-ID: <20200227113648.GA5760@afzalpc> (raw)
In-Reply-To: <87v9nsmhjj.fsf@nanos.tec.linutronix.de>

Hi Thomas,

On Thu, Feb 27, 2020 at 11:49:20AM +0100, Thomas Gleixner wrote:
> afzal mohammed <afzal.mohd.ma@gmail.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