PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Bradley D. LaRonde]

2001-12-07  Bradley D. LaRonde <brad@ltc.com>

* remove detrimental do {...} whiles
* add sequence point to in[b,w,l] to prevent compiler from reordering
* add const modifier to outs[b,w,l] (quiets some compiler warnings)


--- linux-oss-2.4-2001-12-04/include/asm-mips/io.h	Thu Dec  6 17:07:24 2001
+++ linux-patch/include/asm-mips/io.h	Thu Dec  6 16:47:20 2001
@@ -63,7 +63,7 @@
 extern const unsigned long mips_io_port_base;
 
 #define set_io_port_base(base)	\
-	do { * (unsigned long *) &mips_io_port_base = (base); } while (0)
+	*(unsigned long *)&mips_io_port_base = (base);
 
 /*
  * Thanks to James van Artsdalen for a better timing-fix than
@@ -215,41 +215,37 @@
 
 
 #define outb(val,port)							\
-do {									\
-	*(volatile u8 *)(mips_io_port_base + (port)) = __ioswab8(val);	\
-} while(0)
+	*(volatile u8 *)(mips_io_port_base + (port)) = __ioswab8(val)
 
 #define outw(val,port)							\
-do {									\
-	*(volatile u16 *)(mips_io_port_base + (port)) = __ioswab16(val);	\
-} while(0)
+	*(volatile u16 *)(mips_io_port_base + (port)) = __ioswab16(val)
 
 #define outl(val,port)							\
-do {									\
-	*(volatile u32 *)(mips_io_port_base + (port)) = __ioswab32(val);\
-} while(0)
+	*(volatile u32 *)(mips_io_port_base + (port)) = __ioswab32(val)
 
+/* Don't do {...} while(0) these. */
 #define outb_p(val,port)						\
-do {									\
+({									\
 	*(volatile u8 *)(mips_io_port_base + (port)) = __ioswab8(val);	\
 	SLOW_DOWN_IO;							\
-} while(0)
+})
 
 #define outw_p(val,port)						\
-do {									\
+{									\
 	*(volatile u16 *)(mips_io_port_base + (port)) = __ioswab16(val);\
 	SLOW_DOWN_IO;							\
-} while(0)
+}
 
 #define outl_p(val,port)						\
-do {									\
+{									\
 	*(volatile u32 *)(mips_io_port_base + (port)) = __ioswab32(val);\
 	SLOW_DOWN_IO;							\
-} while(0)
+}
 
-#define inb(port) (__ioswab8(*(volatile u8 *)(mips_io_port_base + (port))))
-#define inw(port) (__ioswab16(*(volatile u16 *)(mips_io_port_base + (port))))
-#define inl(port) (__ioswab32(*(volatile u32 *)(mips_io_port_base + (port))))
+/* As in include/asm-arm/io.h, introduce sequence points ({...}) to prevent gcc * from reordering. */
+#define inb(port) ({ unsigned int __v = __ioswab8(*(volatile u8 *)(mips_io_port_base + (port))); __v; })
+#define inw(port) ({ unsigned int __v = __ioswab16(*(volatile u16 *)(mips_io_port_base + (port))); __v; })
+#define inl(port) ({ unsigned int __v = __ioswab32(*(volatile u32 *)(mips_io_port_base + (port))); __v; })
 
 #define inb_p(port)							\
 ({									\
@@ -278,7 +274,7 @@
 	__ioswab32(__val);						\
 })
 
-static inline void outsb(unsigned long port, void *addr, unsigned int count)
+static inline void outsb(unsigned long port, const void *addr, unsigned int count)
 {
 	while (count--) {
 		outb(*(u8 *)addr, port);
@@ -294,7 +290,7 @@
 	}
 }
 
-static inline void outsw(unsigned long port, void *addr, unsigned int count)
+static inline void outsw(unsigned long port, const void *addr, unsigned int count)
 {
 	while (count--) {
 		outw(*(u16 *)addr, port);
@@ -310,7 +306,7 @@
 	}
 }
 
-static inline void outsl(unsigned long port, void *addr, unsigned int count)
+static inline void outsl(unsigned long port, const void *addr, unsigned int count)
 {
 	while (count--) {
 		outl(*(u32 *)addr, port);

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Geert Uytterhoeven]

On Fri, 7 Dec 2001, Bradley D. LaRonde wrote:
> 2001-12-07  Bradley D. LaRonde <brad@ltc.com>
> 
> * remove detrimental do {...} whiles
> * add sequence point to in[b,w,l] to prevent compiler from reordering
> * add const modifier to outs[b,w,l] (quiets some compiler warnings)
> 
> 
> --- linux-oss-2.4-2001-12-04/include/asm-mips/io.h	Thu Dec  6 17:07:24 2001
> +++ linux-patch/include/asm-mips/io.h	Thu Dec  6 16:47:20 2001
> @@ -63,7 +63,7 @@
>  extern const unsigned long mips_io_port_base;
>  
>  #define set_io_port_base(base)	\
> -	do { * (unsigned long *) &mips_io_port_base = (base); } while (0)
> +	*(unsigned long *)&mips_io_port_base = (base);

Now consider someone writing

    if (...)
	set_io_port_base(...);
    else
	...

And see what happens...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Daniel Jacobowitz]

