On Sat, 27 Feb 2021 09:59:50 +0100, Anton Yakovlev wrote:
--- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -11,6 +11,10 @@
#include "virtio_card.h"
+int msg_timeout_ms = MSEC_PER_SEC; +module_param(msg_timeout_ms, int, 0644); +MODULE_PARM_DESC(msg_timeout_ms, "Message completion timeout in milliseconds");
If it's a global variable, better to set a prefix to make it unique, and use module_param_named().
And, it should be unsigned int, no? (unless a negative value has any meaning) Otherwise...
- if (!msg_timeout_ms) {
dev_err(&vdev->dev, "msg_timeout_ms value cannot be zero\n");return -EINVAL;
Here a negative value would pass.
(snip)
+int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
struct scatterlist *out_sgs,struct scatterlist *in_sgs, bool nowait)+{
- struct virtio_device *vdev = snd->vdev;
- struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
- unsigned int js = msecs_to_jiffies(msg_timeout_ms);
- struct virtio_snd_hdr *request = virtsnd_ctl_msg_request(msg);
- struct virtio_snd_hdr *response = virtsnd_ctl_msg_response(msg);
- unsigned int nouts = 0;
- unsigned int nins = 0;
- struct scatterlist *psgs[4];
- bool notify = false;
- unsigned long flags;
- int rc;
- virtsnd_ctl_msg_ref(msg);
- /* Set the default status in case the message was canceled. */
- response->code = cpu_to_le32(VIRTIO_SND_S_IO_ERR);
- psgs[nouts++] = &msg->sg_request;
- if (out_sgs)
psgs[nouts++] = out_sgs;- psgs[nouts + nins++] = &msg->sg_response;
- if (in_sgs)
psgs[nouts + nins++] = in_sgs;- spin_lock_irqsave(&queue->lock, flags);
- rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
GFP_ATOMIC);
It's a bit pity that we have to use GFP_ATOMIC always here... As long as it's in spinlock, it's the only way.
However, this reminds me of another question: may the virtio event be handled in an atomic context, e.g. the period elapsed or xrun events? If so, the current implementation with non-atomic PCM mode is wrong. Since the non-atomic PCM uses mutex instead of spinlock, it'll lead to a sleep-in-atomic in snd_pcm_period_elapsed() handling.
I suggested the non-atomic PCM *iff* the all contexts are sleepable; then the sync can be done in each callback, which makes the code much simpler usually. But you've already implemented the sync via sync_stop call, hence the non-atomic PCM has little benefit by its own.
thanks,
Takashi