USB and DMA on Alpha with 2.6.0-test7

USB and DMA on Alpha with 2.6.0-test7

[Måns Rullgård]

Yesterday, I compiled 2.6.0-test7 for one of my Alpha boxes.  I have
an AX8817X based USB ethernet adaptor connected to it (it's short on
PCI slots), so I compiled the usbnet module.  When I loaded usbnet, I
got a BUG at include/asm-generic/dma-mapping.h:19.  Apparently, DMA
setup only works with PCI here.  How should this be fixed?  It worked
with -test4, albeit slowly, for other reasons.

-- 
Måns Rullgård
mru@users.sf.net

Re: USB and DMA on Alpha with 2.6.0-test7

[Ivan Kokshaysky]

On Fri, Oct 10, 2003 at 11:22:06AM +0200, Måns Rullgård wrote:
> Yesterday, I compiled 2.6.0-test7 for one of my Alpha boxes.  I have
> an AX8817X based USB ethernet adaptor connected to it (it's short on
> PCI slots), so I compiled the usbnet module.  When I loaded usbnet, I
> got a BUG at include/asm-generic/dma-mapping.h:19.  Apparently, DMA
> setup only works with PCI here.  How should this be fixed?  It worked
> with -test4, albeit slowly, for other reasons.

Well, the usage of dma_supported() in usbnet.c is wrong even for i386.
USB device doesn't do DMA, it's USB controller what does. The driver should
check dma_mask of the parent device instead, something like this:

	// possible with some EHCI controllers
	if (*udev->dev->parent->dma_mask == 0xffffffffffffffffULL)
		net->features |= NETIF_F_HIGHDMA;

Ivan.

Re: USB and DMA on Alpha with 2.6.0-test7

[Måns Rullgård]

Ivan Kokshaysky <ink@jurassic.park.msu.ru> writes:

> > Yesterday, I compiled 2.6.0-test7 for one of my Alpha boxes.  I have
> > an AX8817X based USB ethernet adaptor connected to it (it's short on
> > PCI slots), so I compiled the usbnet module.  When I loaded usbnet, I
> > got a BUG at include/asm-generic/dma-mapping.h:19.  Apparently, DMA
> > setup only works with PCI here.  How should this be fixed?  It worked
> > with -test4, albeit slowly, for other reasons.
> 
> Well, the usage of dma_supported() in usbnet.c is wrong even for i386.
> USB device doesn't do DMA, it's USB controller what does. The driver should

That's what I thought.

> check dma_mask of the parent device instead, something like this:
> 
> 	// possible with some EHCI controllers
> 	if (*udev->dev->parent->dma_mask == 0xffffffffffffffffULL)
> 		net->features |= NETIF_F_HIGHDMA;

Is that all there is to it?

-- 
Måns Rullgård
mru@users.sf.net

Re: USB and DMA on Alpha with 2.6.0-test7

[Jamie Lokier]

Ivan Kokshaysky wrote:
> On Fri, Oct 10, 2003 at 11:22:06AM +0200, Måns Rullgård wrote:
> > Yesterday, I compiled 2.6.0-test7 for one of my Alpha boxes.  I have
> > an AX8817X based USB ethernet adaptor connected to it (it's short on
> > PCI slots), so I compiled the usbnet module.  When I loaded usbnet, I
> > got a BUG at include/asm-generic/dma-mapping.h:19.  Apparently, DMA
> > setup only works with PCI here.  How should this be fixed?  It worked
> > with -test4, albeit slowly, for other reasons.
> 
> Well, the usage of dma_supported() in usbnet.c is wrong even for i386.
> USB device doesn't do DMA, it's USB controller what does. The driver should
> check dma_mask of the parent device instead, something like this:
> 
> 	// possible with some EHCI controllers
> 	if (*udev->dev->parent->dma_mask == 0xffffffffffffffffULL)
> 		net->features |= NETIF_F_HIGHDMA;

Isn't the device's dma_mask set equal to the controller's dma_mask
automatically?

What happens if it's on a hub, or a hub on a hub?  Then the parent
isn't the controller, is it?

-- Jamie

Re: USB and DMA on Alpha with 2.6.0-test7

[Ivan Kokshaysky]

On Fri, Oct 10, 2003 at 02:31:04PM +0100, Jamie Lokier wrote:
> Isn't the device's dma_mask set equal to the controller's dma_mask
> automatically?

You are right, just check dma_mask of the device.

Anyway, as it is, usbnet driver won't work on i386 with
more than 4G of RAM and 32-bit DMA USB controller.