On Fri, Dec 07, 2001 at 06:30:43PM +0100, Geert Uytterhoeven wrote:
> On Fri, 7 Dec 2001, Bradley D. LaRonde wrote:
> > 2001-12-07  Bradley D. LaRonde <brad@ltc.com>
> > 
> > * remove detrimental do {...} whiles
> > * add sequence point to in[b,w,l] to prevent compiler from reordering
> > * add const modifier to outs[b,w,l] (quiets some compiler warnings)
> > 
> > 
> > --- linux-oss-2.4-2001-12-04/include/asm-mips/io.h	Thu Dec  6 17:07:24 2001
> > +++ linux-patch/include/asm-mips/io.h	Thu Dec  6 16:47:20 2001
> > @@ -63,7 +63,7 @@
> >  extern const unsigned long mips_io_port_base;
> >  
> >  #define set_io_port_base(base)	\
> > -	do { * (unsigned long *) &mips_io_port_base = (base); } while (0)
> > +	*(unsigned long *)&mips_io_port_base = (base);
> 
> Now consider someone writing
> 
>     if (...)
> 	set_io_port_base(...);
>     else
> 	...
> 
> And see what happens...

If Bradley loses the extra semicolon, what other problem is the
do/while construct supposed to address?  I seem to recall there being
another problem case, but I can't remember what it is.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Jim Paris]

> > >  #define set_io_port_base(base)	\
> > > -	do { * (unsigned long *) &mips_io_port_base = (base); } while (0)
> > > +	*(unsigned long *)&mips_io_port_base = (base);
>
> If Bradley loses the extra semicolon, what other problem is the
> do/while construct supposed to address?  I seem to recall there being
> another problem case, but I can't remember what it is.

For that particular #define, I can't think of any problem cases.  The
do/while helps with multiple statements, gives you a new scope for
variable declaration, and allows the code to do a break, but we don't
need any of that here.

-jim

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Ralf Baechle]

On Fri, Dec 07, 2001 at 12:38:33PM -0500, Daniel Jacobowitz wrote:

> > > -	do { * (unsigned long *) &mips_io_port_base = (base); } while (0)
> > > +	*(unsigned long *)&mips_io_port_base = (base);
> > 
> > Now consider someone writing
> > 
> >     if (...)
> > 	set_io_port_base(...);
> >     else
> > 	...
> > 
> > And see what happens...
> 
> If Bradley loses the extra semicolon, what other problem is the
> do/while construct supposed to address?  I seem to recall there being
> another problem case, but I can't remember what it is.

There is imho not very much sense in such a macro / function being written
in a way that returns any value, that is something like

  foo = set_io_port_base(...)

doesn't make obvious sense.  So it's written in a way which will take care
of any attempt to use the return type.

  Ralf

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Jim Paris]

> There is imho not very much sense in such a macro / function being written
> in a way that returns any value, that is something like
> 
>   foo = set_io_port_base(...)
> 
> doesn't make obvious sense.  So it's written in a way which will take care
> of any attempt to use the return type.

So Brad's way allows things that weren't allowed before.  But does
it break anything that works with the do/while construct?

You can take care of attempts to use the return type by voiding it:

#define set_io_port_base(base)		\
	(void)(*(unsigned long *)&mips_io_port_base = (base))

-jim

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Justin Carlson]

> So Brad's way allows things that weren't allowed before.  But does
> it break anything that works with the do/while construct?
> 
> You can take care of attempts to use the return type by voiding it:
> 
> #define set_io_port_base(base)		\
> 	(void)(*(unsigned long *)&mips_io_port_base = (base))
> 

Maybe I missed this, but is there any reason for the patch, other then 
a personal preference of how to do macros that look like functions? 
I've seen gcc do strange non-optimal things with functions declared
inlines, but I've never seen it generate bad code WRT to do{}while(0)
constructs.

Unless I'm missing something, this patch looks like a solution in search
of a problem...

-Justin

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Jim Paris]

> Maybe I missed this, but is there any reason for the patch, other then 
> a personal preference of how to do macros that look like functions? 
> I've seen gcc do strange non-optimal things with functions declared
> inlines, but I've never seen it generate bad code WRT to do{}while(0)
> constructs.
> 
> Unless I'm missing something, this patch looks like a solution in search
> of a problem...

In the case of set_io_port_base, I see no real reason.  But for the
out[b,w,l] functions, having the do/while can prevent constructs that
might otherwise make sense, like

	for(i=0;i<10;i++,outb(i,port)) {
           ...
        }

Okay, so it's a bad example, but.. :)  Maybe Brad has a better one.

-jim

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Bradley D. LaRonde]

----- Original Message -----
From: "Jim Paris" <jim@jtan.com>
To: "Justin Carlson" <justinca@ri.cmu.edu>
Cc: <linux-mips@oss.sgi.com>
Sent: Friday, December 07, 2001 2:43 PM
Subject: Re: PATCH: io.h remove detrimental do {...} whiles, add sequence
points, add const modifiers


