Hi Dan,
On 27/04/2022 11:49, Dan Carpenter wrote:
> Hello Peter Ujfalusi,
>
> The patch 2a68ff846164: "ASoC: SOF: Intel: hda: Revisit IMR boot
> sequence" from Apr 21, 2022, leads to the following Smatch static
> checker warning:
>
> sound/soc/sof/intel/hda-loader.c:397 hda_dsp_cl_boot_firmware()
> info: return a literal instead of 'ret'
>
> sound/soc/sof/intel/hda-loader.c
> 381 int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
> 382 {
> 383 struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
> 384 struct snd_sof_pdata *plat_data = sdev->pdata;
> 385 const struct sof_dev_desc *desc = plat_data->desc;
> 386 const struct sof_intel_dsp_desc *chip_info;
> 387 struct hdac_ext_stream *hext_stream;
> 388 struct firmware stripped_firmware;
> 389 struct snd_dma_buffer dmab;
> 390 int ret, ret1, i;
> 391
> 392 if (hda->imrboot_supported && !sdev->first_boot) {
> 393 dev_dbg(sdev->dev, "IMR restore supported, booting from IMR directly\n");
> 394 hda->boot_iteration = 0;
> 395 ret = hda_dsp_boot_imr(sdev);
> 396 if (ret >= 0)
> --> 397 return ret;
>
> The hda_dsp_boot_imr() has some similar stuff where it checks for
> positive returns. As far as I can see, this code never returns positive
> values. Normally kernel code returns zero on success and negative
> error codes on failure. When code returns non-standard things then it
> really should be documented what the positive returns mean. Nothing
> complicated, just add a comment at the start of the function.
>
> The TLDR back story of this Smatch check is that it's not published but
> it regularly finds bugs. The issue is that it's more readable, plus it
> looks more deliberate and intentional to write:
>
> if (!ret)
> return 0;
Yes, this should be the correct one to use and also in hda_dsp_boot_imr().
cl_dsp_init() return 0 on success and negative errno on failure.
There is no bug with the current code other than it is juts misleading
the reader to think that a success can be a positive return value when
positive number never going to be received.
> instead of:
>
> if (!ret)
> return ret;
>
> With the latter format, the bug is that people intended to write:
>
> if (ret)
> return ret;
>
> Obviously this kind of bug would get caught in testing, but testing is
> often impossible in the kernel because it depends on hardware
> availability.
>
> 398
> 399 dev_warn(sdev->dev, "IMR restore failed, trying to cold boot\n");
> 400 }
> 401
> 402 chip_info = desc->chip_info;
> 403
>
> regards,
> dan carpenter
--
Péter
