USB and DMA on Alpha with 2.6.0-test7

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.

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

- Dave

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*.

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.

@@ -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;

@@ -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);

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

@@ -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;

@@ -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
[email protected]

...

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

@@ -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

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.

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.

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

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!)

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).

The facts I see don't support that claim.

- Dave

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

@@ -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 *