> > Maybe I missed this, but is there any reason for the patch, other then
> > a personal preference of how to do macros that look like functions?
> > I've seen gcc do strange non-optimal things with functions declared
> > inlines, but I've never seen it generate bad code WRT to do{}while(0)
> > constructs.
> >
> > Unless I'm missing something, this patch looks like a solution in search
> > of a problem...

No Justin.  See below.

> In the case of set_io_port_base, I see no real reason.  But for the
> out[b,w,l] functions, having the do/while can prevent constructs that
> might otherwise make sense, like
>
> for(i=0;i<10;i++,outb(i,port)) {
>            ...
>         }
>
> Okay, so it's a bad example, but.. :)  Maybe Brad has a better one.

>From drivers/net/wireless/heremes.h:

<snip>
/* Register access convenience macros */
#define hermes_read_reg(hw, off) (inw((hw)->iobase + (off)))
#define hermes_write_reg(hw, off, val) (outw_p((val), (hw)->iobase + (off)))

#define hermes_read_regn(hw, name) (hermes_read_reg((hw), HERMES_##name))
#define hermes_write_regn(hw, name, val) (hermes_write_reg((hw),
HERMES_##name, (val)))

/* Note that for the next two, the count is in 16-bit words, not bytes */
#define hermes_read_data(hw, off, buf, count) (insw((hw)->iobase + (off),
(buf), (count)))
#define hermes_write_data(hw, off, buf, count) (outsw((hw)->iobase + (off),
(buf), (count)))
</snip>

That won't compile with the do {...} while(0)s left in io.h.  My patch lets
hermes.h (and all other io code that I've tested) compile.

heremes.h compiles as-is on other platforms.  Why should mips snub it for
some dubious value of do {...} while(0)?

Regards,
Brad

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Bradley D. LaRonde]

----- Original Message -----
From: "Jim Paris" <jim@jtan.com>
To: "Justin Carlson" <justinca@ri.cmu.edu>
Cc: <linux-mips@oss.sgi.com>
Sent: Friday, December 07, 2001 2:43 PM
Subject: Re: PATCH: io.h remove detrimental do {...} whiles, add sequence
points, add const modifiers


> > Maybe I missed this, but is there any reason for the patch, other then
> > a personal preference of how to do macros that look like functions?
> > I've seen gcc do strange non-optimal things with functions declared
> > inlines, but I've never seen it generate bad code WRT to do{}while(0)
> > constructs.
> >
> > Unless I'm missing something, this patch looks like a solution in search
> > of a problem...

No Justin.  See below.

> In the case of set_io_port_base, I see no real reason.  But for the
> out[b,w,l] functions, having the do/while can prevent constructs that
> might otherwise make sense, like
>
> for(i=0;i<10;i++,outb(i,port)) {
>            ...
>         }
>
> Okay, so it's a bad example, but.. :)  Maybe Brad has a better one.

From drivers/net/wireless/heremes.h:

<snip>
/* Register access convenience macros */
#define hermes_read_reg(hw, off) (inw((hw)->iobase + (off)))
#define hermes_write_reg(hw, off, val) (outw_p((val), (hw)->iobase + (off)))

#define hermes_read_regn(hw, name) (hermes_read_reg((hw), HERMES_##name))
#define hermes_write_regn(hw, name, val) (hermes_write_reg((hw),
HERMES_##name, (val)))

/* Note that for the next two, the count is in 16-bit words, not bytes */
#define hermes_read_data(hw, off, buf, count) (insw((hw)->iobase + (off),
(buf), (count)))
#define hermes_write_data(hw, off, buf, count) (outsw((hw)->iobase + (off),
(buf), (count)))
</snip>

That won't compile with the do {...} while(0)s left in io.h.  My patch lets
hermes.h (and all other io code that I've tested) compile.

heremes.h compiles as-is on other platforms.  Why should mips snub it for
some dubious value of do {...} while(0)?

Regards,
Brad

Re: PATCH: io.h remove detrimental do {...} whiles, add sequence points, add const modifiers

[Justin Carlson]

On Fri, 2001-12-07 at 15:23, Bradley D. LaRonde wrote:
> > Okay, so it's a bad example, but.. :)  Maybe Brad has a better one.
> 
> From drivers/net/wireless/heremes.h:
> 
> <snip>
> /* Register access convenience macros */
> #define hermes_read_reg(hw, off) (inw((hw)->iobase + (off)))
> #define hermes_write_reg(hw, off, val) (outw_p((val), (hw)->iobase + (off)))
> That won't compile with the do {...} while(0)s left in io.h.  My patch lets
> hermes.h (and all other io code that I've tested) compile.
> 
> heremes.h compiles as-is on other platforms.  Why should mips snub it for
> some dubious value of do {...} while(0)?
> 

Because using (foo(),bar()) syntax to cram in a compound statement is
just silly and a bad idea IMHO.  The real Right Thing is to fix the
compiler and use inline functions instead of macros in just about all of
these cases. 

But, given as this puts me in the category of wanting to change other
people's code just for the sake of a preferred coding style, I suppose I
have to cede the point.  :)

-Justin