[PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Ben Collins]

At least on PPC, the "op ? op : dma" construct causes a compile failure
because the dma_* is a do{}while(0) macro.

This turns all of them into proper if/else to avoid this problem.

Signed-off-by: Ben Collins <bcollins@ubuntu.com>

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index fd2353f..3c2e105 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1456,9 +1456,9 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd
  */
 static inline int ib_dma_mapping_error(struct ib_device *dev, u64 dma_addr)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->mapping_error(dev, dma_addr) :
-		dma_mapping_error(dma_addr);
+	if (dev->dma_ops)
+		return dev->dma_ops->mapping_error(dev, dma_addr);
+	return dma_mapping_error(dma_addr);
 }
 
 /**
@@ -1472,9 +1472,9 @@ static inline u64 ib_dma_map_single(stru
 				    void *cpu_addr, size_t size,
 				    enum dma_data_direction direction)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->map_single(dev, cpu_addr, size, direction) :
-		dma_map_single(dev->dma_device, cpu_addr, size, direction);
+	if (dev->dma_ops)
+		return dev->dma_ops->map_single(dev, cpu_addr, size, direction);
+	return dma_map_single(dev->dma_device, cpu_addr, size, direction);
 }
 
 /**
@@ -1488,8 +1488,9 @@ static inline void ib_dma_unmap_single(s
 				       u64 addr, size_t size,
 				       enum dma_data_direction direction)
 {
-	dev->dma_ops ?
-		dev->dma_ops->unmap_single(dev, addr, size, direction) :
+	if (dev->dma_ops)
+		dev->dma_ops->unmap_single(dev, addr, size, direction);
+	else
 		dma_unmap_single(dev->dma_device, addr, size, direction);
 }
 
@@ -1507,9 +1508,9 @@ static inline u64 ib_dma_map_page(struct
 				  size_t size,
 					 enum dma_data_direction direction)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->map_page(dev, page, offset, size, direction) :
-		dma_map_page(dev->dma_device, page, offset, size, direction);
+	if (dev->dma_ops)
+		return dev->dma_ops->map_page(dev, page, offset, size, direction);
+	return dma_map_page(dev->dma_device, page, offset, size, direction);
 }
 
 /**
@@ -1523,8 +1524,9 @@ static inline void ib_dma_unmap_page(str
 				     u64 addr, size_t size,
 				     enum dma_data_direction direction)
 {
-	dev->dma_ops ?
-		dev->dma_ops->unmap_page(dev, addr, size, direction) :
+	if (dev->dma_ops)
+		dev->dma_ops->unmap_page(dev, addr, size, direction);
+	else
 		dma_unmap_page(dev->dma_device, addr, size, direction);
 }
 
@@ -1539,9 +1541,9 @@ static inline int ib_dma_map_sg(struct i
 				struct scatterlist *sg, int nents,
 				enum dma_data_direction direction)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->map_sg(dev, sg, nents, direction) :
-		dma_map_sg(dev->dma_device, sg, nents, direction);
+	if (dev->dma_ops)
+		return dev->dma_ops->map_sg(dev, sg, nents, direction);
+	return dma_map_sg(dev->dma_device, sg, nents, direction);
 }
 
 /**
@@ -1555,8 +1557,9 @@ static inline void ib_dma_unmap_sg(struc
 				   struct scatterlist *sg, int nents,
 				   enum dma_data_direction direction)
 {
-	dev->dma_ops ?
-		dev->dma_ops->unmap_sg(dev, sg, nents, direction) :
+	if (dev->dma_ops)
+		dev->dma_ops->unmap_sg(dev, sg, nents, direction);
+	else
 		dma_unmap_sg(dev->dma_device, sg, nents, direction);
 }
 
@@ -1568,8 +1571,9 @@ static inline void ib_dma_unmap_sg(struc
 static inline u64 ib_sg_dma_address(struct ib_device *dev,
 				    struct scatterlist *sg)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->dma_address(dev, sg) : sg_dma_address(sg);
+	if (dev->dma_ops)
+		return dev->dma_ops->dma_address(dev, sg);
+	return sg_dma_address(sg);
 }
 
 /**
@@ -1580,8 +1584,9 @@ static inline u64 ib_sg_dma_address(stru
 static inline unsigned int ib_sg_dma_len(struct ib_device *dev,
 					 struct scatterlist *sg)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->dma_len(dev, sg) : sg_dma_len(sg);
+	if (dev->dma_ops)
+		return dev->dma_ops->dma_len(dev, sg);
+	return sg_dma_len(sg);
 }
 
 /**
@@ -1596,8 +1601,9 @@ static inline void ib_dma_sync_single_fo
 					      size_t size,
 					      enum dma_data_direction dir)
 {
-	dev->dma_ops ?
-		dev->dma_ops->sync_single_for_cpu(dev, addr, size, dir) :
+	if (dev->dma_ops)
+		dev->dma_ops->sync_single_for_cpu(dev, addr, size, dir);
+	else
 		dma_sync_single_for_cpu(dev->dma_device, addr, size, dir);
 }
 
@@ -1613,8 +1619,9 @@ static inline void ib_dma_sync_single_fo
 						 size_t size,
 						 enum dma_data_direction dir)
 {
-	dev->dma_ops ?
-		dev->dma_ops->sync_single_for_device(dev, addr, size, dir) :
+	if (dev->dma_ops)
+		dev->dma_ops->sync_single_for_device(dev, addr, size, dir);
+	else
 		dma_sync_single_for_device(dev->dma_device, addr, size, dir);
 }
 
@@ -1630,9 +1637,9 @@ static inline void *ib_dma_alloc_coheren
 					   u64 *dma_handle,
 					   gfp_t flag)
 {
-	return dev->dma_ops ?
-		dev->dma_ops->alloc_coherent(dev, size, dma_handle, flag) :
-		dma_alloc_coherent(dev->dma_device, size, dma_handle, flag);
+	if (dev->dma_ops)
+		return dev->dma_ops->alloc_coherent(dev, size, dma_handle, flag);
+	return dma_alloc_coherent(dev->dma_device, size, dma_handle, flag);
 }
 
 /**
@@ -1646,8 +1653,9 @@ static inline void ib_dma_free_coherent(
 					size_t size, void *cpu_addr,
 					u64 dma_handle)
 {
-	dev->dma_ops ?
-		dev->dma_ops->free_coherent(dev, size, cpu_addr, dma_handle) :
+	if (dev->dma_ops)
+		dev->dma_ops->free_coherent(dev, size, cpu_addr, dma_handle);
+	else
 		dma_free_coherent(dev->dma_device, size, cpu_addr, dma_handle);
 }
 

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Roland Dreier]

I see Linus already took this, which is fine... blame me for merging
this without fixing my cross-compile testbed.

Anyway:

 >  static inline int ib_dma_mapping_error(struct ib_device *dev, u64 dma_addr)
 >  {
 > -	return dev->dma_ops ?
 > -		dev->dma_ops->mapping_error(dev, dma_addr) :
 > -		dma_mapping_error(dma_addr);
 > +	if (dev->dma_ops)
 > +		return dev->dma_ops->mapping_error(dev, dma_addr);
 > +	return dma_mapping_error(dma_addr);

This stuff wasn't needed, was it?  It's only the wrappers around void
functions that can't use ?: I would think... surely any trivial macro
replacement for a dma API function that returns a value must evaluate
to something like (0) that is safe to use in this context.

 - R.

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Al Viro]

On Wed, Dec 13, 2006 at 10:10:05PM -0500, Ben Collins wrote:
> At least on PPC, the "op ? op : dma" construct causes a compile failure
> because the dma_* is a do{}while(0) macro.
> 
> This turns all of them into proper if/else to avoid this problem.

NAK.

Proper fix is to kill stupid do { } while (0) mess.  It's supposed
to behave like a function returning void, so it should be ((void)0).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

diff --git a/include/asm-alpha/dma-mapping.h b/include/asm-alpha/dma-mapping.h
index 57e09f5..ce759ea 100644
--- a/include/asm-alpha/dma-mapping.h
+++ b/include/asm-alpha/dma-mapping.h
@@ -41,9 +41,9 @@ #define dma_supported(dev, mask)		(mask 
 #define dma_map_single(dev, va, size, dir)	virt_to_phys(va)
 #define dma_map_page(dev, page, off, size, dir)	(page_to_pa(page) + off)
 
-#define dma_unmap_single(dev, addr, size, dir)	do { } while (0)
-#define dma_unmap_page(dev, addr, size, dir)	do { } while (0)
-#define dma_unmap_sg(dev, sg, nents, dir)	do { } while (0)
+#define dma_unmap_single(dev, addr, size, dir)	((void)0)
+#define dma_unmap_page(dev, addr, size, dir)	((void)0)
+#define dma_unmap_sg(dev, sg, nents, dir)	((void)0)
 
 #define dma_mapping_error(addr)  (0)
 
@@ -55,12 +55,12 @@ #define dma_is_consistent(d, h)			(1)
 
 int dma_set_mask(struct device *dev, u64 mask);
 
-#define dma_sync_single_for_cpu(dev, addr, size, dir)	  do { } while (0)
-#define dma_sync_single_for_device(dev, addr, size, dir)  do { } while (0)
-#define dma_sync_single_range(dev, addr, off, size, dir)  do { } while (0)
-#define dma_sync_sg_for_cpu(dev, sg, nents, dir)	  do { } while (0)
-#define dma_sync_sg_for_device(dev, sg, nents, dir)	  do { } while (0)
-#define dma_cache_sync(dev, va, size, dir)		  do { } while (0)
+#define dma_sync_single_for_cpu(dev, addr, size, dir)	  ((void)0)
+#define dma_sync_single_for_device(dev, addr, size, dir)  ((void)0)
+#define dma_sync_single_range(dev, addr, off, size, dir)  ((void)0)
+#define dma_sync_sg_for_cpu(dev, sg, nents, dir)	  ((void)0)
+#define dma_sync_sg_for_device(dev, sg, nents, dir)	  ((void)0)
+#define dma_cache_sync(dev, va, size, dir)		  ((void)0)
 
 #define dma_get_cache_alignment()			  L1_CACHE_BYTES
 
diff --git a/include/asm-powerpc/dma-mapping.h b/include/asm-powerpc/dma-mapping.h
index 7c7de87..a19a6f1 100644
--- a/include/asm-powerpc/dma-mapping.h
+++ b/include/asm-powerpc/dma-mapping.h
@@ -37,9 +37,9 @@ #else /* ! CONFIG_NOT_COHERENT_CACHE */
  */
 
 #define __dma_alloc_coherent(gfp, size, handle)	NULL
-#define __dma_free_coherent(size, addr)		do { } while (0)
-#define __dma_sync(addr, size, rw)		do { } while (0)
-#define __dma_sync_page(pg, off, sz, rw)	do { } while (0)
+#define __dma_free_coherent(size, addr)		((void)0)
+#define __dma_sync(addr, size, rw)		((void)0)
+#define __dma_sync_page(pg, off, sz, rw)	((void)0)
 
 #endif /* ! CONFIG_NOT_COHERENT_CACHE */
 
@@ -251,7 +251,7 @@ dma_map_single(struct device *dev, void 
 }
 
 /* We do nothing. */
-#define dma_unmap_single(dev, addr, size, dir)	do { } while (0)
+#define dma_unmap_single(dev, addr, size, dir)	((void)0)
 
 static inline dma_addr_t
 dma_map_page(struct device *dev, struct page *page,
@@ -266,7 +266,7 @@ dma_map_page(struct device *dev, struct 
 }
 
 /* We do nothing. */
-#define dma_unmap_page(dev, handle, size, dir)	do { } while (0)
+#define dma_unmap_page(dev, handle, size, dir)	((void)0)
 
 static inline int
 dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
@@ -286,7 +286,7 @@ dma_map_sg(struct device *dev, struct sc
 }
 
 /* We don't do anything here. */
-#define dma_unmap_sg(dev, sg, nents, dir)	do { } while (0)
+#define dma_unmap_sg(dev, sg, nents, dir)	((void)0)
 
 #endif /* CONFIG_PPC64 */
 

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Al Viro]

On Thu, Dec 14, 2006 at 06:44:30AM +0000, Al Viro wrote:
> On Wed, Dec 13, 2006 at 10:10:05PM -0500, Ben Collins wrote:
> > At least on PPC, the "op ? op : dma" construct causes a compile failure
> > because the dma_* is a do{}while(0) macro.
> > 
> > This turns all of them into proper if/else to avoid this problem.
> 
> NAK.
> 
> Proper fix is to kill stupid do { } while (0) mess.  It's supposed
> to behave like a function returning void, so it should be ((void)0).

BTW, even though the original patch is already merged, I think that
we ought to get rid of do-while in such stubs, exactly to avoid such
problems in the future.  Probably even add to CodingStyle - it's not
the first time such crap happens.

IOW, do ; while(0) / do { } while (0)  is not a proper way to do a macro
that imitates a function returning void.

Objections?

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Roland Dreier]

 > IOW, do ; while(0) / do { } while (0)  is not a proper way to do a macro
 > that imitates a function returning void.
 > 
 > Objections?

None from me, although the ternary ? : is a pretty odd way to write

	if (blah)
		do_this_void_function();
	else
		do_that_void_function();

so I'm in favor of that half of the patch anyway.  It's my fault for
not noticing that part of the patch in the first place.

Changing the non-void ? : constructions is just churn, but there's no
sense changing it again now that the patch is merged.

 - R.

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Ben Collins]

On Wed, 2006-12-13 at 23:45 -0800, Roland Dreier wrote:
>  > IOW, do ; while(0) / do { } while (0)  is not a proper way to do a macro
>  > that imitates a function returning void.
>  > 
>  > Objections?
> 
> None from me, although the ternary ? : is a pretty odd way to write
> 
> 	if (blah)
> 		do_this_void_function();
> 	else
> 		do_that_void_function();
> 
> so I'm in favor of that half of the patch anyway.  It's my fault for
> not noticing that part of the patch in the first place.
> 
> Changing the non-void ? : constructions is just churn, but there's no
> sense changing it again now that the patch is merged.

The rest of it was just for consistency sake.

Re: [PATCH 2.6.20-rc1] ib_verbs: Use explicit if-else statements to avoid errors with do-while macros

[Andrew Morton]

On Thu, 14 Dec 2006 06:56:24 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Thu, Dec 14, 2006 at 06:44:30AM +0000, Al Viro wrote:
> > On Wed, Dec 13, 2006 at 10:10:05PM -0500, Ben Collins wrote:
> > > At least on PPC, the "op ? op : dma" construct causes a compile failure
> > > because the dma_* is a do{}while(0) macro.
> > > 
> > > This turns all of them into proper if/else to avoid this problem.
> > 
> > NAK.
> > 
> > Proper fix is to kill stupid do { } while (0) mess.  It's supposed
> > to behave like a function returning void, so it should be ((void)0).
> 
> BTW, even though the original patch is already merged, I think that
> we ought to get rid of do-while in such stubs, exactly to avoid such
> problems in the future.  Probably even add to CodingStyle - it's not
> the first time such crap happens.
> 
> IOW, do ; while(0) / do { } while (0)  is not a proper way to do a macro
> that imitates a function returning void.
> 
> Objections?

Would prefer static inline void foo(args){} when possible - for the arg
typechecking and arg existence checking and unused variable warnings.

I end up having to do rather a lot of things like this:

--- a/mm/vmalloc.c~virtual-memmap-on-sparsemem-v3-map-and-unmap-fix-2
+++ a/mm/vmalloc.c
@@ -929,6 +929,6 @@ int unmap_generic_kernel(unsigned long a
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
-	flush_tlb_kernel_range((unsigned long)start_addr, end_addr);
+	flush_tlb_kernel_range(addr, addr);
 	return err;
 }


and this:


@@ -85,12 +84,24 @@ extern void vm_events_fold_cpu(int cpu);
 #else
 
 /* Disable counters */
-#define get_cpu_vm_events(e)	0L
-#define count_vm_event(e)	do { } while (0)
-#define count_vm_events(e,d)	do { } while (0)
-#define __count_vm_event(e)	do { } while (0)
-#define __count_vm_events(e,d)	do { } while (0)
-#define vm_events_fold_cpu(x)	do { } while (0)
+static inline void count_vm_event(enum vm_event_item item)
+{
+}
+static inline void count_vm_events(enum vm_event_item item, long delta)
+{
+}
+static inline void __count_vm_event(enum vm_event_item item)
+{
+}
+static inline void __count_vm_events(enum vm_event_item item, long delta)
+{
+}
+static inline void all_vm_events(unsigned long *ret)
+{
+}
+static inline void vm_events_fold_cpu(int cpu)
+{
+}

because of these problems.

Plus macros are putrid.