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