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

From: Thomas Gleixner <tglx@linutronix.de>
To: afzal mohammed <afzal.mohd.ma@gmail.com>
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 14:29:54 +0100	[thread overview]
Message-ID: <87sgiwma3x.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20200227113648.GA5760@afzalpc>

Afzal,

afzal mohammed <afzal.mohd.ma@gmail.com> writes:
> 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.

Using scripting to find the spots and automatically convert them to
something which is functionally and semantically correct is perfectly
fine. But scripts neither have taste nor common sense.

So going over a scripted conversion and fixing non-sensical stuff up
manually is a good thing.

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

The easy route is fine for the initial step. See above.

Thanks,

        tglx