| Message ID | 20241113115708.4772-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: sma1307: fix uninitialized variable refence | expand |
On Wed, Nov 13, 2024 at 12:56:50PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When firmware loading is disabled, gcc warns that the local > 'fw' variable fails to get initialized: > > sound/soc/codecs/sma1307.c: In function 'sma1307_setting_loaded.isra': > sound/soc/codecs/sma1307.c:1717:12: error: 'fw' is used uninitialized [-Werror=uninitialized] > 1717 | if (!fw) { > | ^ > sound/soc/codecs/sma1307.c:1712:32: note: 'fw' was declared here > 1712 | const struct firmware *fw; > > Initialize this to NULL as that is what it gets checked for in > case of error. This is just shutting the warning up - looking at the stub the real issue is that there's a return value from request_firmware() which isn't being checked, we're checking for the initialisation of fw instead. There is one path in the actual implementation that returns an error code without setting fw, though most do. Either this caller should be updated to check the return code or if checking fw alone is valid (which TBH does look like the intent) the stub should be updated to set it.
On Wed, Nov 13, 2024, at 14:24, Mark Brown wrote: > On Wed, Nov 13, 2024 at 12:56:50PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> When firmware loading is disabled, gcc warns that the local >> 'fw' variable fails to get initialized: >> >> sound/soc/codecs/sma1307.c: In function 'sma1307_setting_loaded.isra': >> sound/soc/codecs/sma1307.c:1717:12: error: 'fw' is used uninitialized [-Werror=uninitialized] >> 1717 | if (!fw) { >> | ^ >> sound/soc/codecs/sma1307.c:1712:32: note: 'fw' was declared here >> 1712 | const struct firmware *fw; >> >> Initialize this to NULL as that is what it gets checked for in >> case of error. > > This is just shutting the warning up - looking at the stub the real > issue is that there's a return value from request_firmware() which isn't > being checked, we're checking for the initialisation of fw instead. I sent the updated version earlier, now just checking the return code. > There is one path in the actual implementation that returns an error > code without setting fw, though most do. Either this caller should be > updated to check the return code or if checking fw alone is valid (which > TBH does look like the intent) the stub should be updated to set it. From what I saw earlier, the fw pointer gets set exactly in the same cases that return success (through *firmware_p), checking one or the other is almost the same, but you are totally right checking the return code is the right thing to do here, plus it avoids the pointless release_firmware(NULL). Arnd
On Wed, Nov 13, 2024 at 11:43:12PM +0100, Arnd Bergmann wrote: > On Wed, Nov 13, 2024, at 14:24, Mark Brown wrote: > > There is one path in the actual implementation that returns an error > > code without setting fw, though most do. Either this caller should be > > updated to check the return code or if checking fw alone is valid (which > > TBH does look like the intent) the stub should be updated to set it. > From what I saw earlier, the fw pointer gets set exactly in the > same cases that return success (through *firmware_p), checking one > or the other is almost the same, but you are totally right checking > the return code is the right thing to do here, plus it avoids > the pointless release_firmware(NULL). Yeah, the case I noticed that doesn't set firmware_p is the check for firmware_p where it's kind of unavoidable.
diff --git a/sound/soc/codecs/sma1307.c b/sound/soc/codecs/sma1307.c index 81638768ac12..9d6a70b9b626 100644 --- a/sound/soc/codecs/sma1307.c +++ b/sound/soc/codecs/sma1307.c @@ -1709,7 +1709,7 @@ static void sma1307_check_fault_worker(struct work_struct *work) static void sma1307_setting_loaded(struct sma1307_priv *sma1307, const char *file) { - const struct firmware *fw; + const struct firmware *fw = NULL; int *data, size, offset, num_mode; request_firmware(&fw, file, sma1307->dev);