Re: [PATCH v3 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Backend DAI operations are erroneously attached to DPCM Frontend links due to a flawed `link->num_codecs > 0` condition.
- [Low] Board-specific `snd_soc_common` match data structures are missing the `const` qualifier.
--

commit a4f95b828cd79a0224347fb7462baf2aac9d444d
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Subject: ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config

The sc8280xp machine driver is currently written with a largely SoC-centric
view and assumes a uniform audio topology across all boards. This change
enhances the driver to support board-specific configuration such as allowing
each board variant to provide its own DAPM widgets and routes.

> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> index 7925aa3f63ba0..b13f39dc5afc2 100644
> --- a/sound/soc/qcom/sc8280xp.c
> +++ b/sound/soc/qcom/sc8280xp.c

[ ... ]

> @@ -127,7 +245,7 @@ static void sc8280xp_add_be_ops(struct snd_soc_card *card)
>  	int i;
>  
>  	for_each_card_prelinks(card, i, link) {
> -		if (link->no_pcm == 1) {
> +		if (link->no_pcm == 1 || link->num_codecs > 0) {

[Severity: High]
Does this unintentionally match DPCM frontend links as well?

Looking at qcom_snd_parse_of(), it explicitly assigns a dummy codec and sets
link->num_codecs = 1 for all DPCM frontend links:

sound/soc/qcom/common.c:qcom_snd_parse_of() {
    ...
		if (codec) {
			...
		} else {
			/* DPCM frontend */
			link->codecs	 = &snd_soc_dummy_dlc;
			link->num_codecs = 1;
			link->dynamic = 1;
		}
    ...
}

This means link->num_codecs > 0 will evaluate to true for every link on the
card, attaching sc8280xp_be_ops and sc8280xp_snd_init to frontend links. If
sc8280xp_snd_prepare() is then invoked for frontend links, it will index the
data->stream_prepared array using frontend CPU DAI IDs. Could this result in
state corruption or out-of-bounds memory accesses if these IDs exceed
AFE_PORT_MAX?

Should this condition check !link->dynamic to safely exclude frontend links?

>  			link->init = sc8280xp_snd_init;
>  			link->be_hw_params_fixup = sc8280xp_be_hw_params_fixup;
>  			link->ops = &sc8280xp_be_ops;

[ ... ]

> @@ -145,37 +263,124 @@ static int sc8280xp_platform_probe(struct platform_device *pdev)
>  	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
>  	if (!card)
>  		return -ENOMEM;
> -	card->owner = THIS_MODULE;
> +
>  	/* Allocate the private data */
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> +	data->snd_soc_common_priv = (struct snd_soc_common *)of_device_get_match_data(dev);

[Severity: Low]
This isn't a bug, but can we avoid this explicit cast by declaring the private
data structures as const?

The of_device_get_match_data() function returns a const void *. Since the
matched data structures are not modified at runtime, could we declare them as
static const and update the snd_soc_common_priv pointer in sc8280xp_snd_data
to be const as well?

> +	if (!data->snd_soc_common_priv)
> +		return -ENODEV;
> +
> +	card->owner = THIS_MODULE;

[ ... ]

> +static struct snd_soc_common kaanapali_priv_data = {

[Severity: Low]
This isn't a bug, but should this and the other snd_soc_common match data
structures introduced below be declared as static const to place them in the
read-only .rodata section?

> +	.driver_name = "kaanapali",
> +	.dapm_widgets = sc8280xp_dapm_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
> +	.wcd_jack = true,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706132009.1496321-1-mohammad.rafi.shaik@oss.qualcomm.com?part=3