| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87tswqp7gl.wl-kuninori.morimoto.gx@renesas.com>
Date: Tue, 13 Jan 2026 02:33:31 +0000
From: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To: Fabio Forni <development@...aril.me>
Cc: Mark Brown <broonie@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function
Hi Fabio
Thank you for your patch
> It wasn't clear to me when different simple_probe's gotos were to be used,
> plus some allocation errors and snd_soc* errors were printed as if they were
> a parsing error. This commit:
>
> 1. Splits the allocation logic and the initialization logic, respectively in
> simple_probe and simple_probe_merge_components
> 2. Adds more error messages to improve the debugging experience
> 3. Reduces the cognitive load of gotos, as there is now only one label (I concede
> that's subjective).
>
> This commit also documents simple_util_init_priv.
>
> Signed-off-by: Fabio Forni <development@...aril.me>
> ---
(snip)
> +/**
> + * simple_util_init_priv - Initializes private data of a simple audio card.
> + * @priv: Private data to initialize. Must be pre-allocated.
> + * @li: Links of the card. Cannot be NULL.
> + *
> + * Returns:
> + * * %0 - OK.
> + * * %-ENOMEM - Could not allocate memory.
> + */
This should be separate patch.
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 298f8cc98130..b8004aa8f452 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -706,47 +706,26 @@ static int simple_soc_probe(struct snd_soc_card *card)
> return simple_ret(priv, ret);
> }
>
> -static int simple_probe(struct platform_device *pdev)
> +static int simple_probe_merge_components(const struct device *dev,
> + struct simple_util_priv *priv,
> + struct link_info *li)
> {
> - struct simple_util_priv *priv;
> - struct device *dev = &pdev->dev;
> - struct device_node *np = dev->of_node;
> - struct snd_soc_card *card;
> + const struct device_node *np = dev->of_node;
> + struct snd_soc_card *card = simple_priv_to_card(priv);
> int ret;
>
> - /* Allocate the private data and the DAI link array */
> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> - if (!priv)
> - return -ENOMEM;
> -
> - card = simple_priv_to_card(priv);
> - card->owner = THIS_MODULE;
> - card->dev = dev;
> - card->probe = simple_soc_probe;
> - card->driver_name = "simple-card";
> -
> - ret = -ENOMEM;
> - struct link_info *li __free(kfree) = kzalloc(sizeof(*li), GFP_KERNEL);
> - if (!li)
> - goto end;
> -
> - ret = simple_get_dais_count(priv, li);
> - if (ret < 0)
> - goto end;
> -
> - ret = -EINVAL;
> if (!li->link)
> - goto end;
> + return -EINVAL;
no dev_err_probe() ?
>
> - ret = simple_util_init_priv(priv, li);
> + ret = simple_get_dais_count(priv, li);
> if (ret < 0)
> - goto end;
> + dev_err_probe(dev, ret, "failed to count DAIs");
Here is missing return ?
> if (np && of_device_is_available(np)) {
> -
> ret = simple_parse_of(priv, li);
> if (ret < 0)
> - goto err;
> + dev_err_probe(dev, ret,
> + "components missing or uninitialized");
Here is missing return ?
Is it clear error message ? It failed parse DT or something ?
> @@ -756,57 +735,85 @@ static int simple_probe(struct platform_device *pdev)
> struct snd_soc_dai_link *dai_link = priv->dai_link;
> struct simple_dai_props *dai_props = priv->dai_props;
>
> - ret = -EINVAL;
> -
> cinfo = dev->platform_data;
> - if (!cinfo) {
> - dev_err(dev, "no info for asoc-simple-card\n");
> - goto err;
> - }
> + if (!cinfo)
> + return dev_err_probe(dev, -EINVAL,
> + "no info for asoc-simple-card\n");
> +
> + if (!cinfo->name || !cinfo->codec_dai.name || !cinfo->codec ||
> + !cinfo->platform || !cinfo->cpu_dai.name)
I like aligned condition, same as original code.
> + return dev_err_probe(
> + dev, -EINVAL,
> + "insufficient simple_util_info settings\n");
This can be 2 or 1 line ?
> + cpus = dai_link->cpus;
> + cpus->dai_name = cinfo->cpu_dai.name;
> +
> + codecs = dai_link->codecs;
> + codecs->name = cinfo->codec;
> + codecs->dai_name = cinfo->codec_dai.name;
> +
> + platform = dai_link->platforms;
> + platform->name = cinfo->platform;
> +
> + card->name = (cinfo->card) ? cinfo->card : cinfo->name;
> + dai_link->name = cinfo->name;
> + dai_link->stream_name = cinfo->name;
> + dai_link->dai_fmt = cinfo->daifmt;
> + dai_link->init = simple_util_dai_init;
I like tag aligned code, same as original
> + memcpy(dai_props->cpu_dai, &cinfo->cpu_dai,
> + sizeof(*dai_props->cpu_dai));
> + memcpy(dai_props->codec_dai, &cinfo->codec_dai,
> + sizeof(*dai_props->codec_dai));
These can be 1 line (with aligned. I like aligned code ;) ?
memcpy(dai_props->cpu_dai, &cinfo->cpu_dai, sizeof(*dai_props->cpu_dai));
memcpy(dai_props->codec_dai, &cinfo->codec_dai, sizeof(*dai_props->codec_dai));
> +static int simple_probe(struct platform_device *pdev)
> +{
> + struct simple_util_priv *priv;
> + struct device *dev = &pdev->dev;
> + struct snd_soc_card *card;
> + int ret;
>
> - codecs = dai_link->codecs;
> - codecs->name = cinfo->codec;
> - codecs->dai_name = cinfo->codec_dai.name;
> + /* Allocate the private data and the DAI link array */
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
>
> - platform = dai_link->platforms;
> - platform->name = cinfo->platform;
> + card = simple_priv_to_card(priv);
> + card->owner = THIS_MODULE;
> + card->dev = dev;
> + card->probe = simple_soc_probe;
> + card->driver_name = "simple-card";
I like to have tab align here, same as original code.
> - card->name = (cinfo->card) ? cinfo->card : cinfo->name;
> - dai_link->name = cinfo->name;
> - dai_link->stream_name = cinfo->name;
> - dai_link->dai_fmt = cinfo->daifmt;
> - dai_link->init = simple_util_dai_init;
> - memcpy(dai_props->cpu_dai, &cinfo->cpu_dai,
> - sizeof(*dai_props->cpu_dai));
> - memcpy(dai_props->codec_dai, &cinfo->codec_dai,
> - sizeof(*dai_props->codec_dai));
> - }
> + struct link_info *li __free(kfree) = kzalloc(sizeof(*li), GFP_KERNEL);
> + if (!li)
> + return -ENOMEM;
no dev_err_probe() ?
> + ret = simple_util_init_priv(priv, li);
> + if (ret < 0)
> + return ret;
no dev_err_probe() ?
It is calling simple_util_init_priv() (A) without calling
simple_get_dais_count() (B).
((B) simple_get_dais_count() fill li->num[x].xxx count, and
(A) simple_util_init_priv() will use it)
I think it doesn't work correctly ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Powered by blists - more mailing lists