Le 23/08/2023 à 16:37, Takashi Iwai a écrit :
On Tue, 22 Aug 2023 22:07:40 +0200,
Christophe JAILLET wrote:
Le 15/06/2023 à 04:17, Su Hui a écrit :
smatch error:
sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error:
we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Hi,
why is the assigment redundant?
It's misleading, yeah. Basically all callers are with non-NULL, hence
we took rather make it mandatory. Maybe it should have been with
WARN_ON() to catch the NULL argument for an out-of-tree stuff.
Should an error occur, the 'struct snd_ac97 **' parameter was garanted
to be set to NULL, now it is left as-is.
I've checked all callers and apparently this is fine because the
probes fail if snd_ac97_mixer() returns an error.
However, some drivers with several mixers seem to rely on the value
being NULL in case of error.
See [1] as an example of such code that forces a NULL value on its
own, to be sure.
So, wouldn't it be safer to leave a "*rac97 = NULL;" just after the
added sanity check?
Yes, we need the NULL initialization.
Care to submit an additional fix patch?
Hi,
Su Hui already did.
CJ
thanks,
Takashi
CJ
[1]:
https://elixir.bootlin.com/linux/v6.5-rc7/source/sound/pci/atiixp.c#L1438
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*")
Signed-off-by: Su Hui <suhui@xxxxxxxxxxxx>
---
sound/pci/ac97/ac97_codec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c
index 9afc5906d662..80a65b8ad7b9 100644
--- a/sound/pci/ac97/ac97_codec.c
+++ b/sound/pci/ac97/ac97_codec.c
@@ -2069,8 +2069,8 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template,
.dev_disconnect = snd_ac97_dev_disconnect,
};
- if (rac97)
- *rac97 = NULL;
+ if (!rac97)
+ return -EINVAL;
if (snd_BUG_ON(!bus || !template))
return -EINVAL;
if (snd_BUG_ON(template->num >= 4))
