Re: [PATCH] ASoC: core: add snd_BUG_ON() check in devm_snd_soc_register_card()

Hi Cezary,

Thank you for your feedback.

> snd_BUG_ON() translates to WARN_ON() in debug conditions.  After reading
> the macro documentation, pr_xxx() or equivalents are recommended when
> dealing with invalid arguments.  Same results after grepping for WARNs
> in sound/soc or in general references such as driver/base/core.c - it's
> clear to me the macro is not used as plain null-arg-check.
>

I couldn't find anything in the snd_BUG_ON() documentation that
explicitly says the
macro should not be used for plain NULL argument checks.

Also, there are quite a few places in the sound subsystem where
WARN_ON() is used to
validate NULL arguments, for example:

https://elixir.bootlin.com/linux/v7.2-rc5/source/sound/usb/endpoint.c#L857

https://elixir.bootlin.com/linux/v7.2-rc5/source/sound/soc/ti/davinci-mcasp.c#L2476

https://elixir.bootlin.com/linux/v7.2-rc5/source/sound/soc/meson/meson-codec-glue.c#L59

and even in the Intel code that you maintain:

https://elixir.bootlin.com/linux/v7.2-rc5/source/sound/soc/intel/atom/sst-mfld-platform-pcm.c#L31

That said, I'm also fine with not using snd_BUG_ON() here. Replacing
it with a simple argument check such as:

if (!dev || !card) {
      pr_err("invalid arguments\n");
      return -EINVAL;
}

is perfectly reasonable to me. It's not a big issue either way.

> Updating just one function with the check raises the question when
> reading the file - Why just one devm_snd_xxx() has the check and the
> rest do not?
>

I think the remaining helper functions can be updated gradually over time.

To me, this discussion itself is a good example. You and I interpreted
the intended use of snd_BUG_ON() differently.
I don't think the macro is wrong; rather, its intended usage is not
clearly documented or consistently reflected by existing code.
Therefore, I think it would be beneficial to either improve the API
documentation to clarify the intended usage,
or add explicit argument validation, such as the check I proposed for
devm_snd_soc_register_card().
Either approach would help avoid misunderstandings and make the API
easier to use correctly.

Of course, that's just my opinion. If the SOUND subsystem does not
intend to make these APIs more explicit or easier to use,
that's perfectly fine as well. I'll follow the preferred direction.

Best regards,
Phuc