public inbox for alsa-devel@alsa-project.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: peter.ujfalusi@linux.intel.com
Cc: alsa-devel@alsa-project.org, sound-open-firmware@alsa-project.org
Subject: [bug report] ASoC: SOF: Intel: hda: Revisit IMR boot sequence
Date: Wed, 27 Apr 2022 11:49:54 +0300	[thread overview]
Message-ID: <YmkDss9WDmk3zjyl@kili> (raw)

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

             reply	other threads:[~2022-04-27  8:51 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-27  8:49 Dan Carpenter [this message]
2022-04-27  9:04 ` [bug report] ASoC: SOF: Intel: hda: Revisit IMR boot sequence Péter Ujfalusi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YmkDss9WDmk3zjyl@kili \
    --to=dan.carpenter@oracle.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=peter.ujfalusi@linux.intel.com \
    --cc=sound-open-firmware@alsa-project.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox