Messages in this thread | | | Date | Thu, 27 Feb 2020 17:06:48 +0530 | From | afzal mohammed <> | 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@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
| |