[PATCH 0/3] ASoC: meson: aiu-encoder-i2s: improve hw constraints checks

[PATCH 0/3] ASoC: meson: aiu-encoder-i2s: improve hw constraints checks

[Valerio Setti]

This is a follow-up to the recently merged series reshaping the AIU
driver following the same design as AXG [1]. During the review,
Jerome pointed out that failing in hw_params() when two streams have
incompatible requirements is too late from userspace's point of view [2].

This series addresses that comment:

- Patch 1 fixes the existing bclk quirk check first: it only rejected
  one direction of the mismatch, and its interface-wide flag was
  cleared too late, making a legal reconfiguration of a single stream
  fail. This is a standalone fix and it's preparing for patch 2.

- Patch 2 is the actual answer to the review comment: the quirk
  incompatibility is expressed as hw rules on channels and sample
  bits, refined against the committed configuration of the opposite
  stream, so it becomes visible during parameter refinement. The
  hw_params() check is kept as the last backstop in case of concurrent
  refinement.

- Patch 3 applies the same philosophy to the interface-wide rate
  symmetry. Instead of manually reimplementing the check, take advantage of
  the core's 'symmetric_rate' handling.

Jerome's comment also mentioned the error returned when 'bs' is not a
multiple of 2. That one intentionally remains a hw_params() time
error: bs depends on the mclk rate, which the DAI only learns via
set_sysclk() from the machine driver's hw_params(), after parameter
refinement has already run. I don't think that can easily be expressed as
a constraint.

Final note. [1] has not been mainlined yet and it's only present in
'broonie/sound.git#for-7.3'. Therefore this series depends on commit
c7852d2dcf66 ("ASoC: meson: aiu: align I2S design to the AXG one").

[1] https://lore.kernel.org/linux-sound/20260610-reshape-aiu-as-axg-v2-0-cac3663a8b51@baylibre.com/
[2] https://lore.kernel.org/linux-sound/1jik7pebk7.fsf@starbuckisacylon.baylibre.com/

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
Valerio Setti (3):
      ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check
      ASoC: meson: aiu-encoder-i2s: reflect bs quirk in hw constraints
      ASoC: meson: aiu-encoder-i2s: use the core symmetric_rate handling

 sound/soc/meson/aiu-encoder-i2s.c | 154 +++++++++++++++++++++++++++-----------
 sound/soc/meson/aiu.c             |   1 +
 sound/soc/meson/gx-interface.h    |   6 --
 3 files changed, 113 insertions(+), 48 deletions(-)
---
base-commit: 8e5a6599a38e5515cd2b5f34fe8a8ac476f8b127
change-id: 20260707-aiu-improve-quirk-check-2b5051119341

Best regards,
--  
Valerio Setti <vsetti@baylibre.com>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check

[Valerio Setti]

The bs-quirk incompatibility check has two flaws:

- It only rejects one direction of the mismatch. A stream that does
  not require the quirk is rejected while a quirked stream is active,
  but the opposite is not true: a stream requiring the quirk passes
  the check while a non-quirked stream is active, silently
  reprogramming the shared mclk/bclk divider with the 50% increase
  and corrupting the output of the running stream.

- 'bs_quirk' is only cleared in hw_free() when the last substream
  closes, but userspace may legally stop/reconfigure/start the stream
  without an intervening hw_free. Reconfiguring a single stream
  from the quirked configuration (8ch/16-bit) to one that does not
  need the quirk therefore fails with -EINVAL due to the stale flag.

Drop the interface-wide flag and instead compare the quirk
requirement of the incoming parameters against the committed
configuration of the opposite stream at hw_params() time. The
committed channels/width are cleared in hw_free() so that a released
stream no longer constrains the other one.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
 sound/soc/meson/aiu-encoder-i2s.c | 66 +++++++++++++++++++++++++--------------
 sound/soc/meson/gx-interface.h    |  3 --
 2 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index 83b579e98f1c..c2a280bfdfe2 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -62,13 +62,36 @@ static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
 	return 0;
 }
 
