Re: [PATCH v2 2/9] ALSA: virtio: add virtio sound driver

Hi Anton,

A couple of mostly cosmetic comments inline.

On Sun, 24 Jan 2021, Anton Yakovlev wrote:

Introduce skeleton of the virtio sound driver. The driver implements the virtio sound device specification, which has become part of the virtio standard.

Initial initialization of the device, virtqueues and creation of an empty ALSA sound device. Also, handling DEVICE_NEEDS_RESET device status.

Signed-off-by: Anton Yakovlev anton.yakovlev@opensynergy.com

MAINTAINERS | 9 + include/uapi/linux/virtio_snd.h | 361 +++++++++++++++++++++++++++ sound/Kconfig | 2 + sound/Makefile | 3 +- sound/virtio/Kconfig | 10 + sound/virtio/Makefile | 7 + sound/virtio/virtio_card.c | 415 ++++++++++++++++++++++++++++++++ sound/virtio/virtio_card.h | 76 ++++++ 8 files changed, 882 insertions(+), 1 deletion(-) create mode 100644 include/uapi/linux/virtio_snd.h create mode 100644 sound/virtio/Kconfig create mode 100644 sound/virtio/Makefile create mode 100644 sound/virtio/virtio_card.c create mode 100644 sound/virtio/virtio_card.h

diff --git a/MAINTAINERS b/MAINTAINERS index 00836f6452f0..3f509d54a457 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18936,6 +18936,15 @@ W: https://virtio-mem.gitlab.io/ F: drivers/virtio/virtio_mem.c F: include/uapi/linux/virtio_mem.h

+VIRTIO SOUND DRIVER +M: Anton Yakovlev anton.yakovlev@opensynergy.com +M: "Michael S. Tsirkin" mst@redhat.com +L: virtualization@lists.linux-foundation.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers) +S: Maintained +F: include/uapi/linux/virtio_snd.h +F: sound/virtio/*

VIRTUAL BOX GUEST DEVICE DRIVER M: Hans de Goede hdegoede@redhat.com M: Arnd Bergmann arnd@arndb.de diff --git a/include/uapi/linux/virtio_snd.h b/include/uapi/linux/virtio_snd.h new file mode 100644 index 000000000000..1ff6310e54d6 --- /dev/null +++ b/include/uapi/linux/virtio_snd.h @@ -0,0 +1,361 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/*

• • Copyright (C) 2020 OpenSynergy GmbH
• • This header is BSD licensed so anyone can use the definitions to
• • implement compatible drivers/servers.
• • Redistribution and use in source and binary forms, with or without
• • modification, are permitted provided that the following conditions
• • are met:

Can a BSD licence actually be further restricted?

• • 1. Redistributions of source code must retain the above copyright
• • notice, this list of conditions and the following disclaimer.
• • 2. Redistributions in binary form must reproduce the above copyright
• • notice, this list of conditions and the following disclaimer in the
• • documentation and/or other materials provided with the distribution.
• • 3. Neither the name of OpenSynergy GmbH nor the names of its contributors
• • may be used to endorse or promote products derived from this software
• • without specific prior written permission.
• • THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
• • ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
• • LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
• • FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL IBM OR

IBM? Also no idea whether this warranty disclaimer is appropriate here. I thought we were transitioning to those SPDX identifiers to eliminate all these headers.

• • CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
• • SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
• • LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
• • USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
• • ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
• • OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
• • OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
• • SUCH DAMAGE.
• */

[snip]

diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c new file mode 100644 index 000000000000..532d823fdf6f --- /dev/null +++ b/sound/virtio/virtio_card.c @@ -0,0 +1,415 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Sound card driver for virtio
• • Copyright (C) 2020 OpenSynergy GmbH
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.

Same here, I think SPDX means you don't need all this here any more.

• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, see http://www.gnu.org/licenses/.
• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/virtio_config.h> +#include <sound/initval.h> +#include <uapi/linux/virtio_ids.h>

+#include "virtio_card.h"

+static void virtsnd_remove(struct virtio_device *vdev);

