From: "Enrico Weigelt, metux IT consult" <lkml@metux.net>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>,
"Enrico Weigelt, metux IT consult" <info@metux.net>,
"Michael S. Tsirkin" <mst@redhat.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Jason Wang <jasowang@redhat.com>,
linux-gpio <linux-gpio@vger.kernel.org>,
virtualization@lists.linux-foundation.org,
linux-riscv@lists.infradead.org
Subject: Re: [PATCH] drivers: gpio: add virtio-gpio guest driver
Date: Thu, 3 Dec 2020 20:00:06 +0100 [thread overview]
Message-ID: <f14c0197-b346-7af5-9dd0-9b8018baaeaf@metux.net> (raw)
In-Reply-To: <CAMpxmJXJLTzM20xLCoM4spjibXbA-FfdPmOBp1QcV+9cScNNMw@mail.gmail.com>
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@metux.net -- +49-151-27565287
next prev parent reply other threads:[~2020-12-03 19:03 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-27 18:30 [PATCH] drivers: gpio: add virtio-gpio guest driver Enrico Weigelt, metux IT consult
2020-11-27 18:45 ` Randy Dunlap
2020-12-03 19:01 ` Enrico Weigelt, metux IT consult
2020-11-27 19:33 ` kernel test robot
2020-11-29 20:11 ` Michael S. Tsirkin
2020-11-29 22:10 ` Jonathan Neuschäfer
2020-12-03 19:12 ` Enrico Weigelt, metux IT consult
2020-12-02 14:15 ` Bartosz Golaszewski
2020-12-03 19:00 ` Enrico Weigelt, metux IT consult [this message]
2020-12-03 22:35 ` Michael Walle
2020-12-04 8:28 ` Enrico Weigelt, metux IT consult
2020-12-04 9:06 ` Michael Walle
2021-06-15 17:49 Enrico Weigelt, metux IT consult
2021-06-16 8:31 ` Linus Walleij
2021-06-16 11:49 ` Viresh Kumar
2021-06-16 15:04 ` Enrico Weigelt, metux IT consult
2021-06-17 3:59 ` Viresh Kumar
2021-06-17 9:54 ` Enrico Weigelt, metux IT consult
2021-06-17 11:47 ` Viresh Kumar
2021-06-16 14:41 ` Enrico Weigelt, metux IT consult
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f14c0197-b346-7af5-9dd0-9b8018baaeaf@metux.net \
--to=lkml@metux.net \
--cc=bgolaszewski@baylibre.com \
--cc=info@metux.net \
--cc=jasowang@redhat.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mst@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).