sound/aoa/soundbus/i2sbus/pcm.c | 53 ++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 11 deletions(-)
The i2sbus PCM code uses pi->active to constrain the sibling stream to
an already prepared duplex format and rate in i2sbus_pcm_open().
That state is set from i2sbus_pcm_prepare(), but the current code only
clears it on close. As a result, the sibling stream can inherit stale
constraints after the prepared state has been torn down, or after a new
prepare attempt fails before completing.
Clear pi->active when hw_params() or hw_free() drops the prepared state,
clear it before starting a new prepare attempt, and set it again only
after prepare succeeds.
Replace the stale FIXME in the duplex constraint comment with
a description of the current driver behavior: i2sbus still programs a
single shared transport configuration for both directions, so mixed
formats are not supported in duplex mode.
Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa")
Cc: stable@vger.kernel.org
Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
---
sound/aoa/soundbus/i2sbus/pcm.c | 53 ++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 11 deletions(-)
diff --git a/sound/aoa/soundbus/i2sbus/pcm.c b/sound/aoa/soundbus/i2sbus/pcm.c
index 97c807e67d56..47a89da43cff 100644
--- a/sound/aoa/soundbus/i2sbus/pcm.c
+++ b/sound/aoa/soundbus/i2sbus/pcm.c
@@ -165,17 +165,16 @@ static int i2sbus_pcm_open(struct i2sbus_dev *i2sdev, int in)
* currently in use (if any). */
hw->rate_min = 5512;
hw->rate_max = 192000;
- /* if the other stream is active, then we can only
- * support what it is currently using.
- * FIXME: I lied. This comment is wrong. We can support
- * anything that works with the same serial format, ie.
- * when recording 24 bit sound we can well play 16 bit
- * sound at the same time iff using the same transfer mode.
+ /* If the other stream is already prepared, keep this stream
+ * on the same duplex format and rate.
+ *
+ * i2sbus_pcm_prepare() still programs one shared transport
+ * configuration for both directions, so mixed duplex formats
+ * are not supported here.
*/
if (other->active) {
- /* FIXME: is this guaranteed by the alsa api? */
hw->formats &= pcm_format_to_bits(i2sdev->format);
- /* see above, restrict rates to the one we already have */
+ /* Restrict rates to the one already in use. */
hw->rate_min = i2sdev->rate;
hw->rate_max = i2sdev->rate;
}
@@ -283,6 +282,22 @@ void i2sbus_wait_for_stop_both(struct i2sbus_dev *i2sdev)
}
#endif
+static void i2sbus_pcm_clear_active(struct i2sbus_dev *i2sdev, int in)
+{
+ struct pcm_info *pi;
+
+ guard(mutex)(&i2sdev->lock);
+
+ get_pcm_info(i2sdev, in, &pi, NULL);
+ pi->active = 0;
+}
+
+static inline int i2sbus_hw_params(struct snd_pcm_substream *substream, int in)
+{
+ i2sbus_pcm_clear_active(snd_pcm_substream_chip(substream), in);
+ return 0;
+}
+
static inline int i2sbus_hw_free(struct snd_pcm_substream *substream, int in)
{
struct i2sbus_dev *i2sdev = snd_pcm_substream_chip(substream);
@@ -291,14 +306,25 @@ static inline int i2sbus_hw_free(struct snd_pcm_substream *substream, int in)
get_pcm_info(i2sdev, in, &pi, NULL);
if (pi->dbdma_ring.stopping)
i2sbus_wait_for_stop(i2sdev, pi);
+ i2sbus_pcm_clear_active(i2sdev, in);
return 0;
}
+static int i2sbus_playback_hw_params(struct snd_pcm_substream *substream)
+{
+ return i2sbus_hw_params(substream, 0);
+}
+
static int i2sbus_playback_hw_free(struct snd_pcm_substream *substream)
{
return i2sbus_hw_free(substream, 0);
}
+static int i2sbus_record_hw_params(struct snd_pcm_substream *substream)
+{
+ return i2sbus_hw_params(substream, 1);
+}
+
static int i2sbus_record_hw_free(struct snd_pcm_substream *substream)
{
return i2sbus_hw_free(substream, 1);
@@ -335,7 +361,7 @@ static int i2sbus_pcm_prepare(struct i2sbus_dev *i2sdev, int in)
return -EINVAL;
runtime = pi->substream->runtime;
- pi->active = 1;
+ pi->active = 0;
if (other->active &&
((i2sdev->format != runtime->format)
|| (i2sdev->rate != runtime->rate)))
@@ -444,9 +470,11 @@ static int i2sbus_pcm_prepare(struct i2sbus_dev *i2sdev, int in)
/* early exit if already programmed correctly */
/* not locking these is fine since we touch them only in this function */
- if (in_le32(&i2sdev->intfregs->serial_format) == sfr
- && in_le32(&i2sdev->intfregs->data_word_sizes) == dws)
+ if (in_le32(&i2sdev->intfregs->serial_format) == sfr &&
+ in_le32(&i2sdev->intfregs->data_word_sizes) == dws) {
+ pi->active = 1;
return 0;
+ }
/* let's notify the codecs about clocks going away.
* For now we only do mastering on the i2s cell... */
@@ -484,6 +512,7 @@ static int i2sbus_pcm_prepare(struct i2sbus_dev *i2sdev, int in)
if (cii->codec->switch_clock)
cii->codec->switch_clock(cii, CLOCK_SWITCH_SLAVE);
+ pi->active = 1;
return 0;
}
@@ -728,6 +757,7 @@ static snd_pcm_uframes_t i2sbus_playback_pointer(struct snd_pcm_substream
static const struct snd_pcm_ops i2sbus_playback_ops = {
.open = i2sbus_playback_open,
.close = i2sbus_playback_close,
+ .hw_params = i2sbus_playback_hw_params,
.hw_free = i2sbus_playback_hw_free,
.prepare = i2sbus_playback_prepare,
.trigger = i2sbus_playback_trigger,
@@ -796,6 +826,7 @@ static snd_pcm_uframes_t i2sbus_record_pointer(struct snd_pcm_substream
static const struct snd_pcm_ops i2sbus_record_ops = {
.open = i2sbus_record_open,
.close = i2sbus_record_close,
+ .hw_params = i2sbus_record_hw_params,
.hw_free = i2sbus_record_hw_free,
.prepare = i2sbus_record_prepare,
.trigger = i2sbus_record_trigger,
---
base-commit: 46a6512f4a74dd7b18d9a455669c226843fc49ce
change-id: 20260329-aoa-i2sbus-clear-stale-active-4d3b706cc0ef
Best regards,
--
Cássio Gabriel <cassiogabrielcontato@gmail.com>
Hi Cássio,
kernel test robot noticed the following build errors:
[auto build test ERROR on 46a6512f4a74dd7b18d9a455669c226843fc49ce]
url: https://github.com/intel-lab-lkp/linux/commits/C-ssio-Gabriel/ALSA-aoa-i2sbus-clear-stale-prepared-state/20260331-113724
base: 46a6512f4a74dd7b18d9a455669c226843fc49ce
patch link: https://lore.kernel.org/r/20260330-aoa-i2sbus-clear-stale-active-v1-1-47a6c0a3ac9e%40gmail.com
patch subject: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20260401/202604010125.AvkWBYKI-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260401/202604010125.AvkWBYKI-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202604010125.AvkWBYKI-lkp@intel.com/
All errors (new ones prefixed by >>):
>> sound/aoa/soundbus/i2sbus/pcm.c:760:25: error: initialization of 'int (*)(struct snd_pcm_substream *, struct snd_pcm_hw_params *)' from incompatible pointer type 'int (*)(struct snd_pcm_substream *)' [-Wincompatible-pointer-types]
760 | .hw_params = i2sbus_playback_hw_params,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
sound/aoa/soundbus/i2sbus/pcm.c:760:25: note: (near initialization for 'i2sbus_playback_ops.hw_params')
sound/aoa/soundbus/i2sbus/pcm.c:313:12: note: 'i2sbus_playback_hw_params' declared here
313 | static int i2sbus_playback_hw_params(struct snd_pcm_substream *substream)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
sound/aoa/soundbus/i2sbus/pcm.c:829:25: error: initialization of 'int (*)(struct snd_pcm_substream *, struct snd_pcm_hw_params *)' from incompatible pointer type 'int (*)(struct snd_pcm_substream *)' [-Wincompatible-pointer-types]
829 | .hw_params = i2sbus_record_hw_params,
| ^~~~~~~~~~~~~~~~~~~~~~~
sound/aoa/soundbus/i2sbus/pcm.c:829:25: note: (near initialization for 'i2sbus_record_ops.hw_params')
sound/aoa/soundbus/i2sbus/pcm.c:323:12: note: 'i2sbus_record_hw_params' declared here
323 | static int i2sbus_record_hw_params(struct snd_pcm_substream *substream)
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +760 sound/aoa/soundbus/i2sbus/pcm.c
756
757 static const struct snd_pcm_ops i2sbus_playback_ops = {
758 .open = i2sbus_playback_open,
759 .close = i2sbus_playback_close,
> 760 .hw_params = i2sbus_playback_hw_params,
761 .hw_free = i2sbus_playback_hw_free,
762 .prepare = i2sbus_playback_prepare,
763 .trigger = i2sbus_playback_trigger,
764 .pointer = i2sbus_playback_pointer,
765 };
766
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 3/31/26 14:10, kernel test robot wrote:
> Hi Cássio,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 46a6512f4a74dd7b18d9a455669c226843fc49ce]
>
> url: https://github.com/intel-lab-lkp/linux/commits/C-ssio-Gabriel/ALSA-aoa-i2sbus-clear-stale-prepared-state/20260331-113724
> base: 46a6512f4a74dd7b18d9a455669c226843fc49ce
> patch link: https://lore.kernel.org/r/20260330-aoa-i2sbus-clear-stale-active-v1-1-47a6c0a3ac9e%40gmail.com
> patch subject: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state
> config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20260401/202604010125.AvkWBYKI-lkp@intel.com/config)
> compiler: powerpc64-linux-gcc (GCC) 15.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260401/202604010125.AvkWBYKI-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202604010125.AvkWBYKI-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>>> sound/aoa/soundbus/i2sbus/pcm.c:760:25: error: initialization of 'int (*)(struct snd_pcm_substream *, struct snd_pcm_hw_params *)' from incompatible pointer type 'int (*)(struct snd_pcm_substream *)' [-Wincompatible-pointer-types]
> 760 | .hw_params = i2sbus_playback_hw_params,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> sound/aoa/soundbus/i2sbus/pcm.c:760:25: note: (near initialization for 'i2sbus_playback_ops.hw_params')
> sound/aoa/soundbus/i2sbus/pcm.c:313:12: note: 'i2sbus_playback_hw_params' declared here
> 313 | static int i2sbus_playback_hw_params(struct snd_pcm_substream *substream)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> sound/aoa/soundbus/i2sbus/pcm.c:829:25: error: initialization of 'int (*)(struct snd_pcm_substream *, struct snd_pcm_hw_params *)' from incompatible pointer type 'int (*)(struct snd_pcm_substream *)' [-Wincompatible-pointer-types]
> 829 | .hw_params = i2sbus_record_hw_params,
> | ^~~~~~~~~~~~~~~~~~~~~~~
> sound/aoa/soundbus/i2sbus/pcm.c:829:25: note: (near initialization for 'i2sbus_record_ops.hw_params')
> sound/aoa/soundbus/i2sbus/pcm.c:323:12: note: 'i2sbus_record_hw_params' declared here
> 323 | static int i2sbus_record_hw_params(struct snd_pcm_substream *substream)
> | ^~~~~~~~~~~~~~~~~~~~~~~
>
>
> vim +760 sound/aoa/soundbus/i2sbus/pcm.c
>
> 756
> 757 static const struct snd_pcm_ops i2sbus_playback_ops = {
> 758 .open = i2sbus_playback_open,
> 759 .close = i2sbus_playback_close,
> > 760 .hw_params = i2sbus_playback_hw_params,
> 761 .hw_free = i2sbus_playback_hw_free,
> 762 .prepare = i2sbus_playback_prepare,
> 763 .trigger = i2sbus_playback_trigger,
> 764 .pointer = i2sbus_playback_pointer,
> 765 };
> 766
>
Oops my bad!
I had added i2sbus_playback_hw_params(), i2sbus_record_hw_params(),
and the helper they use without the struct snd_pcm_hw_params *params argument
expected by struct snd_pcm_ops.hw_params, which caused the compilation error.
After fixing the callback signatures to match the ALSA API, the file
builds cleanly.
I will send a v2 patch with the proposed fixes.
--
Thanks,
Cássio
On Tue, 31 Mar 2026 00:27:28 +0200,
Cássio Gabriel wrote:
>
> The i2sbus PCM code uses pi->active to constrain the sibling stream to
> an already prepared duplex format and rate in i2sbus_pcm_open().
>
> That state is set from i2sbus_pcm_prepare(), but the current code only
> clears it on close. As a result, the sibling stream can inherit stale
> constraints after the prepared state has been torn down, or after a new
> prepare attempt fails before completing.
>
> Clear pi->active when hw_params() or hw_free() drops the prepared state,
> clear it before starting a new prepare attempt, and set it again only
> after prepare succeeds.
>
> Replace the stale FIXME in the duplex constraint comment with
> a description of the current driver behavior: i2sbus still programs a
> single shared transport configuration for both directions, so mixed
> formats are not supported in duplex mode.
>
> Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> ---
> sound/aoa/soundbus/i2sbus/pcm.c | 53 ++++++++++++++++++++++++++++++++---------
> 1 file changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/sound/aoa/soundbus/i2sbus/pcm.c b/sound/aoa/soundbus/i2sbus/pcm.c
> index 97c807e67d56..47a89da43cff 100644
> --- a/sound/aoa/soundbus/i2sbus/pcm.c
> +++ b/sound/aoa/soundbus/i2sbus/pcm.c
> @@ -165,17 +165,16 @@ static int i2sbus_pcm_open(struct i2sbus_dev *i2sdev, int in)
> * currently in use (if any). */
> hw->rate_min = 5512;
> hw->rate_max = 192000;
> - /* if the other stream is active, then we can only
> - * support what it is currently using.
> - * FIXME: I lied. This comment is wrong. We can support
> - * anything that works with the same serial format, ie.
> - * when recording 24 bit sound we can well play 16 bit
> - * sound at the same time iff using the same transfer mode.
> + /* If the other stream is already prepared, keep this stream
> + * on the same duplex format and rate.
> + *
> + * i2sbus_pcm_prepare() still programs one shared transport
> + * configuration for both directions, so mixed duplex formats
> + * are not supported here.
> */
> if (other->active) {
> - /* FIXME: is this guaranteed by the alsa api? */
> hw->formats &= pcm_format_to_bits(i2sdev->format);
> - /* see above, restrict rates to the one we already have */
> + /* Restrict rates to the one already in use. */
> hw->rate_min = i2sdev->rate;
> hw->rate_max = i2sdev->rate;
> }
> @@ -283,6 +282,22 @@ void i2sbus_wait_for_stop_both(struct i2sbus_dev *i2sdev)
> }
> #endif
>
> +static void i2sbus_pcm_clear_active(struct i2sbus_dev *i2sdev, int in)
> +{
> + struct pcm_info *pi;
> +
> + guard(mutex)(&i2sdev->lock);
> +
> + get_pcm_info(i2sdev, in, &pi, NULL);
> + pi->active = 0;
> +}
> +
> +static inline int i2sbus_hw_params(struct snd_pcm_substream *substream, int in)
> +{
> + i2sbus_pcm_clear_active(snd_pcm_substream_chip(substream), in);
> + return 0;
> +}
> +
> static inline int i2sbus_hw_free(struct snd_pcm_substream *substream, int in)
> {
> struct i2sbus_dev *i2sdev = snd_pcm_substream_chip(substream);
> @@ -291,14 +306,25 @@ static inline int i2sbus_hw_free(struct snd_pcm_substream *substream, int in)
> get_pcm_info(i2sdev, in, &pi, NULL);
> if (pi->dbdma_ring.stopping)
> i2sbus_wait_for_stop(i2sdev, pi);
> + i2sbus_pcm_clear_active(i2sdev, in);
> return 0;
> }
>
> +static int i2sbus_playback_hw_params(struct snd_pcm_substream *substream)
> +{
> + return i2sbus_hw_params(substream, 0);
> +}
> +
> static int i2sbus_playback_hw_free(struct snd_pcm_substream *substream)
> {
> return i2sbus_hw_free(substream, 0);
> }
>
> +static int i2sbus_record_hw_params(struct snd_pcm_substream *substream)
> +{
> + return i2sbus_hw_params(substream, 1);
> +}
> +
> static int i2sbus_record_hw_free(struct snd_pcm_substream *substream)
> {
> return i2sbus_hw_free(substream, 1);
> @@ -335,7 +361,7 @@ static int i2sbus_pcm_prepare(struct i2sbus_dev *i2sdev, int in)
> return -EINVAL;
>
> runtime = pi->substream->runtime;
> - pi->active = 1;
> + pi->active = 0;
> if (other->active &&
> ((i2sdev->format != runtime->format)
> || (i2sdev->rate != runtime->rate)))
Do we need to clear the active flag here? It must have been cleared
by hw_params call. Or is it the case for errors?
thanks,
Takashi
On 3/31/26 11:27, Takashi Iwai wrote: > Do we need to clear the active flag here? It must have been cleared > by hw_params call. Or is it the case for errors? Yes, that assignment was meant for the error / re-prepare case. For the normal reconfiguration path, hw_params() already clears pi->active. My intent there was to avoid carrying over a previously successful prepared state if prepare() is called again without a preceding hw_params(), and that new prepare() attempt fails before completion. That said, I can drop the clear at the beginning of i2sbus_pcm_prepare() and keep the state reset in hw_params() and hw_free(), since that is sufficient for the stale-state issue this patch is addressing. What you think? -- Thanks, Cássio
© 2016 - 2026 Red Hat, Inc.