Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Andy Lutomirski]

Paulo Marques wrote:

> Andy Lutomirski wrote:
> 
>> Greg KH wrote:
>>
>>>
>>> Paulo Marques:
>>>   o USB: fix usblp.c
>>>
>>
>> Unless I'm missing something, this won't fix the usblp_write spinning 
>> bug I hit.  If it helps, I can try to reproduce it with some debugging 
>> code attached.
>>
> 
> 

I added some printks to usblp_write on 2.6.1 vanilla and I couldn't reproduce 
the bug (even without your patch).  So, unless I can trigger it again, I won't 
worry about it.

Thanks.

--Andy

Re: [BK PATCH] USB update for 2.6.3

[Andy Lutomirski]

Greg KH wrote:

> 
> Paulo Marques:
>   o USB: fix usblp.c
> 

Unless I'm missing something, this won't fix the usblp_write spinning bug I hit. 
  If it helps, I can try to reproduce it with some debugging code attached.

BTW, I'm posting from a different address, because luto@myrealbox.com seems 
unable to post to lists.sourceforge.net.  Please CC me in replies at 
luto@myrealbox.com.

--Andy

---- Original message copied for linux-usb-devel readers ----

I recently cancelled a print job with the printer's cancel function, and the 
CUPS backend got stuck in usblp_write (using 100% CPU and not responding to 
signals).

The printer is a Kyocera Mita FS-1900 (which has some other problems with the 
linux USB code: usblp_check_status often times out, even though the printer is 
bidirectional -- but that's a whole different issue).

It looks like the problem is that the write failed, so wcomplete got set
to 1 and the status is nonzero.  This case appears to be mishandled in usblp.c:

         while (writecount < count) {
                 if (!usblp->wcomplete) {
             [... not reaching this code]
                 }

                 down (&usblp->sem);
                 if (!usblp->present) {
                         up (&usblp->sem);
                         return -ENODEV;
                 }
                 if (usblp->writeurb->status != 0) {

             [ check status? ]

                         schedule ();
                         continue; <-- problem
                 }

After the write fails, the current code keeps checking the same status, and 
never checks for signals or fails.  I'm not sure what the right fix is, but it 
might be something like this:

--- ./usblp.c.orig      2004-02-15 06:27:29.176169752 -0800
+++ ./usblp.c   2004-02-15 06:29:40.137260640 -0800
@@ -645,13 +645,11 @@
                                 err = usblp->writeurb->status;
                         } else
                                 err = usblp_check_status(usblp, err);
-                       up (&usblp->sem);

-                       /* if the fault was due to disconnect, let khubd's
-                        * call to usblp_disconnect() grab usblp->sem ...
-                        */
-                       schedule ();
-                       continue;
+                       writecount += usblp->writeurb->transfer_buffer_length;
+                       up (&usblp->sem);
+                       count = writecount ? writecount : err;
+                       break;
                 }

                 writecount += usblp->writeurb->transfer_buffer_length;

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Paulo Marques]

Andy Lutomirski wrote:

> Greg KH wrote:
> 
>>
>> Paulo Marques:
>>   o USB: fix usblp.c
>>
> 
> Unless I'm missing something, this won't fix the usblp_write spinning 
> bug I hit.  If it helps, I can try to reproduce it with some debugging 
> code attached.
> 


That is why on our latest thread about this I requested that you tested the 
patch to check your bug went way.

My patch *does* correct *a* bug. I tested it myself because I could trigger the 
bug easily, and after the patch the bug was gone.

I just wanted you to test if our bugs were different, or on the contrary, they 
are in fact the same.

They could be the same, because in my case, the driver would hang sending 
garbage to the printer. Because the printer was a dot-matrix "print what it 
receives" kind of printer, it would output the garbage continously. If it were a 
more "inteligent" printer it might refuse the garbage and not print anything at 
all, giving the same result as if nothing was being printed.

Anyway, if the bugs are different, then yes, another patch is needed to fix 
"your" bug :)

You will be the best person to do it, since you can trigger the bug, and test a 
before / after scenario.

-- 
Paulo Marques - www.grupopie.com

"In a world without walls and fences who needs windows and gates?"

[BK PATCH] USB update for 2.6.3

[Greg KH]

Hi,

Here are some USB patches for 2.6.3.  Here are the main types of changes:
	- switch usb code to use dmapools instead of pcipools which
	  makes the embedded people happy.
	- host controller driver updates for EHCI and UHCI drivers
	- usb storage driver updates.
	- a few other minor bug fixes.

Please pull from:  bk://kernel.bkbits.net/gregkh/linux/usb-2.6

Patches will be posted to linux-usb-devel as a follow-up thread for
those who want to see them.

thanks,

