Linux MIPS Architecture development
 help / color / mirror / Atom feed
* [PATCH] dma_cache_wback, pci DMA cache coherency changes
@ 2002-06-18 17:03 William Jhun
  2002-06-19  9:39 ` Ralf Baechle
  0 siblings, 1 reply; 6+ messages in thread
From: William Jhun @ 2002-06-18 17:03 UTC (permalink / raw)
  To: "linux-mips

This is a re-hash of patches I sent out a while ago which do a more
optimal cache-flushing for pci_map_*() and pci_dma_sync_*(). It
basically does an invalidate for PCI_DMA_FROMDEVICE operations and a
writeback for PCI_DMA_TODEVICE pci_map_* (or writeback/invalidate if
PCI_DMA_BIDIRECTIONAL). This is similar to the ARM implementation.

Additionally, I filled in the _dma_cache_wback calls in the
arch/mips/c-*.c to call *_dma_cache_wback_inv* instead of calling
panic(). Some architectures could probably do a real writeback instead
of just wback_inv, but this will at least allow code that can use
writeback-only if available.

Note: I'm not familiar with a lot of these CPUs, but the change should
be innocuous. Could someone validate/improve these?

Thanks,
William

Index: include/asm/pci.h
===================================================================
RCS file: /cvs/linux/include/asm-mips/pci.h,v
retrieving revision 1.24.2.1
diff -u -r1.24.2.1 pci.h
--- include/asm/pci.h	2002/02/26 06:00:25	1.24.2.1
+++ include/asm/pci.h	2002/06/18 16:51:30
@@ -79,7 +79,40 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+#ifdef CONFIG_NONCOHERENT_IO
+/*
+ * Prepare buffer for DMA transfer
+ */
+static inline void prep_buffer(void *ptr, size_t size, int direction)
+{
+        switch(direction) {
+        case PCI_DMA_TODEVICE:
+                dma_cache_wback((unsigned long)ptr, size);
+                break;
+        case PCI_DMA_FROMDEVICE:
+                dma_cache_inv((unsigned long)ptr, size);
+                break;
+        case PCI_DMA_BIDIRECTIONAL:
+                dma_cache_wback_inv((unsigned long)ptr, size);
+                break;
+        }
+}
+
 /*
+ * Prepare buffer for CPU access after DMA transfer
+ */
+static inline void sync_buffer(void *ptr, size_t size, int direction)
+{
+        switch(direction) {
+        case PCI_DMA_FROMDEVICE:
+        case PCI_DMA_BIDIRECTIONAL:
+                dma_cache_inv((unsigned long)ptr, size);
+                break;
+        }
+}
+#endif
+
+/*
  * Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *
@@ -93,7 +126,7 @@
 		BUG();
 
 #ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv((unsigned long)ptr, size);
+	prep_buffer(ptr, size, direction);
 #endif
 
 	return virt_to_bus(ptr);
@@ -132,7 +165,7 @@
 	addr = (unsigned long) page_address(page);
 	addr += offset;
 #ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv(addr, size);
+	prep_buffer((void*)addr, size, direction);
 #endif
 
 	return virt_to_bus((void *)addr);
@@ -183,7 +216,7 @@
 #ifdef CONFIG_NONCOHERENT_IO
 	/* Make sure that gcc doesn't leave the empty loop body.  */
 	for (i = 0; i < nents; i++, sg++)
-		dma_cache_wback_inv((unsigned long)sg->address, sg->length);
+		prep_buffer(sg->address, sg->length, direction);
 #endif
 
 	return nents;
@@ -221,7 +254,7 @@
 		BUG();
 
 #ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv((unsigned long)bus_to_virt(dma_handle), size);
+	sync_buffer(bus_to_virt(dma_handle), size, direction);
 #endif
 }
 
@@ -246,8 +279,9 @@
 	/* Make sure that gcc doesn't leave the empty loop body.  */
 #ifdef CONFIG_NONCOHERENT_IO
 	for (i = 0; i < nelems; i++, sg++)
-		dma_cache_wback_inv((unsigned long)sg->address, sg->length);
+		sync_buffer(sg->address, sg->length, direction);
 #endif
+
 }
 
 /* Return whether the given PCI device DMA address mask can

Index: arch/mips/mm/c-mips32.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-mips32.c,v
retrieving revision 1.3.2.4
diff -u -r1.3.2.4 c-mips32.c
--- arch/mips/mm/c-mips32.c	2002/05/29 03:03:17	1.3.2.4
+++ arch/mips/mm/c-mips32.c	2002/06/18 16:52:21
@@ -393,12 +393,6 @@
 	}
 }
 
-static void
-mips32_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("mips32_dma_cache called - should not happen.");
-}
-
 /*
  * While we're protected against bad userland addresses we don't care
  * very much about what happens in that case.  Usually a segmentation
@@ -603,7 +597,7 @@
 	_flush_icache_page = mips32_flush_icache_page;
 
 	_dma_cache_wback_inv = mips32_dma_cache_wback_inv_pc;
-	_dma_cache_wback = mips32_dma_cache_wback;
+	_dma_cache_wback = mips32_dma_cache_wback_inv_pc;
 	_dma_cache_inv = mips32_dma_cache_inv_pc;
 }
 
@@ -621,7 +615,7 @@
 	_flush_icache_page = mips32_flush_icache_page_s;
 
 	_dma_cache_wback_inv = mips32_dma_cache_wback_inv_sc;
-	_dma_cache_wback = mips32_dma_cache_wback;
+	_dma_cache_wback = mips32_dma_cache_wback_inv_sc;
 	_dma_cache_inv = mips32_dma_cache_inv_sc;
 }
 
Index: arch/mips/mm/c-r3k.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-r3k.c,v
retrieving revision 1.4.2.1
diff -u -r1.4.2.1 c-r3k.c
--- arch/mips/mm/c-r3k.c	2002/02/18 15:22:30	1.4.2.1
+++ arch/mips/mm/c-r3k.c	2002/06/18 16:52:21
@@ -337,7 +337,9 @@
 	_flush_icache_page = r3k_flush_icache_page;
 	_flush_icache_range = r3k_flush_icache_range;
 
+	_dma_cache_inv = r3k_dma_cache_wback_inv;
 	_dma_cache_wback_inv = r3k_dma_cache_wback_inv;
+	_dma_cache_wback = r3k_dma_cache_wback_inv;
 
 	printk("Primary instruction cache %dkb, linesize %d bytes\n",
 		(int) (icache_size >> 10), (int) icache_lsize);
Index: arch/mips/mm/c-r4k.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-r4k.c,v
retrieving revision 1.3.2.3
diff -u -r1.3.2.3 c-r4k.c
--- arch/mips/mm/c-r4k.c	2002/05/29 03:03:17	1.3.2.3
+++ arch/mips/mm/c-r4k.c	2002/06/18 16:52:21
@@ -1150,12 +1150,6 @@
 	}
 }
 
-static void
-r4k_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("r4k_dma_cache called - should not happen.");
-}
-
 /*
  * While we're protected against bad userland addresses we don't care
  * very much about what happens in that case.  Usually a segmentation
@@ -1358,7 +1352,7 @@
 	_flush_icache_page = r4k_flush_icache_page_p;
 
 	_dma_cache_wback_inv = r4k_dma_cache_wback_inv_pc;
-	_dma_cache_wback = r4k_dma_cache_wback;
+	_dma_cache_wback = r4k_dma_cache_wback_inv_pc;
 	_dma_cache_inv = r4k_dma_cache_inv_pc;
 }
 
@@ -1441,7 +1435,7 @@
 	___flush_cache_all = _flush_cache_all;
 	_flush_icache_page = r4k_flush_icache_page_s;
 	_dma_cache_wback_inv = r4k_dma_cache_wback_inv_sc;
-	_dma_cache_wback = r4k_dma_cache_wback;
+	_dma_cache_wback = r4k_dma_cache_wback_inv_sc;
 	_dma_cache_inv = r4k_dma_cache_inv_sc;
 }
 
Index: arch/mips/mm/c-r5432.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-r5432.c,v
retrieving revision 1.4
diff -u -r1.4 c-r5432.c
--- arch/mips/mm/c-r5432.c	2001/11/30 13:28:06	1.4
+++ arch/mips/mm/c-r5432.c	2002/06/18 16:52:21
@@ -414,12 +414,6 @@
 	bc_inv(addr, size);
 }
 
-static void
-r5432_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("r5432_dma_cache called - should not happen.");
-}
-
 /*
  * While we're protected against bad userland addresses we don't care
  * very much about what happens in that case.  Usually a segmentation
@@ -470,7 +464,7 @@
 	_flush_cache_page = r5432_flush_cache_page_d32i32;
 	_flush_icache_page = r5432_flush_icache_page_i32;
 	_dma_cache_wback_inv = r5432_dma_cache_wback_inv_pc;
-	_dma_cache_wback = r5432_dma_cache_wback;
+	_dma_cache_wback = r5432_dma_cache_wback_inv_pc;
 	_dma_cache_inv = r5432_dma_cache_inv_pc;
 
 	_flush_cache_sigtramp = r5432_flush_cache_sigtramp;
Index: arch/mips/mm/c-rm7k.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-rm7k.c,v
retrieving revision 1.4.2.2
diff -u -r1.4.2.2 c-rm7k.c
--- arch/mips/mm/c-rm7k.c	2002/05/29 03:03:17	1.4.2.2
+++ arch/mips/mm/c-rm7k.c	2002/06/18 16:52:21
@@ -177,12 +177,6 @@
 	}
 }
 
-static void
-rm7k_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("rm7k_dma_cache_wback called - should not happen.");
-}
-
 /*
  * While we're protected against bad userland addresses we don't care
  * very much about what happens in that case.  Usually a segmentation
@@ -342,7 +336,7 @@
 	_flush_icache_page = rm7k_flush_icache_page;
 
 	_dma_cache_wback_inv = rm7k_dma_cache_wback_inv;
-	_dma_cache_wback = rm7k_dma_cache_wback;
+	_dma_cache_wback = rm7k_dma_cache_wback_inv;
 	_dma_cache_inv = rm7k_dma_cache_inv;
 
 	__flush_cache_all_d32i32();
Index: arch/mips/mm/c-tx39.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-tx39.c,v
retrieving revision 1.4
diff -u -r1.4 c-tx39.c
--- arch/mips/mm/c-tx39.c	2001/11/30 13:28:06	1.4
+++ arch/mips/mm/c-tx39.c	2002/06/18 16:52:21
@@ -225,11 +225,6 @@
 	}
 }
 
-static void tx39_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("tx39_dma_cache called - should not happen.");
-}
-
 static void tx39_flush_cache_sigtramp(unsigned long addr)
 {
 	unsigned long config;
@@ -317,7 +312,7 @@
 		_flush_icache_range = tx39_flush_icache_range;
 
 		_dma_cache_wback_inv = tx39_dma_cache_wback_inv;
-		_dma_cache_wback = tx39_dma_cache_wback;
+		_dma_cache_wback = tx39_dma_cache_wback_inv;
 		_dma_cache_inv = tx39_dma_cache_inv;
 
 		break;
Index: arch/mips/mm/c-tx49.c
===================================================================
RCS file: /cvs/linux/arch/mips/mm/c-tx49.c,v
retrieving revision 1.3.2.1
diff -u -r1.3.2.1 c-tx49.c
--- arch/mips/mm/c-tx49.c	2002/05/29 03:03:17	1.3.2.1
+++ arch/mips/mm/c-tx49.c	2002/06/18 16:52:21
@@ -343,12 +343,6 @@
 	}
 }
 
-static void
-r4k_dma_cache_wback(unsigned long addr, unsigned long size)
-{
-	panic("r4k_dma_cache called - should not happen.");
-}
-
 /*
  * While we're protected against bad userland addresses we don't care
  * very much about what happens in that case.  Usually a segmentation
@@ -429,7 +423,7 @@
 	_flush_icache_page = r4k_flush_icache_page;
 
 	_dma_cache_wback_inv = r4k_dma_cache_wback_inv;
-	_dma_cache_wback = r4k_dma_cache_wback;
+	_dma_cache_wback = r4k_dma_cache_wback_inv;
 	_dma_cache_inv = r4k_dma_cache_inv;
 
 	_flush_cache_sigtramp = r4k_flush_cache_sigtramp;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dma_cache_wback, pci DMA cache coherency changes
  2002-06-18 17:03 [PATCH] dma_cache_wback, pci DMA cache coherency changes William Jhun
@ 2002-06-19  9:39 ` Ralf Baechle
  2002-06-19 18:22   ` [PATCH] include/asm-mips/pci.h William Jhun
  0 siblings, 1 reply; 6+ messages in thread
From: Ralf Baechle @ 2002-06-19  9:39 UTC (permalink / raw)
  To: William Jhun; +Cc: "linux-mips

On Tue, Jun 18, 2002 at 10:03:47AM -0700, William Jhun wrote:

> To: "linux-mips@oss.sgi.com"@ayrnetworks.com
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Your mail software is smoking funny stuff ;-)

> This is a re-hash of patches I sent out a while ago which do a more
> optimal cache-flushing for pci_map_*() and pci_dma_sync_*(). It
> basically does an invalidate for PCI_DMA_FROMDEVICE operations and a
> writeback for PCI_DMA_TODEVICE pci_map_* (or writeback/invalidate if
> PCI_DMA_BIDIRECTIONAL). This is similar to the ARM implementation.
> 
> Additionally, I filled in the _dma_cache_wback calls in the
> arch/mips/c-*.c to call *_dma_cache_wback_inv* instead of calling
> panic(). Some architectures could probably do a real writeback instead
> of just wback_inv, but this will at least allow code that can use
> writeback-only if available.
> 
> Note: I'm not familiar with a lot of these CPUs, but the change should
> be innocuous. Could someone validate/improve these?

Can you try to get rid of all these #ifdef CONFIG_NONCOHERENT_IO things?
We already had too many of them and you're adding even more ...
Basically if dma_cache_wback_inv, dma_cache_wback and dma_cache_inv are
just empty macros as they are if CONFIG_NONCOHERENT_IO is undefined
gcc should be able to optimize most of the #ifdef'd code away.

Please always cc patches you want to submit to me or I might miss them on
the list.

  Ralf

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] include/asm-mips/pci.h
  2002-06-19  9:39 ` Ralf Baechle
