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

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

[Dan Carpenter]

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;

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

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

[Péter Ujfalusi]

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