| Message ID | 1534877212-22678-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/xive: Initialize symbol before usage | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
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
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.
Cédric 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/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. Even better! cheers
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 */
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(-)