Re: [PATCH 0/3] Add API for weak DMA masks

Re: [PATCH 0/3] Add API for weak DMA masks

[Arjan van de Ven]

On Thu, 1 May 2008 16:38:15 +0200
Michael Buesch <mb@bu3sch.de> 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?

[PATCH 0/3] Add API for weak DMA masks

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

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.

[PATCH 1/3] Add dma_set_mask_weak() API

[Michael Buesch]

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


-- 
Greetings Michael.

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

[Michael Buesch]

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);
 
 
 #ifdef CONFIG_SSB_PCIHOST

-- 
Greetings Michael.

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

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

Re: [PATCH 0/3] Add API for weak DMA masks

[Christoph Hellwig]

On Thu, May 01, 2008 at 04:38:15PM +0200, Michael Buesch 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.

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

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 17:36:18 Christoph Hellwig wrote:
> On Thu, May 01, 2008 at 04:38:15PM +0200, Michael Buesch 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.
> 
> 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.

-- 
Greetings Michael.

Re: [PATCH 0/3] Add API for weak DMA masks

[Christoph Hellwig]

On Thu, May 01, 2008 at 05:42:04PM +0200, Michael Buesch wrote:
> 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.

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 17:43:58 Christoph Hellwig wrote:
> On Thu, May 01, 2008 at 05:42:04PM +0200, Michael Buesch wrote:
> > 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.

-- 
Greetings Michael.

Re: [PATCH 0/3] Add API for weak DMA masks

[Christoph Hellwig]

On Thu, May 01, 2008 at 05:47:26PM +0200, Michael Buesch wrote:
> 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.

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
> On Thu, May 01, 2008 at 05:47:26PM +0200, Michael Buesch wrote:
> > 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.

-- 
Greetings Michael.

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

[Alan Cox]

On Thu, 1 May 2008 16:40:16 +0200
Michael Buesch <mb@bu3sch.de> 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

Re: [PATCH 0/3] Add API for weak DMA masks

[Alan Cox]

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

Re: [PATCH 0/3] Add API for weak DMA masks

[Jesse Barnes]

On Thursday, May 01, 2008 9:07 am Michael Buesch wrote:
> On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
> > On Thu, May 01, 2008 at 05:47:26PM +0200, Michael Buesch wrote:
> > > 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.

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

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

[Michael Buesch]

On Thursday 01 May 2008 18:20:45 Alan Cox wrote:
> On Thu, 1 May 2008 16:40:16 +0200
> Michael Buesch <mb@bu3sch.de> 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.


-- 
Greetings Michael.

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 18:30:04 Jesse Barnes wrote:
> On Thursday, May 01, 2008 9:07 am Michael Buesch wrote:
> > On Thursday 01 May 2008 17:58:26 Christoph Hellwig wrote:
> > > On Thu, May 01, 2008 at 05:47:26PM +0200, Michael Buesch wrote:
> > > > 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.
> 
> 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.

> Most drivers just do the fallback themselves, right?

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

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.

Re: [PATCH 0/3] Add API for weak DMA masks

[Jesse Barnes]

On Thursday, May 01, 2008 10:16 am Michael Buesch wrote:
> Ok, will redo the patches with that added and the name changed.
>
> > Most drivers just do the fallback themselves, right?
>
> 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... :)
>
> 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

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 19:22:55 Jesse Barnes wrote:
> On Thursday, May 01, 2008 10:16 am Michael Buesch wrote:
> > Ok, will redo the patches with that added and the name changed.
> >
> > > Most drivers just do the fallback themselves, right?
> >
> > 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... :)
> >
> > 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.

-- 
Greetings Michael.

Re: [PATCH 0/3] Add API for weak DMA masks

[Jesse Barnes]

On Thursday, May 01, 2008 10:16 am Michael Buesch wrote:
> > 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

Re: [PATCH 0/3] Add API for weak DMA masks

[Michael Buesch]

On Thursday 01 May 2008 19:27:35 Jesse Barnes wrote:
> On Thursday, May 01, 2008 10:16 am Michael Buesch wrote:
> > > 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).

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

-- 
Greetings Michael.

Re: [PATCH 0/3] Add API for weak DMA masks

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Thu, 1 May 2008 17:29:13 +0100

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