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

[Guennadi Liakhovetski]

One more thing I missed yesterday:

On Mon, 25 Jan 2021, Guennadi Liakhovetski wrote:

> On Sun, 24 Jan 2021, Anton Yakovlev wrote:
>
> > Introduce the operators required for the operation of substreams.
> >
> > Signed-off-by: Anton Yakovlev <anton.yakovlev@xxxxxxxxxxxxxxx>
> > ---
> > sound/virtio/Makefile         |   3 +-
> > sound/virtio/virtio_pcm.c     |   5 +-
> > sound/virtio/virtio_pcm.h     |   2 +
> > sound/virtio/virtio_pcm_ops.c | 513 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 521 insertions(+), 2 deletions(-)
> > create mode 100644 sound/virtio/virtio_pcm_ops.c
>
> [snip]
>
> > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> > new file mode 100644
> > index 000000000000..19882777fcd6
> > --- /dev/null
> > +++ b/sound/virtio/virtio_pcm_ops.c
> > @@ -0,0 +1,513 @@
>
> [snip]
>
> > +/**
> > + * virtsnd_pcm_release() - Release the PCM substream on the device side.
> > + * @substream: VirtIO substream.
> > + *
> > + * Context: Any context that permits to sleep.
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +static inline bool virtsnd_pcm_released(struct virtio_pcm_substream 
> > *substream)
> > +{
> > +	/*
> > +	 * The spec states that upon receipt of the RELEASE command "the 
> > device
> > +	 * MUST complete all pending I/O messages for the specified stream 
> > ID".
> > +	 * Thus, we consider the absence of I/O messages in the queue as an
> > +	 * indication that the substream has been released.
> > +	 */
> > +	return atomic_read(&substream->msg_count) == 0;
>
> Also here having it atomic doesn't really seem to help. This just means, that 
> at some point of time it was == 0.
>
> > +}
> > +
> > +static int virtsnd_pcm_release(struct virtio_pcm_substream *substream)
>
> kernel-doc missing
>
> > +{
> > +	struct virtio_snd *snd = substream->snd;
> > +	struct virtio_snd_msg *msg;
> > +	unsigned int js = msecs_to_jiffies(msg_timeout_ms);
> > +	int rc;
> > +
> > +	msg = virtsnd_pcm_ctl_msg_alloc(substream, VIRTIO_SND_R_PCM_RELEASE,
> > +					GFP_KERNEL);
> > +	if (IS_ERR(msg))
> > +		return PTR_ERR(msg);
> > +
> > +	rc = virtsnd_ctl_msg_send_sync(snd, msg);
> > +	if (rc)
> > +		return rc;
> > +
> > +	return wait_event_interruptible_timeout(substream->msg_empty,
> > +						virtsnd_pcm_released(substream),
> > +						js);

wait_event_interruptible_timeout() will return a positive number in 
success cases, 0 means a timeout and condition still false. Whereas when 
you call this function you interpret 0 as success and you expect any != 0 
to be a negative error. Wondering how this worked during your tests?

Thanks
Guennadi