+/*
+ * Return true if the given combination of channels and sample width requires
+ * the bs quirk. Return false otherwise.
+ */
+static bool aiu_encoder_is_bs_quirk(unsigned int channels, int width)
+{
+	return (channels == 8) && (width == 16);
+}
+
+static int aiu_encoder_check_bs_quirk(struct snd_pcm_substream *substream,
+				      struct snd_pcm_hw_params *params,
+				      struct snd_soc_dai *dai)
+{
+	struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream);
+
+	/* Nothing to do if the other stream doesn't exist or it's not configured yet. */
+	if (!other_stream || !other_stream->channels)
+		return 0;
+
+	if (aiu_encoder_is_bs_quirk(other_stream->channels, other_stream->width) !=
+	    aiu_encoder_is_bs_quirk(params_channels(params), params_width(params)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
 					struct snd_pcm_hw_params *params,
 					unsigned int bs)
 {
-	struct aiu *aiu = snd_soc_component_get_drvdata(component);
-	struct gx_iface *iface = &aiu->i2s.iface;
-
 	/*
 	 * NOTE: this HW is odd.
 	 * In most configuration, the i2s divider is 'mclk / blck'.
@@ -76,25 +99,13 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
 	 * increased by 50% to get the correct output rate.
 	 * No idea why !
 	 */
-	if (params_width(params) == 16 && params_channels(params) == 8) {
+	if (aiu_encoder_is_bs_quirk(params_channels(params), params_width(params))) {
 		if (bs % 2) {
 			dev_err(component->dev,
 				"Cannot increase i2s divider by 50%%\n");
 			return -EINVAL;
 		}
 		bs += bs / 2;
-		iface->bs_quirk = true;
-	} else {
-		/*
-		 * If the bs quirk is currently applied for one stream and another
-		 * ones tries to setup a configuration for which the quirk is
-		 * not required, then fail.
-		 */
-		if (iface->bs_quirk) {
-			dev_err(component->dev,
-				"bclk requirements are incompatible with active stream\n");
-			return -EINVAL;
-		}
 	}
 
 	/* Use CLK_MORE for mclk to bclk divider */
@@ -110,9 +121,11 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
 	return 0;
 }
 
-static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
-				      struct snd_pcm_hw_params *params)
+static int aiu_encoder_i2s_set_clocks(struct snd_pcm_substream *substream,
+				      struct snd_pcm_hw_params *params,
+				      struct snd_soc_dai *dai)
 {
+	struct snd_soc_component *component = dai->component;
 	struct aiu *aiu = snd_soc_component_get_drvdata(component);
 	struct gx_iface *iface = &aiu->i2s.iface;
 	unsigned int srate = params_rate(params);
@@ -133,10 +146,15 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
 
 	bs = fs / 64;
 
-	if (aiu->platform->has_clk_ctrl_more_i2s_div)
+	if (aiu->platform->has_clk_ctrl_more_i2s_div) {
+		if (aiu_encoder_check_bs_quirk(substream, params, dai)) {
+			dev_err(dai->dev, "bclk requirements incompatible with other stream\n");
+			return -EINVAL;
+		}
 		ret = aiu_encoder_i2s_set_more_div(component, params, bs);
-	else
+	} else {
 		ret = aiu_encoder_i2s_set_legacy_div(component, params, bs);
+	}
 
 	if (ret)
 		return ret;
@@ -155,7 +173,6 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
 {
 	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
 	struct gx_iface *iface = ts->iface;
-	struct snd_soc_component *component = dai->component;
 	int ret;
 
 	/*
@@ -170,7 +187,7 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
 		}
 	}
 
-	ret = aiu_encoder_i2s_set_clocks(component, params);
+	ret = aiu_encoder_i2s_set_clocks(substream, params, dai);
 	if (ret) {
 		dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
 		return ret;
@@ -219,7 +236,6 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
 	if (snd_soc_dai_active(dai) <= 1) {
 		aiu_encoder_i2s_divider_enable(component, 0);
 		iface->rate = 0;
-		iface->bs_quirk = false;
 	}
 
 	if (ts->clk_enabled) {
@@ -227,6 +243,10 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
 		ts->clk_enabled = false;
 	}
 
+	ts->channels = 0;
+	ts->width = 0;
+	ts->physical_width = 0;
+
 	return 0;
 }
 
diff --git a/sound/soc/meson/gx-interface.h b/sound/soc/meson/gx-interface.h
index 65c46dcce32a..d9ab894589fa 100644
--- a/sound/soc/meson/gx-interface.h
+++ b/sound/soc/meson/gx-interface.h
@@ -22,9 +22,6 @@ struct gx_iface {
 
 	/* For component wide symmetry */
 	int rate;
-
-	/* Only for GX platform */
-	int bs_quirk;
 };
 
 struct gx_stream {

-- 
2.47.3


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Valerio Setti]

Currently the only check for bs-quirk is implemented in hw_params(), but
this is too late: nothing in the refined hw parameters hints at the
restriction, so userspace has no way to know the configuration is
invalid until the setup fails, as Jerome pointed out during review [1].

Add hw rules on CHANNELS and SAMPLE_BITS at startup() so that the
restriction shows up during parameter refinement instead. The rules
are refined against the committed configuration of the opposite
stream:

- if it uses the quirk, the current stream is narrowed to the same
  8ch/16-bit configuration;
- otherwise, selecting a 16-bit physical width limits the stream to
  2 channels, and selecting 8 channels requires a physical width
  larger than 16 bits.

The rules key on the physical width while the quirk is defined on the
significant bits. This is safe because S16_LE is the only format
supported by the encoder where both are 16 bits.

The check in aiu_encoder_i2s_set_clocks() is kept as the last backstop:
both streams may be refined concurrently before either commits its
configuration.

The rules are only registered on GX platforms where the bs-quirk exists
and only when the DAI has a stream in the opposite direction.

[1] https://lore.kernel.org/r/1jik7pebk7.fsf@starbuckisacylon.baylibre.com/

Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
 sound/soc/meson/aiu-encoder-i2s.c | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index c2a280bfdfe2..4c62ea41d7e8 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -147,6 +147,13 @@ static int aiu_encoder_i2s_set_clocks(struct snd_pcm_substream *substream,
 	bs = fs / 64;
 
 	if (aiu->platform->has_clk_ctrl_more_i2s_div) {
+		/*
+		 * The hw rules added in startup() make this unreachable in the
+		 * sequential case, but both streams may be refined concurrently
+		 * before either commits its config, since only ops->hw_params
+		 * runs under the card's pcm_mutex. Re-check against the committed
+		 * state of the other stream, which is stable under that mutex.
+		 */
 		if (aiu_encoder_check_bs_quirk(substream, params, dai)) {
 			dev_err(dai->dev, "bclk requirements incompatible with other stream\n");
 			return -EINVAL;
@@ -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))
+			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);
+}
+
 static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream,
 				   struct snd_soc_dai *dai)
 {
 	struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
+	struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream);
 	int ret;
 
 	/* Make sure the encoder gets either 2 or 8 channels */
