This patchset adds API and one user for a "weak" dma_set_mask().
Weak means that it will fallback to smaller masks in case the
DMA subsystem rejects a big mask.
Currently such rejection may happen if the driver requests a 64bit
mask on a VIA machine, for example. dma_set_mask_weak() will fallback
to 32bit, in that case, and tell the caller about it by modifying the
passed mask.

I'm not sure how we should merge this patchset.
I'd suggest we merge it all through John Linville for 2.6.27, as the
only current user of the API is the b43 wireless driver.
We could split it and push the nonwireless parts to somebody else's tree,
but I'm not sure it's worth the trouble.

-- 
Greetings Michael.
--

[ message continues ]

This adds a user for the new weak DMA mask API.
This removes the b43 DMA mask probe loop.

Signed-off-by: Michael Buesch <mb@bu3sch.de>


Index: linux-2.6/drivers/net/wireless/b43/dma.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43/dma.c	2008-05-01 13:19:51.000000000 +0200
+++ linux-2.6/drivers/net/wireless/b43/dma.c	2008-05-01 14:45:52.000000000 +0200
@@ -980,37 +980,27 @@ void b43_dma_free(struct b43_wldev *dev)
 	destroy_ring(dma, tx_ring_mcast);
 }
 
+static inline unsigned int mask_to_bit(u64 dma_mask)
+{
+	return fls64(dma_mask);
+}
+
 static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
 {
 	u64 orig_mask = mask;
-	bool fallback = 0;
 	int err;
 
-	/* Try to set the DMA mask. If it fails, try falling back to a
-	 * lower mask, as we can always also support a lower one. */
-	while (1) {
-		err = ssb_dma_set_mask(dev->dev, mask);
-		if (!err)
-			break;
-		if (mask == DMA_64BIT_MASK) {
-			mask = DMA_32BIT_MASK;
-			fallback = 1;
-			continue;
-		}
-		if (mask == DMA_32BIT_MASK) {
-			mask = DMA_30BIT_MASK;
-			fallback = 1;
-			continue;
-		}
+	err = ssb_dma_set_mask_weak(dev->dev, &mask);
+	if (err) {
 		b43err(dev->wl, "The machine/kernel does not support "
 		       "the required %u-bit DMA mask\n",
-		       (unsigned int)dma_mask_to_engine_type(orig_mask));
+		       mask_to_bit(orig_mask));
 		return -EOPNOTSUPP;
 	}
-	if (fallback) {
+	if (mask != orig_mask) {
 		b43info(dev->wl, "DMA mask fallback from %u-bit to %u-bit\n",
-			(unsigned int)dma_mask_to_engine_type(orig_mask),
-			(unsigned int)dma_mask_to_engine_type(mask));
+			mask_to_bit(orig_mask),
+			mask_to_bit(mask));
 	}
 
 	return 0;


-- 
Greetings Michael.
--

[ message continues ]

This adds a new DMA subsystem API call for "weak" setting
of the DMA mask.
Weak means that it will fallback to smaller masks in case the
DMA subsystem rejects a big mask.
Currently such rejection may happen if the driver requests a 64bit
mask on a VIA machine, for example. dma_set_mask_weak() will fallback
to 32bit, in that case, and tell the caller about it by modifying the
passed mask.

Signed-off-by: Michael Buesch <mb@bu3sch.de>


Index: wireless-testing/drivers/base/dma-mapping.c
===================================================================
--- wireless-testing.orig/drivers/base/dma-mapping.c	2008-04-28 23:34:19.000000000 +0200
+++ wireless-testing/drivers/base/dma-mapping.c	2008-04-28 23:46:25.000000000 +0200
@@ -216,3 +216,38 @@ void dmam_release_declared_memory(struct
 EXPORT_SYMBOL(dmam_release_declared_memory);
 
 #endif
+
+/**
+ * dma_set_mask_weak - Set the DMA mask. Retry with smaller masks.
+ * @dev: Device to set the mask on.
+ * @mask: Pointer to the mask that you want to set. The function will
+ * 	modify the mask and set it to the actually used mask, in case
+ * 	it had to fall back to a smaller mask.
+ *
+ * Set the DMA mask and allow falling back to smaller masks in
+ * case of an error.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
+ */
+int dma_set_mask_weak(struct device *dev, u64 *mask)
+{
+	u64 m = *mask;
+	int err;
+
+	if (m < DMA_MIN_FALLBACK_MASK)
+		return -EINVAL;
+	while (1) {
+		err = dma_set_mask(dev, m);
+		if (!err)
+			break;
+		/* Did not like this one. Try a smaller one. */
+		m >>= 1;
+		if (m < DMA_MIN_FALLBACK_MASK)
+			return err;
+	}
+	*mask = m;
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask_weak);
Index: wireless-testing/include/linux/dma-mapping.h
===================================================================
--- wireless-testing.orig/include/linux/dma-mapping.h	2008-04-28 23:34:19.000000000 +0200
+++ wireless-testing/include/linux/dma-mapping.h	2008-04-28 23:35:35.000000000 +0200
@@ -60,6 ...
[ message continues ]

On Thu, 1 May 2008 16:40:16 +0200

weak is an odd term - and we use weak for things like weak symbols so it
might be confusing. dma_request_mask() ?

Alan
--

[ message continues ]

Yeah, as a non-native english speaker I couldn't come up with a good
name for it. But request seems pretty good to me.


-- 
Greetings Michael.
--

[ message continues ]

This adds a "weak" variant of ssb_dma_set_mask().

Signed-off-by: Michael Buesch <mb@bu3sch.de>


Index: linux-2.6/drivers/ssb/main.c
===================================================================
--- linux-2.6.orig/drivers/ssb/main.c	2008-05-01 13:19:53.000000000 +0200
+++ linux-2.6/drivers/ssb/main.c	2008-05-01 13:47:29.000000000 +0200
@@ -1165,6 +1165,12 @@ u32 ssb_dma_translation(struct ssb_devic
 }
 EXPORT_SYMBOL(ssb_dma_translation);
 
+static void do_set_dma_mask(struct device *dma_dev, u64 mask)
+{
+	dma_dev->coherent_dma_mask = mask;
+	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;
+}
+
 int ssb_dma_set_mask(struct ssb_device *ssb_dev, u64 mask)
 {
 	struct device *dma_dev = ssb_dev->dma_dev;
@@ -1173,13 +1179,26 @@ int ssb_dma_set_mask(struct ssb_device *
 	if (ssb_dev->bus->bustype == SSB_BUSTYPE_PCI)
 		return dma_set_mask(dma_dev, mask);
 #endif
-	dma_dev->coherent_dma_mask = mask;
-	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;
+	do_set_dma_mask(dma_dev, mask);
 
 	return 0;
 }
 EXPORT_SYMBOL(ssb_dma_set_mask);
 
+int ssb_dma_set_mask_weak(struct ssb_device *ssb_dev, u64 *mask)
+{
+	struct device *dma_dev = ssb_dev->dma_dev;
+
+#ifdef CONFIG_SSB_PCIHOST
+	if (ssb_dev->bus->bustype == SSB_BUSTYPE_PCI)
+		return dma_set_mask_weak(dma_dev, mask);
+#endif
+	do_set_dma_mask(dma_dev, *mask);
+
+	return 0;
+}
+EXPORT_SYMBOL(ssb_dma_set_mask_weak);
+
 int ssb_bus_may_powerdown(struct ssb_bus *bus)
 {
 	struct ssb_chipcommon *cc;
Index: linux-2.6/include/linux/ssb/ssb.h
===================================================================
--- linux-2.6.orig/include/linux/ssb/ssb.h	2008-05-01 13:19:59.000000000 +0200
+++ linux-2.6/include/linux/ssb/ssb.h	2008-05-01 13:43:17.000000000 +0200
@@ -406,6 +406,7 @@ extern u32 ssb_dma_translation(struct ss
 #define SSB_DMA_TRANSLATION_SHIFT	30
 
 extern int ssb_dma_set_mask(struct ssb_device *ssb_dev, u64 mask);
+extern int ssb_dma_set_mask_weak(struct ssb_device *ssb_dev, u64 *mask);
 
 
 ...
[ message continues ]

Why do we need it?  Is the call to set the 32bit mask when it fails
a too big burden for the driver author?

--

[ message continues ]

Yeah. because it has to be done in every driver.
So we put the implementation into a central place, instead of
reimplementing the wheel over and over again. This way we avoid bugs,
like the "b43 broken on VIA boards" in the first place.
Currently every driver requesting a >32bit mask and not retrying with
a lower mask is broken on VIA hardware. I dunno how many of the current
drivers that are, but everybody can easily see that is not a b43-specific
problem that we should solve for b43 only.

-- 
Greetings Michael.
--

[ message continues ]

Yeah.  Personally I'd rather let set_dma_mask fall back silently,
I can't imagine a lot of cases where the driver cares that it only
gets dma in the lower 32bit.  And if there is any such case it
can still check the dma mask afterwards.

--

[ message continues ]

We've discussed that and this behaviour is not acceptable, as the driver
must know about a possible fallback in case it can do 32bit DMA
more efficiently than 64bit DMA, for example.

So here we go. People rejected that approach, that I also suggested.
Here's what people want.

-- 
Greetings Michael.
--

[ message continues ]

That's what we have dma_get_required_mask() for.  See
Documentation/DMA-API.txt.

--

[ message continues ]

So well. I'm still unsure about the advantage of having some opencoded
probe loop in the driver, instead of implementing it in a common place
and doing all of it with a single API call.
We can still call dma_get_required_mask() and adjust the mask to that
in dma_set_mask_weak(). That can _additionally_ be done there.

-- 
Greetings Michael.
--

[ message continues ]

So I think we're agreed that we want some core function to fall back to 
different mask values, but yeah, making it take dma_get_required_mask into 
account would also be good.

Most drivers just do the fallback themselves, right?  So it makes sense to 
just update the current code to fallback, and update drivers wanting specific 
mask values to check afterwards.  I hate to inflict that kind of driver wide 
update on Michael though... :)

Jesse
--

[ message continues ]

Well, that's a lot of work and I'm not sure it's worth it.
I could live with having dma_set_mask as an API that fails on bad masks
and dma_request_mask as an API above that which retries. I think that's
just fine. Drivers can be migrated over time to the new API (or not. That
can be the driver maintainer's choice).

-- 
Greetings Michael.
--

[ message continues ]

Can you also update the docs with the new call, indicating that it should 
probably used in all but special cases?

Thanks,
Jesse
--

[ message continues ]

Yeah sure. Forgot that.

-- 
Greetings Michael.
--

[ message continues ]

Oh and for dma_set_mask specifically I don't see that many callers, so 
updating the tree appears doable (meye, aic7xxx, lasai700, qla2xxx, 
sni_53c710, ssb & ehci in my quick look). pci_set_dma_mask otoh is used in 
lots more places (and iirc some platforms implement pci_set_dma_mask in terms 
of dma_set_mask, so small updates would be needed there).

Thanks,
Jesse
--

[ message continues ]

I was thinking about also adding a "request" call to the PCI DMA API,
as we have the same retry-issue there.

-- 
Greetings Michael.
--

[ message continues ]

We have tons of drivers that go

	try and set 64bit 
	oh failed
	try and set 32 bit
	oh worked

When what most actually want is "set it no more than this wide" - a
request or even a "pci_set_best_mask()" type idea.

Lots less code duplication.
--

[ message continues ]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>

Agreed, it's a templated construct and it belongs in one
spot.
--

[ message continues ]

On Thu, 1 May 2008 16:38:15 +0200

can we just make the existing API do this instead?
--

[ message continues ]