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
