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@xxxxxxxxxxxxxxx
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@xxxxxxxxx>
> ---
>  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