greg k-h

 drivers/usb/class/usblp.c          |   20 -
 drivers/usb/core/buffer.c          |   23 -
 drivers/usb/core/hcd-pci.c         |   31 +
 drivers/usb/core/hcd.c             |   38 +-
 drivers/usb/core/hcd.h             |    7 
 drivers/usb/core/hub.c             |    9 
 drivers/usb/core/message.c         |   26 +
 drivers/usb/core/usb.c             |   13 
 drivers/usb/host/ehci-dbg.c        |   24 -
 drivers/usb/host/ehci-hcd.c        |   18 -
 drivers/usb/host/ehci-hub.c        |    2 
 drivers/usb/host/ehci-mem.c        |   40 +-
 drivers/usb/host/ehci-q.c          |    2 
 drivers/usb/host/ehci-sched.c      |  592 ++++++++++++++++++++++++-------------
 drivers/usb/host/ehci.h            |   80 +++--
 drivers/usb/host/ohci-hcd.c        |    8 
 drivers/usb/host/ohci-mem.c        |   20 -
 drivers/usb/host/ohci-omap.c       |   12 
 drivers/usb/host/ohci-pci.c        |   45 +-
 drivers/usb/host/ohci-q.c          |    4 
 drivers/usb/host/ohci-sa1111.c     |   16 -
 drivers/usb/host/ohci.h            |   12 
 drivers/usb/host/uhci-debug.c      |   35 +-
 drivers/usb/host/uhci-hcd.c        |  386 +++++++++++-------------
 drivers/usb/host/uhci-hcd.h        |   30 +
 drivers/usb/host/uhci-hub.c        |  157 +++++----
 drivers/usb/media/stv680.c         |   16 -
 drivers/usb/misc/usbtest.c         |   19 +
 drivers/usb/serial/ftdi_sio.c      |   13 
 drivers/usb/serial/ftdi_sio.h      |    4 
 drivers/usb/storage/sddr09.c       |   31 +
 drivers/usb/storage/transport.c    |   78 ++--
 drivers/usb/storage/unusual_devs.h |   86 +++--
 33 files changed, 1111 insertions(+), 786 deletions(-)
-----

Alan Stern:
  o USB: Use driver-model logging in the UHCI driver
  o USB: Repair unusual_devs.h entry
  o USB: Another unusual_devs.h update
  o USB: More UHCI root hub code improvements
  o USB: Improve UHCI root hub code: descriptor, OC bits, etc
  o USB Storage: unusual_devs.h fixup
  o USB Storage: Reduce auto-sensing for CB transport
  o USB Storage: Treat STALL as failure for CB[I]
  o USB Storage: Handle excess 0-length data packets
  o USB: Mask "HC Halted" bit in the UHCI status register
  o USB: Simplify locking in UHCI
  o USB: ERRBUF_LEN compiling error when PAGE_SIZE=64KB on IA64
  o USB: Even out distribution of UHCI interrupt transfers
  o USB: Remove unneeded and error-provoking variable in UHCI

Andries E. Brouwer:
  o USB: add comments to sddr09.c

David Brownell:
  o USB: ehci-hcd, scheduler handles TT collisions (3/3)
  o USB: ehci-hcd, fullspeed iso data structures (2/3)
  o USB: ehci-hcd, fullspeed iso data structures (1/3)
  o USB: usbtest, two more protocol cases
  o USB: usbcore, avoid RNDIS configs
  o USB: usbcore, hub driver enables TT-per-port mode
  o USB: usbcore, scatterlist cleanups
  o USB: EHCI updates (mostly periodic schedule scanning)

Deepak Saxena:
  o USB: Fix USB host code to use generic DMA API

Domen Puncer:
  o USB: some stv680 fixes

Greg Kroah-Hartman:
  o USB storage: sync up with some missing unusual_devs entries that were in my tree
  o USB: fix up compile errors in uhci driver

Ian Abbott:
  o USB: ftdi_sio new PIDs and name fix for sysfs

Matthew Dharm:
  o USB Storage: Fix small endian-ness bug
  o USB Storage: Reduce unsolicited auto-sense
  o USB Storage: Save the SCSI residue value when auto-sensing

Paulo Marques:
  o USB: fix usblp.c

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Thu, 19 Feb 2004, Greg KH wrote:
> 
> Here are some USB patches for 2.6.3.  Here are the main types of changes:
> 	- switch usb code to use dmapools instead of pcipools which
> 	  makes the embedded people happy.

However, this makes the ppc64 people very unhappy.

I've just yesterday and today switched my main home machine to be ppc64, 
and this will not compile for me. Bad boy!

