Skip to contentSkip to conversation

Add API for weak DMA masks

PATCH In review Michael Buesch 6 participants 21 messages May 1, 2008 – May 2, 2008

Subsystems (4)
Driver core Networking drivers (wireless) DMA mapping helpers Sonics silicon backplane driver (ssb)
lore
Share & tools
lore

Michael Buesch

author
#0 lore raw

cover Add API for weak DMA masks

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.

Signature · 1 line
Greetings Michael.
author
#1 lore raw

PATCH 1/3 Add dma_set_mask_weak() API

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 <[email protected]>
drivers/base/dma-mapping.c +35 -0
Index: wireless-testing/drivers/base/dma-mapping.c
===================================================================
@@ -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);
include/linux/dma-mapping.h +5 -0
Index: wireless-testing/include/linux/dma-mapping.h
===================================================================
@@ -60,6 +60,11 @@ static inline int is_device_dma_capable(
 
 extern u64 dma_get_required_mask(struct device *dev);
 
+extern int dma_set_mask_weak(struct device *dev, u64 *mask);
+/* Smallest mask fallback used by dma_set_mask_weak(). */
+#define DMA_MIN_FALLBACK_MASK	DMA_BIT_MASK(24) /* 16 MB */
+
+
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
 {
 	return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536;
Signature · 1 line
Greetings Michael.
#2 lore raw

Re: [PATCH 1/3] Add dma_set_mask_weak() API

Quoted 2 lines · On Thu, 1 May 2008 16:40:16 +0200 Michael Buesch <[email protected]> wrote:

This adds a new DMA subsystem API call for "weak" setting of the DMA mask

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

Alan

author
#3 lore raw

Re: [PATCH 1/3] Add dma_set_mask_weak() API

Quoted 8 lines · On Thursday 01 May 2008 18:20:45 Alan Cox wrote:
Quoted 2 lines · On Thu, 1 May 2008 16:40:16 +0200 Michael Buesch <[email protected]> wrote:

This adds a new DMA subsystem API call for "weak" setting of the DMA mask

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

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.

Signature · 1 line
Greetings Michael.
author
#4 lore raw

PATCH 2/3 ssb: Add weak DMA-mask API

In reply to Michael Buesch

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

Signed-off-by Michael Buesch <[email protected]>
drivers/ssb/main.c +21 -2
Index: linux-2.6/drivers/ssb/main.c
===================================================================
@@ -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;
include/linux/ssb/ssb.h +1 -0
Index: linux-2.6/include/linux/ssb/ssb.h
===================================================================
@@ -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);
 
 
 #ifdef CONFIG_SSB_PCIHOST
Signature · 1 line
Greetings Michael.
author
#5 lore raw

PATCH 3/3 b43: Use the new weak DMA-mask API

In reply to Michael Buesch

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

Signed-off-by Michael Buesch <[email protected]>
drivers/net/wireless/b43/dma.c +11 -21
Index: linux-2.6/drivers/net/wireless/b43/dma.c
===================================================================
@@ -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;
Signature · 1 line
Greetings Michael.
#6 lore raw
In reply to Michael Buesch
Quoted 7 lines · Michael Buesch

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.

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

author
#7 lore raw
Quoted 11 lines · On Thursday 01 May 2008 17:36:18 Christoph Hellwig wrote:
Quoted 7 lines · Michael Buesch

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.

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

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.

Signature · 1 line
Greetings Michael.
#8 lore raw
Quoted 8 lines · Michael Buesch

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.

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.

author
#9 lore raw
Quoted 11 lines · On Thursday 01 May 2008 17:43:58 Christoph Hellwig wrote:
Quoted 8 lines · Michael Buesch

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.

Yeah. Personally I'd rather let set_dma_mask fall back silently,

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.

Signature · 1 line
Greetings Michael.
#10 lore raw
Quoted 3 lines · Michael Buesch

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.

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

author
#11 lore raw
Quoted 7 lines · On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
Quoted 3 lines · Michael Buesch

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.

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

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.

Signature · 1 line
Greetings Michael.
#12 lore raw
Quoted 6 lines · Michael Buesch
Quoted 5 lines · On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
Quoted 3 lines · Michael Buesch

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.

Quoted 8 lines
Quoted 2 lines

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

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.

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

author
#13 lore raw
Quoted 16 lines · On Thursday 01 May 2008 18:30:04 Jesse Barnes wrote:
Quoted 6 lines · Michael Buesch
Quoted 5 lines · On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
Quoted 3 lines · Michael Buesch

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.

Quoted 8 lines
Quoted 2 lines

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

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.

Quoted 3 lines

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.

Ok, will redo the patches with that added and the name changed.

Quoted 1 line

Most drivers just do the fallback themselves, right?

Right.

Quoted 4 lines

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

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

Signature · 1 line
Greetings Michael.
#14 lore raw
Quoted 11 lines · Michael Buesch

Ok, will redo the patches with that added and the name changed.

Quoted 1 line

Most drivers just do the fallback themselves, right?

Right.

Quoted 4 lines

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

Quoted 5 lines

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

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

Thanks,
Jesse

author
#15 lore raw
Quoted 12 lines · On Thursday 01 May 2008 19:22:55 Jesse Barnes wrote:
Quoted 11 lines · Michael Buesch

Ok, will redo the patches with that added and the name changed.

Quoted 1 line

Most drivers just do the fallback themselves, right?

Right.

Quoted 4 lines

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

Quoted 8 lines
Quoted 5 lines

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

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

Yeah sure. Forgot that.

Signature · 1 line
Greetings Michael.
#16 lore raw
In reply to Michael Buesch
Quoted 10 lines · Michael Buesch
Quoted 4 lines

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

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

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

author
#17 lore raw
Quoted 12 lines · On Thursday 01 May 2008 19:27:35 Jesse Barnes wrote:
Quoted 10 lines · Michael Buesch
Quoted 4 lines

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

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

Quoted 5 lines

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

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

Signature · 1 line
Greetings Michael.
#18 lore raw
In reply to Christoph Hellwig
Quoted 2 lines

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

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.

#19 lore raw

From: Alan Cox <[email protected]>
Date: Thu, 1 May 2008 17:29:13 +0100

Quoted 11 lines

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.

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

#20 lore raw
In reply to Michael Buesch
Quoted 7 lines · On Thu, 1 May 2008 16:38:15 +0200 Michael Buesch <[email protected]> wrote:

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.

can we just make the existing API do this instead?