+/**

• • virtsnd_event_send() - Add an event to the event queue.
• • @vqueue: Underlying event virtqueue.
• • @event: Event.
• • @notify: Indicates whether or not to send a notification to the device.
• • @gfp: Kernel flags for memory allocation.
• • Context: Any context.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_event_send(struct virtqueue *vqueue,

• 	      struct virtio_snd_event *event, bool notify,
  

• 	      gfp_t gfp)
  

+{

• struct scatterlist sg;
• struct scatterlist *psgs[1] = { &sg };
• int rc;
• /* reset event content */
• memset(event, 0, sizeof(*event));
• sg_init_one(&sg, event, sizeof(*event));
• rc = virtqueue_add_sgs(vqueue, psgs, 0, 1, event, gfp);
• if (rc)

• return rc;
  

• if (notify)

• if (virtqueue_kick_prepare(vqueue))
  

• 	if (!virtqueue_notify(vqueue))
  

• 		return -EIO;
  

• return 0;

+}

+/**

• • virtsnd_event_notify_cb() - Dispatch all reported events from the event queue.
• • @vqueue: Underlying event virtqueue.
• • This callback function is called upon a vring interrupt request from the
• • device.
• • Context: Interrupt context.
• */

+static void virtsnd_event_notify_cb(struct virtqueue *vqueue) +{

• struct virtio_snd *snd = vqueue->vdev->priv;
• struct virtio_snd_queue *queue = virtsnd_event_queue(snd);
• unsigned long flags;
• spin_lock_irqsave(&queue->lock, flags);
• while (queue->vqueue) {

• virtqueue_disable_cb(queue->vqueue);
  

• for (;;) {
  

• 	struct virtio_snd_event *event;
  

• 	u32 length;
  

• 	event = virtqueue_get_buf(queue->vqueue, &length);
  

• 	if (!event)
  

• 		break;
  

• 	virtsnd_event_send(queue->vqueue, event, true,
  

• 			   GFP_ATOMIC);
  

• }
  

• if (unlikely(virtqueue_is_broken(queue->vqueue)))
  

• 	break;
  

• if (virtqueue_enable_cb(queue->vqueue))
  

• 	break;
  

• }
• spin_unlock_irqrestore(&queue->lock, flags);

+}

+/**

• • virtsnd_find_vqs() - Enumerate and initialize all virtqueues.
• • @snd: VirtIO sound device.
• • After calling this function, the event queue is disabled.
• • Context: Any context.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_find_vqs(struct virtio_snd *snd) +{

• struct virtio_device *vdev = snd->vdev;
• vq_callback_t *callbacks[VIRTIO_SND_VQ_MAX] = { 0 };
• const char *names[VIRTIO_SND_VQ_MAX] = {

• [VIRTIO_SND_VQ_CONTROL] = "virtsnd-ctl",
  

• [VIRTIO_SND_VQ_EVENT] = "virtsnd-event",
  

• [VIRTIO_SND_VQ_TX] = "virtsnd-tx",
  

• [VIRTIO_SND_VQ_RX] = "virtsnd-rx"
  

• };
• struct virtqueue *vqs[VIRTIO_SND_VQ_MAX] = { 0 };
• unsigned int i;
• unsigned int n = 0;
• int rc;
• callbacks[VIRTIO_SND_VQ_EVENT] = virtsnd_event_notify_cb;
• rc = virtio_find_vqs(vdev, VIRTIO_SND_VQ_MAX, vqs, callbacks, names,

• 	     NULL);
  

• if (rc) {

• dev_err(&vdev->dev, "failed to initialize virtqueues\n");
  

• return rc;
  

• }
• for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)

• snd->queues[i].vqueue = vqs[i];
  

• /* Allocate events and populate the event queue */
• virtqueue_disable_cb(vqs[VIRTIO_SND_VQ_EVENT]);
• n = virtqueue_get_vring_size(vqs[VIRTIO_SND_VQ_EVENT]);
• snd->event_msgs = devm_kcalloc(&vdev->dev, n, sizeof(*snd->event_msgs),

• 		       GFP_KERNEL);
  

• if (!snd->event_msgs)

• return -ENOMEM;
  

