[PATCH] of: replace be32_to_cpu to be32_to_cpup

[PATCH] of: replace be32_to_cpu to be32_to_cpup

[Phong Tran]

The cell is a pointer to __be32.
with the be32_to_cpu a lot of clang warning show that:

./include/linux/of.h:238:37: warning: multiple unsequenced modifications
to 'cell' [-Wunsequenced]
                r = (r << 32) | be32_to_cpu(*(cell++));
                                                  ^~
./include/linux/byteorder/generic.h:95:21: note: expanded from macro
'be32_to_cpu'
                    ^
./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
from macro '__be32_to_cpu'
                                                          ^
./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
        ___constant_swab32(x) :                 \
                           ^
./include/uapi/linux/swab.h:18:12: note: expanded from macro
'___constant_swab32'
        (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                  ^

Signed-off-by: Phong Tran <tranmanphong@gmail.com>
---
 include/linux/of.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index e240992e5cb6..1c35fc8f19b0 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -235,7 +235,7 @@ static inline u64 of_read_number(const __be32 *cell, int size)
 {
 	u64 r = 0;
 	while (size--)
-		r = (r << 32) | be32_to_cpu(*(cell++));
+		r = (r << 32) | be32_to_cpup(cell++);
 	return r;
 }
 
-- 
2.21.0

Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup

[Nathan Chancellor]

+ Nick and the list

On Tue, Apr 30, 2019 at 04:00:44PM +0700, Phong Tran wrote:
> The cell is a pointer to __be32.
> with the be32_to_cpu a lot of clang warning show that:
> 
> ./include/linux/of.h:238:37: warning: multiple unsequenced modifications
> to 'cell' [-Wunsequenced]
>                 r = (r << 32) | be32_to_cpu(*(cell++));
>                                                   ^~
> ./include/linux/byteorder/generic.h:95:21: note: expanded from macro
> 'be32_to_cpu'
>                     ^
> ./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
> from macro '__be32_to_cpu'
>                                                           ^
> ./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
>         ___constant_swab32(x) :                 \
>                            ^
> ./include/uapi/linux/swab.h:18:12: note: expanded from macro
> '___constant_swab32'
>         (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
>                   ^
> 
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
>  include/linux/of.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index e240992e5cb6..1c35fc8f19b0 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -235,7 +235,7 @@ static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>  	u64 r = 0;
>  	while (size--)
> -		r = (r << 32) | be32_to_cpu(*(cell++));
> +		r = (r << 32) | be32_to_cpup(cell++);
>  	return r;
>  }
>  
> -- 
> 2.21.0
> 

While the patch does remove the warning, I am not convinced that this
isn't a clang bug based on my brief analysis here:

https://github.com/ClangBuiltLinux/linux/issues/460#issuecomment-487808008

However, I'm waiting for people smarter than I am to comment on whether
that sounds correct or not.

I am not familiar with the various different big/little endian functions
enough to review this but thank you for the patch!

Nathan

RE: [PATCH] of: replace be32_to_cpu to be32_to_cpup

[David Laight]

From: Phong Tran
> Sent: 30 April 2019 10:01
> The cell is a pointer to __be32.
> with the be32_to_cpu a lot of clang warning show that:
> 
> ./include/linux/of.h:238:37: warning: multiple unsequenced modifications
> to 'cell' [-Wunsequenced]
>                 r = (r << 32) | be32_to_cpu(*(cell++));
>                                                   ^~
> ./include/linux/byteorder/generic.h:95:21: note: expanded from macro
> 'be32_to_cpu'
>                     ^
> ./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
> from macro '__be32_to_cpu'
>                                                           ^
> ./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
>         ___constant_swab32(x) :                 \
>                            ^
> ./include/uapi/linux/swab.h:18:12: note: expanded from macro
> '___constant_swab32'
>         (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
>                   ^
> 
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
>  include/linux/of.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index e240992e5cb6..1c35fc8f19b0 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -235,7 +235,7 @@ static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>  	u64 r = 0;
>  	while (size--)
> -		r = (r << 32) | be32_to_cpu(*(cell++));
> +		r = (r << 32) | be32_to_cpup(cell++);
>  	return r;

That is a very strange loop.
It is probably equivalent to:
	r = be32_to_cpu(*cell);
	if (size)
		r = r << 32 | be32_to_cpu(cell[1]);
	return r;

In any case replacing the while with (say):
	for (; size--; cell++)
		r = (r << 32) | be32_to_cpu(*cell);
would remove the ambiguity.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()

[Phong Tran]

Now, make the loop explicit to avoid clang warning.

./include/linux/of.h:238:37: warning: multiple unsequenced modifications
to 'cell' [-Wunsequenced]
                r = (r << 32) | be32_to_cpu(*(cell++));
                                                  ^~
./include/linux/byteorder/generic.h:95:21: note: expanded from macro
'be32_to_cpu'
                    ^
./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
from macro '__be32_to_cpu'
                                                          ^
./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
        ___constant_swab32(x) :                 \
                           ^
./include/uapi/linux/swab.h:18:12: note: expanded from macro
'___constant_swab32'
        (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                  ^

Signed-off-by: Phong Tran <tranmanphong@gmail.com>
---
 include/linux/of.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index e240992e5cb6..71ca25ac01f6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -234,8 +234,8 @@ extern struct device_node *of_find_all_nodes(struct device_node *prev);
 static inline u64 of_read_number(const __be32 *cell, int size)
 {
 	u64 r = 0;
-	while (size--)
-		r = (r << 32) | be32_to_cpu(*(cell++));
+	for(; size--; cell++)
+		r = (r << 32) | be32_to_cpu(*cell);
 	return r;
 }
 
-- 
2.21.0

Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()

[Nick Desaulniers]

On Tue, Apr 30, 2019 at 7:58 AM Phong Tran <tranmanphong@gmail.com> wrote:
>
> Now, make the loop explicit to avoid clang warning.
>
> ./include/linux/of.h:238:37: warning: multiple unsequenced modifications
> to 'cell' [-Wunsequenced]
>                 r = (r << 32) | be32_to_cpu(*(cell++));
>                                                   ^~
> ./include/linux/byteorder/generic.h:95:21: note: expanded from macro
> 'be32_to_cpu'
>                     ^
> ./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
> from macro '__be32_to_cpu'
>                                                           ^
> ./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
>         ___constant_swab32(x) :                 \
>                            ^
> ./include/uapi/linux/swab.h:18:12: note: expanded from macro
> '___constant_swab32'
>         (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
>                   ^
>
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>

Thanks for the patch.
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/460
Suggested-by: David Laight <David.Laight@ACULAB.COM>

> ---
>  include/linux/of.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index e240992e5cb6..71ca25ac01f6 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -234,8 +234,8 @@ extern struct device_node *of_find_all_nodes(struct device_node *prev);
>  static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>         u64 r = 0;
> -       while (size--)
> -               r = (r << 32) | be32_to_cpu(*(cell++));
> +       for(; size--; cell++)
> +               r = (r << 32) | be32_to_cpu(*cell);
>         return r;
>  }
>
> --
> 2.21.0
>


--
Thanks,
~Nick Desaulniers

Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()

[Nick Desaulniers]

On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Apr 30, 2019 at 7:58 AM Phong Tran <tranmanphong@gmail.com> wrote:
> >
> > Now, make the loop explicit to avoid clang warning.
> >
> > ./include/linux/of.h:238:37: warning: multiple unsequenced modifications
> > to 'cell' [-Wunsequenced]
> >                 r = (r << 32) | be32_to_cpu(*(cell++));
> >                                                   ^~
> > ./include/linux/byteorder/generic.h:95:21: note: expanded from macro
> > 'be32_to_cpu'
> >                     ^
> > ./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded
> > from macro '__be32_to_cpu'
> >                                                           ^
> > ./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
> >         ___constant_swab32(x) :                 \
> >                            ^
> > ./include/uapi/linux/swab.h:18:12: note: expanded from macro
> > '___constant_swab32'
> >         (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
> >                   ^
> >
> > Signed-off-by: Phong Tran <tranmanphong@gmail.com>
>
> Thanks for the patch.
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/460
> Suggested-by: David Laight <David.Laight@ACULAB.COM>

sent too soon...
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()

[Nick Desaulniers]

On Tue, Apr 30, 2019 at 9:29 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> > Thanks for the patch.
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/460
> > Suggested-by: David Laight <David.Laight@ACULAB.COM>
>
> sent too soon...
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

We'll also need this for stable, can the maintainers please add the
following tag if it's not too late:

Cc: stable@vger.kernel.org

to unbreak ppc back through at least 4.14.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()

[Rob Herring]

On Wed, May 1, 2019 at 1:13 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Apr 30, 2019 at 9:29 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > Thanks for the patch.
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/460
> > > Suggested-by: David Laight <David.Laight@ACULAB.COM>
> >
> > sent too soon...
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> We'll also need this for stable, can the maintainers please add the
> following tag if it's not too late:
>
> Cc: stable@vger.kernel.org

Applied with a whitespace fixup and stable tag.

Rob

Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup

[Christoph Hellwig]

On Tue, Apr 30, 2019 at 04:00:44PM +0700, Phong Tran wrote:
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index e240992e5cb6..1c35fc8f19b0 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -235,7 +235,7 @@ static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>  	u64 r = 0;
>  	while (size--)
> -		r = (r << 32) | be32_to_cpu(*(cell++));
> +		r = (r << 32) | be32_to_cpup(cell++);
>  	return r;

This whole function looks odd.  It could simply be replaced with
calls to get_unaligned_be64 / get_unaligned_be32.  Given that we have a
lot of callers we can't easily do that, but at least we could try
something like

static inline u64 of_read_number(const __be32 *cell, int size)
{
	WARN_ON_ONCE(size < 1);
	WARN_ON_ONCE(size > 2);

	if (size == 1)
		return get_unaligned_be32(cell);
	return get_unaligned_be64(cell);
}

Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup

[Phong Tran]

On Tue, Apr 30, 2019 at 8:32 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Apr 30, 2019 at 04:00:44PM +0700, Phong Tran wrote:
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index e240992e5cb6..1c35fc8f19b0 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -235,7 +235,7 @@ static inline u64 of_read_number(const __be32 *cell, int size)
> >  {
> >       u64 r = 0;
> >       while (size--)
> > -             r = (r << 32) | be32_to_cpu(*(cell++));
> > +             r = (r << 32) | be32_to_cpup(cell++);
> >       return r;
>
> This whole function looks odd.  It could simply be replaced with
> calls to get_unaligned_be64 / get_unaligned_be32.  Given that we have a
> lot of callers we can't easily do that, but at least we could try
> something like
>
It's risky. there are many callers of of_read_number().
There is suggestion from David
(https://lore.kernel.org/lkml/46b3e8edf27e4c8f98697f9e7f2117d6@AcuMS.aculab.com/)
only changing the loop.

> static inline u64 of_read_number(const __be32 *cell, int size)
> {
>         WARN_ON_ONCE(size < 1);
>         WARN_ON_ONCE(size > 2);
>
>         if (size == 1)
>                 return get_unaligned_be32(cell);
>         return get_unaligned_be64(cell);
> }

Thank you for your support.

Phong.