All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] of: replace be32_to_cpu to be32_to_cpup
@ 2019-04-30  9:00 Phong Tran
  2019-04-30  9:37 ` Nathan Chancellor
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Phong Tran @ 2019-04-30  9:00 UTC (permalink / raw)
  To: robh+dt, frowand.list, pantelis.antoniou
  Cc: natechancellor, devicetree, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup
  2019-04-30  9:00 [PATCH] of: replace be32_to_cpu to be32_to_cpup Phong Tran
@ 2019-04-30  9:37 ` Nathan Chancellor
  2019-04-30 10:52 ` David Laight
  2019-04-30 13:32 ` [PATCH] of: replace be32_to_cpu to be32_to_cpup Christoph Hellwig
  2 siblings, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2019-04-30  9:37 UTC (permalink / raw)
  To: Phong Tran
  Cc: robh+dt, frowand.list, pantelis.antoniou, devicetree,
	linux-kernel, Nick Desaulniers, clang-built-linux

+ 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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [PATCH] of: replace be32_to_cpu to be32_to_cpup
  2019-04-30  9:00 [PATCH] of: replace be32_to_cpu to be32_to_cpup Phong Tran
  2019-04-30  9:37 ` Nathan Chancellor
@ 2019-04-30 10:52 ` David Laight
  2019-04-30 14:56   ` [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu() Phong Tran
  2019-04-30 13:32 ` [PATCH] of: replace be32_to_cpu to be32_to_cpup Christoph Hellwig
  2 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2019-04-30 10:52 UTC (permalink / raw)
  To: 'Phong Tran', robh+dt, frowand.list, pantelis.antoniou
  Cc: natechancellor, devicetree, linux-kernel

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)


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup
  2019-04-30  9:00 [PATCH] of: replace be32_to_cpu to be32_to_cpup Phong Tran
  2019-04-30  9:37 ` Nathan Chancellor
  2019-04-30 10:52 ` David Laight
@ 2019-04-30 13:32 ` Christoph Hellwig
  2019-04-30 14:44   ` Phong Tran
  2 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2019-04-30 13:32 UTC (permalink / raw)
  To: Phong Tran
  Cc: robh+dt, frowand.list, pantelis.antoniou, natechancellor,
	devicetree, linux-kernel

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);
}

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: replace be32_to_cpu to be32_to_cpup
  2019-04-30 13:32 ` [PATCH] of: replace be32_to_cpu to be32_to_cpup Christoph Hellwig
@ 2019-04-30 14:44   ` Phong Tran
  0 siblings, 0 replies; 10+ messages in thread
From: Phong Tran @ 2019-04-30 14:44 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: robh+dt, frowand.list, Pantelis Antoniou, natechancellor,
	devicetree, linux-kernel, Nick Desaulniers, clang-built-linux

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()
  2019-04-30 10:52 ` David Laight
@ 2019-04-30 14:56   ` Phong Tran
  2019-04-30 16:28     ` Nick Desaulniers
  0 siblings, 1 reply; 10+ messages in thread
From: Phong Tran @ 2019-04-30 14:56 UTC (permalink / raw)
  To: robh+dt, frowand.list, pantelis.antoniou
  Cc: David.Laight, hch, ndesaulniers, natechancellor, devicetree,
	linux-kernel, clang-built-linux, 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()
  2019-04-30 14:56   ` [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu() Phong Tran
@ 2019-04-30 16:28     ` Nick Desaulniers
  2019-04-30 16:29       ` Nick Desaulniers
  0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2019-04-30 16:28 UTC (permalink / raw)
  To: Phong Tran
  Cc: robh+dt, Frank Rowand, pantelis.antoniou, David.Laight, hch,
	Nathan Chancellor, devicetree, LKML, clang-built-linux

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()
  2019-04-30 16:28     ` Nick Desaulniers
@ 2019-04-30 16:29       ` Nick Desaulniers
  2019-05-01 18:13         ` Nick Desaulniers
  0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2019-04-30 16:29 UTC (permalink / raw)
  To: Phong Tran
  Cc: robh+dt, Frank Rowand, pantelis.antoniou, David.Laight, hch,
	Nathan Chancellor, devicetree, LKML, clang-built-linux

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()
  2019-04-30 16:29       ` Nick Desaulniers
@ 2019-05-01 18:13         ` Nick Desaulniers
  2019-05-01 19:33           ` Rob Herring
  0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2019-05-01 18:13 UTC (permalink / raw)
  To: robh+dt, Frank Rowand, pantelis.antoniou
  Cc: David.Laight, hch, Nathan Chancellor, devicetree, LKML,
	clang-built-linux, Phong Tran

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu()
  2019-05-01 18:13         ` Nick Desaulniers
@ 2019-05-01 19:33           ` Rob Herring
  0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2019-05-01 19:33 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Frank Rowand, Pantelis Antoniou, David Laight, Christoph Hellwig,
	Nathan Chancellor, devicetree, LKML, clang-built-linux,
	Phong Tran

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-01 19:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-30  9:00 [PATCH] of: replace be32_to_cpu to be32_to_cpup Phong Tran
2019-04-30  9:37 ` Nathan Chancellor
2019-04-30 10:52 ` David Laight
2019-04-30 14:56   ` [PATCH V2] of: fix clang -Wunsequenced for be32_to_cpu() Phong Tran
2019-04-30 16:28     ` Nick Desaulniers
2019-04-30 16:29       ` Nick Desaulniers
2019-05-01 18:13         ` Nick Desaulniers
2019-05-01 19:33           ` Rob Herring
2019-04-30 13:32 ` [PATCH] of: replace be32_to_cpu to be32_to_cpup Christoph Hellwig
2019-04-30 14:44   ` Phong Tran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.