recent IDE regression

recent IDE regression

[David Miller]

After today's IDE merge my sparc64 workstation stopped booting.

It's due to this change:

commit 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
Author: Harvey Harrison <harvey.harrison@gmail.com>
Date:   Thu Jul 24 22:53:34 2008 +0200

    ide: trivial sparse annotations
    
    Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
    Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Heh, "trivial", indeed.

Specifically, this part of the change:

diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index 07da5fb..8aae917 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -510,10 +510,8 @@ void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
 
 	if (byteswap) {
 		/* convert from big-endian to host byte order */
-		for (p = end ; p != s;) {
-			unsigned short *pp = (unsigned short *) (p -= 2);
-			*pp = ntohs(*pp);
-		}
+		for (p = end ; p != s;)
+			be16_to_cpus((u16 *)(p -= 2));
 	}
 	/* strip leading blanks */
 	while (s != end && *s == ' ')

On big-endian, be16_to_cpus() (via __be16_to_cpus()) is:

	do { } while (0)

which therefore does not evaluate the argument, and thus this loop
will make no forward progress.

Probably the fix is in be16_to_cpus(), making it do something like
"(void) (x);" in the do/while body.

Something like this:

endian: Always evaluate arguments.

Changeset 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
("ide: trivial sparse annotations") created an IDE bootup
regression on big-endian systems.  In drivers/ide/ide-iops.c,
function ide_fixstring() we now have the loop:

		for (p = end ; p != s;)
			be16_to_cpus((u16 *)(p -= 2));

which will never terminate on big-endian because in such
a configuration be16_to_cpus() evaluates to "do { } while (0)"

Therefore, always evaluate the arguments to nop endian transformation
operations.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/byteorder/big_endian.h b/include/linux/byteorder/big_endian.h
index 961ed4b..44f95b9 100644
--- a/include/linux/byteorder/big_endian.h
+++ b/include/linux/byteorder/big_endian.h
@@ -94,12 +94,12 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
 #define __le32_to_cpus(x) __swab32s((x))
 #define __cpu_to_le16s(x) __swab16s((x))
 #define __le16_to_cpus(x) __swab16s((x))
-#define __cpu_to_be64s(x) do {} while (0)
-#define __be64_to_cpus(x) do {} while (0)
-#define __cpu_to_be32s(x) do {} while (0)
-#define __be32_to_cpus(x) do {} while (0)
-#define __cpu_to_be16s(x) do {} while (0)
-#define __be16_to_cpus(x) do {} while (0)
+#define __cpu_to_be64s(x) do { (void)(x); } while (0)
+#define __be64_to_cpus(x) do { (void)(x); } while (0)
+#define __cpu_to_be32s(x) do { (void)(x); } while (0)
+#define __be32_to_cpus(x) do { (void)(x); } while (0)
+#define __cpu_to_be16s(x) do { (void)(x); } while (0)
+#define __be16_to_cpus(x) do { (void)(x); } while (0)
 
 #ifdef __KERNEL__
 #include <linux/byteorder/generic.h>
diff --git a/include/linux/byteorder/little_endian.h b/include/linux/byteorder/little_endian.h
index 05dc7c3..4cc170a 100644
--- a/include/linux/byteorder/little_endian.h
+++ b/include/linux/byteorder/little_endian.h
@@ -88,12 +88,12 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
 {
 	return __swab16p((__u16 *)p);
 }
-#define __cpu_to_le64s(x) do {} while (0)
-#define __le64_to_cpus(x) do {} while (0)
-#define __cpu_to_le32s(x) do {} while (0)
-#define __le32_to_cpus(x) do {} while (0)
-#define __cpu_to_le16s(x) do {} while (0)
-#define __le16_to_cpus(x) do {} while (0)
+#define __cpu_to_le64s(x) do { (void)(x); } while (0)
+#define __le64_to_cpus(x) do { (void)(x); } while (0)
+#define __cpu_to_le32s(x) do { (void)(x); } while (0)
+#define __le32_to_cpus(x) do { (void)(x); } while (0)
+#define __cpu_to_le16s(x) do { (void)(x); } while (0)
+#define __le16_to_cpus(x) do { (void)(x); } while (0)
 #define __cpu_to_be64s(x) __swab64s((x))
 #define __be64_to_cpus(x) __swab64s((x))
 #define __cpu_to_be32s(x) __swab32s((x))