• for (i = 0; i < n; ++i) {

• rc = virtsnd_event_send(vqs[VIRTIO_SND_VQ_EVENT],
  

• 			&snd->event_msgs[i], false, GFP_KERNEL);
  

• if (rc)
  

• 	return rc;
  

• }
• return 0;

+}

+/**

• • virtsnd_enable_event_vq() - Enable the event virtqueue.
• • @snd: VirtIO sound device.
• • Context: Any context.
• */

+static void virtsnd_enable_event_vq(struct virtio_snd *snd) +{

• struct virtio_snd_queue *queue = virtsnd_event_queue(snd);
• if (!virtqueue_enable_cb(queue->vqueue))

• virtsnd_event_notify_cb(queue->vqueue);
  

+}

+/**

• • virtsnd_disable_vqs() - Disable all virtqueues.
• • @snd: VirtIO sound device.
• • Also free all allocated events and control messages.
• • Context: Any context.
• */

+static void virtsnd_disable_vqs(struct virtio_snd *snd) +{

• struct virtio_device *vdev = snd->vdev;
• unsigned int i;
• unsigned long flags;
• for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i) {

• struct virtio_snd_queue *queue = &snd->queues[i];
  

• spin_lock_irqsave(&queue->lock, flags);
  

• /* Prohibit the use of the queue */
  

• if (queue->vqueue)
  

• 	virtqueue_disable_cb(queue->vqueue);
  

• queue->vqueue = NULL;
  

• spin_unlock_irqrestore(&queue->lock, flags);
  

• }
• if (snd->event_msgs)

Check not needed, kfree(NULL) is ok.

• devm_kfree(&vdev->dev, snd->event_msgs);
  

I think there are very few cases when managed resources have to be explicitly freed. If explicit freeing is always required, then there's no need to have them managed. If there's a clear case for managed resources, usually you don't need to free them explicitly. Here.event_msgs are allocated in virtsnd_find_vqs() above, which is only called during probing. And this function is only called during release. So, I'd assume, that you don't need to free memory explicitly here.

• snd->event_msgs = NULL;

snd is about to be freed, so do you really need this?

+}

+/**

• • virtsnd_reset_fn() - Kernel worker's function to reset the device.
• • @work: Reset device work.
• • Context: Process context.
• */

+static void virtsnd_reset_fn(struct work_struct *work) +{

• struct virtio_snd *snd =

• container_of(work, struct virtio_snd, reset_work);
  

• struct virtio_device *vdev = snd->vdev;
• struct device *dev = &vdev->dev;
• int rc;
• dev_info(dev, "sound device needs reset\n");
• /*

• * It seems that the only way to properly reset the device is to remove
  

• * and re-create the ALSA sound card device.
  

• *
  

• * Also resetting the device involves a number of steps with setting the
  

• * status bits described in the virtio specification. And the easiest
  

• * way to get everything right is to use the virtio bus interface.
  

• */
  

• rc = dev->bus->remove(dev);
• if (rc)

• dev_warn(dev, "bus->remove() failed: %d", rc);
  

• rc = dev->bus->probe(dev);
• if (rc)

• dev_err(dev, "bus->probe() failed: %d", rc);
  

This looks very suspicious to me. Wondering what ALSA maintainers will say to this.

+}

+/**

• • virtsnd_build_devs() - Read configuration and build ALSA devices.
• • @snd: VirtIO sound device.
• • Context: Any context that permits to sleep.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_build_devs(struct virtio_snd *snd) +{

• struct virtio_device *vdev = snd->vdev;
• int rc;
• rc = snd_card_new(&vdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,

• 	  THIS_MODULE, 0, &snd->card);
  

• if (rc < 0)

• return rc;
  

• snd->card->private_data = snd;
• strscpy(snd->card->id, "viosnd", sizeof(snd->card->id));
• strscpy(snd->card->driver, "virtio_snd", sizeof(snd->card->driver));
• strscpy(snd->card->shortname, "VIOSND", sizeof(snd->card->shortname));
• strscpy(snd->card->longname, "VirtIO Sound Card",

• sizeof(snd->card->longname));
  

• return snd_card_register(snd->card);

+}

+/**

• • virtsnd_validate() - Validate if the device can be started.
• • @vdev: VirtIO parent device.
• • Context: Any context.
• • Return: 0 on success, -EINVAL on failure.
• */