(And I haven't used ppc64 enough to be able to make sense of the DMA 
setup, so I can't even fix it myself yet. Aaarghh!)

Help me, Obi Wan.

		Linus

Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

On Fri, 2004-02-20 at 16:58, Linus Torvalds wrote:
> On Thu, 19 Feb 2004, Greg KH wrote:
> > 
> > Here are some USB patches for 2.6.3.  Here are the main types of changes:
> > 	- switch usb code to use dmapools instead of pcipools which
> > 	  makes the embedded people happy.
> 
> However, this makes the ppc64 people very unhappy.
> 
> I've just yesterday and today switched my main home machine to be ppc64, 
> and this will not compile for me. Bad boy!
> 
> (And I haven't used ppc64 enough to be able to make sense of the DMA 
> setup, so I can't even fix it myself yet. Aaarghh!)

Hehe ;)

Well, last I heard, people here were trying to make that work, but
still, our dma mapping API will probably want the device passed in
to be a pci_dev...

The problem with the generic API is that it makes it difficult
for us to actually go back to the PCI device hosting the USB
device we get passed in... Which we what we want (our IO MMU
setup is based on what PCI device is there, for PCI at least,
for VIO, it's a bit different, but the idea is the same).

A while ago, I've advertised making this API a set of function
pointers attached to the struct device inherited from the bus
parent, so the core code just set one for the root PCIs and
everybody inherits them.... But of course, since x86 isn't
affected, nobody cared ;)

I think the "generic" DMA API is just not suitable at the moment
to be really used. It gets passed "generic" struct device, which,
due to the way Pat designed it, are almost useless alone (we
can't quite match them to anything and don't have any kind of
"inheritance" of methods via function pointers). There is no
simple hook for archs to carry informations attached to struct
devices neither, etc...

Ben

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Fri, 20 Feb 2004, Benjamin Herrenschmidt wrote:
> 
> A while ago, I've advertised making this API a set of function
> pointers attached to the struct device inherited from the bus
> parent, so the core code just set one for the root PCIs and
> everybody inherits them.... But of course, since x86 isn't
> affected, nobody cared ;)

Well, in all fairness, that _is_ what "platform_data" is supposed to be. A
platform-specific pointer to whatever data structure that platform needs
to have to do the device ops. Platforms that don't need the function 
pointers wouldn't have any function pointers there, while others would 
have not just the function pointers, but could have some other 
bus-dependent data too.

(And this is what at least HP-PA does, as does ARM...)

So it's definitely not unsuitable for this. It does seem like USB jumped 
the gun a bit, though.

		Linus

Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

On Fri, 2004-02-20 at 17:30, Linus Torvalds wrote:
> On Fri, 20 Feb 2004, Benjamin Herrenschmidt wrote:
> > 
> > A while ago, I've advertised making this API a set of function
> > pointers attached to the struct device inherited from the bus
> > parent, so the core code just set one for the root PCIs and
> > everybody inherits them.... But of course, since x86 isn't
> > affected, nobody cared ;)
> 
> Well, in all fairness, that _is_ what "platform_data" is supposed to be. A
> platform-specific pointer to whatever data structure that platform needs
> to have to do the device ops. Platforms that don't need the function 
> pointers wouldn't have any function pointers there, while others would 
> have not just the function pointers, but could have some other 
> bus-dependent data too.

Yes, but we don't have a hook for actually filling this pointer, do we ?

We should have a way, when creating a device, to fill it properly, like

platform_device_setup(struct device *new_dev, struct device *parent)

That would allow me to deal with the hierarchy thing (inheriting by
default the DMA stuff from the parent typically).

Now, the question is where is the best place to call this "hook" ?

> (And this is what at least HP-PA does, as does ARM...)
> 
> So it's definitely not unsuitable for this. It does seem like USB jumped 
> the gun a bit, though.

Yes. I also remember a time where the dma mask for the DMA API was all
broken too (would not be possible to map the PCI one on top of it),
but I think that got fixed. 

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Fri, 20 Feb 2004, Benjamin Herrenschmidt wrote:
> 
> Yes, but we don't have a hook for actually filling this pointer, do we ?

Well, we do. The pcibios_xxx routines get called for all PCI devices 
during discovery, and that's when you'd fill them in.

Anyway, I found the bug - the "asm-generic/dma-mapping.h" compatibility 
macros _do_ work, but the EHCI controller driver doesn't actually include 
that header file. Oops.

> We should have a way, when creating a device, to fill it properly, like
> 
> platform_device_setup(struct device *new_dev, struct device *parent)

No no. That wouldn't work AT ALL, since the whole point is that you need 
to know what the device is - ie you need to fill in the information when 
you get the "struct pci_dev *" (because different buses would most likely 
have different behaviour, and could have different requirements for DMA 
mapping etc). 

So the platform code that actually knows what the device is (pcibios_xxx 
for PCI devices) would fill in the platform pointer.

Anyway, I got it all working with the trivial patch..

===== drivers/usb/host/ehci-hcd.c 1.67 vs edited =====
--- 1.67/drivers/usb/host/ehci-hcd.c    Wed Feb 11 03:42:39 2004
+++ edited/drivers/usb/host/ehci-hcd.c  Thu Feb 19 22:42:53 2004
@@ -41,6 +41,7 @@
 #include <linux/reboot.h>
 #include <linux/usb.h>
 #include <linux/moduleparam.h>
+#include <linux/dma-mapping.h>
 
 #include "../core/hcd.h"
 

so this works for now, even though it's really really ugly (and it _will_
BUG() out if anybody ever passes a non-PCI-related "struct device" to the
thing).

Let's see if it boots too. So far it's only compiled successfully ;)

		Linus

Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

> Well, we do. The pcibios_xxx routines get called for all PCI devices 
> during discovery, and that's when you'd fill them in.

But what about USB or FireWire devices ? In theory, I'd like to see
the driver for those not have to bother about beeing hosted by a PCI
device or whatever else (there are typically non-PCI OHCI USBs on
embedded platform, faking a pci_dev is becoming painful).

So it would actually make sense to be able to pass whatever struct
device we are on, and have a real inheritance of the DMA functions
going down the bus, don't you think ? 

> Anyway, I found the bug - the "asm-generic/dma-mapping.h" compatibility 
> macros _do_ work, but the EHCI controller driver doesn't actually include 
> that header file. Oops.

Heh.

> > We should have a way, when creating a device, to fill it properly, like
> > 
> > platform_device_setup(struct device *new_dev, struct device *parent)
> 
> No no. That wouldn't work AT ALL, since the whole point is that you need 
> to know what the device is - ie you need to fill in the information when 
> you get the "struct pci_dev *" (because different buses would most likely 
> have different behaviour, and could have different requirements for DMA 
> mapping etc). 

And ? Where is the problem ? By default, you inherit from the parent
and that's just fine. The platform sets the ops for the root PCIs and
everybody below that inherit from them, same goes for VIO. If at one
level in the hierarchy, there is a "brake" (a bridge to a different
kind of bus that has additional restrictions), then the above routine
can "know" this via the new_dev bus type and setup appropriate mapping
functions.

> So the platform code that actually knows what the device is (pcibios_xxx 
> for PCI devices) would fill in the platform pointer.

Hrm... and we keep it empty for USB, FireWire, etc... or we add
platform_usb_xxxx, platform_ieee1394_* etc... for all those cases ?

> Anyway, I got it all working with the trivial patch..
> 
> ===== drivers/usb/host/ehci-hcd.c 1.67 vs edited =====
> --- 1.67/drivers/usb/host/ehci-hcd.c    Wed Feb 11 03:42:39 2004
> +++ edited/drivers/usb/host/ehci-hcd.c  Thu Feb 19 22:42:53 2004
> @@ -41,6 +41,7 @@
>  #include <linux/reboot.h>
>  #include <linux/usb.h>
>  #include <linux/moduleparam.h>
> +#include <linux/dma-mapping.h>
>  
>  #include "../core/hcd.h"
>  
> 
> so this works for now, even though it's really really ugly (and it _will_
> BUG() out if anybody ever passes a non-PCI-related "struct device" to the
> thing).
> 
> Let's see if it boots too. So far it's only compiled successfully ;)
> 
> 		Linus
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [BK PATCH] USB update for 2.6.3

[Greg KH]

On Fri, Feb 20, 2004 at 05:42:56PM +1100, Benjamin Herrenschmidt wrote:
> 
> > Well, we do. The pcibios_xxx routines get called for all PCI devices 
> > during discovery, and that's when you'd fill them in.
> 
> But what about USB or FireWire devices ? In theory, I'd like to see
> the driver for those not have to bother about beeing hosted by a PCI
> device or whatever else (there are typically non-PCI OHCI USBs on
> embedded platform, faking a pci_dev is becoming painful).

This is the main reason this patch was done.  The arm people were
getting tired or having to do this for their USB controller drivers.
This round of patches (and the previous ones with the dmapool stuff)
removed that dependency.

As for how ARM deals with their devices on non-pci busses, I really do
not know, I never looked into that.

But for PPC64 this should not be a problem, as all of the code should
just work the same as it did before because you only have PCI based USB
controllers, right?  Odds are your header files just don't include the
same files so Linus's patch should be all that is needed.

As for the bigger "generic" dma mapping discussions for devices, hasn't
this been hashed out a bunch already?  For some reason I thought
everyone was happy for now with the way things work, and for 2.7 it was
going to be expanded a bit to help support non-pci based busses (much
like the ARM people just did.)

Hm, I wonder if I can convince anyone that I have to have a PPC64 box
now to make sure I don't break the build anytime in the future :)

thanks,

greg k-h

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[David Brownell]

Greg KH wrote:

> As for how ARM deals with their devices on non-pci busses, I really do
> not know, I never looked into that.

One way is that initialization specific to a given board or machine
is called from its INIT_MACHINE callback.  That can define platform
devices (SOC, ASICs, discrete parts, etc) and initialize platform_data
to feed drivers the right hardware info (addressing, which GPIOs etc).

Then normal platform_bus binding magic can apply, so that a driver
named "fred" gets probed for devices named "fred".

- Dave

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Fri, 20 Feb 2004, Benjamin Herrenschmidt wrote:
> 
> > Well, we do. The pcibios_xxx routines get called for all PCI devices 
> > during discovery, and that's when you'd fill them in.
> 
> But what about USB or FireWire devices ? In theory, I'd like to see
> the driver for those not have to bother about beeing hosted by a PCI
> device or whatever else (there are typically non-PCI OHCI USBs on
> embedded platform, faking a pci_dev is becoming painful).

Well, a USB device can't actually do DMA, so .. (it's only the USB _host_ 
that does DMA, and while those aren't always PCI, they normally are).

Basically, we only care about host devices, since anything else is going 
to be talked to through a driver and is thus not even platform-dependent 
any more. See what I'm saying? 

So we'd only have hust buses here: either PCI or ISA or something like 
that (NuBus, VME, EISA, Microchannel.. You get the idea).

And host buses - pretty much by definition - are scanned by the host. So 
all those buses tend to have host fixups. In the case of PCI, it's the 
"pcibios_xxx()" macros that are the host fixups. Most other buses tend to 
be _so_ host-centric that they don't have anything _but_ the host 
environment (ie things like the embedded buses used by ARM chips etc).

> So it would actually make sense to be able to pass whatever struct
> device we are on, and have a real inheritance of the DMA functions
> going down the bus, don't you think ? 

Only for devices on host buses. 

> > No no. That wouldn't work AT ALL, since the whole point is that you need 
> > to know what the device is - ie you need to fill in the information when 
> > you get the "struct pci_dev *" (because different buses would most likely 
> > have different behaviour, and could have different requirements for DMA 
> > mapping etc). 
> 
> And ? Where is the problem ? By default, you inherit from the parent
> and that's just fine.

Yes, as long as you set up the "root" of the bus, and then just inherit 
the pointer, you should be ok and not need to do anything further. I 
agree.

(And btw, yes, it all booted, so PPC64 is happy again. I pushed out the 
one-liner fix).

		Linus

Re: [BK PATCH] USB update for 2.6.3

[David S. Miller]

On Thu, 19 Feb 2004 23:03:31 -0800 (PST)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> 
> On Fri, 20 Feb 2004, Benjamin Herrenschmidt wrote:
> > 
> > > Well, we do. The pcibios_xxx routines get called for all PCI devices 
> > > during discovery, and that's when you'd fill them in.
> > 
> > But what about USB or FireWire devices ? In theory, I'd like to see
> > the driver for those not have to bother about beeing hosted by a PCI
> > device or whatever else (there are typically non-PCI OHCI USBs on
> > embedded platform, faking a pci_dev is becoming painful).
> 
> Well, a USB device can't actually do DMA, so .. (it's only the USB _host_ 
> that does DMA, and while those aren't always PCI, they normally are).

You miss how all of this stuff is being used :-)

USB drivers do things like map DMA memory, and the generic DMA layer vectors it
so that if the USB device is attached to a PCI host the PCI DMA mapping routines
get used.

Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

> You miss how all of this stuff is being used :-)
> 
> USB drivers do things like map DMA memory, and the generic DMA layer vectors it
> so that if the USB device is attached to a PCI host the PCI DMA mapping routines
> get used.

Hrm... so if the USB device drivers are actually doing the dma mapping
themselves, it make sense for them to pass their own struct device, no ?

Or do they have always to pass their host controller one ?

In the former case, we need that inheritance stuff. In the later case
we don't. At this point, it's pretty much a matter of policy.

Ben.

Re: [BK PATCH] USB update for 2.6.3

[David S. Miller]

On Fri, 20 Feb 2004 18:10:41 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Hrm... so if the USB device drivers are actually doing the dma mapping
> themselves, it make sense for them to pass their own struct device, no ?

That's right, at least that was the idea.

I get the impression though, based upon other posts in this thread, that
this scheme has basically been abandoned until 2.7.x or something like
that.

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Thu, 19 Feb 2004, David S. Miller wrote:
> On Fri, 20 Feb 2004 18:10:41 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Hrm... so if the USB device drivers are actually doing the dma mapping
> > themselves, it make sense for them to pass their own struct device, no ?
> 
> That's right, at least that was the idea.

No. That would be _fundamentally_ wrong.

There's no way a USB device can do DMA in the first place. It has no DMA 
controller, and no way to read/write memory except through the USB host.

So it is the host - and only the host - that matters. Anything else is a 
bug.

		Linus

Re: [BK PATCH] USB update for 2.6.3

[Hollis Blanchard]

On Feb 20, 2004, at 9:15 AM, Linus Torvalds wrote:
>
> On Thu, 19 Feb 2004, David S. Miller wrote:
>> On Fri, 20 Feb 2004 18:10:41 +1100
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>>
>>> Hrm... so if the USB device drivers are actually doing the dma 
>>> mapping
>>> themselves, it make sense for them to pass their own struct device, 
>>> no ?
>>
>> That's right, at least that was the idea.
>
> No. That would be _fundamentally_ wrong.
>
> There's no way a USB device can do DMA in the first place. It has no 
> DMA
> controller, and no way to read/write memory except through the USB 
> host.
>
> So it is the host - and only the host - that matters. Anything else is 
> a
> bug.

Sure. So dma-mapping.h does this:
	int dma_supported(struct device *dev, u64 mask) {
		return device->bus->dma_supported(dev, u64 mask);
	}

And USB, when it creates its bus_type, does this:
	int usb_dma_supported(struct device *dev, u64 mask) {
		usb_dev *usbdev = to_usb_device(dev);
		return usbdev->root_hub->controller->bus->dma_supported(controller, 
u64 mask)
	}
And of then PCI has:
	int pci_dma_supported(struct device *dev, u64 mask) {
		pci_dev *pcidev = to_pci_dev(dev, u64 mask);
		...
	}

Then a USB driver uses its own usb_device->dev and it all ends up back 
at the PCI bus.

For PCI devices of course, device->bus->dma_supported() *is* 
pci_dma_supported(), so there's no middleman (as USB is above).

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Fri, 20 Feb 2004, Hollis Blanchard wrote:
> 
> And USB, when it creates its bus_type, does this:
> 	int usb_dma_supported(struct device *dev, u64 mask) {
> 		usb_dev *usbdev = to_usb_device(dev);
> 		return usbdev->root_hub->controller->bus->dma_supported(controller, u64 mask)
> 	}

So? The above has absolutely nothing to do with "dma_alloc_coherent()". 
Also, it is wrong. It is not necessarily guaranteed that the device that 
is the host controller ha sanythign to do with the "bus->dma_supported" 
thing.

The point is, that DMA is always _always_ done on the host controller. 
Trying to make things look any different is silly and wrong.

THE ABOVE CODE IS CRAP!

The correct thing to do is

	int usb_dma_supported(struct usb_dev *dev, u64 mask)
	{
		struct device *host = dev->root_hub->controller;
		return dma_supported(host);
	}

and anything else is FUNDAMENTALLY WRONG!

Imagine, for example, that the bus is a PCI bus, but the USB host 
controller has a bug in that it only supports 24-bit DMA. Asking for what 
the bus of the host controller supports is non-sensical, and has 
absolutely zero to do with that the actual host device supports.

See?

(That actual bug is totally irrelevant, though. The _fundamnetal_ bug is
in your way of thinking. For one thing, if you have a function called
"usb_dma_xxx()", then it takes a _USB_ device, not a generic device.  
Because the function clearly doesn't even WORK with a generic device, it
only works with a "struct usb_dev". So don't "lie" about things like that
in your interfaces and confuse the issue).

			Linus

Re: [BK PATCH] USB update for 2.6.3

[Hollis Blanchard]

On Feb 20, 2004, at 12:39 PM, Linus Torvalds wrote:
> (That actual bug is totally irrelevant, though. The _fundamnetal_ bug 
> is
> in your way of thinking. For one thing, if you have a function called
> "usb_dma_xxx()", then it takes a _USB_ device, not a generic device.
> Because the function clearly doesn't even WORK with a generic device, 
> it
> only works with a "struct usb_dev". So don't "lie" about things like 
> that
> in your interfaces and confuse the issue).

Well, I was picturing all those *_dma_supported() functions as being 
plugged into (new) fields in struct bus_type:
	struct bus_type {
		...
		int (*dma_supported)(struct device *dev, u64 mask);
	}

If your *_dma_supported functions only take usb_dev, pci_dev, etc, then 
you end up with code like asm-generic/dma-mapping.h:
	int dma_supported(struct device *dev, u64 mask)
	{
#ifdef CONFIG_PCI
		if (dev->bus == pci_bus_type)
			return pci_dma_supported(to_pci_dev(dev), mask);
#endif
#ifdef CONFIG_ISA
		if (dev->bus == isa_bus_type)
			return isa_dma_supported(to_isa_dev(dev), mask);
#endif
#ifdef CONFIG_BRANDNEWSUPERBUS
		if (dev->bus == brandnewsuper_bus_type)
			return brandnewsuper_dma_supported(to_brandnewsuper_dev(dev), mask);
#endif
		... list every possible bus type here ...
	}

That just seems silly to me, when you can dispatch through a 
bus_type->dma_supported() function pointer.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [BK PATCH] USB update for 2.6.3

[Linus Torvalds]

On Fri, 20 Feb 2004, Hollis Blanchard wrote:
> 
> Well, I was picturing all those *_dma_supported() functions as being 
> plugged into (new) fields in struct bus_type:
> 	struct bus_type {
> 		...
> 		int (*dma_supported)(struct device *dev, u64 mask);
> 	}

Ok, that would work. It might even be a good idea (not just DMA-related) 
to make sure that everything you can portably "do" with a device would 
show up as device operations. Right now it's not very well specified, and 
there's obviously a lot of confusion.

> If your *_dma_supported functions only take usb_dev, pci_dev, etc, then 
> you end up with code like asm-generic/dma-mapping.h:

I agree, that is horrible. On the other hand, some architectures don't 
need any indirection or any conditionals at all, since they know that they 
only have one type of DMA. 

Making the device operations explicit would be good, though, and would 
match how we do things in general. It's a fairly big change at this point, 
but if somebody is willing to put the effort into the cleanup, then I'm 
all for it.

I'd still ask that people don't do DMA on non-host devices. I'd rather 
have a USB "struct device" just return "DMA not supported", to make sure 
that everybody asks the right chip.

		Linus

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

On Sat, 2004-02-21 at 06:32, Linus Torvalds wrote:
> On Fri, 20 Feb 2004, Hollis Blanchard wrote:
> > 
> > Well, I was picturing all those *_dma_supported() functions as being 
> > plugged into (new) fields in struct bus_type:
> > 	struct bus_type {
> > 		...
> > 		int (*dma_supported)(struct device *dev, u64 mask);
> > 	}
> 
> Ok, that would work. It might even be a good idea (not just DMA-related) 
> to make sure that everything you can portably "do" with a device would 
> show up as device operations. Right now it's not very well specified, and 
> there's obviously a lot of confusion.

Yes, which is why I don't agree with Hollis about having that in
bus_type, but rather in the device itself and inherit from the
parents by default ;) (Hrm... did I repeat myself ?)

The bus_type was is pretty useless for things like USB, and we
may have several controllers of the same bus with different
caracteristics (an on-chip OHCI and a PCI EHCI comes to mind)

Hrm... reading your last sentence, it seems you don't want people
to pass the struct device of the USB device but the host one instead,
fair enough...

> > If your *_dma_supported functions only take usb_dev, pci_dev, etc, then 
> > you end up with code like asm-generic/dma-mapping.h:
> 
> I agree, that is horrible. On the other hand, some architectures don't 
> need any indirection or any conditionals at all, since they know that they 
> only have one type of DMA. 
> 
> Making the device operations explicit would be good, though, and would 
> match how we do things in general. It's a fairly big change at this point, 
> but if somebody is willing to put the effort into the cleanup, then I'm 
> all for it.
> 
> I'd still ask that people don't do DMA on non-host devices. I'd rather 
> have a USB "struct device" just return "DMA not supported", to make sure 
> that everybody asks the right chip.
> 

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Alan Stern]

Just to clear up a small disagreement...

On Fri, 20 Feb 2004, Linus Torvalds wrote:

> On Thu, 19 Feb 2004, David S. Miller wrote:
> > On Fri, 20 Feb 2004 18:10:41 +1100
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > 
> > > Hrm... so if the USB device drivers are actually doing the dma mapping
> > > themselves, it make sense for them to pass their own struct device, no ?
> > 
> > That's right, at least that was the idea.
> 
> No. That would be _fundamentally_ wrong.
> 
> There's no way a USB device can do DMA in the first place. It has no DMA 
> controller, and no way to read/write memory except through the USB host.
> 
> So it is the host - and only the host - that matters. Anything else is a 
> bug.

Both of you are right.  Linus is right that USB devices don't do DMA.  
Ben is right that sometimes a USB device _driver_ will set up a DMA 
mapping.  And of course, it is set up on behalf of the corresponding USB 
controller, so the controller's struct device would be passed.

Alan Stern

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

> Basically, we only care about host devices, since anything else is going 
> to be talked to through a driver and is thus not even platform-dependent 
> any more. See what I'm saying? 

I see. Well... as long as the actual dma mapping calls are always
done at the host controller level, that's fine with me.

> .../...

> (And btw, yes, it all booted, so PPC64 is happy again. I pushed out the 
> one-liner fix).

BTW. Did you ever reproduce that lockup ?

Ben.

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[David Brownell]

Benjamin Herrenschmidt wrote:

> Yes. I also remember a time where the dma mask for the DMA API was all
> broken too (would not be possible to map the PCI one on top of it),
> but I think that got fixed. 

I thought it was still broken.  Last I compared different asm-* arch
implementations of dma_supported(), the semantics were inconsistent.
The "mask" was sometimes ignored, sometimes treated as an upper
bound on the address, once I recall it even being used as a mask!
Some of that inconsistency seemed to come from PCI though.

- Dave

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Russell King]

On Fri, Feb 20, 2004 at 12:08:08AM -0800, David Brownell wrote:
> Benjamin Herrenschmidt wrote:
> > Yes. I also remember a time where the dma mask for the DMA API was all
> > broken too (would not be possible to map the PCI one on top of it),
> > but I think that got fixed. 
> 
> I thought it was still broken.  Last I compared different asm-* arch
> implementations of dma_supported(), the semantics were inconsistent.
> The "mask" was sometimes ignored, sometimes treated as an upper
> bound on the address, once I recall it even being used as a mask!
> Some of that inconsistency seemed to come from PCI though.

My understanding is that it's a mask, which just happens to be treated
as an upper address limit on sane platforms.

However, we have a case where it's a real mask (no surprises there!)
Intel decided not to fix a bug in a chip which means that an address
line (normally A20) must never be asserted during DMA cycles, although
it can address more than 1MB of memory.  The effect of this is that
even MB are dma-able and odd MB aren't.  Not nice.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [BK PATCH] USB update for 2.6.3

[Deepak Saxena]

On Feb 20 2004, at 17:03, Benjamin Herrenschmidt was caught saying:
> On Fri, 2004-02-20 at 16:58, Linus Torvalds wrote:
> > On Thu, 19 Feb 2004, Greg KH wrote:
> > > 
> > > Here are some USB patches for 2.6.3.  Here are the main types of changes:
> > > 	- switch usb code to use dmapools instead of pcipools which
> > > 	  makes the embedded people happy.
> > 
> > However, this makes the ppc64 people very unhappy.
> > 
> > I've just yesterday and today switched my main home machine to be ppc64, 
> > and this will not compile for me. Bad boy!
> > 
> > (And I haven't used ppc64 enough to be able to make sense of the DMA 
> > setup, so I can't even fix it myself yet. Aaarghh!)
> 
> Hehe ;)
> 
> Well, last I heard, people here were trying to make that work, but
> still, our dma mapping API will probably want the device passed in
> to be a pci_dev...
> 
> The problem with the generic API is that it makes it difficult
> for us to actually go back to the PCI device hosting the USB
> device we get passed in... Which we what we want (our IO MMU
> setup is based on what PCI device is there, for PCI at least,
> for VIO, it's a bit different, but the idea is the same).

Why can't you just do the following to  get to the PCI device?

if (dev->bus_type == &pci_bus_type) {
	struct pci_dev pci_usb_dev = to_pci_dev(dev);
	...
}

If you mean the USB target device itself, can't you walk the
tree until you find a device that is no longer on bus_type
usb to determine your root?

> A while ago, I've advertised making this API a set of function
> pointers attached to the struct device inherited from the bus
> parent, so the core code just set one for the root PCIs and
> everybody inherits them.... But of course, since x86 isn't
> affected, nobody cared ;)

You could stuff that into platform_data on PCI devices on your platforms.

> I think the "generic" DMA API is just not suitable at the moment
> to be really used. It gets passed "generic" struct device, which,
> due to the way Pat designed it, are almost useless alone (we
> can't quite match them to anything and don't have any kind of
> "inheritance" of methods via function pointers). There is no
> simple hook for archs to carry informations attached to struct
> devices neither, etc...

I think we're not quite there yet, but once you have the device
struct, in theory, you can walk up the tree to grab the platform_data
for say the device's parent and do any tweaks based on platform-specific
bus parameters.  With PCI, you could even stuff this into pci_bus->sysdata.

I think having a function pointer table for things like dma mapping
and ioremap on all devices would be a very good thing, but not sure
if the powers that be would allow that in 2.6.

~Deepak

-- 
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

> If you mean the USB target device itself, can't you walk the
> tree until you find a device that is no longer on bus_type
> usb to determine your root?

I don't feel like walking the tree on each pci_dma access

> You could stuff that into platform_data on PCI devices on your platforms.

I want automatic inheritance to child devices, shouldb't be _that_
difficult to do ;)

> I think we're not quite there yet, but once you have the device
> struct, in theory, you can walk up the tree to grab the platform_data
> for say the device's parent and do any tweaks based on platform-specific
> bus parameters.  With PCI, you could even stuff this into pci_bus->sysdata.
> 
> I think having a function pointer table for things like dma mapping
> and ioremap on all devices would be a very good thing, but not sure
> if the powers that be would allow that in 2.6.
> 
> ~Deepak
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Deepak Saxena]

On Feb 20 2004, at 18:47, Benjamin Herrenschmidt was caught saying:
> 
> > If you mean the USB target device itself, can't you walk the
> > tree until you find a device that is no longer on bus_type
> > usb to determine your root?
> 
> I don't feel like walking the tree on each pci_dma access

I never said it would be pretty. :)

> 
> > You could stuff that into platform_data on PCI devices on your platforms.
> 
> I want automatic inheritance to child devices, shouldb't be _that_
> difficult to do ;)

Hmm, I wonder if the easiet way to do this at the moment would be
to add a platform specific hook that gets called during device_add().
On arches that don't need to do this, it would just be a nop, but on 
PPC64 and others it could do whatever is required.  By the time that 
device_add() is called, it is already attached to a bus, so this 
function could walk the tree to inherit parameters at discovery time 
instead of the above suggestion. 

Seems doable without impacting other arches.

~Deepak

-- 
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[David Brownell]

Deepak Saxena wrote:
> On Feb 20 2004, at 18:47, Benjamin Herrenschmidt was caught saying:
> 
>>>If you mean the USB target device itself, can't you walk the
>>>tree until you find a device that is no longer on bus_type
>>>usb to determine your root?
>>
>>I don't feel like walking the tree on each pci_dma access

Why should that be needed though?


> I never said it would be pretty. :)

Well, usb_device->bus->controller is the only access that
should be needed ... much prettier than a tree walk!  It's
set up as part of device enumeration.

Some of the usb_buffer_*() mapping calls could probably
start to get inlined now, using the generic DMA calls.

- Dave

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Benjamin Herrenschmidt]

> Well, usb_device->bus->controller is the only access that
> should be needed ... much prettier than a tree walk!  It's
> set up as part of device enumeration.
> 
> Some of the usb_buffer_*() mapping calls could probably
> start to get inlined now, using the generic DMA calls.

It all depends what the USB device driver does. If it does
pass the struct device of it's controller, it's fine. If we
want it to be able to pass its own struct device, we need
this walk... it's a matter of how we want this API to behave.

Same goes for firewire, and possibly others

Anyway, a platform hook in device_add() seem like it could be
useful for other things as well...

Ben.

Re: [linux-usb-devel] Re: [BK PATCH] USB update for 2.6.3

[Russell King]

On Fri, Feb 20, 2004 at 07:48:13PM +1100, Benjamin Herrenschmidt wrote:
> Anyway, a platform hook in device_add() seem like it could be
> useful for other things as well...

What about platform_notify() which already seems to be provided for you ?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core