@ 2002-06-19 18:22   ` William Jhun
  2002-06-19 20:38     ` Paul Jackson
  0 siblings, 1 reply; 6+ messages in thread
From: William Jhun @ 2002-06-19 18:22 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: linux-mips

On Wed, Jun 19, 2002 at 11:39:33AM +0200, Ralf Baechle wrote:
> On Tue, Jun 18, 2002 at 10:03:47AM -0700, William Jhun wrote:
> 
> > To: "linux-mips@oss.sgi.com"@ayrnetworks.com
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Your mail software is smoking funny stuff ;-)

Well! I'll just have to give it a strict spanking and send it off to
rehab. No wonder the office window is always wide open when I arrive in
the morning!

> Can you try to get rid of all these #ifdef CONFIG_NONCOHERENT_IO things?
> We already had too many of them and you're adding even more ...
> Basically if dma_cache_wback_inv, dma_cache_wback and dma_cache_inv are
> just empty macros as they are if CONFIG_NONCOHERENT_IO is undefined
> gcc should be able to optimize most of the #ifdef'd code away.

Ok, so I got rid of most except the ones in the *_sg() functions; gcc
won't optimize the loop out, even with -O2. It just creates a tight loop
that increments sg..

Note that without the #ifdefs, you may get warnings like:

