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 <[email protected]>
---
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
+ 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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)
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);
}
On Tue, Apr 30, 2019 at 8:32 PM Christoph Hellwig <[email protected]> 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/[email protected]/)
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.
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 <[email protected]>
---
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
On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Tue, Apr 30, 2019 at 7:58 AM Phong Tran <[email protected]> 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 <[email protected]>
>
> Thanks for the patch.
> Reported-by: Nick Desaulniers <[email protected]>
> Link: https://github.com/ClangBuiltLinux/linux/issues/460
> Suggested-by: David Laight <[email protected]>
sent too soon...
Reviewed-by: Nick Desaulniers <[email protected]>
--
Thanks,
~Nick Desaulniers
On Tue, Apr 30, 2019 at 7:58 AM Phong Tran <[email protected]> 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 <[email protected]>
Thanks for the patch.
Reported-by: Nick Desaulniers <[email protected]>
Link: https://github.com/ClangBuiltLinux/linux/issues/460
Suggested-by: David Laight <[email protected]>
> ---
> 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
On Tue, Apr 30, 2019 at 9:29 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
> <[email protected]> wrote:
> > Thanks for the patch.
> > Reported-by: Nick Desaulniers <[email protected]>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/460
> > Suggested-by: David Laight <[email protected]>
>
> sent too soon...
> Reviewed-by: Nick Desaulniers <[email protected]>
We'll also need this for stable, can the maintainers please add the
following tag if it's not too late:
Cc: [email protected]
to unbreak ppc back through at least 4.14.
--
Thanks,
~Nick Desaulniers
On Wed, May 1, 2019 at 1:13 PM Nick Desaulniers <[email protected]> wrote:
>
> On Tue, Apr 30, 2019 at 9:29 AM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Tue, Apr 30, 2019 at 9:28 AM Nick Desaulniers
> > <[email protected]> wrote:
> > > Thanks for the patch.
> > > Reported-by: Nick Desaulniers <[email protected]>
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/460
> > > Suggested-by: David Laight <[email protected]>
> >
> > sent too soon...
> > Reviewed-by: Nick Desaulniers <[email protected]>
>
> We'll also need this for stable, can the maintainers please add the
> following tag if it's not too late:
>
> Cc: [email protected]
Applied with a whitespace fixup and stable tag.
Rob