.../...
> 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. Herber
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;
> synchronize_irq(pci_
On Saturday 20 October 2007 07:46:24 Benjamin Herrenschmidt wrote:
>
> > 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 e
> 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 underestimati
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(),
> 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 id
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
> > - 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 b
> > - 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
>
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:
>
> 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 ca
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 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:
>
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,
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.
>
On Fri, Oct 19, 2007 at 02:26:54PM +1000, Benjamin Herrenschmidt wrote:
>
> I think a simple smp_mb(); here after foo = 1; is enough, which means
> basically just having an smp_mp(); inside napi_synchronize(), before
> the test_bit(). Or do I miss something ?
Yes I think you're right. In this cas
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;
>
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/
On Fri, Oct 19, 2007 at 12:20:25PM +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_INPROG
> 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_INP
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.
> 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.
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_
>
> 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)
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 *t
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);
/*
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
> 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 *h
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 i
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
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 vi
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);
> >
> >
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
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
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 barri
>
> 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,
> wh
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, sy
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
> > 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
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
> usi
42 matches
Mail list logo
