| Message ID | 1442822916-26238-1-git-send-email-rogerq@ti.com |
|---|---|
| State | Accepted |
| Commit | a66c275b3d5d8467d770dabd30927f5d5e857294 |
| Headers | show |
On Mon, 21 Sep 2015, Roger Quadros wrote: > Using spin_lock() in hard irq handler is pointless > and causes a BUG() in RT (real-time) configuration > so get rid of it. Wait a minute. Who says spin_lock is pointless in an IRQ handler? And who says it causes a BUG in RT configurations? And if those claims are true, why do we have spin_lock in IRQ handlers for various host controller drivers? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > Using spin_lock() in hard irq handler is pointless > > and causes a BUG() in RT (real-time) configuration > > so get rid of it. > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? in the top half IRQs are already disabled, how can this race ? > And who says it causes a BUG in RT configurations? spin_locks are reimplemented as RT-aware mutexes when the RT patch is applied. > And if those claims are true, why do we have spin_lock in IRQ handlers > for various host controller drivers? because they might be wrong too ? :-p Or I'm missing something, but if IRQs are disabled in top half, how can this race ? Unless you're running in a thread without IRQF_ONESHOT.
On Mon, 21 Sep 2015, Felipe Balbi wrote: > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > > > Using spin_lock() in hard irq handler is pointless > > > and causes a BUG() in RT (real-time) configuration > > > so get rid of it. > > > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? > > in the top half IRQs are already disabled, how can this race ? It can race with code running on a different CPU. > > And who says it causes a BUG in RT configurations? > > spin_locks are reimplemented as RT-aware mutexes when the RT patch is applied. > > > And if those claims are true, why do we have spin_lock in IRQ handlers > > for various host controller drivers? > > because they might be wrong too ? :-p Or I'm missing something, but if IRQs > are disabled in top half, how can this race ? Unless you're running in a > thread without IRQF_ONESHOT. I just looked more closely. The difference is that dwc3 uses a threaded interrupt handler whereas those other drivers don't. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > > > > > Using spin_lock() in hard irq handler is pointless > > > > and causes a BUG() in RT (real-time) configuration > > > > so get rid of it. > > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? > > > > in the top half IRQs are already disabled, how can this race ? > > It can race with code running on a different CPU. fair point. > > > And who says it causes a BUG in RT configurations? > > > > spin_locks are reimplemented as RT-aware mutexes when the RT patch is applied. > > > > > And if those claims are true, why do we have spin_lock in IRQ handlers > > > for various host controller drivers? > > > > because they might be wrong too ? :-p Or I'm missing something, but if IRQs > > are disabled in top half, how can this race ? Unless you're running in a > > thread without IRQF_ONESHOT. > > I just looked more closely. The difference is that dwc3 uses a > threaded interrupt handler whereas those other drivers don't. yeah, and we make sure to mask dwc3's interrupts before returning from the top half.
On Mon, 21 Sep 2015, Felipe Balbi wrote: > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > > > > > > > Using spin_lock() in hard irq handler is pointless > > > > > and causes a BUG() in RT (real-time) configuration > > > > > so get rid of it. > > > > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? > > > > > > in the top half IRQs are already disabled, how can this race ? > > > > It can race with code running on a different CPU. > > fair point. In fact, isn't this the main purpose of spinlocks? There's no point using a spinlock to protect against races occurring on a single CPU, because (in non-RT situations) the kernel can't schedule or preempt while a spinlock is held, so no race can occur. The whole idea of spinlocks is to protect against cross-CPU races. Now, maybe the spinlock usage that Roger is removing really is unnecessary in this top-half handler. I don't know; I haven't looked at the code. But in general, spinlocks are highly necessary in IRQ handlers. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 11:37:47AM -0400, Alan Stern wrote: > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > > > > > > > > > Using spin_lock() in hard irq handler is pointless > > > > > > and causes a BUG() in RT (real-time) configuration > > > > > > so get rid of it. > > > > > > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? > > > > > > > > in the top half IRQs are already disabled, how can this race ? > > > > > > It can race with code running on a different CPU. > > > > fair point. > > In fact, isn't this the main purpose of spinlocks? There's no point > using a spinlock to protect against races occurring on a single CPU, > because (in non-RT situations) the kernel can't schedule or preempt > while a spinlock is held, so no race can occur. The whole idea of > spinlocks is to protect against cross-CPU races. > > Now, maybe the spinlock usage that Roger is removing really is > unnecessary in this top-half handler. I don't know; I haven't looked > at the code. But in general, spinlocks are highly necessary in IRQ > handlers. this does raise some questions of what to do then ? We can't use spin_locks when in RT because they're reimplemented as mutexes and can sleep. Converting every single driver to raw_spin_locks defeats the purpose of having everything and their dogs preemptable. Difficult to say. Let's defer this patch until we can come up with a good answer to this.
On Mon, 21 Sep 2015, Felipe Balbi wrote: > On Mon, Sep 21, 2015 at 11:37:47AM -0400, Alan Stern wrote: > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > > > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > > > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > > > > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > > > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > > > > > > > > > > > > > Using spin_lock() in hard irq handler is pointless > > > > > > > and causes a BUG() in RT (real-time) configuration > > > > > > > so get rid of it. > > > > > > > > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ handler? > > > > > > > > > > in the top half IRQs are already disabled, how can this race ? > > > > > > > > It can race with code running on a different CPU. > > > > > > fair point. > > > > In fact, isn't this the main purpose of spinlocks? There's no point > > using a spinlock to protect against races occurring on a single CPU, > > because (in non-RT situations) the kernel can't schedule or preempt > > while a spinlock is held, so no race can occur. The whole idea of > > spinlocks is to protect against cross-CPU races. > > > > Now, maybe the spinlock usage that Roger is removing really is > > unnecessary in this top-half handler. I don't know; I haven't looked > > at the code. But in general, spinlocks are highly necessary in IRQ > > handlers. > > this does raise some questions of what to do then ? We can't use > spin_locks when in RT because they're reimplemented as mutexes > and can sleep. Converting every single driver to raw_spin_locks > defeats the purpose of having everything and their dogs preemptable. > > Difficult to say. Let's defer this patch until we can come up > with a good answer to this. Someone more familiar with the -RT patches should be able to answer these questions. Steve, any advice? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On September 21, 2015 9:27:43 AM PDT, Alan Stern <stern@rowland.harvard.edu> wrote: >On Mon, 21 Sep 2015, Felipe Balbi wrote: > >> On Mon, Sep 21, 2015 at 11:37:47AM -0400, Alan Stern wrote: >> > On Mon, 21 Sep 2015, Felipe Balbi wrote: >> > >> > > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: >> > > > On Mon, 21 Sep 2015, Felipe Balbi wrote: >> > > > >> > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: >> > > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: >> > > > > > >> > > > > > > Using spin_lock() in hard irq handler is pointless >> > > > > > > and causes a BUG() in RT (real-time) configuration >> > > > > > > so get rid of it. >> > > > > > >> > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ >handler? >> > > > > >> > > > > in the top half IRQs are already disabled, how can this race >? >> > > > >> > > > It can race with code running on a different CPU. >> > > >> > > fair point. >> > >> > In fact, isn't this the main purpose of spinlocks? There's no >point >> > using a spinlock to protect against races occurring on a single >CPU, >> > because (in non-RT situations) the kernel can't schedule or preempt >> > while a spinlock is held, so no race can occur. The whole idea of >> > spinlocks is to protect against cross-CPU races. >> > >> > Now, maybe the spinlock usage that Roger is removing really is >> > unnecessary in this top-half handler. I don't know; I haven't >looked >> > at the code. But in general, spinlocks are highly necessary in IRQ >> > handlers. >> >> this does raise some questions of what to do then ? We can't use >> spin_locks when in RT because they're reimplemented as mutexes >> and can sleep. Converting every single driver to raw_spin_locks >> defeats the purpose of having everything and their dogs preemptable. >> >> Difficult to say. Let's defer this patch until we can come up >> with a good answer to this. > >Someone more familiar with the -RT patches should be able to answer >these questions. Spin lock (without irqsave) may have a bug if the irq handler spin locks the same one, and the irqs aren't already disabled. E.g. any context (atomic or not) spin locks, then the irq handler takes place on *same* cpu and tries to spin lock again. It will be a deadlock. I believe that's what Roger misinterpreted with his patch description. Br, David Cohen > >Steve, any advice? > >Alan Stern > >-- >To unsubscribe from this list: send the line "unsubscribe linux-usb" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html Hi,
Hi, On Mon, Sep 21, 2015 at 09:46:30AM -0700, David Cohen wrote: > On September 21, 2015 9:27:43 AM PDT, Alan Stern <stern@rowland.harvard.edu> wrote: > >On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > >> On Mon, Sep 21, 2015 at 11:37:47AM -0400, Alan Stern wrote: > >> > On Mon, 21 Sep 2015, Felipe Balbi wrote: > >> > > >> > > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > >> > > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > >> > > > > >> > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > >> > > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > >> > > > > > > >> > > > > > > Using spin_lock() in hard irq handler is pointless > >> > > > > > > and causes a BUG() in RT (real-time) configuration > >> > > > > > > so get rid of it. > >> > > > > > > >> > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ > >handler? > >> > > > > > >> > > > > in the top half IRQs are already disabled, how can this race > >? > >> > > > > >> > > > It can race with code running on a different CPU. > >> > > > >> > > fair point. > >> > > >> > In fact, isn't this the main purpose of spinlocks? There's no > >point > >> > using a spinlock to protect against races occurring on a single > >CPU, > >> > because (in non-RT situations) the kernel can't schedule or preempt > >> > while a spinlock is held, so no race can occur. The whole idea of > >> > spinlocks is to protect against cross-CPU races. > >> > > >> > Now, maybe the spinlock usage that Roger is removing really is > >> > unnecessary in this top-half handler. I don't know; I haven't > >looked > >> > at the code. But in general, spinlocks are highly necessary in IRQ > >> > handlers. > >> > >> this does raise some questions of what to do then ? We can't use > >> spin_locks when in RT because they're reimplemented as mutexes > >> and can sleep. Converting every single driver to raw_spin_locks > >> defeats the purpose of having everything and their dogs preemptable. > >> > >> Difficult to say. Let's defer this patch until we can come up > >> with a good answer to this. > > > >Someone more familiar with the -RT patches should be able to answer > >these questions. > > Spin lock (without irqsave) may have a bug if the irq handler spin locks the > same one, and the irqs aren't already disabled. > E.g. any context (atomic or not) spin locks, then the irq handler takes place on > *same* cpu and tries to spin lock again. It will be a deadlock. > > I believe that's what Roger misinterpreted with his patch description. no, no. you're missing the point here. The problem is that when RT is applied, spinlocks get reimplemented as RT-aware mutexes which works pretty well as long as you don't install your own top and bottom halves. If you do, RT patch can't force your handler to run as a thread and you're left with, essentially, a mutex for synchronization in hardirq context. So the question is really for Steve, what should we do here ? Use a raw_spinlock ? I'd like to avoid that if possible. If we have another option, I'm all ears.
On Mon, 21 Sep 2015 12:04:16 -0500 Felipe Balbi <balbi@ti.com> wrote: > no, no. you're missing the point here. The problem is that when RT > is applied, spinlocks get reimplemented as RT-aware mutexes which > works pretty well as long as you don't install your own top and bottom > halves. If you do, RT patch can't force your handler to run as a thread > and you're left with, essentially, a mutex for synchronization in > hardirq context. > > So the question is really for Steve, what should we do here ? Use a > raw_spinlock ? I'd like to avoid that if possible. If we have another > option, I'm all ears. > I may be missing something here. Yes, in RT spinlocks become rtmutexes and can sleep. But in RT all interrupts become threads. With the slight exception that if you declare your interrupt handle as a thread, then you do have a top half handler. This top half should not be grabbing any spin locks, and should simply disable any more interrupts from happening on the device until the thread handler can run. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 03:01:42PM -0400, Steven Rostedt wrote: > On Mon, 21 Sep 2015 12:04:16 -0500 > Felipe Balbi <balbi@ti.com> wrote: > > > > no, no. you're missing the point here. The problem is that when RT > > is applied, spinlocks get reimplemented as RT-aware mutexes which > > works pretty well as long as you don't install your own top and bottom > > halves. If you do, RT patch can't force your handler to run as a thread > > and you're left with, essentially, a mutex for synchronization in > > hardirq context. > > > > So the question is really for Steve, what should we do here ? Use a > > raw_spinlock ? I'd like to avoid that if possible. If we have another > > option, I'm all ears. > > > > I may be missing something here. Yes, in RT spinlocks become rtmutexes > and can sleep. But in RT all interrupts become threads. With the slight > exception that if you declare your interrupt handle as a thread, then > you do have a top half handler. This top half should not be grabbing > any spin locks, and should simply disable any more interrupts from > happening on the device until the thread handler can run. and that's basically we're doing. $SUBJECT is removing the spin lock for that reason. Register accesses should be satomic anyway, right??
On Mon, 21 Sep 2015, Felipe Balbi wrote: > On Mon, Sep 21, 2015 at 03:01:42PM -0400, Steven Rostedt wrote: > > On Mon, 21 Sep 2015 12:04:16 -0500 > > Felipe Balbi <balbi@ti.com> wrote: > > > > > > > no, no. you're missing the point here. The problem is that when RT > > > is applied, spinlocks get reimplemented as RT-aware mutexes which > > > works pretty well as long as you don't install your own top and bottom > > > halves. If you do, RT patch can't force your handler to run as a thread > > > and you're left with, essentially, a mutex for synchronization in > > > hardirq context. > > > > > > So the question is really for Steve, what should we do here ? Use a > > > raw_spinlock ? I'd like to avoid that if possible. If we have another > > > option, I'm all ears. > > > > > > > I may be missing something here. Yes, in RT spinlocks become rtmutexes > > and can sleep. But in RT all interrupts become threads. With the slight > > exception that if you declare your interrupt handle as a thread, then > > you do have a top half handler. This top half should not be grabbing > > any spin locks, and should simply disable any more interrupts from > > happening on the device until the thread handler can run. > > and that's basically we're doing. $SUBJECT is removing the spin lock for > that reason. Register accesses should be satomic anyway, right?? I guess the patch is the right thing to do. But the description sure could be improved. Roger wrote: "Using spin_lock() in hard irq handler is pointless and causes a BUG() in RT (real-time) configuration", which is ungrammatical as well as misleading. It should say something more along the lines of: Top-half handlers for threaded interrupts are not supposed to acquire any locks. None are needed because the top half is guaranteed to be mutually exclusive with the bottom-half thread handler. Furthermore, spin_lock() in a top-half handler causes a BUG() in RT-enabled kernels. Remove the unnecessary spin_lock() in dwc3_interrupt(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 21 Sep 2015 15:48:53 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> wrote: > It should say something more along the lines of: > > Top-half handlers for threaded interrupts are not supposed to > acquire any locks. None are needed because the top half is > guaranteed to be mutually exclusive with the bottom-half > thread handler. Furthermore, spin_lock() in a top-half handler > causes a BUG() in RT-enabled kernels. > > Remove the unnecessary spin_lock() in dwc3_interrupt(). Ack! -- Steve > > Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 03:48:53PM -0400, Alan Stern wrote: > On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > On Mon, Sep 21, 2015 at 03:01:42PM -0400, Steven Rostedt wrote: > > > On Mon, 21 Sep 2015 12:04:16 -0500 > > > Felipe Balbi <balbi@ti.com> wrote: > > > > > > > > > > no, no. you're missing the point here. The problem is that when RT > > > > is applied, spinlocks get reimplemented as RT-aware mutexes which > > > > works pretty well as long as you don't install your own top and bottom > > > > halves. If you do, RT patch can't force your handler to run as a thread > > > > and you're left with, essentially, a mutex for synchronization in > > > > hardirq context. > > > > > > > > So the question is really for Steve, what should we do here ? Use a > > > > raw_spinlock ? I'd like to avoid that if possible. If we have another > > > > option, I'm all ears. > > > > > > > > > > I may be missing something here. Yes, in RT spinlocks become rtmutexes > > > and can sleep. But in RT all interrupts become threads. With the slight > > > exception that if you declare your interrupt handle as a thread, then > > > you do have a top half handler. This top half should not be grabbing > > > any spin locks, and should simply disable any more interrupts from > > > happening on the device until the thread handler can run. > > > > and that's basically we're doing. $SUBJECT is removing the spin lock for > > that reason. Register accesses should be satomic anyway, right?? > > I guess the patch is the right thing to do. But the description sure > could be improved. Roger wrote: "Using spin_lock() in hard irq handler > is pointless and causes a BUG() in RT (real-time) configuration", > which is ungrammatical as well as misleading. > > It should say something more along the lines of: > > Top-half handlers for threaded interrupts are not supposed to > acquire any locks. None are needed because the top half is > guaranteed to be mutually exclusive with the bottom-half > thread handler. Furthermore, spin_lock() in a top-half handler > causes a BUG() in RT-enabled kernels. > > Remove the unnecessary spin_lock() in dwc3_interrupt(). I kept Roger's commit log and just added one extra paragraph: Author: Roger Quadros <rogerq@ti.com> Date: Mon Sep 21 11:08:36 2015 +0300 usb: dwc3: gadget: Fix BUG in RT config Using spin_lock() in hard irq handler is pointless and causes a BUG() in RT (real-time) configuration so get rid of it. The reason it's pointless is because the driver is basically accessing register which is, anyways, atomic. Signed-off-by: Roger Quadros <rogerq@ti.com> Signed-off-by: Felipe Balbi <balbi@ti.com> Probably should have added the detail that top and bottom halves are guaranteed to be mutually exclusive, sorry.
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 0c25704..1e8bdf8 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2665,8 +2665,6 @@ static irqreturn_t dwc3_interrupt(int irq, void *_dwc) int i; irqreturn_t ret = IRQ_NONE; - spin_lock(&dwc->lock); - for (i = 0; i < dwc->num_event_buffers; i++) { irqreturn_t status; @@ -2675,8 +2673,6 @@ static irqreturn_t dwc3_interrupt(int irq, void *_dwc) ret = status; } - spin_unlock(&dwc->lock); - return ret; }
Using spin_lock() in hard irq handler is pointless and causes a BUG() in RT (real-time) configuration so get rid of it. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/dwc3/gadget.c | 4 ---- 1 file changed, 4 deletions(-)