| 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
| ||
|
Message-ID: <f14c0197-b346-7af5-9dd0-9b8018baaeaf@metux.net>
Date: Thu, 3 Dec 2020 20:00:06 +0100
From: "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To: Bartosz Golaszewski <bgolaszewski@...libre.com>,
"Enrico Weigelt, metux IT consult" <info@...ux.net>,
"Michael S. Tsirkin" <mst@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Jason Wang <jasowang@...hat.com>,
linux-gpio <linux-gpio@...r.kernel.org>,
virtualization@...ts.linux-foundation.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] drivers: gpio: add virtio-gpio guest driver
On 02.12.20 15:15, Bartosz Golaszewski wrote:
Hi,
<snip>
> With a third entry (after gpio-mockup and gpio-aggregator) I think
> these deserve a separate submenu for "virtual GPIO drivers" or
> something like that.
just sent a separate patch.
>
> Please keep headers ordered alphabetically.
fixed in v2.
> Don't use tabs here - not only doesn't it improve readability but
> you're not even consistent.
fixed in v2.
>
>> +
>> +static void virtio_gpio_prepare_inbuf(struct virtio_gpio_priv *priv)
>> +{
>> + struct scatterlist rcv_sg;
>> +
>> + sg_init_one(&rcv_sg, &priv->rcv_buf, sizeof(priv->rcv_buf));
>> + virtqueue_add_inbuf(priv->vq_rx, &rcv_sg, 1, &priv->rcv_buf,
>> + GFP_KERNEL);
>> + virtqueue_kick(priv->vq_rx);
>> +}
>> +
>> +static int virtio_gpio_xmit(struct virtio_gpio_priv *priv, int type,
>> + int pin, int value, struct virtio_gpio_event *ev)
>> +{
>> + struct scatterlist sg[1];
>> + int ret;
>> + unsigned long flags;
>> +
>> + WARN_ON(!ev);
>> +
>> + ev->type = type;
>> + ev->pin = pin;
>> + ev->value = value;
>> +
>> + sg_init_table(sg, 1);
>> + sg_set_buf(&sg[0], ev, sizeof(struct virtio_gpio_event));
>> +
>> + spin_lock_irqsave(&priv->vq_lock, flags);
>> + ret = virtqueue_add_outbuf(priv->vq_tx, sg, ARRAY_SIZE(sg),
>> + priv, GFP_KERNEL);
>> + if (ret < 0) {
>> + dev_err(&priv->vdev->dev,
>> + "virtqueue_add_outbuf() failed: %d\n", ret);
>> + goto out;
>> + }
>> + virtqueue_kick(priv->vq_tx);
>> +
>> +out:
>> + spin_unlock_irqrestore(&priv->vq_lock, flags);
>> + return 0;
>> +}
>> +
>> +static inline void wakeup_event(struct virtio_gpio_priv *priv, int id)
>> +{
>> + set_bit(id, &priv->reply_wait);
>> +}
>> +
>> +static inline int check_event(struct virtio_gpio_priv *priv, int id)
>> +{
>> + return test_bit(id, &priv->reply_wait);
>> +}
>> +
>> +static inline void clear_event(struct virtio_gpio_priv *priv, int id)
>> +{
>> + clear_bit(id, &priv->reply_wait);
>> +}
>> +
>> +static int virtio_gpio_req(struct virtio_gpio_priv *priv, int type,
>> + int pin, int value)
>> +{
>> + struct virtio_gpio_event *ev
>> + = devm_kzalloc(&priv->vdev->dev,
>> + sizeof(struct virtio_gpio_event), GFP_KERNEL);
>
> Just move the allocation to a separate line because this is super ugly.
done.
>
>> +
>> + if (!ev)
>> + return -ENOMEM;
>> +
>> + clear_event(priv, type);
>> + virtio_gpio_xmit(priv, type, pin, value, ev);
>> + wait_event_interruptible(priv->waitq, check_event(priv, type));
>> +
>> + devm_kfree(&priv->vdev->dev, ev);
>
> In fact you don't need the managed variant if you're freeing it in the
> same function. Managed resources should live as long as a device is
> attached to a driver.
thx, this was a left from originally global buffer.
>> +static void virtio_gpio_data_rx(struct virtqueue *vq)
>> +{
>> + struct virtio_gpio_priv *priv = vq->vdev->priv;
>> + void *data;
>> + unsigned int len;
>> + struct virtio_gpio_event *ev;
>> +
>> + data = virtqueue_get_buf(priv->vq_rx, &len);
>> + if (!data || !len) {
>> + dev_warn(&vq->vdev->dev, "RX received no data ! %d\n", len);
>> + return;
>> + }
>> +
>> + ev = data;
>> + WARN_ON(data != &priv->rcv_buf);
>
> Is it fine to proceed if this is the case?
Good question :o
Actually, it should never happen - if it does, we might have a bug in
either this driver or even virtio subsystem. But still it *should*
return a valid buffer (unless virtio is broken)
>> +
>> + memcpy(&priv->last, &priv->rcv_buf, sizeof(struct virtio_gpio_event));
>> +
>> + switch (ev->type) {
>> + case VIRTIO_GPIO_EV_HOST_LEVEL:
>> + virtio_gpio_signal(priv, ev->type, ev->pin, ev->value);
>> + break;
>
> Break should be one tab farther.
fixed in v2
>> + default:
>> + wakeup_event(priv, ev->type & ~VIRTIO_GPIO_EV_REPLY);
>> + break;
>> + }
>> + virtio_gpio_prepare_inbuf(priv);
>> + wake_up_all(&priv->waitq);
>> +}
>> +
>> +static int virtio_gpio_alloc_vq(struct virtio_gpio_priv *priv)
>> +{
>> + struct virtqueue *vqs[2];
>> + vq_callback_t *cbs[] = {
>> + NULL,
>> + virtio_gpio_data_rx,
>> + };
>> + static const char * const names[] = { "in", "out", };
>> + int ret;
>> +
>> + ret = virtio_find_vqs(priv->vdev, 2, vqs, cbs, names, NULL);
>> + if (ret) {
>> + dev_err(&priv->vdev->dev, "failed to alloc vqs: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + priv->vq_rx = vqs[0];
>> + priv->vq_tx = vqs[1];
>> +
>> + virtio_gpio_prepare_inbuf(priv);
>> +
>> + virtio_config_enable(priv->vdev);
>> + virtqueue_enable_cb(priv->vq_rx);
>> + virtio_device_ready(priv->vdev);
>> +
>> + return 0;
>> +}
>> +
>> +static int virtio_gpio_probe(struct virtio_device *vdev)
>> +{
>> + struct virtio_gpio_priv *priv;
>> + struct virtio_gpio_config cf = {};
>> + char *name_buffer;
>> + const char **gpio_names = NULL;
>> + struct device *dev = &vdev->dev;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->name = devm_kzalloc(dev, sizeof(cf.name)+1, GFP_KERNEL);
>
> This can fail.
ups!
>> + spin_lock_init(&priv->vq_lock);
>> + spin_lock_init(&priv->op_lock);
>> +
>> + virtio_cread(vdev, struct virtio_gpio_config, version, &cf.version);
>> + virtio_cread(vdev, struct virtio_gpio_config, num_gpios,
>> + &cf.num_gpios);
>> + virtio_cread(vdev, struct virtio_gpio_config, names_size,
>> + &cf.names_size);
>> + virtio_cread_bytes(vdev, offsetof(struct virtio_gpio_config, name),
>> + priv->name, sizeof(cf.name));
>> +
>> + if (cf.version != 1) {
>> + dev_err(dev, "unsupported interface version %d\n", cf.version);
>> + return -EINVAL;
>> + }
>> +
>> + priv->num_gpios = cf.num_gpios;
>> +
>> + if (cf.names_size) {
>> + char *bufwalk;
>> + int idx = 0;
>> +
>> + name_buffer = devm_kzalloc(&vdev->dev, cf.names_size,
>> + GFP_KERNEL)+1;
>> + virtio_cread_bytes(vdev, sizeof(struct virtio_gpio_config),
>> + name_buffer, cf.names_size);
>> + name_buffer[cf.names_size] = 0;
>> +
>> + gpio_names = devm_kzalloc(dev,
>> + sizeof(char *) * priv->num_gpios,
>> + GFP_KERNEL);
>
> Use devm_kcalloc() for arrays.
ok.
>
>> + bufwalk = name_buffer;
>> +
>> + while (idx < priv->num_gpios &&
>> + bufwalk < (name_buffer+cf.names_size)) {
>> + gpio_names[idx] = (strlen(bufwalk) ? bufwalk : NULL);
>> + bufwalk += strlen(bufwalk)+1;
>> + idx++;
>
>
> Something's wrong with indentation here.
i dont think so: the "bufwalk ..." line belongs to the while expression
and is right under the "idx", as it should be. I didn't want to break up
at the "<" operator. shall i do this instead ?
>> + }
>> + }
>> +
>> + priv->gc.owner = THIS_MODULE;
>> + priv->gc.parent = dev;
>> + priv->gc.label = (priv->name[0] ? priv->name
>> + : dev_name(dev));
>> + priv->gc.ngpio = priv->num_gpios;
>> + priv->gc.names = gpio_names;
>> + priv->gc.base = -1;
>> + priv->gc.request = virtio_gpio_request;
>> + priv->gc.direction_input = virtio_gpio_direction_input;
>> + priv->gc.direction_output = virtio_gpio_direction_output;
>> + priv->gc.get_direction = virtio_gpio_get_direction;
>> + priv->gc.get = virtio_gpio_get;
>> + priv->gc.set = virtio_gpio_set;
>> +
>> + priv->vdev = vdev;
>> + vdev->priv = priv;
>> +
>> + priv->irq_base = devm_irq_alloc_descs(dev, -1, 0, priv->num_gpios,
>> + NUMA_NO_NODE);
>> + if (priv->irq_base < 0) {
>> + dev_err(&vdev->dev, "failed to alloc irqs\n");
>> + priv->irq_base = -1;
>> + return -ENOMEM;
>> + }
>> +
>> + init_waitqueue_head(&priv->waitq);
>> +
>> + priv->reply_wait = 0;
>> +
>> + virtio_gpio_alloc_vq(priv);
>> +
>> + return devm_gpiochip_add_data(dev, &priv->gc, priv);
>> +}
>
> I don't know virtio at all - Michael: could you take a look at the
> code here and see if it looks good to you?
>
>> +
>> +static void virtio_gpio_remove(struct virtio_device *vdev)
>> +{
>> +}
>
> Just don't implement it. Or does virtio actually require the remove() callback?
yes, it does, unfortunately -- see: virtio_dev_remove()
I'll propose a patch for virtio for fixing that.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287
Powered by blists - more mailing lists