On Fri, Mar 22, 2019 at 04:00:19PM +0100, Takashi Iwai wrote:
> On Fri, 22 Mar 2019 15:06:28 +0100,
> Arnd Bergmann wrote:
> >
> > When CONFIG_PCI is disabled, clang gets confused about the
> > control flow of the switch() statement always ending up
> > in the default case, and warns:
> >
> > sound/pci/hda/patch_ca0132.c:7558:6: error: variable 'fw_entry' is used
> > uninitialized whenever 'if' condition is false
> > [-Werror,-Wsometimes-uninitialized]
> > if (!spec->alt_firmware_present) {
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > sound/pci/hda/patch_ca0132.c:7565:42: note: uninitialized use occurs here
> > dsp_os_image = (struct dsp_image_seg *)(fw_entry->data);
> > ^~~~~~~~
> > sound/pci/hda/patch_ca0132.c:7558:2: note: remove the 'if' if its condition
> > is always true
> > if (!spec->alt_firmware_present) {
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > sound/pci/hda/patch_ca0132.c:7521:33: note: initialize the variable
> > 'fw_entry' to silence this warning
> > const struct firmware *fw_entry;
> > ^
> > = NULL
> >
> > Adding an explicit check for CONFIG_PCI avoids the issue.
> > Unfortunately this is not very intuitive here.
> >
> > Link: https://bugs.llvm.org/show_bug.cgi?id=41197#c1
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > Any suggestions for other workarounds appreciated. If you can think
> > of a better fix, please treat this as a reported-by:
>
> Can it be addressed by the code simplification like below, instead?
>
Yes, the warning is fixed with that diff and makes the code easier to
follow I think.
Reviewed-by: Nathan Chancellor <[email protected]>
>
> thanks,
>
> Takashi
>
> ---
> sound/pci/hda/patch_ca0132.c | 20 ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index 29882bda7632..e1ebc6d5f382 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -1005,7 +1005,6 @@ struct ca0132_spec {
> unsigned int scp_resp_header;
> unsigned int scp_resp_data[4];
> unsigned int scp_resp_count;
> - bool alt_firmware_present;
> bool startup_check_entered;
> bool dsp_reload;
>
> @@ -7518,7 +7517,7 @@ static bool ca0132_download_dsp_images(struct hda_codec
> *codec)
> bool dsp_loaded = false;
> struct ca0132_spec *spec = codec->spec;
> const struct dsp_image_seg *dsp_os_image;
> - const struct firmware *fw_entry;
> + const struct firmware *fw_entry = NULL;
> /*
> * Alternate firmwares for different variants. The Recon3Di apparently
> * can use the default firmware, but I'll leave the option in case
> @@ -7529,33 +7528,26 @@ static bool ca0132_download_dsp_images(struct
> hda_codec *codec)
> case QUIRK_R3D:
> case QUIRK_AE5:
> if (request_firmware(&fw_entry, DESKTOP_EFX_FILE,
> - codec->card->dev) != 0) {
> + codec->card->dev) != 0)
> codec_dbg(codec, "Desktop firmware not found.");
> - spec->alt_firmware_present = false;
> - } else {
> + else
> codec_dbg(codec, "Desktop firmware selected.");
> - spec->alt_firmware_present = true;
> - }
> break;
> case QUIRK_R3DI:
> if (request_firmware(&fw_entry, R3DI_EFX_FILE,
> - codec->card->dev) != 0) {
> + codec->card->dev) != 0)
> codec_dbg(codec, "Recon3Di alt firmware not detected.");
> - spec->alt_firmware_present = false;
> - } else {
> + else
> codec_dbg(codec, "Recon3Di firmware selected.");
> - spec->alt_firmware_present = true;
> - }
> break;
> default:
> - spec->alt_firmware_present = false;
> break;
> }
> /*
> * Use default ctefx.bin if no alt firmware is detected, or if none
> * exists for your particular codec.
> */
> - if (!spec->alt_firmware_present) {
> + if (!fw_entry) {
> codec_dbg(codec, "Default firmware selected.");
> if (request_firmware(&fw_entry, EFX_FILE,
> codec->card->dev) != 0)
> --
> 2.16.4
>
