[PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[Cássio Gabriel]

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>

Re: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[Takashi Iwai]

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

Re: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[Cássio Gabriel Monteiro Pires]

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

Re: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[kernel test robot]

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

Re: [PATCH] ALSA: aoa: i2sbus: clear stale prepared state

[Cássio Gabriel Monteiro Pires]

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