Ivan.

Re: USB and DMA on Alpha with 2.6.0-test7

[Måns Rullgård]

Ivan Kokshaysky <ink@jurassic.park.msu.ru> writes:

>> Isn't the device's dma_mask set equal to the controller's dma_mask
>> automatically?
>
> You are right, just check dma_mask of the device.
>
> Anyway, as it is, usbnet driver won't work on i386 with
> more than 4G of RAM and 32-bit DMA USB controller.

So, what should the patch look like?  I'd like to use this thing.

-- 
Måns Rullgård
mru@users.sf.net

Re: USB and DMA on Alpha with 2.6.0-test7

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

> Ivan Kokshaysky wrote:
>>
>> This doesn't work. You will always return success if mask = ~0ULL.

Whoops.  I didn't copy what I thought I was copying:

            (mask & *dev->dma_mask) == mask  /* not "== dma_mask" */

Updated code is appended ... which also provides a BUG()-free
version of dma_set_mask().  I think the only arch/platform hook
needed would be for dma_supported().

- Dave


[-- Attachment #2: Diff.dma --]
[-- Type: text/plain, Size: 877 bytes --]

--- 1.4/include/asm-generic/dma-mapping.h	Mon Jan 13 14:37:47 2003
+++ edited/include/asm-generic/dma-mapping.h	Sat Oct 11 15:33:56 2003
@@ -13,20 +13,22 @@
 /* need struct page definitions */
 #include <linux/mm.h>
 
+/* FIXME use this everywhere there's no platform_dma_supported() */
 static inline int
 dma_supported(struct device *dev, u64 mask)
 {
-	BUG_ON(dev->bus != &pci_bus_type);
-
-	return pci_dma_supported(to_pci_dev(dev), mask);
+	/* device can dma, using those address bits */
+	return dev->dma_mask
+		&& (mask & *dev->dma_mask) == mask;
 }
 
 static inline int
-dma_set_mask(struct device *dev, u64 dma_mask)
+dma_set_mask(struct device *dev, u64 mask)
 {
-	BUG_ON(dev->bus != &pci_bus_type);
-
-	return pci_set_dma_mask(to_pci_dev(dev), dma_mask);
+	if (!dma_supported(dev, mask))
+		return 0;
+	*dev->dma_mask = mask;
+	return 1;
 }
 
 static inline void *

Re: USB and DMA on Alpha with 2.6.0-test7

[David Brownell]

Ivan Kokshaysky wrote:
> On Fri, Oct 10, 2003 at 12:43:24PM -0700, David Brownell wrote:
> 
>> static inline int
>> dma_supported(struct device *dev, u64 mask)
>> {
>>-	BUG_ON(dev->bus != &pci_bus_type);
>>-
>>-	return pci_dma_supported(to_pci_dev(dev), mask);
>>+	/* device can dma, using those address bits */
>>+	return dev->dma_mask
>>+		&& (mask & *dev->dma_mask) == *dev->dma_mask;
>> }
> 
> 
> This doesn't work. You will always return success if mask = ~0ULL.

Only if *dma_mask == ~0ULL too ... meaning that device supports
64 bit DMA addressing, in which case the test should certainly
return TRUE since that's what the caller wanted to check.

Not in cases like *dma_mask == 0xffffffff (32 bit DMA only)
or 0x00ffffff (ISA dma only) ... if the proposed mask has even
just one address bit that is NOT set in the "these bits work"
mask, that second test returns FALSE.

(That test is basically what sparc64 does.  Most implementations
of the dma "mask" checking don't actually use them as masks; so
I expected that more-correct check to get second looks!)


> But more important thing is that dma_supported() must not expect
> valid *dev->dma_mask, as it's called from dma_set_mask().

It doesn't make sense to me that the dma mask _ever_ be invalid
after its "struct device" is initialized (and registered).

The bus setup code must establish initial values for DMA masks,
based on bus capabilities:  64bit, normal 32bit, 24bit ISA, etc.
So from then on dma_set_mask() will be usable.  All USB devices
inherit that mask from the Host Controller Driver (HCD) which
they happen to be using; usually for 32bit DMA, sometimes 64bit.

Some device drivers can add restrictions, with dma_set_mask(),
based on actual device capabilities:  certain address bits
are always zero, etc.   That call is illegal for USB device
drivers to use -- only HCD initialization could know enough
about that hardware to change the initial value.


Some of the confusion here is surely because historically
drivers have been told to call pci_dma_set_mask() directly,
to achieve what dma_supported() achieves:  testing whether
a given class of DMA addresses will work.  Which is clearly
the wrong model, except for hardware that's less capable
than the bus it's running on ... it'd also be broken for a
"generic" model, since it can't work for drivers that only
work through a HAL (like all USB device drivers).


> That is, your usage of dma_supported() is buggy regardless of
> DMA API implementation.

The facts I see don't support that claim.

- Dave

Re: USB and DMA on Alpha with 2.6.0-test7

[Ivan Kokshaysky]

On Fri, Oct 10, 2003 at 12:43:24PM -0700, David Brownell wrote:
>  static inline int
>  dma_supported(struct device *dev, u64 mask)
>  {
> -	BUG_ON(dev->bus != &pci_bus_type);
> -
> -	return pci_dma_supported(to_pci_dev(dev), mask);
> +	/* device can dma, using those address bits */
> +	return dev->dma_mask
> +		&& (mask & *dev->dma_mask) == *dev->dma_mask;
>  }

This doesn't work. You will always return success if mask = ~0ULL.
But more important thing is that dma_supported() must not expect
valid *dev->dma_mask, as it's called from dma_set_mask().
That is, your usage of dma_supported() is buggy regardless of
DMA API implementation.

Ivan.

Re: USB and DMA on Alpha with 2.6.0-test7

[David Brownell]

Ivan Kokshaysky wrote:
> 
> Sigh. The generic dma_* stuff wasn't a well thought-out idea,
> and it's too late to change it in 2.6. :-(

The API is more reasonable than the "all is PCI" assumption
from that "asm-generic" implementation.  I think it should be
reasonable to update any architecture's implementation there.


> Right now calling dma_* functions for non-busmaster devices
> just *doesn't work*.

See the patch I posted for dma_supported(), which should
make that one behave.  It's obvious how to generalize that
specific call with a platform_dma_supported().  USB won't
care about the other dma_* functions ... though it does suck
that all non-PCI ohci-hcd code still needs use the "fake PCI"
kluges.  (Proof that the implementation of those APIs is
still only usable with PCI.)

For other functions, it seems you're mostly saying that some
drivers are missing tests for non-null dma_mask pointers.


> David, do you mind applying that?

Yes, because that's nowhere near where the bugs are!
Also, it's good to get rid of BUG()s ... and better to
get rid of them when they're broken like that one is.

- Dave

Re: USB and DMA on Alpha with 2.6.0-test7

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

David Brownell wrote:
> The BUG_ON at include/asm-generic/dma-mapping.h:19 is a
> bug in that "generic DMA" code ... and I've seen the
> same BUG reported from PPC folk too.

Something like this should be correct even on x86, but
there may be some cases where a platform_dma_supported()
is necessary.

IMO this needs an all-architectures patch.  Not many
will need a platform_dma_supported() ... but almost
every implementation of that simple call is broken.

- Dave



[-- Attachment #2: Diff.dma --]
[-- Type: text/plain, Size: 589 bytes --]

--- 1.4/include/asm-generic/dma-mapping.h	Mon Jan 13 14:37:47 2003
+++ edited/include/asm-generic/dma-mapping.h	Fri Oct 10 10:53:25 2003
@@ -13,12 +13,13 @@
 /* need struct page definitions */
 #include <linux/mm.h>
 
+/* FIXME use this everywhere there's no platform_dma_supported() */
 static inline int
 dma_supported(struct device *dev, u64 mask)
 {
-	BUG_ON(dev->bus != &pci_bus_type);
-
-	return pci_dma_supported(to_pci_dev(dev), mask);
+	/* device can dma, using those address bits */
+	return dev->dma_mask
+		&& (mask & *dev->dma_mask) == *dev->dma_mask;
 }
 
 static inline int

Re: USB and DMA on Alpha with 2.6.0-test7

[Ivan Kokshaysky]

On Fri, Oct 10, 2003 at 08:45:18PM +0200, MЕns RullgЕrd wrote:
> There was a little typo in there.  This seems to work.
...
> -	if (dma_supported (&udev->dev, 0xffffffffffffffffULL))
> +	if (*udev->dev.dma_mask == 0xffffffffffffffffULL))

Argh, what a maze of pointers ;-)
Thanks!

David, do you mind applying that?

Ivan.

Re: USB and DMA on Alpha with 2.6.0-test7

[Måns Rullgård]

Ivan Kokshaysky <ink@jurassic.park.msu.ru> writes:

> I'd suggest following (untested) patch.

There was a little typo in there.  This seems to work.

--- t7/drivers/usb/net/usbnet.c	Wed Oct  8 23:24:07 2003
+++ linux/drivers/usb/net/usbnet.c	Fri Oct 10 22:10:24 2003
@@ -2970,7 +2970,7 @@ usbnet_probe (struct usb_interface *udev
 	memcpy (net->dev_addr, node_id, sizeof node_id);
 
 	// possible with some EHCI controllers
-	if (dma_supported (&udev->dev, 0xffffffffffffffffULL))
+	if (*udev->dev.dma_mask == 0xffffffffffffffffULL))
 		net->features |= NETIF_F_HIGHDMA;
 
 	net->change_mtu = usbnet_change_mtu;
--- t7/drivers/usb/net/kaweth.c	Wed Oct  8 23:24:26 2003
+++ linux/drivers/usb/net/kaweth.c	Fri Oct 10 22:11:21 2003
@@ -1120,7 +1120,7 @@ static int kaweth_probe(
 
 	usb_set_intfdata(intf, kaweth);
 
-	if (dma_supported (&intf->dev, 0xffffffffffffffffULL))
+	if (*intf->dev.dma_mask == 0xffffffffffffffffULL))
 		kaweth->net->features |= NETIF_F_HIGHDMA;
 
 	SET_NETDEV_DEV(netdev, &intf->dev);