../../../ayr/linux/include/asm/pci.h: In function `prep_buffer':
../../../ayr/linux/include/asm/pci.h:89: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:89: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:92: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:92: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:95: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:95: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h: In function `sync_buffer':
../../../ayr/linux/include/asm/pci.h:108: warning: statement with no effect
../../../ayr/linux/include/asm/pci.h:108: warning: statement with no effect

So are we still sure we want to leave them out?

Will

---
Index: include/asm-mips/pci.h
===================================================================
RCS file: /cvs/linux/include/asm-mips/pci.h,v
retrieving revision 1.24.2.1
diff -u -r1.24.2.1 pci.h
--- include/asm-mips/pci.h	2002/02/26 06:00:25	1.24.2.1
+++ include/asm-mips/pci.h	2002/06/19 18:20:51
@@ -80,6 +80,37 @@
 				void *vaddr, dma_addr_t dma_handle);
 
 /*
+ * Prepare buffer for DMA transfer
+ */
+static inline void prep_buffer(void *ptr, size_t size, int direction)
+{
+        switch(direction) {
+        case PCI_DMA_TODEVICE:
+                dma_cache_wback((unsigned long)ptr, size);
+                break;
+        case PCI_DMA_FROMDEVICE:
+                dma_cache_inv((unsigned long)ptr, size);
+                break;
+        case PCI_DMA_BIDIRECTIONAL:
+                dma_cache_wback_inv((unsigned long)ptr, size);
+                break;
+        }
+}
+
+/*
+ * Prepare buffer for CPU access after DMA transfer
+ */
+static inline void sync_buffer(void *ptr, size_t size, int direction)
+{
+        switch(direction) {
+        case PCI_DMA_FROMDEVICE:
+        case PCI_DMA_BIDIRECTIONAL:
+                dma_cache_inv((unsigned long)ptr, size);
+                break;
+        }
+}
+
+/*
  * Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *
@@ -92,9 +123,7 @@
 	if (direction == PCI_DMA_NONE)
 		BUG();
 
-#ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv((unsigned long)ptr, size);
-#endif
+	prep_buffer(ptr, size, direction);
 
 	return virt_to_bus(ptr);
 }
@@ -131,9 +160,7 @@
 
 	addr = (unsigned long) page_address(page);
 	addr += offset;
-#ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv(addr, size);
-#endif
+	prep_buffer((void*)addr, size, direction);
 
 	return virt_to_bus((void *)addr);
 }
@@ -183,7 +210,7 @@
 #ifdef CONFIG_NONCOHERENT_IO
 	/* Make sure that gcc doesn't leave the empty loop body.  */
 	for (i = 0; i < nents; i++, sg++)
-		dma_cache_wback_inv((unsigned long)sg->address, sg->length);
+		prep_buffer(sg->address, sg->length, direction);
 #endif
 
 	return nents;
@@ -220,9 +247,7 @@
 	if (direction == PCI_DMA_NONE)
 		BUG();
 
-#ifdef CONFIG_NONCOHERENT_IO
-	dma_cache_wback_inv((unsigned long)bus_to_virt(dma_handle), size);
-#endif
+	sync_buffer(bus_to_virt(dma_handle), size, direction);
 }
 
 /*
@@ -246,7 +271,7 @@
 	/* Make sure that gcc doesn't leave the empty loop body.  */
 #ifdef CONFIG_NONCOHERENT_IO
 	for (i = 0; i < nelems; i++, sg++)
-		dma_cache_wback_inv((unsigned long)sg->address, sg->length);
+		sync_buffer(sg->address, sg->length, direction);
 #endif
 }
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] include/asm-mips/pci.h
  2002-06-19 18:22   ` [PATCH] include/asm-mips/pci.h William Jhun
@ 2002-06-19 20:38     ` Paul Jackson
  2002-06-19 20:38       ` Paul Jackson
  2002-06-20 10:25       ` Ralf Baechle
  0 siblings, 2 replies; 6+ messages in thread
From: Paul Jackson @ 2002-06-19 20:38 UTC (permalink / raw)
  To: William Jhun; +Cc: Ralf Baechle, linux-mips

On Wed, 19 Jun 2002, William Jhun wrote:
> Note that without the #ifdefs, you may get warnings like:
>
> ../../../ayr/linux/include/asm/pci.h: In function `prep_buffer':
> ../../../ayr/linux/include/asm/pci.h:89: warning: statement with no effect
> ...
>
> So are we still sure we want to leave them out?


Yes - leave them out (speaking out of context here - hopefully still
useful).

Remove the warnings instead.

I've gotten in the habit of having the following form to
optional code logic:

In some header file foobar.h:

    #if CONFIG_FOOBAR
    #define init_foobar(x) do {		\
	    int f = 2 * (x);		\
	    foobar_initialize(f);	\
	| while (0)
    #else
    #define init_foobar(x) do {} while (0)
    #endif

I don't see any warnings from this, and it provides just
the right sort of syntax wrapper on the macro init_foobar(),
forcing it to be a single statement, regardless of context,
while providing a nested block context for any local variables.


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] include/asm-mips/pci.h
  2002-06-19 20:38     ` Paul Jackson
@ 2002-06-19 20:38       ` Paul Jackson
  2002-06-20 10:25       ` Ralf Baechle
  1 sibling, 0 replies; 6+ messages in thread
