Re: [bug report] ASoC: SOF: Intel: hda: Revisit IMR boot sequence

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

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