diff mbox series

[06/12] openrisc: Use of_get_cpu_hwid()

Message ID 20211006164332.1981454-7-robh@kernel.org
State Accepted
Commit 4e0fa9eeb102165c38c51b68191ebfd84ccc5e04
Headers show
Series DT: CPU h/w id parsing clean-ups and cacheinfo id support | expand

Commit Message

Rob Herring (Arm) Oct. 6, 2021, 4:43 p.m. UTC
Replace open coded parsing of CPU nodes' 'reg' property with
of_get_cpu_hwid().

Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Stafford Horne <shorne@gmail.com>
Cc: openrisc@lists.librecores.org
Signed-off-by: Rob Herring <robh@kernel.org>

---
 arch/openrisc/kernel/smp.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.30.2

Comments

Stafford Horne Oct. 6, 2021, 8:44 p.m. UTC | #1
On Wed, Oct 06, 2021 at 11:43:26AM -0500, Rob Herring wrote:
> Replace open coded parsing of CPU nodes' 'reg' property with

> of_get_cpu_hwid().

> 

> Cc: Jonas Bonn <jonas@southpole.se>

> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>

> Cc: Stafford Horne <shorne@gmail.com>

> Cc: openrisc@lists.librecores.org

> Signed-off-by: Rob Herring <robh@kernel.org>

> ---

>  arch/openrisc/kernel/smp.c | 6 +-----

>  1 file changed, 1 insertion(+), 5 deletions(-)

> 

> diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c

> index 415e209732a3..7d5a4f303a5a 100644

> --- a/arch/openrisc/kernel/smp.c

> +++ b/arch/openrisc/kernel/smp.c

> @@ -65,11 +65,7 @@ void __init smp_init_cpus(void)

>  	u32 cpu_id;

>  

>  	for_each_of_cpu_node(cpu) {

> -		if (of_property_read_u32(cpu, "reg", &cpu_id)) {

> -			pr_warn("%s missing reg property", cpu->full_name);

> -			continue;

> -		}

> -

> +		cpu_id = of_get_cpu_hwid(cpu);


You have defined of_get_cpu_hwid to return u64, will this create compiler
warnings when since we are storing a u64 into a u32?

It seems only if we make with W=3.

I thought we usually warned on this.  Oh well, for the openrisc bits.

Acked-by: Stafford Horne <shorne@gmail.com>


>  		if (cpu_id < NR_CPUS)

>  			set_cpu_possible(cpu_id, true);

>  	}

> -- 

> 2.30.2

>
Rob Herring (Arm) Oct. 6, 2021, 9:08 p.m. UTC | #2
On Wed, Oct 6, 2021 at 3:44 PM Stafford Horne <shorne@gmail.com> wrote:
>

> On Wed, Oct 06, 2021 at 11:43:26AM -0500, Rob Herring wrote:

> > Replace open coded parsing of CPU nodes' 'reg' property with

> > of_get_cpu_hwid().

> >

> > Cc: Jonas Bonn <jonas@southpole.se>

> > Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>

> > Cc: Stafford Horne <shorne@gmail.com>

> > Cc: openrisc@lists.librecores.org

> > Signed-off-by: Rob Herring <robh@kernel.org>

> > ---

> >  arch/openrisc/kernel/smp.c | 6 +-----

> >  1 file changed, 1 insertion(+), 5 deletions(-)

> >

> > diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c

> > index 415e209732a3..7d5a4f303a5a 100644

> > --- a/arch/openrisc/kernel/smp.c

> > +++ b/arch/openrisc/kernel/smp.c

> > @@ -65,11 +65,7 @@ void __init smp_init_cpus(void)

> >       u32 cpu_id;

> >

> >       for_each_of_cpu_node(cpu) {

> > -             if (of_property_read_u32(cpu, "reg", &cpu_id)) {

> > -                     pr_warn("%s missing reg property", cpu->full_name);

> > -                     continue;

> > -             }

> > -

> > +             cpu_id = of_get_cpu_hwid(cpu);


Oops, that should be: of_get_cpu_hwid(cpu, 0);

I thought I double checked all those...

> You have defined of_get_cpu_hwid to return u64, will this create compiler

> warnings when since we are storing a u64 into a u32?


I'm counting on the caller to know the max size for their platform.

>

> It seems only if we make with W=3.

>

> I thought we usually warned on this.  Oh well, for the openrisc bits.


That's only on ptr truncation I think.

> Acked-by: Stafford Horne <shorne@gmail.com>

>

> >               if (cpu_id < NR_CPUS)

> >                       set_cpu_possible(cpu_id, true);

> >       }

> > --

> > 2.30.2

> >
Stafford Horne Oct. 6, 2021, 9:25 p.m. UTC | #3
On Wed, Oct 06, 2021 at 04:08:38PM -0500, Rob Herring wrote:
> On Wed, Oct 6, 2021 at 3:44 PM Stafford Horne <shorne@gmail.com> wrote:

> >

> > On Wed, Oct 06, 2021 at 11:43:26AM -0500, Rob Herring wrote:

> > > Replace open coded parsing of CPU nodes' 'reg' property with

> > > of_get_cpu_hwid().

> > >

> > > Cc: Jonas Bonn <jonas@southpole.se>

> > > Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>

> > > Cc: Stafford Horne <shorne@gmail.com>

> > > Cc: openrisc@lists.librecores.org

> > > Signed-off-by: Rob Herring <robh@kernel.org>

> > > ---

> > >  arch/openrisc/kernel/smp.c | 6 +-----

> > >  1 file changed, 1 insertion(+), 5 deletions(-)

> > >

> > > diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c

> > > index 415e209732a3..7d5a4f303a5a 100644

> > > --- a/arch/openrisc/kernel/smp.c

> > > +++ b/arch/openrisc/kernel/smp.c

> > > @@ -65,11 +65,7 @@ void __init smp_init_cpus(void)

> > >       u32 cpu_id;

> > >

> > >       for_each_of_cpu_node(cpu) {

> > > -             if (of_property_read_u32(cpu, "reg", &cpu_id)) {

> > > -                     pr_warn("%s missing reg property", cpu->full_name);

> > > -                     continue;

> > > -             }

> > > -

> > > +             cpu_id = of_get_cpu_hwid(cpu);

> 

> Oops, that should be: of_get_cpu_hwid(cpu, 0);


OK. I checked all other patches in the series, it seems OpenRISC was the only
one missing that.  Sorry I missed it initially.

> I thought I double checked all those...

> 

> > You have defined of_get_cpu_hwid to return u64, will this create compiler

> > warnings when since we are storing a u64 into a u32?

> 

> I'm counting on the caller to know the max size for their platform.


OK.

> >

> > It seems only if we make with W=3.

> >

> > I thought we usually warned on this.  Oh well, for the openrisc bits.

> 

> That's only on ptr truncation I think.


Right, that makes sense.

-Stafford
Segher Boessenkool Oct. 6, 2021, 9:27 p.m. UTC | #4
On Thu, Oct 07, 2021 at 05:44:00AM +0900, Stafford Horne wrote:
> You have defined of_get_cpu_hwid to return u64, will this create compiler

> warnings when since we are storing a u64 into a u32?

> 

> It seems only if we make with W=3.


Yes.  This is done by -Wconversion, "Warn for implicit conversions that
may alter a value."

> I thought we usually warned on this.


This warning is not in -Wall or -Wextra either, it suffers too much from
false positives.  It is very natural to just ignore the high bits of
modulo types (which is what "unsigned" types *are*).  Or the bits that
"fall off" on a conversion.  The C standard makes this required
behaviour, it is useful, and it is the only convenient way of getting
this!


Segher
Stafford Horne Oct. 6, 2021, 10:37 p.m. UTC | #5
Hi Segher,

On Wed, Oct 06, 2021 at 04:27:28PM -0500, Segher Boessenkool wrote:
> On Thu, Oct 07, 2021 at 05:44:00AM +0900, Stafford Horne wrote:

> > You have defined of_get_cpu_hwid to return u64, will this create compiler

> > warnings when since we are storing a u64 into a u32?

> > 

> > It seems only if we make with W=3.

> 

> Yes.  This is done by -Wconversion, "Warn for implicit conversions that

> may alter a value."


Yeah, that is what I found out when I looked into it.

> > I thought we usually warned on this.

> 

> This warning is not in -Wall or -Wextra either, it suffers too much from

> false positives.  It is very natural to just ignore the high bits of

> modulo types (which is what "unsigned" types *are*).  Or the bits that

> "fall off" on a conversion.  The C standard makes this required

> behaviour, it is useful, and it is the only convenient way of getting

> this!


Thanks for the background, It does make sense. I guess I was confused with java
which requires casting when you store to a smaller size.  I.e.

    Test.java:5: error: incompatible types: possible lossy conversion from int to short
	s = i;

-Stafford
David Laight Oct. 7, 2021, 7:53 a.m. UTC | #6
From: Segher Boessenkool

> Sent: 06 October 2021 22:27

> 

> On Thu, Oct 07, 2021 at 05:44:00AM +0900, Stafford Horne wrote:

> > You have defined of_get_cpu_hwid to return u64, will this create compiler

> > warnings when since we are storing a u64 into a u32?

> >

> > It seems only if we make with W=3.

> 

> Yes.  This is done by -Wconversion, "Warn for implicit conversions that

> may alter a value."

> 

> > I thought we usually warned on this.


The microsoft compiler does - best to turn all those warnings off.

> This warning is not in -Wall or -Wextra either, it suffers too much from

> false positives.  It is very natural to just ignore the high bits of

> modulo types (which is what "unsigned" types *are*).  Or the bits that

> "fall off" on a conversion.  The C standard makes this required

> behaviour, it is useful, and it is the only convenient way of getting

> this!


I've also seen a compiler convert:
	struct->char_member = (char)(int_val & 0xff);
into:
	reg = int_val;
	reg &= 0xff; // for the & 0xff
	reg &= 0xff; // for the cast
	struct->char_member = low_8bits(reg);

You really don't want the extra noise.

I'll bet that (char)int_val is actually an arithmetic expression.
So its type will be 'int'.

	David

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

Patch

diff --git a/arch/openrisc/kernel/smp.c b/arch/openrisc/kernel/smp.c
index 415e209732a3..7d5a4f303a5a 100644
--- a/arch/openrisc/kernel/smp.c
+++ b/arch/openrisc/kernel/smp.c
@@ -65,11 +65,7 @@  void __init smp_init_cpus(void)
 	u32 cpu_id;
 
 	for_each_of_cpu_node(cpu) {
-		if (of_property_read_u32(cpu, "reg", &cpu_id)) {
-			pr_warn("%s missing reg property", cpu->full_name);
-			continue;
-		}
-
+		cpu_id = of_get_cpu_hwid(cpu);
 		if (cpu_id < NR_CPUS)
 			set_cpu_possible(cpu_id, true);
 	}