[1/2] ASoC: cs35l32: avoid uninitialized variable access
diff mbox

Message ID 1453741678-1988125-1-git-send-email-arnd@arndb.de
State New
Headers show

Commit Message

Arnd Bergmann Jan. 25, 2016, 5:07 p.m. UTC
gcc warns about the possibilty of accessing a property read from
devicetree in cs35l32_i2c_probe() when it has not been initialized
because CONFIG_OF is disabled:

sound/soc/codecs/cs35l32.c: In function 'cs35l32_i2c_probe':
sound/soc/codecs/cs35l32.c:278:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]

The code is actually correct because it checks the dev->of_node
variable first and we know this is NULL here, but by adding a
check for IS_ENABLED(CONFIG_OF), we can let the compiler know
as well, and also generate smaller object code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/codecs/cs35l32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Austin, Brian Jan. 25, 2016, 5:24 p.m. UTC | #1
On Jan 25, 2016, at 11:07 AM, Arnd Bergmann <arnd@arndb.de<mailto:arnd@arndb.de>> wrote:

gcc warns about the possibilty of accessing a property read from
devicetree in cs35l32_i2c_probe() when it has not been initialized
because CONFIG_OF is disabled:

sound/soc/codecs/cs35l32.c: In function 'cs35l32_i2c_probe':
sound/soc/codecs/cs35l32.c:278:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]

The code is actually correct because it checks the dev->of_node
variable first and we know this is NULL here, but by adding a
check for IS_ENABLED(CONFIG_OF), we can let the compiler know
as well, and also generate smaller object code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de<mailto:arnd@arndb.de>>
---
sound/soc/codecs/cs35l32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Thanks,

Acked-by: Brian Austin <brian.austin@cirrus.com<mailto:brian.austin@cirrus.com>>
Mark Brown March 5, 2016, 5:39 a.m. UTC | #2
On Mon, Jan 25, 2016 at 06:07:32PM +0100, Arnd Bergmann wrote:

> -		if (i2c_client->dev.of_node) {
> +		if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {

This feels it's going to be happening a lot and we should probably have
a dev_has_of_node() helper that does the IS_ENABLED() check.
Russell King - ARM Linux March 5, 2016, 2:54 p.m. UTC | #3
On Mon, Jan 25, 2016 at 06:07:32PM +0100, Arnd Bergmann wrote:
> gcc warns about the possibilty of accessing a property read from
> devicetree in cs35l32_i2c_probe() when it has not been initialized
> because CONFIG_OF is disabled:
> 
> sound/soc/codecs/cs35l32.c: In function 'cs35l32_i2c_probe':
> sound/soc/codecs/cs35l32.c:278:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> The code is actually correct because it checks the dev->of_node
> variable first and we know this is NULL here, but by adding a
> check for IS_ENABLED(CONFIG_OF), we can let the compiler know
> as well, and also generate smaller object code.

No, the code is buggy, and the compiler is very correct in warning about
it.

The code there is:

        of_property_read_u32(np, "cirrus,boost-manager", &val);
        switch (val) {

of_property_read_u32() is aliased to of_property_read_u32_array() via:

static inline int of_property_read_u32(const struct device_node *np,
                                       const char *propname,
                                       u32 *out_value)
{
        return of_property_read_u32_array(np, propname, out_value, 1);
}

which does this:

int of_property_read_u32_array(const struct device_node *np,
                               const char *propname, u32 *out_values,
                               size_t sz)
{
        const __be32 *val = of_find_property_value_of_size(np, propname,
                                                (sz * sizeof(*out_values)));

        if (IS_ERR(val))
                return PTR_ERR(val);

        while (sz--)
                *out_values++ = be32_to_cpup(val++);
        return 0;
}

Note that 'out_values' is not written to if of_find_property_value_of_size()
returns an error.  Therefore, if cirrus,boost-manager is missing, the
resulting value of 'val' is left uninitialised.
Arnd Bergmann March 6, 2016, 8:43 p.m. UTC | #4
On Saturday 05 March 2016, Russell King - ARM Linux wrote:
> On Mon, Jan 25, 2016 at 06:07:32PM +0100, Arnd Bergmann wrote:
> > gcc warns about the possibilty of accessing a property read from
> > devicetree in cs35l32_i2c_probe() when it has not been initialized
> > because CONFIG_OF is disabled:
> > 
> > sound/soc/codecs/cs35l32.c: In function 'cs35l32_i2c_probe':
> > sound/soc/codecs/cs35l32.c:278:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 
> > The code is actually correct because it checks the dev->of_node
> > variable first and we know this is NULL here, but by adding a
> > check for IS_ENABLED(CONFIG_OF), we can let the compiler know
> > as well, and also generate smaller object code.
> 
> No, the code is buggy, and the compiler is very correct in warning about
> it.

...
> 
> Note that 'out_values' is not written to if of_find_property_value_of_size()
> returns an error.  Therefore, if cirrus,boost-manager is missing, the
> resulting value of 'val' is left uninitialised.

You are right, this is an actual bug in the driver, and my patch just
hides it. Interestingly, this is not the case that the warning was about,
as we get the warning only when CONFIG_OF is disabled and the code
is correct (because dev->of_node is guaranteed to be NULL), but we
don't get the warning when CONFIG_OF is enabled and we can actually
run into the problem.

Fixing the driver to have correct error handling on the property
functions will make the warning go away and fix the bug you pointed
out, so we should definitely do that and drop my patch.

	Arnd

Patch
diff mbox

diff --git a/sound/soc/codecs/cs35l32.c b/sound/soc/codecs/cs35l32.c
index 44c30fe3e315..52ffaa8eb02b 100644
--- a/sound/soc/codecs/cs35l32.c
+++ b/sound/soc/codecs/cs35l32.c
@@ -372,7 +372,7 @@  static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 			dev_err(&i2c_client->dev, "could not allocate pdata\n");
 			return -ENOMEM;
 		}
-		if (i2c_client->dev.of_node) {
+		if (IS_ENABLED(CONFIG_OF) && i2c_client->dev.of_node) {
 			ret = cs35l32_handle_of_data(i2c_client,
 						     &cs35l32->pdata);
 			if (ret != 0)