Re: [PATCH v6 6/9] ALSA: virtio: PCM substream operators

[Takashi Iwai]

On Sat, 27 Feb 2021 09:59:53 +0100,
Anton Yakovlev wrote:
> 
> +static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct virtio_snd *snd = vss->snd;
> +	struct virtio_snd_msg *msg;
> +	unsigned long flags;
> +	int rc;
> +
> +	switch (command) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: {
> +		struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +
> +		spin_lock_irqsave(&queue->lock, flags);
> +		spin_lock(&vss->lock);
> +		rc = virtsnd_pcm_msg_send(vss);
> +		if (!rc)
> +			vss->xfer_enabled = true;
> +		spin_unlock(&vss->lock);
> +		spin_unlock_irqrestore(&queue->lock, flags);
> +		if (rc)
> +			return rc;
> +
> +		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
> +						GFP_KERNEL);

If we drop nonatomic PCM, this has to be changed: GFP_KERNEL is no
longer valid in the trigger and the pointer callbacks.
I wonder, though, the code below uses GFP_ATOMIC inconsistently...

> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH: {
> +		spin_lock_irqsave(&vss->lock, flags);
> +		vss->xfer_enabled = false;
> +		spin_unlock_irqrestore(&vss->lock, flags);
> +
> +		/*
> +		 * The substream needs to be released on the device side only
> +		 * when it is completely stopped.
> +		 */
> +		vss->release = (command == SNDRV_PCM_TRIGGER_STOP);
> +
> +		/*
> +		 * The STOP command can be issued in an atomic context after
> +		 * the drain is complete. Therefore, in general, we cannot sleep
> +		 * here.
> +		 */
> +		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
> +						GFP_ATOMIC);

BTW...
> +	default: {
> +		return -EINVAL;
> +	}

Let's avoid braces inside the switch() unless it's inevitably needed.
It makes the code harder to read.


> +static snd_pcm_uframes_t
> +virtsnd_pcm_pointer(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	snd_pcm_uframes_t hw_ptr = SNDRV_PCM_POS_XRUN;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vss->lock, flags);
> +	if (!vss->xfer_xrun)
> +		hw_ptr = bytes_to_frames(substream->runtime, vss->hw_ptr);
> +	spin_unlock_irqrestore(&vss->lock, flags);

Oh, and if we drop nonatomc PCM, both trigger and pointer callbacks
are guaranteed to be called inside the spinlock, hence you can remove
*_irqsave() but just us spin_lock() in those two callbacks.


thanks,

Takashi