Re: [PATCH v4 3/7] ASoC: meson: aiu-encoder-i2s: ensure clk divider gets disabled in hw_free

[Valerio Setti]

Hi, thanks for the feedback and sorry for the late reply.

> > diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> > index 58dce9f08c9d..70ea39c77b32 100644
> > --- a/sound/soc/meson/aiu-encoder-i2s.c
> > +++ b/sound/soc/meson/aiu-encoder-i2s.c
> > @@ -219,16 +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_stream *other = snd_soc_dai_dma_data_get(dai, !substream->stream);
>
> Not sure about peeking on the other path like this :/

Yeah, I know this is perhaps not the nicest approach, but it's the same 
one I used for the bs-quirk handling (see the "startup()" and 
"hw_params()" callbacks). Since there can only be the playback and the 
capture stream, checking the other stream's "clk_enabled" is equivalent 
to a user counter in "gx_iface", without adding a second piece of state 
that has to be kept in sync with it.
Is that acceptable to you?

> >   	struct snd_soc_component *component = dai->component;
> >   
> > -	/*
> > -	 * If this is the last substream being closed then disable the i2s
> > -	 * clock divider.
> > -	 */
> > -	if (snd_soc_dai_active(dai) <= 1)
> > -		aiu_encoder_i2s_divider_enable(component, 0);
> > -
> >   	if (ts->clk_enabled) {
> > +		/* Disable the clk divider only if also the other stream is not using it */
> > +		if (!other || !other->clk_enabled)
> > +			aiu_encoder_i2s_divider_enable(component, false);
> > +
>
> Looks like a race condition is possible here if one stream closes while
> the other one starts.
>
> I think you need a common structure between the 2 paths (with a counter
> possibly) and it needs to be protected by a lock.

I might be wrong, but I'm not sure a race condition can happen while 
managing these 2 clocks. Here's what I found:
- "iface->mclk" and "i2s_divider" are enabled/disabled on 
"prepare()"/"hw_free()" respectively.
- when "prepare()"/"hw_free()" are called the framework holds the 
"card->pcm_mutex".

So when one stream performs one operation, either "prepare()" or 
"hw_free()", it can read and modify (if needed) "ts->clk_enabled" both 
for itself and for the other stream safely.

Am I missing something?

--
Valerio