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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5hr1l0juld.wl-tiwai@suse.de>
Date:   Sun, 28 Feb 2021 12:32:46 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Anton Yakovlev <anton.yakovlev@...nsynergy.com>
Cc:     <virtualization@...ts.linux-foundation.org>,
        <alsa-devel@...a-project.org>, <virtio-dev@...ts.oasis-open.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, <linux-kernel@...r.kernel.org>
Subject: 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