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

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