Re: recent IDE regression

[Ben Dooks]

On Thu, Jul 24, 2008 at 11:38:31PM -0700, David Miller wrote:
> 
> After today's IDE merge my sparc64 workstation stopped booting.
> 
> It's due to this change:
> 
> commit 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
> Author: Harvey Harrison <harvey.harrison@gmail.com>
> Date:   Thu Jul 24 22:53:34 2008 +0200
> 
>     ide: trivial sparse annotations
>     
>     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>     Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> Heh, "trivial", indeed.
> 
> Specifically, this part of the change:
> 
> diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> index 07da5fb..8aae917 100644
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -510,10 +510,8 @@ void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
>  
>  	if (byteswap) {
>  		/* convert from big-endian to host byte order */
> -		for (p = end ; p != s;) {
> -			unsigned short *pp = (unsigned short *) (p -= 2);
> -			*pp = ntohs(*pp);
> -		}
> +		for (p = end ; p != s;)
> +			be16_to_cpus((u16 *)(p -= 2));

personally, i would much prefer to see the loop being less evil
like:

	for (p = s; p < end; p += 2)
		be16_to_cpus((u16 *)p);

is there an architecture/compiler combo which really makes this
evil worthwile? on arm (gcc 4.2), both evaluate to the same number of
instructions.

>  	}
>  	/* strip leading blanks */
>  	while (s != end && *s == ' ')
> 
> On big-endian, be16_to_cpus() (via __be16_to_cpus()) is:
> 
> 	do { } while (0)
> 
> which therefore does not evaluate the argument, and thus this loop
> will make no forward progress.
> 
> Probably the fix is in be16_to_cpus(), making it do something like
> "(void) (x);" in the do/while body.
> 
> Something like this:
> 
> endian: Always evaluate arguments.
> 
> Changeset 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
> ("ide: trivial sparse annotations") created an IDE bootup
> regression on big-endian systems.  In drivers/ide/ide-iops.c,
> function ide_fixstring() we now have the loop:
> 
> 		for (p = end ; p != s;)
> 			be16_to_cpus((u16 *)(p -= 2));
> 
> which will never terminate on big-endian because in such
> a configuration be16_to_cpus() evaluates to "do { } while (0)"
> 
> Therefore, always evaluate the arguments to nop endian transformation
> operations.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/include/linux/byteorder/big_endian.h b/include/linux/byteorder/big_endian.h
> index 961ed4b..44f95b9 100644
> --- a/include/linux/byteorder/big_endian.h
> +++ b/include/linux/byteorder/big_endian.h
> @@ -94,12 +94,12 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
>  #define __le32_to_cpus(x) __swab32s((x))
>  #define __cpu_to_le16s(x) __swab16s((x))
>  #define __le16_to_cpus(x) __swab16s((x))
> -#define __cpu_to_be64s(x) do {} while (0)
> -#define __be64_to_cpus(x) do {} while (0)
> -#define __cpu_to_be32s(x) do {} while (0)
> -#define __be32_to_cpus(x) do {} while (0)
> -#define __cpu_to_be16s(x) do {} while (0)
> -#define __be16_to_cpus(x) do {} while (0)
> +#define __cpu_to_be64s(x) do { (void)(x); } while (0)
> +#define __be64_to_cpus(x) do { (void)(x); } while (0)
> +#define __cpu_to_be32s(x) do { (void)(x); } while (0)
> +#define __be32_to_cpus(x) do { (void)(x); } while (0)
> +#define __cpu_to_be16s(x) do { (void)(x); } while (0)
> +#define __be16_to_cpus(x) do { (void)(x); } while (0)
>  
>  #ifdef __KERNEL__
>  #include <linux/byteorder/generic.h>
> diff --git a/include/linux/byteorder/little_endian.h b/include/linux/byteorder/little_endian.h
> index 05dc7c3..4cc170a 100644
> --- a/include/linux/byteorder/little_endian.h
> +++ b/include/linux/byteorder/little_endian.h
> @@ -88,12 +88,12 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
>  {
>  	return __swab16p((__u16 *)p);
>  }
> -#define __cpu_to_le64s(x) do {} while (0)
> -#define __le64_to_cpus(x) do {} while (0)
> -#define __cpu_to_le32s(x) do {} while (0)
> -#define __le32_to_cpus(x) do {} while (0)
> -#define __cpu_to_le16s(x) do {} while (0)
> -#define __le16_to_cpus(x) do {} while (0)
> +#define __cpu_to_le64s(x) do { (void)(x); } while (0)
> +#define __le64_to_cpus(x) do { (void)(x); } while (0)
> +#define __cpu_to_le32s(x) do { (void)(x); } while (0)
> +#define __le32_to_cpus(x) do { (void)(x); } while (0)
> +#define __cpu_to_le16s(x) do { (void)(x); } while (0)
> +#define __le16_to_cpus(x) do { (void)(x); } while (0)
>  #define __cpu_to_be64s(x) __swab64s((x))
>  #define __be64_to_cpus(x) __swab64s((x))
>  #define __cpu_to_be32s(x) __swab32s((x))
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: recent IDE regression

[David Miller]

From: Ben Dooks <ben-linux@fluff.org>
Date: Fri, 25 Jul 2008 09:34:48 +0100

> On Thu, Jul 24, 2008 at 11:38:31PM -0700, David Miller wrote:
> > diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> > index 07da5fb..8aae917 100644
> > --- a/drivers/ide/ide-iops.c
> > +++ b/drivers/ide/ide-iops.c
> > @@ -510,10 +510,8 @@ void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
> >  
> >  	if (byteswap) {
> >  		/* convert from big-endian to host byte order */
> > -		for (p = end ; p != s;) {
> > -			unsigned short *pp = (unsigned short *) (p -= 2);
> > -			*pp = ntohs(*pp);
> > -		}
> > +		for (p = end ; p != s;)
> > +			be16_to_cpus((u16 *)(p -= 2));
> 
> personally, i would much prefer to see the loop being less evil
> like:
> 
> 	for (p = s; p < end; p += 2)
> 		be16_to_cpus((u16 *)p);
> 
> is there an architecture/compiler combo which really makes this
> evil worthwile? on arm (gcc 4.2), both evaluate to the same number of
> instructions.

Regardless of what we want to do with this ugly loop, the endianness
macros should be fixed to consistently evaluate their arguments
once just like real function calls do.

Re: recent IDE regression

[Ben Dooks]

On Fri, Jul 25, 2008 at 01:42:52AM -0700, David Miller wrote:
> From: Ben Dooks <ben-linux@fluff.org>
> Date: Fri, 25 Jul 2008 09:34:48 +0100
> 
> > On Thu, Jul 24, 2008 at 11:38:31PM -0700, David Miller wrote:
> > > diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> > > index 07da5fb..8aae917 100644
> > > --- a/drivers/ide/ide-iops.c
> > > +++ b/drivers/ide/ide-iops.c
> > > @@ -510,10 +510,8 @@ void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
> > >  
> > >  	if (byteswap) {
> > >  		/* convert from big-endian to host byte order */
> > > -		for (p = end ; p != s;) {
> > > -			unsigned short *pp = (unsigned short *) (p -= 2);
> > > -			*pp = ntohs(*pp);
> > > -		}
> > > +		for (p = end ; p != s;)
> > > +			be16_to_cpus((u16 *)(p -= 2));
> > 
> > personally, i would much prefer to see the loop being less evil
> > like:
> > 
> > 	for (p = s; p < end; p += 2)
> > 		be16_to_cpus((u16 *)p);
> > 
> > is there an architecture/compiler combo which really makes this
> > evil worthwile? on arm (gcc 4.2), both evaluate to the same number of
> > instructions.
> 
> Regardless of what we want to do with this ugly loop, the endianness
> macros should be fixed to consistently evaluate their arguments
> once just like real function calls do.

Yes, I wasn't saying the macro fixes are not worthwile. I would also
like to see the loop being fixed to not perpetrate this nasty code
any further.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: recent IDE regression

[Linus Torvalds]

On Fri, 25 Jul 2008, Ben Dooks wrote:
> 
> personally, i would much prefer to see the loop being less evil
> like:
> 
> 	for (p = s; p < end; p += 2)
> 		be16_to_cpus((u16 *)p);

Well, in this case, the code actually depends on 'p' being back at the 
start of the buffer by the end of it all, so it would need some more 
changes than that. 

But yes, I applied David's patch, but I _also_ suspect that we would be 
better off without code that does horrid things like casts and assignments 
inside the function arguments.

So it would be nice to re-code that loop to be more readable. But due to 
the reliance of 'p' being 's' after the loop, the minimal patch would be 
something like the appended.

Bartlomiej - take this or not, I'm not going to commit it - I haven't 
tested it, nor do I even have any machines that would trigger it. So this 
is more a "maybe something like this" than anything else.

		Linus

---
 drivers/ide/ide-iops.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index 8aae917..ae03151 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -506,14 +506,16 @@ void ide_fix_driveid (struct hd_driveid *id)
 
 void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
 {
-	u8 *p = s, *end = &s[bytecount & ~1]; /* bytecount must be even */
+	u8 *p, *end = &s[bytecount & ~1]; /* bytecount must be even */
 
 	if (byteswap) {
 		/* convert from big-endian to host byte order */
-		for (p = end ; p != s;)
-			be16_to_cpus((u16 *)(p -= 2));
+		for (p = s ; p != end ; p += 2)
+			be16_to_cpus((u16 *) p);
 	}
+
 	/* strip leading blanks */
+	p = s;
 	while (s != end && *s == ' ')
 		++s;
 	/* compress internal blanks and strip trailing blanks */

Re: recent IDE regression

[Ben Dooks]

On Fri, Jul 25, 2008 at 09:37:25AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 25 Jul 2008, Ben Dooks wrote:
> > 
> > personally, i would much prefer to see the loop being less evil
> > like:
> > 
> > 	for (p = s; p < end; p += 2)
> > 		be16_to_cpus((u16 *)p);
> 
> Well, in this case, the code actually depends on 'p' being back at the 
> start of the buffer by the end of it all, so it would need some more 
> changes than that. 

I'm not adverse to the loop running backwards, it is just that we end
up evaluating macro with side-effects to the pointer itself, which is
"just asking for trouble". 

		for (p = end ; p != s; p -= 2)
			be16_to_cpus((u16 *)p);

This removes the side-effect of be16_to_cpus(), which means that the
change to p is moved back into the for statement; After all, the for
construct has three code blocks for a reason!

I just wonder what would happen if be16_to_cpus(x) evaluated 'x' more
than once... it would be difficult to find problems introduced if this
happened as it would fail to hang, just not do the 'right' thing... 

> But yes, I applied David's patch, but I _also_ suspect that we would be 
> better off without code that does horrid things like casts and assignments 
> inside the function arguments.
> 
> So it would be nice to re-code that loop to be more readable. But due to 
> the reliance of 'p' being 's' after the loop, the minimal patch would be 
> something like the appended.

As noted above, I don't have any problems with the looping going backwards,
just the problems with potential side-effects.
 
> Bartlomiej - take this or not, I'm not going to commit it - I haven't 
> tested it, nor do I even have any machines that would trigger it. So this 
> is more a "maybe something like this" than anything else.
> 
> 		Linus
> 
> ---
>  drivers/ide/ide-iops.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> index 8aae917..ae03151 100644
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -506,14 +506,16 @@ void ide_fix_driveid (struct hd_driveid *id)
>  
>  void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
>  {
> -	u8 *p = s, *end = &s[bytecount & ~1]; /* bytecount must be even */
> +	u8 *p, *end = &s[bytecount & ~1]; /* bytecount must be even */
>  
>  	if (byteswap) {
>  		/* convert from big-endian to host byte order */
> -		for (p = end ; p != s;)
> -			be16_to_cpus((u16 *)(p -= 2));
> +		for (p = s ; p != end ; p += 2)
> +			be16_to_cpus((u16 *) p);
>  	}
> +
>  	/* strip leading blanks */
> +	p = s;
>  	while (s != end && *s == ' ')
>  		++s;
>  	/* compress internal blanks and strip trailing blanks */

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: recent IDE regression

[Bartlomiej Zolnierkiewicz]

On Friday 25 July 2008, David Miller wrote:

[...]

> Something like this:
> 
> endian: Always evaluate arguments.
> 
> Changeset 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
> ("ide: trivial sparse annotations") created an IDE bootup
> regression on big-endian systems.  In drivers/ide/ide-iops.c,
> function ide_fixstring() we now have the loop:
> 
> 		for (p = end ; p != s;)
> 			be16_to_cpus((u16 *)(p -= 2));
> 
> which will never terminate on big-endian because in such
> a configuration be16_to_cpus() evaluates to "do { } while (0)"
> 
> Therefore, always evaluate the arguments to nop endian transformation
> operations.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

Thanks David.

PS We need more big-endian users testing linux-next (this particular patch
despite being trivial has been put there just-in-case for a week)
 

Re: recent IDE regression

[Bartlomiej Zolnierkiewicz]

On Friday 25 July 2008, Linus Torvalds wrote:
> 
> On Fri, 25 Jul 2008, Ben Dooks wrote:
> > 
> > personally, i would much prefer to see the loop being less evil
> > like:
> > 
> > 	for (p = s; p < end; p += 2)
> > 		be16_to_cpus((u16 *)p);
> 
> Well, in this case, the code actually depends on 'p' being back at the 
> start of the buffer by the end of it all, so it would need some more 
> changes than that. 
> 
> But yes, I applied David's patch, but I _also_ suspect that we would be 
> better off without code that does horrid things like casts and assignments 
> inside the function arguments.
> 
> So it would be nice to re-code that loop to be more readable. But due to 
> the reliance of 'p' being 's' after the loop, the minimal patch would be 
> something like the appended.
> 
> Bartlomiej - take this or not, I'm not going to commit it - I haven't 
> tested it, nor do I even have any machines that would trigger it. So this 
> is more a "maybe something like this" than anything else.
> 
> 		Linus

Looks fine and I added it to pata tree but a test+ACK from
one of big-endianners would be great.

> ---
>  drivers/ide/ide-iops.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> index 8aae917..ae03151 100644
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -506,14 +506,16 @@ void ide_fix_driveid (struct hd_driveid *id)
>  
>  void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
>  {
> -	u8 *p = s, *end = &s[bytecount & ~1]; /* bytecount must be even */
> +	u8 *p, *end = &s[bytecount & ~1]; /* bytecount must be even */
>  
>  	if (byteswap) {
>  		/* convert from big-endian to host byte order */
> -		for (p = end ; p != s;)
> -			be16_to_cpus((u16 *)(p -= 2));
> +		for (p = s ; p != end ; p += 2)
> +			be16_to_cpus((u16 *) p);
>  	}
> +
>  	/* strip leading blanks */
> +	p = s;
>  	while (s != end && *s == ' ')
>  		++s;
>  	/* compress internal blanks and strip trailing blanks */

Re: recent IDE regression

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 26 Jul 2008 14:04:49 +0200

> PS We need more big-endian users testing linux-next (this particular patch
> despite being trivial has been put there just-in-case for a week)

Normally I do test linux-next from time to time.

But in the past week or so my efforts have been purely focused on
merging the tree(s) that I maintain.

It's going to be like this for bascially every other person doing
development.

Therefore I would suggest not adding changes during the merge window
that you or someone else has not actually tested.

Re: recent IDE regression

[Bartlomiej Zolnierkiewicz]

On Sunday 27 July 2008, David Miller wrote:

[...]

> Therefore I would suggest not adding changes during the merge window
> that you or someone else has not actually tested.

This the standard policy, unfortunately having 100% testing
coverage is not possible without active help from other people.

Anyway, I'll try to make stricter selection next time.

Thanks,
Bart