.../...
This patch (mostly written by Linus) fixes this by using spin
locks instead of memory barries on the synchronize_irq() path.
Signed-off-by: Herbert Xu [EMAIL PROTECTED]
Good for me.
Acked-by: Benjamin Herrenschmidt [EMAIL PROTECTED]
Hrm... not on yet. Herbert, care to
On Sat, 2007-10-20 at 08:06 +0200, Maxim Levitsky wrote:
/* Disable interrupts, DMA, and rest of the chip*/
saa_writel(SAA7134_IRQ1, 0);
saa_writel(SAA7134_IRQ2, 0);
saa_writel(SAA7134_MAIN_CTRL, 0);
dev-insuspend = 1;
On Thursday 18 October 2007 03:25:42 Benjamin Herrenschmidt wrote:
synchronize_irq needs at the very least a compiler barrier and a
read barrier on SMP, but there are enough cases around where a
write barrier is also needed and it's not a hot path so I prefer
using a full smp_mb() here.
It
On Saturday 20 October 2007 04:25:34 Linus Torvalds wrote:
On Sat, 20 Oct 2007, Maxim Levitsky wrote:
and the interrupt handler:
smp_rmb();
if (dev-insuspend)
goto out;
Something like that can work, yes. However, you need to make sure that:
- even when
On Sat, Oct 20, 2007 at 02:02:42AM +, Maxim Levitsky wrote:
Thus I now understand that .suspend() should do:
saa_writel(SAA7134_IRQ1, 0);
saa_writel(SAA7134_IRQ2, 0);
saa_writel(SAA7134_MAIN_CTRL, 0);
dev-insuspend = 1;
smp_wmb();
If we patch
On Sat, 20 Oct 2007, Maxim Levitsky wrote:
and the interrupt handler:
smp_rmb();
if (dev-insuspend)
goto out;
Something like that can work, yes. However, you need to make sure that:
- even when you ignore the interrupt (because the driver doesn't care,
- even when you ignore the interrupt (because the driver doesn't care,
it's suspending), you need to make sure the hardware gets shut up by
reading (or writing) the proper interrupt status register.
Otherwise, with a level interrupt, the interrupt will continue to be
- even when you ignore the interrupt (because the driver doesn't care,
it's suspending), you need to make sure the hardware gets shut up by
reading (or writing) the proper interrupt status register.
I agree, but while device is powered off, its registers can't be accessed
Thus,
On Fri, 2007-10-19 at 19:25 -0700, Linus Torvalds wrote:
On Sat, 20 Oct 2007, Maxim Levitsky wrote:
and the interrupt handler:
smp_rmb();
if (dev-insuspend)
goto out;
Something like that can work, yes. However, you need to make sure that:
- even when
I have read this thread and I concluded few things:
1) It is impossible to know that the card won't send more interrupts:
Even if I do a read from the device, the IRQ can be pending in the bus/APIC
It is even possible (and likely) that the IRQ line will be shared, thus the
handler can be
On Saturday 20 October 2007 05:56:01 Benjamin Herrenschmidt wrote:
I have read this thread and I concluded few things:
1) It is impossible to know that the card won't send more interrupts:
Even if I do a read from the device, the IRQ can be pending in the bus/APIC
It is even possible
1) some drivers use pci_disable_device(), and pci_enable_device().
should I use it too?
I generally don't do the former, and I would expect the late to be done
by pci_restore_state() for you. pci_disable_device(), last I looked,
only cleared the bus master bit though, which might be a good
I probably need to add this synchronize_irq() logic in dmfe.c too, but I
probably do it later,
I think I am overestimating this race, since most drivers don't do
dev-insuspend checks in IRQ handler.
Maybe even just use free_irq() after all
Most drivers are probably underestimating
On Saturday 20 October 2007 07:04:35 Benjamin Herrenschmidt wrote:
1) some drivers use pci_disable_device(), and pci_enable_device().
should I use it too?
I generally don't do the former, and I would expect the late to be done
by pci_restore_state() for you. pci_disable_device(), last I
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
Note that some kind of read barrier or compiler barrier should be needed
regardless, or we are just not sync'ing with anything at all (we may
have loaded the value ages ago and thus operate on a totally stale
value). I prefer a full barrier to
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
Take a real life example:
drivers/message/fusion/mptbase.c
/* Disable interrupts! */
CHIPREG_WRITE32(ioc-chip-IntMask, 0x);
ioc-active = 0;
synchronize_irq(pdev-irq);
And we aren't in a spinlock
On Thu, 2007-10-18 at 22:35 +0800, Herbert Xu wrote:
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
Note that some kind of read barrier or compiler barrier should be needed
regardless, or we are just not sync'ing with anything at all (we may
have loaded the value ages ago and thus
On Thu, 2007-10-18 at 22:56 +0800, Herbert Xu wrote:
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
Take a real life example:
drivers/message/fusion/mptbase.c
/* Disable interrupts! */
CHIPREG_WRITE32(ioc-chip-IntMask, 0x);
ioc-active = 0;
On Fri, 2007-10-19 at 12:48 +0800, Herbert Xu wrote:
[IRQ]: Fix synchronize_irq races with IRQ handler
As it is some callers of synchronize_irq rely on memory barriers
to provide synchronisation against the IRQ handlers. For example,
the tg3 driver does
tp-irq_sync = 1;
What may happen is that action can either float upwards to give
spin_lock
action
set IRQ_INPROGRESS
spin_unlock
spin_lock
clear IRQ_INPROGRESS
spin_unlock
or it can float downwards to give
spin_lock
set IRQ_INPROGRESS
On Fri, 2007-10-19 at 12:20 +0800, Herbert Xu wrote:
That's why I think this patch is in fact the only one that
solves all the races in this thread. The case that it solves
which the lock/unlock patch does not is the one where action
flows downwards past the clearing of IRQ_INPROGRESS. I
The whole lock/set IRQ_INPROGRESS/unlock path can then only happen
before the locked section above, in which case we see and wait nicely
and all is good, or after, in which case the store to foo will be
visible to the IRQ handler as it will be ordered with the unlock in the
code above.
Note
On Thu, Oct 18, 2007 at 08:26:45PM -0700, Linus Torvalds wrote:
On Thu, 18 Oct 2007, Linus Torvalds wrote:
I *think* it should work with something like
for (;;) {
smp_rmb();
if (!spin_is_locked(desc-lock)) {
smp_rmb();
repeat:
/* Optimistic, no-locking loop */
while (desc-status IRQ_INPROGRESS)
cpu_relax();
/* Ok, that indicated we're done: double-check carefully */
spin_lock_irqsave(desc-lock, flags);
Nick Piggin [EMAIL PROTECTED] wrote:
First of all let's agree on some basic assumptions:
* A pair of spin lock/unlock subsumes the effect of a full mb.
Not unless you mean a pair of spin lock/unlock as in
2 spin lock/unlock pairs (4 operations).
*X = 10;
spin_lock(lock);
/* *Y
On Thu, 18 Oct 2007, Linus Torvalds wrote:
I *think* it should work with something like
for (;;) {
smp_rmb();
if (!spin_is_locked(desc-lock)) {
smp_rmb();
if (!(desc-status IRQ_INPROGRESS)
On Fri, 19 Oct 2007, Herbert Xu wrote:
In other words I think this patch is great :)
Hey, I appreciate it, but I do think that the spinlock only to
immediately unlock it again is pretty horrid.
I'm convinced that there should be some way to do this without actually
taking the lock. I
On Thu, Oct 18, 2007 at 04:39:59PM -0700, Linus Torvalds wrote:
Basic notion: the only thing that serializes the IRQ_INPROGRESS flag is
the descriptor lock. And we don't have to (or even want to!) hold it while
waiting for the thing, but we want to *have*held*it* in between whatever
we're
So how about something like this (untested! not necessarily very well
thought through either!)
Basic notion: the only thing that serializes the IRQ_INPROGRESS flag is
the descriptor lock. And we don't have to (or even want to!) hold it while
waiting for the thing, but we want to
On Fri, 19 Oct 2007, Benjamin Herrenschmidt wrote:
I agree and you can see that in fact, we don't have enough barrier on
the other side since spin_unlock doesn't prevent subsequent loads from
crossing a previous store...
I wonder if that's worth trying to address, adding a barrier in
On Thu, 2007-10-18 at 15:52 -0700, Linus Torvalds wrote:
On Fri, 19 Oct 2007, Benjamin Herrenschmidt wrote:
The barrier would guarantee that ioc-active (and in fact the write to
the chip too above) are globally visible
No, it doesn't really guarantee that.
The thing is, there is
On Fri, 19 Oct 2007, Benjamin Herrenschmidt wrote:
The barrier would guarantee that ioc-active (and in fact the write to
the chip too above) are globally visible
No, it doesn't really guarantee that.
The thing is, there is no such thing as globally visible.
There is a ordering of
On Friday 19 October 2007 12:32, Herbert Xu wrote:
First of all let's agree on some basic assumptions:
* A pair of spin lock/unlock subsumes the effect of a full mb.
Not unless you mean a pair of spin lock/unlock as in
2 spin lock/unlock pairs (4 operations).
*X = 10;
spin_lock(lock);
/* *Y
On Friday 19 October 2007 13:28, Herbert Xu wrote:
Nick Piggin [EMAIL PROTECTED] wrote:
First of all let's agree on some basic assumptions:
* A pair of spin lock/unlock subsumes the effect of a full mb.
Not unless you mean a pair of spin lock/unlock as in
2 spin lock/unlock pairs (4
synchronize_irq needs at the very least a compiler barrier and a
read barrier on SMP, but there are enough cases around where a
write barrier is also needed and it's not a hot path so I prefer
using a full smp_mb() here.
It will degrade to a compiler barrier on !SMP.
Signed-off-by: Benjamin
On Thu, 18 Oct 2007 11:25:42 +1000
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
synchronize_irq needs at the very least a compiler barrier and a
read barrier on SMP,
Why?
but there are enough cases around where a
write barrier is also needed and it's not a hot path so I prefer
using a
Index: linux-work/kernel/irq/manage.c
===
--- linux-work.orig/kernel/irq/manage.c 2007-10-18 11:22:16.0
+1000
+++ linux-work/kernel/irq/manage.c 2007-10-18 11:22:20.0 +1000
@@ -33,6 +33,7 @@ void
On Thu, 18 Oct 2007, Benjamin Herrenschmidt wrote:
+ smp_mb();
while (desc-status IRQ_INPROGRESS)
cpu_relax();
So, what exactly does it protect against? At a minimum, this needs a
comment in the changelog, and probably preferably in the source code too.
The
On Wed, 2007-10-17 at 19:12 -0700, Linus Torvalds wrote:
So, what exactly does it protect against? At a minimum, this needs a
comment in the changelog, and probably preferably in the source code too.
I replied to Andrew, but I agree, it's worth a comment, I'll add one.
The thing is,
In general, I tend to think that for this function to make any sense
(that is, to synchronize anything at all), it needs a barrier or you are
just making a decision based on a totally random value of desc-status
since it can have been re-ordered, speculatively loaded, pre-fetched,
40 matches
Mail list logo