From: Paul Jackson @ 2002-06-19 20:38 UTC (permalink / raw)
  To: William Jhun; +Cc: Ralf Baechle, linux-mips

On Wed, 19 Jun 2002, William Jhun wrote:
> Note that without the #ifdefs, you may get warnings like:
>
> ../../../ayr/linux/include/asm/pci.h: In function `prep_buffer':
> ../../../ayr/linux/include/asm/pci.h:89: warning: statement with no effect
> ...
>
> So are we still sure we want to leave them out?


Yes - leave them out (speaking out of context here - hopefully still
useful).

Remove the warnings instead.

I've gotten in the habit of having the following form to
optional code logic:

In some header file foobar.h:

    #if CONFIG_FOOBAR
    #define init_foobar(x) do {		\
	    int f = 2 * (x);		\
	    foobar_initialize(f);	\
	| while (0)
    #else
    #define init_foobar(x) do {} while (0)
    #endif

I don't see any warnings from this, and it provides just
the right sort of syntax wrapper on the macro init_foobar(),
forcing it to be a single statement, regardless of context,
while providing a nested block context for any local variables.


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] include/asm-mips/pci.h
  2002-06-19 20:38     ` Paul Jackson
  2002-06-19 20:38       ` Paul Jackson
@ 2002-06-20 10:25       ` Ralf Baechle
  1 sibling, 0 replies; 6+ messages in thread
From: Ralf Baechle @ 2002-06-20 10:25 UTC (permalink / raw)
  To: Paul Jackson; +Cc: William Jhun, linux-mips

On Wed, Jun 19, 2002 at 01:38:52PM -0700, Paul Jackson wrote:

> Yes - leave them out (speaking out of context here - hopefully still
> useful).
> 
> Remove the warnings instead.
> 
> I've gotten in the habit of having the following form to
> optional code logic:
> 
> In some header file foobar.h:
> 
>     #if CONFIG_FOOBAR
>     #define init_foobar(x) do {		\
> 	    int f = 2 * (x);		\
> 	    foobar_initialize(f);	\
> 	| while (0)
>     #else
>     #define init_foobar(x) do {} while (0)
>     #endif
> 
> I don't see any warnings from this, and it provides just
> the right sort of syntax wrapper on the macro init_foobar(),
> forcing it to be a single statement, regardless of context,
> while providing a nested block context for any local variables.

Note your variant doesn't deal with side effects of the argument expression
x (basically none of the equivalent constructions in the kernels do!) which
is why our code in question does something like this:

     #if CONFIG_FOOBAR
     #define init_foobar(x) do {               \
           int f = 2 * (x);            \
           foobar_initialize(f);       \
       | while (0)
     #else
     #define init_foobar(x) do { (x); } while (0)
     #endif

This can potencially expand into something like:

     do { 42; } while(0)

which will result in warnings.  The solution is:

     #define init_foobar(x) do { (void) (x); } while (0)

  Ralf

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2002-06-20 10:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2002-06-18 17:03 [PATCH] dma_cache_wback, pci DMA cache coherency changes William Jhun
2002-06-19  9:39 ` Ralf Baechle
2002-06-19 18:22   ` [PATCH] include/asm-mips/pci.h William Jhun
2002-06-19 20:38     ` Paul Jackson
2002-06-19 20:38       ` Paul Jackson
2002-06-20 10:25       ` Ralf Baechle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox