Re: [PATCH v6 6/9] ALSA: virtio: PCM substream operators
From: Takashi Iwai
Date: Sun Feb 28 2021 - 06:33:48 EST
On 2021年2月27日 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