Use BUG_ON(foo) instead of "if (foo) BUG()" in include/asm-i386/dma-mapping.h

PATCH In review Rolf Eike Beer 8 participants 15 messages Jul 28, 2006 – Aug 7, 2006 DMA mapping helpers

lore

Share & tools

lore

Rolf Eike Beer

author

2006-07-28 07:26 UTC #0 lore raw

PATCH 1/1 Use BUG_ON(foo) instead of "if (foo) BUG()" in include/asm-i386/dma-mapping.h

We have BUG_ON() right for this, don't we?

Signed-off-by Rolf Eike Beer <[email protected]>

---
commit e3f6da52c54970711fb90ffb0f6765f8fd0ee93e
tree 093ee6b4d8fb087a54c2c90e006b66783186aaff
parent e7156df18ec5f0ee8c9fe7ac0c0367ff9a1532e8
author Rolf Eike Beer <[email protected]> Fri, 28 Jul 2006 09:27:31 +0200 committer Rolf Eike Beer <[email protected]> Fri, 28 Jul 2006 09:27:31 +0200

 include/asm-i386/dma-mapping.h |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

include/asm-i386/dma-mapping.h +3 -6

index 9cf20ca..576ae01 100644
@@ -21,8 +21,7 @@ static inline dma_addr_t
 dma_map_single(struct device *dev, void *ptr, size_t size,
 	       enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE)
-		BUG();
+	BUG_ON(direction == DMA_NONE);
 	WARN_ON(size == 0);
 	flush_write_buffers();
 	return virt_to_phys(ptr);

@@ -32,8 +31,7 @@ static inline void
 dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
 		 enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE)
-		BUG();
+	BUG_ON(direction == DMA_NONE);
 }
 
 static inline int

