Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[Alan Stern]

On Mon, 14 Jun 2004, David Brownell wrote:

> Seems like the dma_alloc_coherent() API spec can't be
> implemented on such machines then, since it's defined
> to return memory(*) such that:
> 
>    ... a write by either the device or the processor
>    can immediately be read by the processor or device
>    without having to worry about caching effects.
> 
> Seems like the documentation should change to explain
> under what circumstances "coherent" memory will exhibit
> cache-incoherent behavior, and how to cope with that.
> (Then lots of drivers would need to change.)
> 
> OR ... maybe the bug is just that those PPC processors
> can't/shouldn't claim to implement that API.  At which
> point all drivers relying on that API (including all
> the USB HCDs and many of the USB drivers) stop working.
> 
> - Dave
>
> (*) DMA-API.txt uses two terms for this:  "coherent" and "consistent".
>      DMA-mapping.txt only uses "consistent".

That text strikes me as rather ambiguous.  Maybe it's intended to mean
that a write by either side can be read immediately by the other side, and
the values read will be the ones written (i.e., the read won't get stale
data from some cache).  It doesn't specify what happens to the other data
bytes in the same cache line which _weren't_ written -- maybe they'll be
messed up.

In other words, with "coherent" or "consistent" memory (there is some
technical distinction between the two terms but I don't know what it is)  
you don't have to worry about reading stale data from a cache, but you
might still have to worry about data unintentionally being overwritten
with garbage.

Clearly this is not a tremendously useful guarantee, but I guess it's 
better than nothing.

Maybe someone on linux-mm can clarify things for the rest of us.  For
anyone interested, this thread started with:

http://marc.theaimsgroup.com/?l=linux-usb-devel&m=108728413809788&w=2

Alan Stern


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[David Brownell]

Alan Stern wrote:
> On Mon, 14 Jun 2004, David Brownell wrote:
> 
> 
>>Seems like the dma_alloc_coherent() API spec can't be
>>implemented on such machines then, since it's defined
>>to return memory(*) such that:
>>
>>   ... a write by either the device or the processor
>>   can immediately be read by the processor or device
>>   without having to worry about caching effects.
>>
>>...
> 
> That text strikes me as rather ambiguous.  ...
> ...  It doesn't specify what happens to the other data
> bytes in the same cache line which _weren't_ written -- maybe they'll be
> messed up.

Actually I thought it was quite explicit:  "without having
to worry about caching effects".  What you described is
clearly a caching effect:  caused by caching.  And maybe
fixable by appropriate cache-flushing, or other cache-aware
driver logic ... making it "worry about caching effects".
Like the patch from Nicolas.

Maybe what we really need is patches to make USB switch to
dma_alloc_noncoherent(), checking dma_is_consistent() to
see whether a given QH/TD/ED/iTD/sITD/FSTN/... needs to be
explicitly flushed from cpu cache before handover to HC.

- Dave



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[Duncan Sands]

> Actually I thought it was quite explicit:  "without having
> to worry about caching effects".  What you described is
> clearly a caching effect:  caused by caching.

Is it really a cache effect?  Isn't it caused by the hc writing
more bytes to memory than you expected?  Now, it so
happens that the number of bytes it writes is equal to the
cache line size (or so it seems), but isn't that irrelevant?

Ciao,

Duncan.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[Alan Stern]

On Tue, 15 Jun 2004, David Brownell wrote:

> Alan Stern wrote:
> > On Mon, 14 Jun 2004, David Brownell wrote:
> > 
> > 
> >>Seems like the dma_alloc_coherent() API spec can't be
> >>implemented on such machines then, since it's defined
> >>to return memory(*) such that:
> >>
> >>   ... a write by either the device or the processor
> >>   can immediately be read by the processor or device
> >>   without having to worry about caching effects.
> >>
> >>...
> > 
> > That text strikes me as rather ambiguous.  ...
> > ...  It doesn't specify what happens to the other data
> > bytes in the same cache line which _weren't_ written -- maybe they'll be
> > messed up.
> 
> Actually I thought it was quite explicit:  "without having
> to worry about caching effects".  What you described is
> clearly a caching effect:  caused by caching.  And maybe
> fixable by appropriate cache-flushing, or other cache-aware
> driver logic ... making it "worry about caching effects".
> Like the patch from Nicolas.

No, you misunderstood what I wrote and misinterpreted the text.  The text 
says:

	... a write by either the device or the processor
	can immediately be read by the processor or device 
	without having to worry about caching effects.

This means that when you read _the data that was written_ you don't have 
to worry about caching effects.

This does _not_ mean that when you read _other data stored nearby_ you
don't have to worry about caching effects.

That is the problem Nicolas wants to solve.  According to him, the problem 
occurs not when reading the data stored by the device but when reading 
data stored nearby by the CPU and mangled during the DMA transfer.

Now maybe the intention behind "consistent" or "coherent" mappings is that 
the memory really will behave as one would naively expect, and a write to 
one portion of a cache line won't mess up the contents of the remainder.  
But the text doesn't say this.  That's why I said it is ambiguous.

> Maybe what we really need is patches to make USB switch to
> dma_alloc_noncoherent(), checking dma_is_consistent() to
> see whether a given QH/TD/ED/iTD/sITD/FSTN/... needs to be
> explicitly flushed from cpu cache before handover to HC.

Will flushing the CPU cache really solve these problems?  If the hardware
that handles the DMA transfer always writes an entire cache line, and if
it doesn't read the old contents before doing a partial write, then data
stored in the same cache line as a DMA buffer is subject to overwriting
whether it has been flushed or not.

Alan Stern

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[David Brownell]

>>>That text strikes me as rather ambiguous.  ...
>>>...  It doesn't specify what happens to the other data
>>>bytes in the same cache line which _weren't_ written -- maybe they'll be
>>>messed up.
>>
>>Actually I thought it was quite explicit:  "without having
>>to worry about caching effects".  What you described is
>>clearly a caching effect:  caused by caching.  And maybe
>>fixable by appropriate cache-flushing, or other cache-aware
>>driver logic ... making it "worry about caching effects".
>>Like the patch from Nicolas.
> 
> 
> No, you misunderstood what I wrote and misinterpreted the text.  The text 
> says:
> 
> 	... a write by either the device or the processor
> 	can immediately be read by the processor or device 
> 	without having to worry about caching effects.
> 
> This means that when you read _the data that was written_ you don't have 
> to worry about caching effects.

It doesn't limit the "without having to worry" to just the bytes
written.  And the rest of that API spec doesn't even suggest that
there might be an issue there.  I think you're trying to read
things into that text that aren't there.

On the other hand, see dma_alloc_noncoherent() ... I hope you'll
agree that the specification for "noncoherent" memory clearly
expects those caching effects to exist.  It specifically says
that you need to understand cache line sharing issues to use the
API correctly, and even exposes the cache line size.

It seems most likely to me that this particular PPC hardware
can't implement dma_alloc_coherent(), and the code implementing
that routine should be called dma_alloc_noncoherent() instead.



>>Maybe what we really need is patches to make USB switch to
>>dma_alloc_noncoherent(), checking dma_is_consistent() to
>>see whether a given QH/TD/ED/iTD/sITD/FSTN/... needs to be
>>explicitly flushed from cpu cache before handover to HC.
> 
> 
> Will flushing the CPU cache really solve these problems?  If the hardware

That's why we need PPC expertise applied here.  Nicolas didn't
seem to have answers for all the questions I was asking.


> that handles the DMA transfer always writes an entire cache line, and if
> it doesn't read the old contents before doing a partial write, then data
> stored in the same cache line as a DMA buffer is subject to overwriting
> whether it has been flushed or not.

Addressed by using dma_alloc_noncoherent() and dma_cache_sync(); yes?

- Dave




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[Alan Stern]

On Tue, 15 Jun 2004, David Brownell wrote:

> > 	... a write by either the device or the processor
> > 	can immediately be read by the processor or device 
> > 	without having to worry about caching effects.
> > 
> > This means that when you read _the data that was written_ you don't have 
> > to worry about caching effects.
> 
> It doesn't limit the "without having to worry" to just the bytes
> written.

It's hard to say exactly what it means because it's ungrammatical.  "A 
write ... can immediately be read..." -- what does it mean to read a 
write?

>  And the rest of that API spec doesn't even suggest that
> there might be an issue there.  I think you're trying to read
> things into that text that aren't there.

Maybe so.  It's not worth spending more time on this point, anyhow.

Alan Stern

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [linux-usb-devel] Patch for UHCI driver (from kernel 2.6.6).

[Guennadi Liakhovetski]

On Tue, 15 Jun 2004, Alan Stern wrote:

> On Mon, 14 Jun 2004, David Brownell wrote:
>
>> Seems like the dma_alloc_coherent() API spec can't be
>> implemented on such machines then, since it's defined
>> to return memory(*) such that:
>>
>>    ... a write by either the device or the processor
>>    can immediately be read by the processor or device
>>    without having to worry about caching effects.
>>
>> Seems like the documentation should change to explain
>> under what circumstances "coherent" memory will exhibit
>> cache-incoherent behavior, and how to cope with that.
>> (Then lots of drivers would need to change.)
>>
>> OR ... maybe the bug is just that those PPC processors
>> can't/shouldn't claim to implement that API.  At which
>> point all drivers relying on that API (including all
>> the USB HCDs and many of the USB drivers) stop working.
>>
>> - Dave
>>
>> (*) DMA-API.txt uses two terms for this:  "coherent" and "consistent".
>>      DMA-mapping.txt only uses "consistent".
>
> That text strikes me as rather ambiguous.  Maybe it's intended to mean
> that a write by either side can be read immediately by the other side, and
> the values read will be the ones written (i.e., the read won't get stale
> data from some cache).  It doesn't specify what happens to the other data
> bytes in the same cache line which _weren't_ written -- maybe they'll be
> messed up.
>
> In other words, with "coherent" or "consistent" memory (there is some
> technical distinction between the two terms but I don't know what it is)

<quote>

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Sven Luther]

On Thu, Jun 17, 2004 at 07:34:54PM +0200, Sven Luther wrote:
> On Mon, Jun 14, 2004 at 08:38:52PM +0100, Alan Cox wrote:
> > On Llu, 2004-06-14 at 16:12, Nicolas DET wrote:
> > > Also, we replace spin_lock_irqxxx/spin_unlock_irq by spin_lock/unlock
> > > (for SMP/PREEMPT kernel) + stop/start_interrupt.
> >
> > This requires a lot of care to do right. Remember that on PC systems
> > interrupts can be substantially posted. A "stop_interrupt" may prevent
> > IRQ issue but if an IRQ is already on the PC APIC bus it will kill you
> > later on because the IRQ delivery and PCI bus access on the PC class
> > machines are totally asynchronous
>
> And what about the other part of the patch ? The one about the
> alignement of the buffer descriptors ? Does it seem sound, should we
> ward it with ppc specific stuff ? Is it fit to be included in the main
> kernel ?

Mmm, it seems that inversing the remove_list and the list also makes the
problem go away, or at least be less important, in :

struct uhci_td {
        /* Hardware fields */
        __u32 link;
        __u32 status;
        __u32 token;
        __u32 buffer;

        /* Software fields */
        dma_addr_t dma_handle;

        struct usb_device *dev;
        struct urb *urb;

        struct list_head list;          /* P: urb->lock */
        struct list_head remove_list;   /* P: uhci->td_remove_list_lock */

        int frame;                      /* for iso: what frame? */
        struct list_head fl_list;       /* P: uhci->frame_list_lock */
} __attribute__((aligned(16)));

Could it be possible that the warning in :

/*
 * The documentation says "4 words for hardware, 4 words for software".
 *
 * That's silly, the hardware doesn't care. The hardware only cares that
 * the hardware words are 16-byte aligned, and we can have any amount of
 * sw space after the TD entry as far as I can tell.

be still important, at least on G4 powerpc ? remove_list is the 5th
software word here. Strange, because supposedly it works on x86 and
other uhci using arches, and it works on the G3, but not on the G4.

Friendly,

Sven Luther

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Benjamin Herrenschmidt]

> be still important, at least on G4 powerpc ? remove_list is the 5th
> software word here. Strange, because supposedly it works on x86 and
> other uhci using arches, and it works on the G3, but not on the G4.

I think it's more likely we are dealing with an ordering issue of
accesses to memory vs. mmio, those aren't order unless you use memory
barriers. Actually, it's worse, you need a full mb() to order them,
which is why lately, we made ppc64 writeX() do full sync's ... that sucks
but it's near to impossible to get an abstract IO API that would cover
our needs here and still make other archs happy it seems...

Ben.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Alan Stern]

On Fri, 18 Jun 2004, Benjamin Herrenschmidt wrote:

> I think it's more likely we are dealing with an ordering issue of
> accesses to memory vs. mmio, those aren't order unless you use memory
> barriers. Actually, it's worse, you need a full mb() to order them,
> which is why lately, we made ppc64 writeX() do full sync's ... that sucks
> but it's near to impossible to get an abstract IO API that would cover
> our needs here and still make other archs happy it seems...

I recently changed a few mb() calls to wmb(), because they only protected
data the CPU was writing to be read by the device.  Do you think changing
all the wmb()'s back to mb()'s would make a difference?

(Actually it seems likely that this is _not_ directly related to the
original problem, but it might be important anyway.)

Alan Stern


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Benjamin Herrenschmidt]

On Fri, 2004-06-18 at 16:41, Alan Stern wrote:
> On Fri, 18 Jun 2004, Benjamin Herrenschmidt wrote:
>
> > I think it's more likely we are dealing with an ordering issue of
> > accesses to memory vs. mmio, those aren't order unless you use memory
> > barriers. Actually, it's worse, you need a full mb() to order them,
> > which is why lately, we made ppc64 writeX() do full sync's ... that sucks
> > but it's near to impossible to get an abstract IO API that would cover
> > our needs here and still make other archs happy it seems...
>
> I recently changed a few mb() calls to wmb(), because they only protected
> data the CPU was writing to be read by the device.  Do you think changing
> all the wmb()'s back to mb()'s would make a difference?
>
> (Actually it seems likely that this is _not_ directly related to the
> original problem, but it might be important anyway.)

Well, the problem on ppc is that the eieio done by wmb() (or implicitely
done by all writeX IO accessors) will only order stores in the same
domain. That is cacheable aren't ordered vs. non cacheables.

For example, write to a descriptor in memory, then writel() to your
device, that isn't guaranteed to happen in order.

In this case, you indeed need an mb(), but that's a ppc thing and the
race on ppc32 CPUs is quite small (though ppc64, typically POWER4 and
POWER5, will eat you for lunch with their multiple deep store queues).

Ben


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Alan Stern]

On Fri, 18 Jun 2004, Benjamin Herrenschmidt wrote:

> > I recently changed a few mb() calls to wmb(), because they only protected
> > data the CPU was writing to be read by the device.  Do you think changing
> > all the wmb()'s back to mb()'s would make a difference?
> >
> > (Actually it seems likely that this is _not_ directly related to the
> > original problem, but it might be important anyway.)
>
> Well, the problem on ppc is that the eieio done by wmb() (or implicitely
> done by all writeX IO accessors) will only order stores in the same
> domain. That is cacheable aren't ordered vs. non cacheables.

I'm not familiar with the term "eieio"; can you explain it?

> For example, write to a descriptor in memory, then writel() to your
> device, that isn't guaranteed to happen in order.

Then it shouldn't matter for what I'm doing, which involves multiple
writes to the same region of "consistent" memory.  Or maybe it's only
"coherent" memory -- the important thing is that the first write must
complete and be visible to the device's DMA before the second.  wmb()
will suffice for that, right?

> In this case, you indeed need an mb(), but that's a ppc thing and the
> race on ppc32 CPUs is quite small (though ppc64, typically POWER4 and
> POWER5, will eat you for lunch with their multiple deep store queues).

Thanks,

Alan Stern


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Sven Luther]

On Sat, Jun 19, 2004 at 09:59:53AM -0400, Alan Stern wrote:
> On Fri, 18 Jun 2004, Benjamin Herrenschmidt wrote:
>
> > > I recently changed a few mb() calls to wmb(), because they only protected
> > > data the CPU was writing to be read by the device.  Do you think changing
> > > all the wmb()'s back to mb()'s would make a difference?
> > >
> > > (Actually it seems likely that this is _not_ directly related to the
> > > original problem, but it might be important anyway.)
> >
> > Well, the problem on ppc is that the eieio done by wmb() (or implicitely
> > done by all writeX IO accessors) will only order stores in the same
> > domain. That is cacheable aren't ordered vs. non cacheables.
>
> I'm not familiar with the term "eieio"; can you explain it?

Enforced In-Order Execution of I/O. Page 8-61 of the Programing
Environments for 32-Bit Microprocessors, by motorola, well, at least on
my version.

Friendly,

Sven Luther

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Segher Boessenkool]

> Enforced In-Order Execution of I/O. Page 8-61 of the Programing
> Environments for 32-Bit Microprocessors, by motorola, well, at least on
> my version.

Don't use that old stuff; use either the new PEM or the Book I/II/III:

PEM (64- and 32-bit) v2.0:
http://www-3.ibm.com/chips/techlib/techlib.nsf/techdocs/
F6153E213FDD912E87256D49006C6541/$file/pem._64bit.d20030611.pdf

PowerPC Book I/II/III v2.01:
www-106.ibm.com/developerworks/eserver/pdfs/archpub1.pdf
www-106.ibm.com/developerworks/eserver/pdfs/archpub2.pdf
www-106.ibm.com/developerworks/eserver/pdfs/archpub3.pdf

Most of the time the Books are better; sometimes the PEM helps
as well.

Have fun,


Segher


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Sven Luther]

On Mon, Jun 21, 2004 at 10:18:40AM +0200, Segher Boessenkool wrote:
> >Enforced In-Order Execution of I/O. Page 8-61 of the Programing
> >Environments for 32-Bit Microprocessors, by motorola, well, at least on
> >my version.
>
> Don't use that old stuff; use either the new PEM or the Book I/II/III:

Well, the nice thing about the old stuff like you said, is that i got it
in nice paper book format, and not a bunch of .pdfs, which Motorola used
to ship for free back in the days. But sure, i will look at those new
documents.

> PEM (64- and 32-bit) v2.0:
> http://www-3.ibm.com/chips/techlib/techlib.nsf/techdocs/
> F6153E213FDD912E87256D49006C6541/$file/pem._64bit.d20030611.pdf
>
> PowerPC Book I/II/III v2.01:
> www-106.ibm.com/developerworks/eserver/pdfs/archpub1.pdf
> www-106.ibm.com/developerworks/eserver/pdfs/archpub2.pdf
> www-106.ibm.com/developerworks/eserver/pdfs/archpub3.pdf
>
> Most of the time the Books are better; sometimes the PEM helps
> as well.

Friendly,

Sven Luther

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [linux-usb-devel] [Patch] for UHCI driver (from kernel 2.6.6).

[Oliver Neukum]

Am Freitag, 18. Juni 2004 23:41 schrieb Alan Stern:
> > barriers. Actually, it's worse, you need a full mb() to order them,
> > which is why lately, we made ppc64 writeX() do full sync's ... that sucks
> > but it's near to impossible to get an abstract IO API that would cover
> > our needs here and still make other archs happy it seems...
>
> I recently changed a few mb() calls to wmb(), because they only protected
> data the CPU was writing to be read by the device.  Do you think changing
> all the wmb()'s back to mb()'s would make a difference?

Are these issues documented somewhere?

	Regards
		Oliver

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/