From: "Austin, Brian" <Brian.Austin@cirrus.com>
To: Arnd Bergmann <arnd@arndb.de>
CC: Mark Brown <broonie@kernel.org>, "Austin, Brian" <Brian.Austin@cirrus.com>,
        "Handrigan, Paul" <Paul.Handrigan@cirrus.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access
Thread-Topic: [PATCH] ASoC: cs35l32: avoid uninitialized variable access
Thread-Index: AQHReCij5n32gF7nukGZFpl/WI0yJZ9OTD2A
Date: Mon, 7 Mar 2016 12:22:18 +0000
Message-ID: <E771F005-D3E6-48A9-A0B1-1C11F1439E55@cirrus.com>
References: <1457324374-3405023-1-git-send-email-arnd@arndb.de>
In-Reply-To: <1457324374-3405023-1-git-send-email-arnd@arndb.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
Content-ID: <ADE8BC8E3101A143AB2BED850E6592CC@ad.cirrus.com>
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 2155
Lines: 57


> On Mar 6, 2016, at 10:19 PM, Arnd Bergmann <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 when CONFIG_OF
> is disabled, but Russell King noticed that it's broken when
> we probe the device using DT, and the properties are absent.
> 
> The code already has some checking for incorrect values, and
> I keep that checking unchanged here, but add an additional
> check for an error returned by the property accessor functions
> that now gets handled the same way as incorrect data in the
> properties.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> sound/soc/codecs/cs35l32.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs35l32.c b/sound/soc/codecs/cs35l32.c
> index 44c30fe3e315..c490dc74121b 100644
> --- a/sound/soc/codecs/cs35l32.c
> +++ b/sound/soc/codecs/cs35l32.c
> @@ -274,21 +274,24 @@ static int cs35l32_handle_of_data(struct i2c_client *i2c_client,
> 	if (of_property_read_u32(np, "cirrus,sdout-share", &val) >= 0)
> 		pdata->sdout_share = val;
> 
> -	of_property_read_u32(np, "cirrus,boost-manager", &val);
> +	if (of_property_read_u32(np, "cirrus,boost-manager", &val))
> +		val = -1u;
> +
> 	switch (val) {
> 	case CS35L32_BOOST_MGR_AUTO:
> 	case CS35L32_BOOST_MGR_AUTO_AUDIO:
> 	case CS35L32_BOOST_MGR_BYPASS:
> 	case CS35L32_BOOST_MGR_FIXED:
> -		pdata->boost_mng = val;
With this one line removed won’t that keep from assigning the value for later?
The other ones don’t seem to do this but just check for bad value.
> 		break;
> +	case -1u:
> 	default:
> 		dev_err(&i2c_client->dev,
> 			"Wrong cirrus,boost-manager DT value %d\n", val);
> 		pdata->boost_mng = CS35L32_BOOST_MGR_BYPASS;
> 	}
> 
Regards,
Brian