Re: [PATCH 2/3] ASoC: meson: aiu-encoder-i2s: reflect bs quirk in hw constraints

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race during concurrent hardware parameter refinement can lead to torn reads and spurious configuration failures.
--

--- Patch [2]: [PATCH 2/3] ASoC: meson: aiu-encoder-i2s: reflect bs quirk in hw constraints ---
commit 80ff857e4b9fdf2f2e781845fa6e9a69b30c0738
Author: Valerio Setti <[email protected]>

ASoC: meson: aiu-encoder-i2s: reflect bs quirk in hw constraints
    
This commit adds hw rules on CHANNELS and SAMPLE_BITS at startup so that
the bs-quirk restriction shows up during parameter refinement. The rules
are refined against the committed configuration of the opposite stream.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index c2a280bfdfe2e..4c62ea41d7e8b 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c

[ ... ]

> @@ -333,10 +340,45 @@ static const struct snd_pcm_hw_constraint_list hw_channel_constraints = {
>  	.mask = 0,
>  };
>  
> +static int aiu_encoder_i2s_pcm_hw_rule(struct snd_pcm_hw_params *params,
> +				       struct snd_pcm_hw_rule *rule)
> +{
> +	struct gx_stream *other = rule->private;
> +	struct snd_interval *ch = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
> +	/*
> +	 * The quirk is technically based on the significant bits whereas here
> +	 * we're using the physical width for simplicity. This works because
> +	 * S16_LE is the only format supported by this encoder that has:
> +	 * significant bits = physical width = 16-bits
> +	 */
> +	struct snd_interval *phys_width = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
> +	struct snd_interval new_i;
> +
> +	if (other->channels == 0)
> +		return 0;
> +
> +	snd_interval_any(&new_i);
> +
> +	if (rule->var == SNDRV_PCM_HW_PARAM_CHANNELS) {
> +		if (aiu_encoder_is_bs_quirk(other->channels, other->width))

[Severity: Medium]
Is there a risk of a data race when reading other->channels and other->width
here?

If one application calls SNDRV_PCM_IOCTL_HW_REFINE on a substream, it will
execute this rule locklessly. Concurrently, if another application calls
SNDRV_PCM_IOCTL_HW_PARAMS or SNDRV_PCM_IOCTL_HW_FREE on the opposite
substream, it updates these fields sequentially without atomic protection:

sound/soc/meson/aiu-encoder-i2s.c:aiu_encoder_i2s_hw_free() {
    ...
    ts->channels = 0;
    ts->width = 0;
    ts->physical_width = 0;
    ...
}

Could this lead to a torn read where this rule sees an inconsistent state (for
example channels=8 but width=0), resulting in spurious configuration failures?

> +			new_i.min = new_i.max = 8;
> +		else if (snd_interval_single(phys_width) && phys_width->min == 16)
> +			new_i.max = 2; /* Force 2ch */
> +	} else { /* SNDRV_PCM_HW_PARAM_SAMPLE_BITS */
> +		if (aiu_encoder_is_bs_quirk(other->channels, other->width))
> +			new_i.min = new_i.max = 16;
> +		else if (snd_interval_single(ch) && ch->min == 8)
> +			new_i.min = 17; /* Request physical width > 16 bits */
> +	}
> +
> +	return snd_interval_refine(hw_param_interval(params, rule->var), &new_i);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic