After today's IDE merge my sparc64 workstation stopped booting.
It's due to this change:
commit 7fa897b91a3ea0f16c2873b869d7a0eef05acff4
Author: Harvey Harrison <[email protected]>
Date: Thu Jul 24 22:53:34 2008 +0200
ide: trivial sparse annotations
Signed-off-by: Harvey Harrison <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
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 <[email protected]>
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))
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 <[email protected]>
> Date: Thu Jul 24 22:53:34 2008 +0200
>
> ide: trivial sparse annotations
>
> Signed-off-by: Harvey Harrison <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
>
> 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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
From: Ben Dooks <[email protected]>
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.
On Fri, Jul 25, 2008 at 01:42:52AM -0700, David Miller wrote:
> From: Ben Dooks <[email protected]>
> 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.
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 */
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.
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 */
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 <[email protected]>
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)
From: Bartlomiej Zolnierkiewicz <[email protected]>
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.
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