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