@@ -42,8 +40,7 @@ dma_map_sg(struct device *dev, struct sc
 {
 	int i;
 
-	if (direction == DMA_NONE)
-		BUG();
+	BUG_ON(direction == DMA_NONE);
 	WARN_ON(nents == 0 || sg[0].length == 0);
 
 	for (i = 0; i < nents; i++ ) {

Andrew Morton

2006-07-28 07:48 UTC #1 lore raw

Quoted 1 line · On Fri, 28 Jul 2006 09:28:49 +0200 Rolf Eike Beer <[email protected]> wrote:

We have BUG_ON() right for this, don't we?

Well yes, but there are over a thousand BUG->BUG_ON conversion possibilities in the tree. If people start sending them three-at-a-time we'll all go mad.

So. If we're going to do this, bigger patches, please.

Muli Ben-Yehuda

2006-07-28 17:44 UTC #4 lore raw

In reply to Rolf Eike Beer

Quoted 1 line · Rolf Eike Beer

We have BUG_ON() right for this, don't we?

Even better, we have valid_dma_direction() in x86-64. Care to move it to generic code and update i386 to use it?

Cheers,
Muli

Rolf Eike Beer

author

2006-08-02 15:22 UTC #5 lore raw

PATCH 1/1 Use valid_dma_direction() in include/asm-i386/dma-mapping.h

Now that the generic DMA code has a function to decide if a given DMA mapping is valid use it. This will catch cases where direction is not any of the defined enum values but some random number outside the valid range. The current implementation will only catch the defined but invalid case DMA_NONE.

Signed-off-by Rolf Eike Beer

---
commit 61f76f37d18da432ea1b55b92c98dd39388077f0
tree e8c5d79bdb2e2b786b4d5c719d64f28dfc54c4fb
parent 9f990495512e3f106ce56f885a675636b47ff421
author Rolf Eike Beer <[email protected]> Wed, 02 Aug 2006 17:11:04 +0200 committer Rolf Eike Beer <[email protected]> Wed, 02 Aug 2006 17:11:04 +0200

 include/asm-i386/dma-mapping.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

include/asm-i386/dma-mapping.h +6 -6

index 576ae01..81999a3 100644
@@ -21,7 +21,7 @@ static inline dma_addr_t
 dma_map_single(struct device *dev, void *ptr, size_t size,
 	       enum dma_data_direction direction)
 {
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 	WARN_ON(size == 0);
 	flush_write_buffers();
 	return virt_to_phys(ptr);

@@ -31,7 +31,7 @@ static inline void
 dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
 		 enum dma_data_direction direction)
 {
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 }
 
 static inline int

@@ -40,7 +40,7 @@ dma_map_sg(struct device *dev, struct sc
 {
 	int i;
 
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 	WARN_ON(nents == 0 || sg[0].length == 0);
 
 	for (i = 0; i < nents; i++ ) {

@@ -57,7 +57,7 @@ static inline dma_addr_t
 dma_map_page(struct device *dev, struct page *page, unsigned long offset,
 	     size_t size, enum dma_data_direction direction)
 {
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 	return page_to_phys(page) + offset;
 }

@@ -65,7 +65,7 @@ static inline void
 dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
 	       enum dma_data_direction direction)
 {
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 }

@@ -73,7 +73,7 @@ static inline void
 dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
 	     enum dma_data_direction direction)
 {
-	BUG_ON(direction == DMA_NONE);
+	BUG_ON(!valid_dma_direction(direction));
 }
 
 static inline void

Rolf Eike Beer

author

2006-08-02 15:22 UTC #7 lore raw

PATCH 1/1 Move valid_dma_direction() from x86_64 to generic code

In reply to Muli Ben-Yehuda

As suggested by Muli Ben-Yehuda this function is moved to generic code as may be useful for all archs.

Signed-off-by Rolf Eike Beer <[email protected]>

---
commit 9f990495512e3f106ce56f885a675636b47ff421
tree ad2864a5204c57d9836ff2360d6bf80c0d2246e3
parent 3ac898b16fb75cd996ec98fa578ea06b6ffb2760
author Rolf Eike Beer <[email protected]> Wed, 02 Aug 2006 17:07:56 +0200 committer Rolf Eike Beer <[email protected]> Wed, 02 Aug 2006 17:07:56 +0200

 include/asm-x86_64/dma-mapping.h |    7 -------
 include/linux/dma-mapping.h      |    7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

include/asm-x86_64/dma-mapping.h +0 -7

index b6da83d..10174b1 100644
@@ -55,13 +55,6 @@ extern dma_addr_t bad_dma_address;
 extern struct dma_mapping_ops* dma_ops;
 extern int iommu_merge;
 
-static inline int valid_dma_direction(int dma_direction)
-{
-	return ((dma_direction == DMA_BIDIRECTIONAL) ||
-		(dma_direction == DMA_TO_DEVICE) ||
-		(dma_direction == DMA_FROM_DEVICE));
-}
-
 static inline int dma_mapping_error(dma_addr_t dma_addr)
 {
 	if (dma_ops->mapping_error)

include/linux/dma-mapping.h +7 -0

index 635690c..ff203c4 100644
@@ -24,6 +24,13 @@ #define DMA_29BIT_MASK	0x000000001ffffff
 #define DMA_28BIT_MASK	0x000000000fffffffULL
 #define DMA_24BIT_MASK	0x0000000000ffffffULL
 
+static inline int valid_dma_direction(int dma_direction)
+{
+	return ((dma_direction == DMA_BIDIRECTIONAL) ||
+		(dma_direction == DMA_TO_DEVICE) ||
+		(dma_direction == DMA_FROM_DEVICE));
+}
+
 #include <asm/dma-mapping.h>
 
 /* Backwards compat, remove in 2.7.x */

Muli Ben-Yehuda

2006-08-02 18:55 UTC #8 lore raw

Re: [PATCH] Move valid_dma_direction() from x86_64 to generic code

Quoted 2 lines · Rolf Eike Beer

As suggested by Muli Ben-Yehuda this function is moved to generic code as may be useful for all archs.

I like it, but ...

Quoted 5 lines

include/asm-x86_64/dma-mapping.h +0 -0
index b6da83d..10174b1 100644

include/asm-x86_64/dma-mapping.h extern dma_addr_t bad_dma_address; · line 64

Quoted 10 lines

include/asm-x86_64/dma-mapping.h +0 -7

@@ -55,13 +55,6 @@ extern dma_addr_t bad_dma_address;
 extern struct dma_mapping_ops* dma_ops;
 extern int iommu_merge;
 
-static inline int valid_dma_direction(int dma_direction)
-{
-	return ((dma_direction == DMA_BIDIRECTIONAL) ||
-		(dma_direction == DMA_TO_DEVICE) ||
-		(dma_direction == DMA_FROM_DEVICE));
-}
-

Several files include asm/dma-mapping.h directly, which will now cause them to fail to compile on x86-64 due to the missing definition for valid_dma_direction, unless by chance another header already brought it in indirectly. I guess the right thing to do is convert them all to using linux/dma-mapping.h instead.

./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h> ./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>

Cheers,
Muli

Muli Ben-Yehuda

Acked-by on v1 1/1

2006-08-02 18:56 UTC #6 lore raw

Re: [PATCH] Use valid_dma_direction() in include/asm-i386/dma-mapping.h

In reply to Rolf Eike Beer

Quoted 7 lines · Rolf Eike Beer

Now that the generic DMA code has a function to decide if a given DMA mapping is valid use it. This will catch cases where direction is not any of the defined enum values but some random number outside the valid range. The current implementation will only catch the defined but invalid case DMA_NONE.
Signed-off-by Rolf Eike Beer

Acked-by Muli Ben-Yehuda <[email protected]>

... but see my comments to your other patch this one depends on.

Cheers,
Muli

Rolf Eike Beer

author

2006-08-03 06:22 UTC #9 lore raw

Re: [PATCH] Move valid_dma_direction() from x86_64 to generic code

In reply to Muli Ben-Yehuda

Quoted 11 lines · Muli Ben-Yehuda

Quoted 2 lines · Rolf Eike Beer
As suggested by Muli Ben-Yehuda this function is moved to generic code as may be useful for all archs.
I like it, but ...
Several files include asm/dma-mapping.h directly, which will now cause them to fail to compile on x86-64 due to the missing definition for valid_dma_direction, unless by chance another header already brought it in indirectly. I guess the right thing to do is convert them all to using linux/dma-mapping.h instead.

Yes, akpm already fixed it up. Sorry guys, currently no x86 kernel around here. Need to fix that.

Quoted 4 lines

./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h>

I suspect it to be a bug anyway that every of this files ever included asm/dma-mapping.h.

Quoted 1 line

./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>

This is perfectly valid, isn't it :)

Eike

Muli Ben-Yehuda

2006-08-03 07:23 UTC #10 lore raw

Re: [PATCH] Move valid_dma_direction() from x86_64 to generic code

Quoted 7 lines · Rolf Eike Beer

Quoted 4 lines
./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h>
I suspect it to be a bug anyway that every of this files ever included asm/dma-mapping.h.

Agreed wrt the fs_enet and fec_8xx; the swiotlb stuff I dimly recall I had a reason for. I'll take a look in bit to verify akpm's fix works.

Quoted 3 lines

Quoted 1 line
./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>
This is perfectly valid, isn't it :)

:-)

Cheers,
Muli

Duran, Leo

2006-08-03 21:11 UTC #11 lore raw

IOMMU (Calgary) patches

Andi, Jon, Muli,

I'm trying to build with the latest IOMMU patches for x86_64, so, I've pulled down 2.6.17, and the 2.6.18-rc3 patches... But, that's obviously lagging behind a bit.

Is there a source tree that you guys work from?
(I'm hoping that there's a better mechanism than keeping track of patches as they are submitted here).

Leo Duran.

-----Original Message-----
From: Muli Ben-Yehuda [mailto:[email protected]]
Sent: Thursday, August 03, 2006 2:24 AM
To: Rolf Eike Beer
Cc: Andrew Morton; [email protected]; Andi Kleen; [email protected]
Subject: [discuss] Re: [PATCH] Move valid_dma_direction() from x86_64 to generic code

Quoted 8 lines · Rolf Eike Beer

Quoted 4 lines
./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h>
I suspect it to be a bug anyway that every of this files ever included
asm/dma-mapping.h.

Agreed wrt the fs_enet and fec_8xx; the swiotlb stuff I dimly recall I had a reason for. I'll take a look in bit to verify akpm's fix works.

Quoted 3 lines

Quoted 1 line
./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>
This is perfectly valid, isn't it :)

:-)

Cheers,
Muli

Jon Mason

2006-08-03 21:26 UTC #12 lore raw

Re: IOMMU (Calgary) patches

Quoted 7 lines · Leo

Andi, Jon, Muli,
I'm trying to build with the latest IOMMU patches for x86_64, so, I've pulled down 2.6.17, and the 2.6.18-rc3 patches... But, that's obviously lagging behind a bit.
Is there a source tree that you guys work from?

Hey Leo,

I personally use the mercurial tree hosted on kernel.org
(http://www.kernel.org/hg/linux-2.6). Alternatively, you can use git to access the latest tree.

Quoted 2 lines

(I'm hoping that there's a better mechanism than keeping track of patches as they are submitted here).

Andrew Morton is good about pulling in the IOMMU patches as soon as we push them. So the -mm kernel is an option. Andi also queues up the patches in his firstfloor repository. So you can pull the patches from there.

So, if you can't wait for the latest kernel and don't want to get the latest kernel and patch it with Andi's patches on firstfloor, then the mm kernel will be the best option. But, the mercurial tree I mentioned above should be sufficient.

Thanks,
Jon

Quoted 37 lines

Leo Duran.
-----Original Message-----
From: Muli Ben-Yehuda [mailto:[email protected]]
Sent: Thursday, August 03, 2006 2:24 AM
To: Rolf Eike Beer
Cc: Andrew Morton; [email protected]; Andi Kleen; [email protected]
Subject: [discuss] Re: [PATCH] Move valid_dma_direction() from x86_64 to generic code
Quoted 8 lines · Rolf Eike Beer
Quoted 4 lines
./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h>
I suspect it to be a bug anyway that every of this files ever included
asm/dma-mapping.h.
Agreed wrt the fs_enet and fec_8xx; the swiotlb stuff I dimly recall I had a reason for. I'll take a look in bit to verify akpm's fix works.
Quoted 3 lines
Quoted 1 line
./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>
This is perfectly valid, isn't it :)
:-)
Cheers,
Muli

Duran, Leo

2006-08-03 21:36 UTC #13 lore raw

RE: IOMMU (Calgary) patches

Jon,
Many thanks. That helps.
Leo.

-----Original Message-----
From: Jon Mason [mailto:[email protected]]
Sent: Thursday, August 03, 2006 4:26 PM
To: Duran, Leo
Cc: Muli Ben-Yehuda; Andi Kleen; [email protected]; [email protected]
Subject: Re: IOMMU (Calgary) patches

Quoted 4 lines · Leo

Andi, Jon, Muli,
I'm trying to build with the latest IOMMU patches for x86_64, so, I've pulled down 2.6.17, and the 2.6.18-rc3 patches... But, that's

obviously

Quoted 3 lines

lagging behind a bit.
Is there a source tree that you guys work from?

Hey Leo,

I personally use the mercurial tree hosted on kernel.org
(http://www.kernel.org/hg/linux-2.6). Alternatively, you can use git to access the latest tree.

Quoted 2 lines

(I'm hoping that there's a better mechanism than keeping track of patches as they are submitted here).

Thanks,
Jon

Quoted 9 lines

Leo Duran.
-----Original Message-----
From: Muli Ben-Yehuda [mailto:[email protected]]
Sent: Thursday, August 03, 2006 2:24 AM
To: Rolf Eike Beer
Cc: Andrew Morton; [email protected]; Andi Kleen; [email protected]
Subject: [discuss] Re: [PATCH] Move valid_dma_direction() from x86_64

Quoted 10 lines

generic code
Quoted 6 lines · Rolf Eike Beer
Quoted 4 lines
./arch/x86_64/kernel/pci-swiotlb.c:6:#include <asm/dma-mapping.h> ./drivers/net/fec_8xx/fec_main.c:40:#include <asm/dma-mapping.h> ./drivers/net/fs_enet/fs_enet.h:11:#include <asm/dma-mapping.h> ./include/asm-x86_64/swiotlb.h:5:#include <asm/dma-mapping.h>
I suspect it to be a bug anyway that every of this files ever

included

Quoted 18 lines

Quoted 2 lines
asm/dma-mapping.h.
Agreed wrt the fs_enet and fec_8xx; the swiotlb stuff I dimly recall I had a reason for. I'll take a look in bit to verify akpm's fix works.
Quoted 3 lines
Quoted 1 line
./include/linux/dma-mapping.h:27:#include <asm/dma-mapping.h>
This is perfectly valid, isn't it :)
:-)
Cheers,
Muli

Andi Kleen

2006-08-03 22:46 UTC #14 lore raw

Re: [discuss] IOMMU (Calgary) patches

In reply to Duran, Leo

Quoted 7 lines · Leo

Andi, Jon, Muli,
I'm trying to build with the latest IOMMU patches for x86_64, so, I've pulled down 2.6.17, and the 2.6.18-rc3 patches... But, that's obviously lagging behind a bit.
Is there a source tree that you guys work from?

My latest source tree which contains all the submitted Calgary updates is in ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/ (in quilt format)

-Andi

Pavel Machek

2006-08-06 22:02 UTC #2 lore raw

In reply to Andrew Morton

Hi!

Quoted 7 lines

Quoted 1 line
We have BUG_ON() right for this, don't we?
Well yes, but there are over a thousand BUG->BUG_ON conversion possibilities in the tree. If people start sending them three-at-a-time we'll all go mad.
So. If we're going to do this, bigger patches, please.

If we are going that way... I guess we should specify if BUG_ON() has to evaluate its arguments even if it is compiled out...

Or probably better pecify that BUG_ON() must not have side effects?

Pavel

Signature · 1 line

Thanks for all the (sleeping) penguins.

Adrian Bunk

2006-08-07 01:20 UTC #3 lore raw

Quoted 8 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 1 line
We have BUG_ON() right for this, don't we?
Well yes, but there are over a thousand BUG->BUG_ON conversion possibilities in the tree. If people start sending them three-at-a-time we'll all go mad.

Quoted 5 lines

Quoted 1 line
So. If we're going to do this, bigger patches, please.
If we are going that way... I guess we should specify if BUG_ON() has to evaluate its arguments even if it is compiled out...
...

This is already implemented for ages.

Quoted 1 line

Pavel

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed