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 <[email protected]>
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);
}
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.
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 <[email protected]>
---
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 */
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?
> 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.
On Thu, 14 Dec 2006 06:56:24 +0000
Al Viro <[email protected]> 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.
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.