[PATCH] powerpc/xive: Initialize symbol before usage

[PATCH] powerpc/xive: Initialize symbol before usage

[Breno Leitao]

Function xive_native_get_ipi() might uses chip_id without it being
initialized. This gives the following error on 'smatch' tool:

	error: uninitialized symbol 'chip_id'

This patch simply sets chip_id initial value to 0.

CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/sysdev/xive/native.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 311185b9960a..fc56673a3c0f 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -239,7 +239,7 @@ static bool xive_native_match(struct device_node *node)
 static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
 {
 	struct device_node *np;
-	unsigned int chip_id;
+	unsigned int chip_id = 0;
 	s64 irq;
 
 	/* Find the chip ID */
-- 
2.16.3

Re: [PATCH] powerpc/xive: Initialize symbol before usage

[Michael Ellerman]

Hi Breno,

Breno Leitao <leitao@debian.org> writes:
> Function xive_native_get_ipi() might uses chip_id without it being
> initialized. This gives the following error on 'smatch' tool:
>
> 	error: uninitialized symbol 'chip_id'

Which is correct, it can be used uninitialised. I'm surprised GCC
doesn't warn about it.

> This patch simply sets chip_id initial value to 0.

I'd prefer we fixed it differently, by explicitly initialising to zero
at the appropriate place in the code.

> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 311185b9960a..fc56673a3c0f 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -239,7 +239,7 @@ static bool xive_native_match(struct device_node *node)
>  static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  {
>  	struct device_node *np;
> -	unsigned int chip_id;
> +	unsigned int chip_id = 0;
>  	s64 irq;
>  
>  	/* Find the chip ID */

The current code is:

	/* Find the chip ID */
	np = of_get_cpu_node(cpu, NULL);
	if (np) {
		if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
			chip_id = 0;
	}

Where if np is NULL then we don't initialise chip_id.

Which could be:

	np = of_get_cpu_node(cpu, NULL);
        if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
                chip_id = 0;

Because of_property_read_u32() will just return an error if np is NULL.

It's also missing an of_node_put() of np, you should do a separate patch
to fix that. You can just do it unconditionally after the
of_property_read_u32().

cheers

Re: [PATCH] powerpc/xive: Initialize symbol before usage

[Cédric Le Goater]

On 08/23/2018 05:24 AM, Michael Ellerman wrote:
> Hi Breno,
> 
> Breno Leitao <leitao@debian.org> writes:
>> Function xive_native_get_ipi() might uses chip_id without it being
>> initialized. This gives the following error on 'smatch' tool:
>>
>> 	error: uninitialized symbol 'chip_id'
> 
> Which is correct, it can be used uninitialised. I'm surprised GCC
> doesn't warn about it.
> 
>> This patch simply sets chip_id initial value to 0.
> 
> I'd prefer we fixed it differently, by explicitly initialising to zero
> at the appropriate place in the code.
> 
>> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
>> index 311185b9960a..fc56673a3c0f 100644
>> --- a/arch/powerpc/sysdev/xive/native.c
>> +++ b/arch/powerpc/sysdev/xive/native.c
>> @@ -239,7 +239,7 @@ static bool xive_native_match(struct device_node *node)
>>  static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>>  {
>>  	struct device_node *np;
>> -	unsigned int chip_id;
>> +	unsigned int chip_id = 0;
>>  	s64 irq;
>>  
>>  	/* Find the chip ID */
> 
> The current code is:
> 
> 	/* Find the chip ID */
> 	np = of_get_cpu_node(cpu, NULL);
> 	if (np) {
> 		if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
> 			chip_id = 0;
> 	}
> 
> Where if np is NULL then we don't initialise chip_id.
> 
> Which could be:
> 
> 	np = of_get_cpu_node(cpu, NULL);
>         if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
>                 chip_id = 0;
> 
> Because of_property_read_u32() will just return an error if np is NULL.
> 
> It's also missing an of_node_put() of np, you should do a separate patch
> to fix that. You can just do it unconditionally after the
> of_property_read_u32().

I think we can simply get rid of the OF code under xive_native_get_ipi()
and use xc->chip_id instead. It should be safe to use as xive_prepare_cpu() 
should have initialized ->chip_id by the time xive_native_get_ipi() is 
called. 

Cheers,

C.

Re: [PATCH] powerpc/xive: Initialize symbol before usage

[Michael Ellerman]

C=C3=A9dric Le Goater <clg@kaod.org> writes:
> On 08/23/2018 05:24 AM, Michael Ellerman wrote:
>> Breno Leitao <leitao@debian.org> writes:
>>> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xi=
ve/native.c
>>> index 311185b9960a..fc56673a3c0f 100644
>>> --- a/arch/powerpc/sysdev/xive/native.c
>>> +++ b/arch/powerpc/sysdev/xive/native.c
>>> @@ -239,7 +239,7 @@ static bool xive_native_match(struct device_node *n=
ode)
>>>  static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>>>  {
>>>  	struct device_node *np;
>>> -	unsigned int chip_id;
>>> +	unsigned int chip_id =3D 0;
>>>  	s64 irq;
>>>=20=20
>>>  	/* Find the chip ID */
>>=20
>> The current code is:
>>=20
>> 	/* Find the chip ID */
>> 	np =3D of_get_cpu_node(cpu, NULL);
>> 	if (np) {
>> 		if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
>> 			chip_id =3D 0;
>> 	}
>>=20
>> Where if np is NULL then we don't initialise chip_id.
>>=20
>> Which could be:
>>=20
>> 	np =3D of_get_cpu_node(cpu, NULL);
>>         if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
>>                 chip_id =3D 0;
>>=20
>> Because of_property_read_u32() will just return an error if np is NULL.
>>=20
>> It's also missing an of_node_put() of np, you should do a separate patch
>> to fix that. You can just do it unconditionally after the
>> of_property_read_u32().
>
> I think we can simply get rid of the OF code under xive_native_get_ipi()
> and use xc->chip_id instead. It should be safe to use as xive_prepare_cpu=
()=20
> should have initialized ->chip_id by the time xive_native_get_ipi() is=20
> called.=20

Even better!

cheers

[PATCH v2] powerpc/xive: Avoid unitialized variable

[Breno Leitao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1417 bytes --]

From: Breno Leitao <breno.leitao@gmail.com>

Function xive_native_get_ipi() might uses chip_id without it being
initialized.

This gives the following error on 'smatch' tool:

	error: uninitialized symbol 'chip_id'

The suggestion is using xc->chip_id instead of consulting the OF for chip id,
which is safe since xive_prepare_cpu() should have initialized ->chip_id by
the time xive_native_get_ipi() is called.

CC: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/sysdev/xive/native.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 311185b9960a..bd90fd464a3a 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -238,20 +238,11 @@ static bool xive_native_match(struct device_node *node)
 #ifdef CONFIG_SMP
 static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
 {
-	struct device_node *np;
-	unsigned int chip_id;
 	s64 irq;
 
-	/* Find the chip ID */
-	np = of_get_cpu_node(cpu, NULL);
-	if (np) {
-		if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
-			chip_id = 0;
-	}
-
 	/* Allocate an IPI and populate info about it */
 	for (;;) {
-		irq = opal_xive_allocate_irq(chip_id);
+		irq = opal_xive_allocate_irq(xc->chip_id);
 		if (irq == OPAL_BUSY) {
 			msleep(1);
 			continue;
-- 
2.17.1

Re: [PATCH v2] powerpc/xive: Avoid unitialized variable

[Cédric Le Goater]

On 08/24/2018 01:26 AM, Breno Leitao wrote:
> From: Breno Leitao <breno.leitao@gmail.com>
> 
> Function xive_native_get_ipi() might uses chip_id without it being
> initialized.
> 
> This gives the following error on 'smatch' tool:
> 
> 	error: uninitialized symbol 'chip_id'
> 
> The suggestion is using xc->chip_id instead of consulting the OF for chip id,
> which is safe since xive_prepare_cpu() should have initialized ->chip_id by
> the time xive_native_get_ipi() is called.
> 
> CC: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  arch/powerpc/sysdev/xive/native.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 311185b9960a..bd90fd464a3a 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -238,20 +238,11 @@ static bool xive_native_match(struct device_node *node)
>  #ifdef CONFIG_SMP
>  static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
>  {
> -	struct device_node *np;
> -	unsigned int chip_id;
>  	s64 irq;
>  
> -	/* Find the chip ID */
> -	np = of_get_cpu_node(cpu, NULL);
> -	if (np) {
> -		if (of_property_read_u32(np, "ibm,chip-id", &chip_id) < 0)
> -			chip_id = 0;
> -	}
> -
>  	/* Allocate an IPI and populate info about it */
>  	for (;;) {
> -		irq = opal_xive_allocate_irq(chip_id);
> +		irq = opal_xive_allocate_irq(xc->chip_id);
>  		if (irq == OPAL_BUSY) {
>  			msleep(1);
>  			continue;
> 

Re: [v2] powerpc/xive: Avoid unitialized variable

[Michael Ellerman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Thu, 2018-08-23 at 23:26:39 UTC, Breno Leitao wrote:
> From: Breno Leitao <breno.leitao@gmail.com>
> 
> Function xive_native_get_ipi() might uses chip_id without it being
> initialized.
> 
> This gives the following error on 'smatch' tool:
> 
> 	error: uninitialized symbol 'chip_id'
> 
> The suggestion is using xc->chip_id instead of consulting the OF for chip id,
> which is safe since xive_prepare_cpu() should have initialized ->chip_id by
> the time xive_native_get_ipi() is called.
> 
> CC: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8ac9e5bfd8cf41ef106ac97267117e

cheers