[alsa-devel] [PATCH 2/2] ALSA: x86: Avoid unconditional call of snd_pcm_period_elapsed()

[Takashi Iwai]

On Tue, 07 Feb 2017 17:38:00 +0100,
Takashi Iwai wrote:
> 
> At the interrupt handler, we usually call snd_pcm_period_elapsed() to
> inform the PCM core to proceed the hwptr.  However, the hwptr update
> might have been already processed by the explicit call of PCM pointer
> callback via another thread.  If both happen concurrently, the call of
> snd_pcm_period_elapsed() might be wrong.
> 
> Here is the fix for this slightly possible race: had_process_ringbuf()
> returns the number of processed BDs, and the irq handler calls
> snd_pcm_period_elapsed() only when it's not zero.

Actually this looks like a wrong assumption.  The call of
snd_pcm_period_elapsed() is still OK even after other thread already
updating hw_ptr, and it rather must call.  The PCM timer notification
is triggered only in snd_pcm_period_elapsed(), thus without its call,
the PCM timer won't be updated properly.  I drop this one.


Takashi

> 
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  sound/x86/intel_hdmi_audio.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
> index 15147fec1a7e..a0c9a4e0c95d 100644
> --- a/sound/x86/intel_hdmi_audio.c
> +++ b/sound/x86/intel_hdmi_audio.c
> @@ -899,10 +899,13 @@ static void had_advance_ringbuf(struct snd_pcm_substream *substream,
>  }
>  
>  /* process the current BD(s);
> - * returns the current PCM buffer byte position, or -EPIPE for underrun.
> + * returns the number of processed BDs, zero if no BD was processed,
> + * or -EPIPE for underrun.
> + * When @pos_ret is non-NULL, the current PCM buffer byte position is stored.
>   */
>  static int had_process_ringbuf(struct snd_pcm_substream *substream,
> -			       struct snd_intelhad *intelhaddata)
> +			       struct snd_intelhad *intelhaddata,
> +			       int *pos_ret)
>  {
>  	int len, processed;
>  	unsigned long flags;
> @@ -917,7 +920,7 @@ static int had_process_ringbuf(struct snd_pcm_substream *substream,
>  		if (len < 0 || len > intelhaddata->period_bytes) {
>  			dev_dbg(intelhaddata->dev, "Invalid buf length %d\n",
>  				len);
> -			len = -EPIPE;
> +			processed = -EPIPE;
>  			goto out;
>  		}
>  
> @@ -926,23 +929,27 @@ static int had_process_ringbuf(struct snd_pcm_substream *substream,
>  
>  		/* len=0 => already empty, check the next buffer */
>  		if (++processed >= intelhaddata->num_bds) {
> -			len = -EPIPE; /* all empty? - report underrun */
> +			processed = -EPIPE; /* all empty? - report underrun */
>  			goto out;
>  		}
>  		had_advance_ringbuf(substream, intelhaddata);
>  	}
>  
> -	len = intelhaddata->period_bytes - len;
> -	len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
> +	if (pos_ret) {
> +		len = intelhaddata->period_bytes - len;
> +		len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
> +		*pos_ret = len;
> +	}
>   out:
>  	spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
> -	return len;
> +	return processed;
>  }
>  
>  /* called from irq handler */
>  static void had_process_buffer_done(struct snd_intelhad *intelhaddata)
>  {
>  	struct snd_pcm_substream *substream;
> +	int processed;
>  
>  	if (!intelhaddata->connected)
>  		return; /* disconnected? - bail out */
> @@ -952,9 +959,10 @@ static void had_process_buffer_done(struct snd_intelhad *intelhaddata)
>  		return; /* no stream? - bail out */
>  
>  	/* process or stop the stream */
> -	if (had_process_ringbuf(substream, intelhaddata) < 0)
> +	processed = had_process_ringbuf(substream, intelhaddata, NULL);
> +	if (processed < 0)
>  		snd_pcm_stop_xrun(substream);
> -	else
> +	else if (processed > 0)
>  		snd_pcm_period_elapsed(substream);
>  
>  	had_substream_put(intelhaddata);
> @@ -1254,8 +1262,7 @@ static snd_pcm_uframes_t had_pcm_pointer(struct snd_pcm_substream *substream)
>  	if (!intelhaddata->connected)
>  		return SNDRV_PCM_POS_XRUN;
>  
> -	len = had_process_ringbuf(substream, intelhaddata);
> -	if (len < 0)
> +	if (had_process_ringbuf(substream, intelhaddata, &len) < 0)
>  		return SNDRV_PCM_POS_XRUN;
>  	return bytes_to_frames(substream->runtime, len);
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>