-- 
Måns Rullgård
mru@users.sf.net

Re: USB and DMA on Alpha with 2.6.0-test7

[Ivan Kokshaysky]

On Fri, Oct 10, 2003 at 10:18:15AM -0700, David Brownell wrote:
> We might need arch-specific implementations of that
> method, and maybe Alpha is even one of them.  But if
> there's going to be a default implementation for that
> method, the current scheme has portability problems.

Sigh. The generic dma_* stuff wasn't a well thought-out idea,
and it's too late to change it in 2.6. :-(
Right now calling dma_* functions for non-busmaster devices
just *doesn't work*.

> Nope -- there's EHCI, which can handle 64-bit DMA when the
> silicon allows ... which is why that test exists.

That test is not a test at all - it's precise equivalent
of "if (1)" on i386. On other platforms it's just a BUG().
I'd suggest following (untested) patch.

Ivan.

--- t7/drivers/usb/net/usbnet.c	Wed Oct  8 23:24:07 2003
+++ linux/drivers/usb/net/usbnet.c	Fri Oct 10 22:10:24 2003
@@ -2970,7 +2970,7 @@ usbnet_probe (struct usb_interface *udev
 	memcpy (net->dev_addr, node_id, sizeof node_id);
 
 	// possible with some EHCI controllers
-	if (dma_supported (&udev->dev, 0xffffffffffffffffULL))
+	if (*udev->dev->dma_mask == 0xffffffffffffffffULL))
 		net->features |= NETIF_F_HIGHDMA;
 
 	net->change_mtu = usbnet_change_mtu;
--- t7/drivers/usb/net/kaweth.c	Wed Oct  8 23:24:26 2003
+++ linux/drivers/usb/net/kaweth.c	Fri Oct 10 22:11:21 2003
@@ -1120,7 +1120,7 @@ static int kaweth_probe(
 
 	usb_set_intfdata(intf, kaweth);
 
-	if (dma_supported (&intf->dev, 0xffffffffffffffffULL))
+	if (*intf->dev->dma_mask == 0xffffffffffffffffULL))
 		kaweth->net->features |= NETIF_F_HIGHDMA;
 
 	SET_NETDEV_DEV(netdev, &intf->dev);

Re: USB and DMA on Alpha with 2.6.0-test7

[David Brownell]

The BUG_ON at include/asm-generic/dma-mapping.h:19 is a
bug in that "generic DMA" code ... and I've seen the
same BUG reported from PPC folk too.

All implementations of dma_supported() should check
those DMA masks directly ... instead we have

  - A "generic" implementation that only works for PCI,
    even though that method (in particular!) was intended
    to really be generic enough to work with USB;

  - Some arch-specific implementations (x86) that don't
    handle the 64-bit DMA case correctly.

We might need arch-specific implementations of that
method, and maybe Alpha is even one of them.  But if
there's going to be a default implementation for that
method, the current scheme has portability problems.


Ivan Kokshaysky wrote:
> Anyway, as it is, usbnet driver won't work on i386 with
> more than 4G of RAM and 32-bit DMA USB controller.

Nope -- there's EHCI, which can handle 64-bit DMA when the
silicon allows ... which is why that test exists.

- Dave