* Re: Documentation/io_ordering.txt is wrong
2004-09-18 17:57 ` Documentation/io_ordering.txt is wrong Matthew Wilcox
@ 2004-09-20 23:39 ` Jesse Barnes
2004-09-21 0:38 ` Jesse Barnes
1 sibling, 0 replies; 3+ messages in thread
From: Jesse Barnes @ 2004-09-20 23:39 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Grant Grundler, Andrew Vasquez, pj, linux-scsi, mdr, jeremy, djh,
Andrew Morton, linux-kernel
On Saturday, September 18, 2004 10:57 am, Matthew Wilcox wrote:
> On Sat, Sep 18, 2004 at 12:10:01AM -0600, Grant Grundler wrote:
> > Jesse Barnes wrote:
> > ...
> >
> > > Btw Andrew (Vasquez), there's a small doc I put together that should
> > > describe when you have to worry about PCI posting. It's in the tree:
> > > Documentation/io_ordering.txt. If it's incomplete or confusing, just
> > > let me know and I'll update it.
> >
> > Jesse,
> > Both. incomplete and confusing.
> > "concrete example of a hypothetical driver" wasn't my first warning
> > this document needed work. :^)
>
> Not just incomplete and confusing, but actively wrong. spin_lock/
> spin_unlock should imply ordering of readb(). What you're describing
> there is __readb(). See Documentation/DocBook/deviceiobook.tmpl. Also,
> rmb() should ensure ordering of io reads; there should be no need to
> touch the device.
I already sent a reply to this, so you now know that I was wrong in describing
the ordering issue as one of weak ordering, rather it's just supposed to be
describing simple write posting. Also, the first sample code is incorrect,
since the second spinlock protected region has a read before a write and
incorrectly states that the second write may arrive before the first. I
think Grant has some updates, if not I'll post a patch to fixup the doc.
Thanks,
Jesse
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Documentation/io_ordering.txt is wrong
2004-09-18 17:57 ` Documentation/io_ordering.txt is wrong Matthew Wilcox
2004-09-20 23:39 ` Jesse Barnes
@ 2004-09-21 0:38 ` Jesse Barnes
1 sibling, 0 replies; 3+ messages in thread
From: Jesse Barnes @ 2004-09-21 0:38 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Grant Grundler, Andrew Vasquez, pj, linux-scsi, mdr, jeremy, djh,
Andrew Morton, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On Saturday, September 18, 2004 10:57 am, Matthew Wilcox wrote:
> On Sat, Sep 18, 2004 at 12:10:01AM -0600, Grant Grundler wrote:
> > Jesse Barnes wrote:
> > ...
> >
> > > Btw Andrew (Vasquez), there's a small doc I put together that should
> > > describe when you have to worry about PCI posting. It's in the tree:
> > > Documentation/io_ordering.txt. If it's incomplete or confusing, just
> > > let me know and I'll update it.
> >
> > Jesse,
> > Both. incomplete and confusing.
> > "concrete example of a hypothetical driver" wasn't my first warning
> > this document needed work. :^)
>
> Not just incomplete and confusing, but actively wrong. spin_lock/
> spin_unlock should imply ordering of readb(). What you're describing
> there is __readb(). See Documentation/DocBook/deviceiobook.tmpl. Also,
> rmb() should ensure ordering of io reads; there should be no need to
> touch the device.
Is this any better? I just sent it off to Grant too.
Thanks,
Jesse
[-- Attachment #2: io_ordering.txt --]
[-- Type: text/plain, Size: 2098 bytes --]
Dealing with posted writes
--------------------------
On some platforms platforms, driver writers are responsible for
ensuring that I/O writes to memory-mapped addresses on their device
arrive in the order intended. This is typically done by reading a
'safe' device or bridge register, causing the I/O chipset to flush
pending writes to the device before any reads are posted. A driver
would usually use this technique immediately prior to the exit of a
critical section of code protected by spinlocks. This would ensure
that subsequent writes to I/O space arrived only after all prior
writes (much like a memory barrier op, mb(), only with respect to
I/O).
Some pseudocode to illustrate the problem:
...
CPU A: spin_lock_irqsave(&dev_lock, flags)
CPU A: ...
CPU A: writel(newval, ring_ptr);
CPU A: spin_unlock_irqrestore(&dev_lock, flags)
...
CPU B: spin_lock_irqsave(&dev_lock, flags)
CPU B: ...
CPU B: writel(newval2, ring_ptr);
CPU B: spin_unlock_irqrestore(&dev_lock, flags)
...
In the case above, the device may receive newval2 before it receives newval,
which could cause problems. Fixing it is easy enough though:
...
CPU A: spin_lock_irqsave(&dev_lock, flags)
CPU A: ...
CPU A: writel(newval, ring_ptr);
CPU A: (void)readl(safe_register); /* maybe a config register? */
CPU A: spin_unlock_irqrestore(&dev_lock, flags)
...
CPU B: spin_lock_irqsave(&dev_lock, flags)
CPU B: ...
CPU B: writel(newval2, ring_ptr);
CPU B: (void)readl(safe_register); /* or read_relaxed() */
CPU B: spin_unlock_irqrestore(&dev_lock, flags)
Here, the reads from safe_register will cause the I/O chipset to flush any
pending writes before actually posting the read to the chipset, preventing
possible data corruption.
This sort of synchronization is only necessary for read/write calls,
not in/out calls, since they're by definition strongly ordered.
We should probably add a writeflush call or something to deal with the
above in an easier to read way. Some platforms could even implement
such a routine more efficiently than a regular read.
^ permalink raw reply [flat|nested] 3+ messages in thread