* handle_bad_irq and locking
@ 2017-05-11 23:21 Gregory Fong
2017-05-12 8:40 ` Thomas Gleixner
0 siblings, 1 reply; 3+ messages in thread
From: Gregory Fong @ 2017-05-11 23:21 UTC (permalink / raw)
To: tglx; +Cc: linux-kernel, linux, Florian Fainelli
Hi Thomas,
I noticed that when you changed arm irq handling to use the generic
implementation back in 2006 that you changed do_bad_IRQ() to the
following:
+#define do_bad_IRQ(irq,desc,regs) \
+do { \
+ spin_lock(&desc->lock); \
+ handle_bad_irq(irq, desc, regs); \
+ spin_unlock(&desc->lock); \
+} while(0)
and it's mostly stayed the same since then. As such, there are a few
examples of this being open-coded in the kernel in various irqchip
handlers such as that of drivers/irqchip/irq-brcmstb-l2.c, and even
more cases where the lock isn't being grabbed before calling
handle_bad_irq().
Since handle_bad_irq() is sort of a flow handler like
handle_edge_irq() etc., do you think it would make sense to do the
same as those and have it do the locking on its own? A quick look
through existing users of handle_bad_irq() at [1] suggests that either
the locking isn't necessary or it's almost always done wrong.
Apologies if this has been asked before, but it's remarkably difficult
to search for.
[1] http://elixir.free-electrons.com/linux/latest/ident/handle_bad_irq
Thanks and regards,
Gregory
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: handle_bad_irq and locking
2017-05-11 23:21 handle_bad_irq and locking Gregory Fong
@ 2017-05-12 8:40 ` Thomas Gleixner
2017-05-15 8:46 ` Gregory Fong
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Gleixner @ 2017-05-12 8:40 UTC (permalink / raw)
To: Gregory Fong; +Cc: linux-kernel, linux, Florian Fainelli
On Thu, 11 May 2017, Gregory Fong wrote:
> Hi Thomas,
>
> I noticed that when you changed arm irq handling to use the generic
> implementation back in 2006 that you changed do_bad_IRQ() to the
> following:
>
> +#define do_bad_IRQ(irq,desc,regs) \
> +do { \
> + spin_lock(&desc->lock); \
> + handle_bad_irq(irq, desc, regs); \
> + spin_unlock(&desc->lock); \
> +} while(0)
>
> and it's mostly stayed the same since then. As such, there are a few
> examples of this being open-coded in the kernel in various irqchip
> handlers such as that of drivers/irqchip/irq-brcmstb-l2.c, and even
> more cases where the lock isn't being grabbed before calling
> handle_bad_irq().
>
> Since handle_bad_irq() is sort of a flow handler like
> handle_edge_irq() etc., do you think it would make sense to do the
> same as those and have it do the locking on its own? A quick look
> through existing users of handle_bad_irq() at [1] suggests that either
> the locking isn't necessary or it's almost always done wrong.
Right. So the proper solution to this is to create a generic function
handle_bad_irq_desc()
have proper locking in it and move all users (including do_bad_IRQ()) over
to it.
Thanks,
tglx
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: handle_bad_irq and locking
2017-05-12 8:40 ` Thomas Gleixner
@ 2017-05-15 8:46 ` Gregory Fong
0 siblings, 0 replies; 3+ messages in thread
From: Gregory Fong @ 2017-05-15 8:46 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, linux, Florian Fainelli
On Fri, May 12, 2017 at 1:40 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 11 May 2017, Gregory Fong wrote:
>> Hi Thomas,
>>
>> I noticed that when you changed arm irq handling to use the generic
>> implementation back in 2006 that you changed do_bad_IRQ() to the
>> following:
>>
>> +#define do_bad_IRQ(irq,desc,regs) \
>> +do { \
>> + spin_lock(&desc->lock); \
>> + handle_bad_irq(irq, desc, regs); \
>> + spin_unlock(&desc->lock); \
>> +} while(0)
>>
>> and it's mostly stayed the same since then. As such, there are a few
>> examples of this being open-coded in the kernel in various irqchip
>> handlers such as that of drivers/irqchip/irq-brcmstb-l2.c, and even
>> more cases where the lock isn't being grabbed before calling
>> handle_bad_irq().
>>
>> Since handle_bad_irq() is sort of a flow handler like
>> handle_edge_irq() etc., do you think it would make sense to do the
>> same as those and have it do the locking on its own? A quick look
>> through existing users of handle_bad_irq() at [1] suggests that either
>> the locking isn't necessary or it's almost always done wrong.
>
> Right. So the proper solution to this is to create a generic function
>
> handle_bad_irq_desc()
>
> have proper locking in it and move all users (including do_bad_IRQ()) over
> to it.
I'm guessing the different name would be to avoid breaking existing
users. Wouldn't it be better to just add locking in handle_bad_irq(),
simultaneously removing the lock/unlock around in its invocations in
the few places where that was actually done? There aren't any
existing builtin IRQ flow handlers that end in "_desc".
Otherwise I'll move forward with implementing handle_bad_irq_desc()
and updating existing users.
Thanks,
Gregory
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-05-15 8:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-11 23:21 handle_bad_irq and locking Gregory Fong
2017-05-12 8:40 ` Thomas Gleixner
2017-05-15 8:46 ` Gregory Fong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.