+static int virtsnd_validate(struct virtio_device *vdev) +{

• if (!vdev->config->get) {

• dev_err(&vdev->dev, "configuration access disabled\n");
  

• return -EINVAL;
  

• }
• if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {

• dev_err(&vdev->dev,
  

• 	"device does not comply with spec version 1.x\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+/**

• • virtsnd_probe() - Create and initialize the device.
• • @vdev: VirtIO parent device.
• • Context: Any context that permits to sleep.
• • Return: 0 on success, -errno on failure.
• */

+static int virtsnd_probe(struct virtio_device *vdev) +{

• struct virtio_snd *snd;
• unsigned int i;
• int rc;
• snd = devm_kzalloc(&vdev->dev, sizeof(*snd), GFP_KERNEL);
• if (!snd)

• return -ENOMEM;
  

• snd->vdev = vdev;
• INIT_WORK(&snd->reset_work, virtsnd_reset_fn);
• vdev->priv = snd;
• for (i = 0; i < VIRTIO_SND_VQ_MAX; ++i)

• spin_lock_init(&snd->queues[i].lock);
  

• rc = virtsnd_find_vqs(snd);
• if (rc)

• goto on_failure;
  

• virtio_device_ready(vdev);
• rc = virtsnd_build_devs(snd);
• if (rc)

• goto on_failure;
  

• virtsnd_enable_event_vq(snd);

+on_failure:

• if (rc)

• virtsnd_remove(vdev);
  

• return rc;

+}

+/**

• • virtsnd_remove() - Remove VirtIO and ALSA devices.
• • @vdev: VirtIO parent device.
• • Context: Any context that permits to sleep.
• */

+static void virtsnd_remove(struct virtio_device *vdev) +{

• struct virtio_snd *snd = vdev->priv;
• if (!snd)

• return;
  

• /*

• * Make sure no one is accessing the virtqueues and sending synchronous
  

• * requests to the device. This can happen if we got here because the
  

• * device needs to be reset.
  

• */
  

• virtsnd_disable_vqs(snd);
• if (snd->card)

• snd_card_free(snd->card);
  

• vdev->config->reset(vdev);
• vdev->config->del_vqs(vdev);
• devm_kfree(&vdev->dev, snd);

No need for this.

• vdev->priv = NULL;

and for this either.

+}

+/**

• • virtsnd_config_changed() - Handle configuration change notification.
• • @vdev: VirtIO parent device.
• • This callback function is called upon a configuration change interrupt
• • request from the device. Currently only used to handle NEEDS_RESET device
• • status.
• • Context: Interrupt context.
• */

+static void virtsnd_config_changed(struct virtio_device *vdev) +{

• struct virtio_snd *snd = vdev->priv;
• unsigned int status = vdev->config->get_status(vdev);
• if (status & VIRTIO_CONFIG_S_NEEDS_RESET)

• schedule_work(&snd->reset_work);
  

• else

• dev_warn(&vdev->dev,
  

• 	 "sound device configuration was changed\n");
  

+}

+static const struct virtio_device_id id_table[] = {

• { VIRTIO_ID_SOUND, VIRTIO_DEV_ANY_ID },
• { 0 },

+};

+static struct virtio_driver virtsnd_driver = {

• .driver.name = KBUILD_MODNAME,
• .driver.owner = THIS_MODULE,
• .id_table = id_table,
• .validate = virtsnd_validate,
• .probe = virtsnd_probe,
• .remove = virtsnd_remove,
• .config_changed = virtsnd_config_changed,

+};

+static int __init init(void) +{

• return register_virtio_driver(&virtsnd_driver);

+} +module_init(init);

+static void __exit fini(void) +{

• unregister_virtio_driver(&virtsnd_driver);

+} +module_exit(fini);

+MODULE_DEVICE_TABLE(virtio, id_table); +MODULE_DESCRIPTION("Virtio sound card driver"); +MODULE_LICENSE("GPL");

Thanks Guennadi