@@ -348,6 +390,31 @@ static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
+	/*
+	 * If DAI supports both playback and capture streams ensure the bs-quirk is
+	 * handled correctly.
+	 * This is only valid for GX platforms (has_clk_ctrl_more_i2s_div=true).
+	 */
+	if (aiu->platform->has_clk_ctrl_more_i2s_div && other_stream) {
+		ret = snd_pcm_hw_rule_add(substream->runtime, 0,
+					  SNDRV_PCM_HW_PARAM_CHANNELS,
+					  aiu_encoder_i2s_pcm_hw_rule,
+					  other_stream,
+					  SNDRV_PCM_HW_PARAM_CHANNELS,
+					  SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1);
+		if (ret)
+			return ret;
+
+		ret = snd_pcm_hw_rule_add(substream->runtime, 0,
+					  SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
+					  aiu_encoder_i2s_pcm_hw_rule,
+					  other_stream,
+					  SNDRV_PCM_HW_PARAM_CHANNELS,
+					  SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Enable only clocks which are required for the interface internal
 	 * logic. MCLK is enabled/disabled from the formatter and the I2S

-- 
2.47.3


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 3/3] ASoC: meson: aiu-encoder-i2s: use the core symmetric_rate handling

[Valerio Setti]

The driver manually implement the interface-wide rate symmetry enforcement
in hw_params(), which suffers from the same problem addressed in the
previous patch: the restriction is not visible in the hw parameter
constraints, so a stream with a mismatching rate only finds out via
-EINVAL late in the stream setup.

The ASoC core already provides this feature through the DAI's
'symmetric_rate' flag: when another stream of the DAI is active,
soc_pcm_apply_symmetry() constrains the rate at open time so the
restriction shows up during parameter refinement, and
soc_pcm_params_symmetry() still rejects a mismatch at hw_params()
time as a backstop.

Set 'symmetric_rate' on the I2S encoder DAI and drop the open-coded
check along with the now unused 'rate' member of struct gx_iface.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
 sound/soc/meson/aiu-encoder-i2s.c | 21 ++-------------------
 sound/soc/meson/aiu.c             |  1 +
 sound/soc/meson/gx-interface.h    |  3 ---
 3 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index 4c62ea41d7e8..58dce9f08c9d 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -179,28 +179,14 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
 				     struct snd_soc_dai *dai)
 {
 	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
-	struct gx_iface *iface = ts->iface;
 	int ret;
 
-	/*
-	 * Enforce interface wide rate symmetry only if there is more than
-	 * 1 stream active.
-	 */
-	if (snd_soc_dai_active(dai) > 1) {
-		if (iface->rate && iface->rate != params_rate(params)) {
-			dev_err(dai->dev, "can't set iface rate (%d != %d)\n",
-				iface->rate, params_rate(params));
-			return -EINVAL;
-		}
-	}
-
 	ret = aiu_encoder_i2s_set_clocks(substream, params, dai);
 	if (ret) {
 		dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
 		return ret;
 	}
 
-	iface->rate = params_rate(params);
 	ts->physical_width = params_physical_width(params);
 	ts->width = params_width(params);
 	ts->channels = params_channels(params);
@@ -233,17 +219,14 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
 				   struct snd_soc_dai *dai)
 {
 	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
-	struct gx_iface *iface = ts->iface;
 	struct snd_soc_component *component = dai->component;
 
 	/*
 	 * If this is the last substream being closed then disable the i2s
-	 * clock divider and clear 'iface->rate'.
+	 * clock divider.
 	 */
-	if (snd_soc_dai_active(dai) <= 1) {
+	if (snd_soc_dai_active(dai) <= 1)
 		aiu_encoder_i2s_divider_enable(component, 0);
-		iface->rate = 0;
-	}
 
 	if (ts->clk_enabled) {
 		clk_disable_unprepare(ts->iface->mclk);
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
index 64ace4d25d92..2668646e3597 100644
--- a/sound/soc/meson/aiu.c
+++ b/sound/soc/meson/aiu.c
@@ -154,6 +154,7 @@ static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = {
 			.formats = AIU_FORMATS,
 		},
 		.ops = &aiu_encoder_i2s_dai_ops,
+		.symmetric_rate = 1,
 	},
 	[CPU_SPDIF_ENCODER] = {
 		.name = "SPDIF Encoder",
diff --git a/sound/soc/meson/gx-interface.h b/sound/soc/meson/gx-interface.h
index d9ab894589fa..2a6207e393e8 100644
--- a/sound/soc/meson/gx-interface.h
+++ b/sound/soc/meson/gx-interface.h
@@ -19,9 +19,6 @@ struct gx_iface {
 
 	/* format is common to all the DAIs of the iface */
 	unsigned int fmt;
-
-	/* For component wide symmetry */
-	int rate;
 };
 
 struct gx_stream {

-- 
2.47.3


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[sashiko-bot]

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 <vsetti@baylibre.com>

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/1jik7pebk7.fsf@starbuckisacylon.baylibre.com/

> 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/20260710-aiu-improve-quirk-check-v1-0-2fdd1b6f8896@baylibre.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The bs-quirk incompatibility check evaluates a non-existent capture stream and is unconditionally bypassed.

Pre-existing issues:
- [High] The AIU encoder I2S trigger callback unconditionally acquires a mutex while holding an IRQ-disabled spinlock.
--

--- Patch [1]: [PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check ---
commit dc420735065ef5215776c2cbf7d64821269c3f60
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check

This patch modifies the bs-quirk incompatibility check to evaluate the quirk
requirement of incoming parameters against the committed configuration of the
opposite stream at hw_params() time. It removes the interface-wide flag and
clears committed channels/width in hw_free().

> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 83b579e98f1c7..c2a280bfdfe2e 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
[ ... ]
> @@ -62,13 +62,36 @@ static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
[ ... ]
> +static int aiu_encoder_check_bs_quirk(struct snd_pcm_substream *substream,
> +				      struct snd_pcm_hw_params *params,
> +				      struct snd_soc_dai *dai)
> +{
> +	struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream);
> +
> +	/* Nothing to do if the other stream doesn't exist or it's not configured yet. */
> +	if (!other_stream || !other_stream->channels)
> +		return 0;

[Severity: High]
Will other_stream always be NULL here?

Looking at aiu_cpu_dai_drv in sound/soc/meson/aiu.c, CPU_I2S_ENCODER is
defined as a playback-only DAI:

[CPU_I2S_ENCODER] = {
    .name = "I2S Encoder",
    .playback = {
    ...

Since there is no capture stream, won't !substream->stream always resolve to a
non-existent stream?

If so, other_stream will be unconditionally NULL, which would cause this
function to immediately return 0 and bypass the incompatibility check entirely.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the trigger callback acquire a mutex
while in atomic context?

The ALSA .trigger operation calls aiu_encoder_i2s_trigger(), which invokes
gx_stream_start(). This then acquires a mutex:

aiu_encoder_i2s_trigger()
    gx_stream_start(ts)
        mutex_lock(&ts->lock);

Since the Meson GX machine driver does not set nonatomic = true on its DAI
links, the PCM stream lock is an IRQ-disabled spinlock when .trigger is
invoked.

Would this result in a scheduling while atomic bug when the sleepable mutex is
acquired while holding the IRQ-disabled spinlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-aiu-improve-quirk-check-v1-0-2fdd1b6f8896@baylibre.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic