Re: [PATCH v3] Add virtio-input driver.

Re: [PATCH v3] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> +{
> +	u32 mi, ma, re, fu, fl;
> +
> +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> +	input_abs_set_res(vi->idev, abs, re);
> +}

> +struct virtio_input_absinfo {
> +	__virtio32  min;
> +	__virtio32  max;
> +	__virtio32  fuzz;
> +	__virtio32  flat;
> +	__virtio32  res;
> +};

Damn, had sparse disabled for the test builds.  [ Too bad there are way
too many warnings on a full kernel build so having sparse enabled all
the time doesn't fly. ]

So this doesn't work either.

Hmm, back to using "u32" in the virtio config structs?

cheers,
  Gerd

Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 08:32:01AM +0100, Gerd Hoffmann wrote:
> virtio-input is basically evdev-events-over-virtio, so this driver isn't
> much more than reading configuration from config space and forwarding
> incoming events to the linux input layer.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Looks pretty neat overall. I think I still see some
small issues, but it's getting there.

> ---
>  MAINTAINERS                       |   6 +
>  drivers/virtio/Kconfig            |  10 ++
>  drivers/virtio/Makefile           |   1 +
>  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/Kbuild         |   1 +
>  include/uapi/linux/virtio_ids.h   |   1 +
>  include/uapi/linux/virtio_input.h |  76 +++++++++
>  7 files changed, 408 insertions(+)
>  create mode 100644 drivers/virtio/virtio_input.c
>  create mode 100644 include/uapi/linux/virtio_input.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 358eb01..6f233dd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10442,6 +10442,12 @@ S:	Maintained
>  F:	drivers/vhost/
>  F:	include/uapi/linux/vhost.h
>  
> +VIRTIO INPUT DRIVER
> +M:	Gerd Hoffmann <kraxel@redhat.com>
> +S:	Maintained
> +F:	drivers/virtio/virtio_input.c
> +F:	include/uapi/linux/virtio_input.h
> +
>  VIA RHINE NETWORK DRIVER
>  M:	Roger Luethi <rl@hellgate.ch>
>  S:	Maintained
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index b546da5..cab9f3f 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
>  
>  	 If unsure, say M.
>  
> +config VIRTIO_INPUT
> +	tristate "Virtio input driver"
> +	depends on VIRTIO
> +	depends on INPUT
> +	---help---
> +	 This driver supports virtio input devices such as
> +	 keyboards, mice and tablets.
> +
> +	 If unsure, say M.
> +
>   config VIRTIO_MMIO
>  	tristate "Platform bus driver for memory mapped virtio devices"
>  	depends on HAS_IOMEM
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index d85565b..41e30e3 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> new file mode 100644
> index 0000000..cf112b2
> --- /dev/null
> +++ b/drivers/virtio/virtio_input.c
> @@ -0,0 +1,313 @@
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/input.h>
> +
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_input.h>
> +
> +struct virtio_input {
> +	struct virtio_device       *vdev;
> +	struct input_dev           *idev;
> +	char                       name[64];
> +	char                       serial[64];
> +	char                       phys[64];
> +	struct virtqueue           *evt, *sts;
> +	struct virtio_input_event  evts[64];
> +	spinlock_t                 lock;
> +};
> +
> +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> +				   struct virtio_input_event *evtbuf)
> +{
> +	struct scatterlist sg[1];
> +
> +	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> +	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> +}
> +
> +static void virtinput_recv_events(struct virtqueue *vq)
> +{
> +	struct virtio_input *vi = vq->vdev->priv;
> +	struct virtio_input_event *event;
> +	unsigned long flags;
> +	unsigned int len;
> +
> +	spin_lock_irqsave(&vi->lock, flags);
> +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> +		input_event(vi->idev,
> +			    le16_to_cpu(event->type),
> +			    le16_to_cpu(event->code),
> +			    le32_to_cpu(event->value));

What happens if input layer gets an
unexpected event code or value?
Or does something prevent it?



> +		virtinput_queue_evtbuf(vi, event);
> +	}
> +	virtqueue_kick(vq);
> +	spin_unlock_irqrestore(&vi->lock, flags);
> +}
> +
> +static int virtinput_send_status(struct virtio_input *vi,
> +				 u16 type, u16 code, s32 value)
> +{
> +	struct virtio_input_event *stsbuf;
> +	struct scatterlist sg[1];
> +	unsigned long flags;
> +	int rc;
> +
> +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> +	if (!stsbuf)
> +		return -ENOMEM;
> +
> +	stsbuf->type  = cpu_to_le16(type);
> +	stsbuf->code  = cpu_to_le16(code);
> +	stsbuf->value = cpu_to_le32(value);
> +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> +
> +	spin_lock_irqsave(&vi->lock, flags);
> +	rc = virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> +	virtqueue_kick(vi->sts);
> +	spin_unlock_irqrestore(&vi->lock, flags);
> +
> +	if (rc != 0)
> +		kfree(stsbuf);
> +	return rc;

This means that caller will get errors if it happens to call
send_status at a rate that's faster than host's consumption of them.
To me this looks wrong.
Poking at input layer, it seems to simply discard errors.
Is it always safe to discard status updates?
If yes, some kind of comment to clarify the logic would
make sense IMHO.



> +}
> +
> +static void virtinput_recv_status(struct virtqueue *vq)
> +{
> +	struct virtio_input *vi = vq->vdev->priv;
> +	struct virtio_input_event *stsbuf;
> +	unsigned long flags;
> +	unsigned int len;
> +
> +	spin_lock_irqsave(&vi->lock, flags);
> +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> +		kfree(stsbuf);
> +	spin_unlock_irqrestore(&vi->lock, flags);
> +}
> +
> +static int virtinput_status(struct input_dev *idev, unsigned int type,
> +			    unsigned int code, int value)
> +{
> +	struct virtio_input *vi = input_get_drvdata(idev);
> +
> +	return virtinput_send_status(vi, type, code, value);
> +}
> +
> +static size_t virtinput_cfg_select(struct virtio_input *vi,
> +				   u8 select, u8 subsel)
> +{
> +	u8 size;
> +
> +	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> +	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> +	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> +	return size;
> +}
> +
> +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> +			       unsigned long *bits, unsigned int bitcount)
> +{
> +	unsigned int bit;
> +	size_t bytes;
> +	u8 *virtio_bits;
> +
> +	bytes = virtinput_cfg_select(vi, select, subsel);
> +	if (!bytes)
> +		return;

How about limiting bytes to sizeof struct virtio_input_config->u?

> +	if (bitcount > bytes*8)
> +		bitcount = bytes*8;

Space around * pls.

> +
> +	/*
> +	 * Bitmap in virtio config space is a simple stream of bytes,
> +	 * with the first byte carrying bits 0-7, second bits 8-15 and
> +	 * so on.
> +	 */
> +	virtio_bits = kzalloc(bytes, GFP_KERNEL);
> +	if (!virtio_bits)
> +		return;
> +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +			   virtio_bits, bytes);
> +	for (bit = 0; bit < bitcount; bit++) {
> +		if (virtio_bits[bit / 8] & (1 << (bit % 8)))
> +			__set_bit(bit, bits);
> +	}
> +	kfree(virtio_bits);
> +
> +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> +		__set_bit(subsel, vi->idev->evbit);
> +}
> +
> +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> +{
> +	u32 mi, ma, re, fu, fl;
> +
> +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> +	input_abs_set_res(vi->idev, abs, re);
> +}
> +
> +static int virtinput_init_vqs(struct virtio_input *vi)
> +{
> +	struct virtqueue *vqs[2];
> +	vq_callback_t *cbs[] = { virtinput_recv_events,
> +				 virtinput_recv_status };
> +	static const char * names[] = { "events", "status" };

No space between * and names expected

> +	int i, err, size;
> +
> +	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> +	if (err)
> +		return err;
> +	vi->evt = vqs[0];
> +	vi->sts = vqs[1];
> +
> +	size = virtqueue_get_vring_size(vi->evt);
> +	if (size > ARRAY_SIZE(vi->evts))
> +		size = ARRAY_SIZE(vi->evts);
> +	for (i = 0; i < size; i++)
> +		virtinput_queue_evtbuf(vi, &vi->evts[i]);
> +	virtqueue_kick(vi->evt);
> +
> +	return 0;
> +}
> +
> +static int virtinput_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_input *vi;
> +	size_t size;
> +	int abs, err;
> +
> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> +		return -ENODEV;
> +
> +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> +	if (!vi)
> +		return -ENOMEM;
> +
> +	vdev->priv = vi;
> +	vi->vdev = vdev;
> +	spin_lock_init(&vi->lock);
> +
> +	err = virtinput_init_vqs(vi);
> +	if (err)
> +		goto err_init_vq;
> +
> +	vi->idev = input_allocate_device();
> +	if (!vi->idev) {
> +		err = -ENOMEM;
> +		goto err_input_alloc;
> +	}
> +	input_set_drvdata(vi->idev, vi);
> +
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +			   vi->name, min(size, sizeof(vi->name)));
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +			   vi->serial, min(size, sizeof(vi->serial)));
> +	snprintf(vi->phys, sizeof(vi->phys),
> +		 "virtio%d/input0", vdev->index);
> +	vi->idev->name = vi->name;
> +	vi->idev->phys = vi->phys;
> +	vi->idev->uniq = vi->serial;
> +
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> +	if (size >= 8) {

What does 8 mean here? Should be sizeof virtio_input_devids?

> +		virtio_cread(vi->vdev, struct virtio_input_config,
> +			     u.ids.bustype, &vi->idev->id.bustype);
> +		virtio_cread(vi->vdev, struct virtio_input_config,
> +			     u.ids.vendor, &vi->idev->id.vendor);
> +		virtio_cread(vi->vdev, struct virtio_input_config,
> +			     u.ids.product, &vi->idev->id.product);
> +		virtio_cread(vi->vdev, struct virtio_input_config,
> +			     u.ids.version, &vi->idev->id.version);
> +	} else {
> +		vi->idev->id.bustype = BUS_VIRTUAL;
> +	}
> +
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> +			   vi->idev->propbit, INPUT_PROP_CNT);
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> +	if (size)
> +		__set_bit(EV_REP, vi->idev->evbit);
> +
> +	vi->idev->dev.parent = &vdev->dev;
> +	vi->idev->event = virtinput_status;
> +
> +	/* device -> kernel */
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> +			   vi->idev->keybit, KEY_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> +			   vi->idev->relbit, REL_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> +			   vi->idev->absbit, ABS_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> +			   vi->idev->mscbit, MSC_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> +			   vi->idev->swbit,  SW_CNT);
> +
> +	/* kernel -> device */
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> +			   vi->idev->ledbit, LED_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> +			   vi->idev->sndbit, SND_CNT);
> +
> +	if (test_bit(EV_ABS, vi->idev->evbit)) {
> +		for (abs = 0; abs < ABS_CNT; abs++) {
> +			if (!test_bit(abs, vi->idev->absbit))
> +				continue;
> +			virtinput_cfg_abs(vi, abs);
> +		}
> +	}
> +	virtio_device_ready(vdev);
> +
At this point you can already get interrupts.
This will cause events to be forwarded.
I'm guessing this is ok since you called
input_allocate_device, but worth checking,
and maybe adding a comment.

> +	err = input_register_device(vi->idev);
> +	if (err)
> +		goto err_input_register;
> +
> +	return 0;
> +
> +err_input_register:

> +	input_free_device(vi->idev);

At this point you can already get interrupts
since you called virtio_device_ready, and
getting events from a freed device likely won't
DTRT.

> +err_input_alloc:
> +	vdev->config->del_vqs(vdev);
> +err_init_vq:
> +	kfree(vi);
> +	return err;
> +}
> +
> +static void virtinput_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_input *vi = vdev->priv;
> +
> +	input_unregister_device(vi->idev);

same thing here, you might get an event at this point.
You need to somehow block new events
being sent to device while keeping
device around.

Since you already do everything under a spinlock,
it's probably easiest to add a flag discarding
recv events. You can then check it in virtinput_recv_events
before calling input_event.



> +	vdev->config->del_vqs(vdev);
> +	kfree(vi);
> +}
> +
> +static unsigned int features[] = {
> +};
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
> +	{ 0 },
> +};
> +
> +static struct virtio_driver virtio_input_driver = {
> +	.driver.name         = KBUILD_MODNAME,
> +	.driver.owner        = THIS_MODULE,
> +	.feature_table       = features,
> +	.feature_table_size  = ARRAY_SIZE(features),
> +	.id_table            = id_table,
> +	.probe               = virtinput_probe,
> +	.remove              = virtinput_remove,

I note this driver doesn't seem to handle hybernation,
that's probably a bug?


> +};
> +
> +module_virtio_driver(virtio_input_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Virtio input device driver");
> +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 68ceb97..04b829e 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -430,6 +430,7 @@ header-y += virtio_blk.h
>  header-y += virtio_config.h
>  header-y += virtio_console.h
>  header-y += virtio_ids.h
> +header-y += virtio_input.h
>  header-y += virtio_net.h
>  header-y += virtio_pci.h
>  header-y += virtio_ring.h
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 284fc3a..5f60aa4 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -39,5 +39,6 @@
>  #define VIRTIO_ID_9P		9 /* 9p virtio console */
>  #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
>  #define VIRTIO_ID_CAIF	       12 /* Virtio caif */
> +#define VIRTIO_ID_INPUT        18 /* virtio input */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
> new file mode 100644
> index 0000000..7fceabd
> --- /dev/null
> +++ b/include/uapi/linux/virtio_input.h
> @@ -0,0 +1,76 @@
> +#ifndef _LINUX_VIRTIO_INPUT_H
> +#define _LINUX_VIRTIO_INPUT_H
> +/* 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:
> + * 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 IBM 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
> + * 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. */
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +enum virtio_input_config_select {
> +	VIRTIO_INPUT_CFG_UNSET      = 0x00,
> +	VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
> +	VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
> +	VIRTIO_INPUT_CFG_ID_DEVIDS  = 0x03,
> +	VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
> +	VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
> +	VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
> +};
> +
> +struct virtio_input_absinfo {
> +	__virtio32  min;
> +	__virtio32  max;
> +	__virtio32  fuzz;
> +	__virtio32  flat;
> +	__virtio32  res;
> +};
> +
> +struct virtio_input_devids {
> +	__virtio16  bustype;
> +	__virtio16  vendor;
> +	__virtio16  product;
> +	__virtio16  version;
> +};
> +

this padding bt two spaces looks weird.

> +struct virtio_input_config {
> +	__u8    select;
> +	__u8    subsel;
> +	__u8    size;
> +	__u8    reserved;
> +	union {
> +		char string[128];
> +		__u8 bitmap[128];

I note that neither string nor bitmap are used by
driver. What are they in aid of?

> +		struct virtio_input_absinfo abs;
> +		struct virtio_input_devids ids;
> +	} u;
> +};
> +
> +struct virtio_input_event {
> +	__le16 type;
> +	__le16 code;
> +	__le32 value;
> +};
> +
> +#endif /* _LINUX_VIRTIO_INPUT_H */
> -- 
> 1.8.3.1

Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 11:26:50AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > +{
> > +	u32 mi, ma, re, fu, fl;
> > +
> > +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> > +	input_abs_set_res(vi->idev, abs, re);
> > +}
> 
> > +struct virtio_input_absinfo {
> > +	__virtio32  min;
> > +	__virtio32  max;
> > +	__virtio32  fuzz;
> > +	__virtio32  flat;
> > +	__virtio32  res;
> > +};
> 
> Damn, had sparse disabled for the test builds.  [ Too bad there are way
> too many warnings on a full kernel build so having sparse enabled all
> the time doesn't fly. ]
> 
> So this doesn't work either.
> 
> Hmm, back to using "u32" in the virtio config structs?
> 
> cheers,
>   Gerd
> 

Weird.
Let me try this and figure out what the issue is.

Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 11:26:50AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > +{
> > +	u32 mi, ma, re, fu, fl;
> > +
> > +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> > +	input_abs_set_res(vi->idev, abs, re);
> > +}
> 
> > +struct virtio_input_absinfo {
> > +	__virtio32  min;
> > +	__virtio32  max;
> > +	__virtio32  fuzz;
> > +	__virtio32  flat;
> > +	__virtio32  res;
> > +};
> 
> Damn, had sparse disabled for the test builds.  [ Too bad there are way
> too many warnings on a full kernel build so having sparse enabled all
> the time doesn't fly. ]
> 
> So this doesn't work either.
> 
> Hmm, back to using "u32" in the virtio config structs?
> 
> cheers,
>   Gerd
> 

You are right, I was confused. Currently everyone uses simple
__u32 and friends in config structs, under the understanding
that they are always accessed using virtio_cread and friends.

Maybe I'll look into adding more type safety, but the
issue is not virtio-input specific so, let's not
defer virtio input because of this.

Sorry about wasting your time on this.

-- 
MST

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > +	spin_lock_irqsave(&vi->lock, flags);
> > +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > +		input_event(vi->idev,
> > +			    le16_to_cpu(event->type),
> > +			    le16_to_cpu(event->code),
> > +			    le32_to_cpu(event->value));
> 
> What happens if input layer gets an
> unexpected event code or value?

input layer checks it and ignores events not supported (according to the
support bitmaps).

> > +static int virtinput_send_status(struct virtio_input *vi,
> > +				 u16 type, u16 code, s32 value)
> > +{

> This means that caller will get errors if it happens to call
> send_status at a rate that's faster than host's consumption of them.
> To me this looks wrong.
> Poking at input layer, it seems to simply discard errors.
> Is it always safe to discard status updates?

Typical use case is updating the leds of your keyboard.  Loosing one
update isn't the end of the world.  Also that are _very_ low rate
events, and in case that really actually happens you likely have bigger
problems anyway ;)

> > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > +			       unsigned long *bits, unsigned int bitcount)
> > +{
> > +	unsigned int bit;
> > +	size_t bytes;
> > +	u8 *virtio_bits;
> > +
> > +	bytes = virtinput_cfg_select(vi, select, subsel);
> > +	if (!bytes)
> > +		return;
> 
> How about limiting bytes to sizeof struct virtio_input_config->u?

It's limited to 256 anyway because size is u8 in config space.

> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> > +	if (size >= 8) {
> 
> What does 8 mean here? Should be sizeof virtio_input_devids?

Yes, fixed.

> > +struct virtio_input_config {
> > +	__u8    select;
> > +	__u8    subsel;
> > +	__u8    size;
> > +	__u8    reserved;
> > +	union {
> > +		char string[128];
> > +		__u8 bitmap[128];
> 
> I note that neither string nor bitmap are used by
> driver. What are they in aid of?

Fixed.  Just sloppy coding, the name should use u.string, the bitmap
u.bitmap, instead of just "u" (although for offsetof it doesn't make a
difference).

cheers,
  Gerd

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 12:46:21PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +	spin_lock_irqsave(&vi->lock, flags);
> > > +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > > +		input_event(vi->idev,
> > > +			    le16_to_cpu(event->type),
> > > +			    le16_to_cpu(event->code),
> > > +			    le32_to_cpu(event->value));
> > 
> > What happens if input layer gets an
> > unexpected event code or value?
> 
> input layer checks it and ignores events not supported (according to the
> support bitmaps).

Right but support bitmaps come from host too, no?


> > > +static int virtinput_send_status(struct virtio_input *vi,
> > > +				 u16 type, u16 code, s32 value)
> > > +{
> 
> > This means that caller will get errors if it happens to call
> > send_status at a rate that's faster than host's consumption of them.
> > To me this looks wrong.
> > Poking at input layer, it seems to simply discard errors.
> > Is it always safe to discard status updates?
> 
> Typical use case is updating the leds of your keyboard.  Loosing one
> update isn't the end of the world.  Also that are _very_ low rate
> events, and in case that really actually happens you likely have bigger
> problems anyway ;)

Until QE opens a bug report I guess :)
OK but pls add a comment.

> > > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > > +			       unsigned long *bits, unsigned int bitcount)
> > > +{
> > > +	unsigned int bit;
> > > +	size_t bytes;
> > > +	u8 *virtio_bits;
> > > +
> > > +	bytes = virtinput_cfg_select(vi, select, subsel);
> > > +	if (!bytes)
> > > +		return;
> > 
> > How about limiting bytes to sizeof struct virtio_input_config->u?
> 
> It's limited to 256 anyway because size is u8 in config space.

That struct is 128 bytes though.
Also, change bytes and virtinput_cfg_select to be u8 to make this
clearer?

> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> > > +	if (size >= 8) {
> > 
> > What does 8 mean here? Should be sizeof virtio_input_devids?
> 
> Yes, fixed.
> 
> > > +struct virtio_input_config {
> > > +	__u8    select;
> > > +	__u8    subsel;
> > > +	__u8    size;
> > > +	__u8    reserved;
> > > +	union {
> > > +		char string[128];
> > > +		__u8 bitmap[128];
> > 
> > I note that neither string nor bitmap are used by
> > driver. What are they in aid of?
> 
> Fixed.  Just sloppy coding, the name should use u.string, the bitmap
> u.bitmap, instead of just "u" (although for offsetof it doesn't make a
> difference).
> 
> cheers,
>   Gerd
> 

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > input layer checks it and ignores events not supported (according to the
> > support bitmaps).
> 
> Right but support bitmaps come from host too, no?

Yes, but the driver will not set invalid bits (bitcount argument for the
virtinput_cfg_bits() function is the number of valid bits of the
specific bitmap).

cheers,
  Gerd

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 02:37:24PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > input layer checks it and ignores events not supported (according to the
> > > support bitmaps).
> > 
> > Right but support bitmaps come from host too, no?
> 
> Yes, but the driver will not set invalid bits (bitcount argument for the
> virtinput_cfg_bits() function is the number of valid bits of the
> specific bitmap).
> 
> cheers,
>   Gerd
> 
> 
> 

Question: does linux ever get such events from userspace
as opposed to sending them to userspace?

-- 
MST

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Gerd Hoffmann]

On Di, 2015-03-24 at 15:14 +0100, Michael S. Tsirkin wrote:
> On Tue, Mar 24, 2015 at 02:37:24PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > input layer checks it and ignores events not supported (according to the
> > > > support bitmaps).
> > > 
> > > Right but support bitmaps come from host too, no?
> > 
> > Yes, but the driver will not set invalid bits (bitcount argument for the
> > virtinput_cfg_bits() function is the number of valid bits of the
> > specific bitmap).
> > 
> > cheers,
> >   Gerd
> > 
> > 
> > 
> 
> Question: does linux ever get such events from userspace
> as opposed to sending them to userspace?

Yes, it's possible using the userspace input driver
(CONFIG_INPUT_UINPUT)

cheers,
  Gerd

Re: [virtio-dev] Re: [PATCH v3] Add virtio-input driver.

[Dmitry Torokhov]

On Tue, Mar 24, 2015 at 04:25:14PM +0100, Gerd Hoffmann wrote:
> On Di, 2015-03-24 at 15:14 +0100, Michael S. Tsirkin wrote:
> > On Tue, Mar 24, 2015 at 02:37:24PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > input layer checks it and ignores events not supported (according to the
> > > > > support bitmaps).
> > > > 
> > > > Right but support bitmaps come from host too, no?
> > > 
> > > Yes, but the driver will not set invalid bits (bitcount argument for the
> > > virtinput_cfg_bits() function is the number of valid bits of the
> > > specific bitmap).
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > > 
> > > 
> > 
> > Question: does linux ever get such events from userspace
> > as opposed to sending them to userspace?
> 
> Yes, it's possible using the userspace input driver
> (CONFIG_INPUT_UINPUT)

No, not through uinput (as from kernel POV uinput is also a driver), but
users can write into evdev.

Sending unknown codes is OK: events of unknown type will be dropped by
the input core, unknown event codes will be passed on; users not
recognizing event code should simply ignore it.

Thanks.

-- 
Dmitry

Re: [PATCH v3] Add virtio-input driver.

[Dmitry Torokhov]

On Tue, Mar 24, 2015 at 11:36:31AM +0100, Michael S. Tsirkin wrote:
> On Tue, Mar 24, 2015 at 08:32:01AM +0100, Gerd Hoffmann wrote:
> > virtio-input is basically evdev-events-over-virtio, so this driver isn't
> > much more than reading configuration from config space and forwarding
> > incoming events to the linux input layer.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Looks pretty neat overall. I think I still see some
> small issues, but it's getting there.
> 
> > ---
> >  MAINTAINERS                       |   6 +
> >  drivers/virtio/Kconfig            |  10 ++
> >  drivers/virtio/Makefile           |   1 +
> >  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/Kbuild         |   1 +
> >  include/uapi/linux/virtio_ids.h   |   1 +
> >  include/uapi/linux/virtio_input.h |  76 +++++++++
> >  7 files changed, 408 insertions(+)
> >  create mode 100644 drivers/virtio/virtio_input.c
> >  create mode 100644 include/uapi/linux/virtio_input.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 358eb01..6f233dd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10442,6 +10442,12 @@ S:	Maintained
> >  F:	drivers/vhost/
> >  F:	include/uapi/linux/vhost.h
> >  
> > +VIRTIO INPUT DRIVER
> > +M:	Gerd Hoffmann <kraxel@redhat.com>
> > +S:	Maintained
> > +F:	drivers/virtio/virtio_input.c
> > +F:	include/uapi/linux/virtio_input.h
> > +
> >  VIA RHINE NETWORK DRIVER
> >  M:	Roger Luethi <rl@hellgate.ch>
> >  S:	Maintained
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index b546da5..cab9f3f 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
> >  
> >  	 If unsure, say M.
> >  
> > +config VIRTIO_INPUT
> > +	tristate "Virtio input driver"
> > +	depends on VIRTIO
> > +	depends on INPUT
> > +	---help---
> > +	 This driver supports virtio input devices such as
> > +	 keyboards, mice and tablets.
> > +
> > +	 If unsure, say M.
> > +
> >   config VIRTIO_MMIO
> >  	tristate "Platform bus driver for memory mapped virtio devices"
> >  	depends on HAS_IOMEM
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index d85565b..41e30e3 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> > new file mode 100644
> > index 0000000..cf112b2
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_input.c
> > @@ -0,0 +1,313 @@
> > +#include <linux/module.h>
> > +#include <linux/virtio.h>
> > +#include <linux/input.h>
> > +
> > +#include <uapi/linux/virtio_ids.h>
> > +#include <uapi/linux/virtio_input.h>
> > +
> > +struct virtio_input {
> > +	struct virtio_device       *vdev;
> > +	struct input_dev           *idev;
> > +	char                       name[64];
> > +	char                       serial[64];
> > +	char                       phys[64];
> > +	struct virtqueue           *evt, *sts;
> > +	struct virtio_input_event  evts[64];
> > +	spinlock_t                 lock;
> > +};
> > +
> > +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> > +				   struct virtio_input_event *evtbuf)
> > +{
> > +	struct scatterlist sg[1];
> > +
> > +	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> > +	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> > +}
> > +
> > +static void virtinput_recv_events(struct virtqueue *vq)
> > +{
> > +	struct virtio_input *vi = vq->vdev->priv;
> > +	struct virtio_input_event *event;
> > +	unsigned long flags;
> > +	unsigned int len;
> > +
> > +	spin_lock_irqsave(&vi->lock, flags);
> > +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > +		input_event(vi->idev,
> > +			    le16_to_cpu(event->type),
> > +			    le16_to_cpu(event->code),
> > +			    le32_to_cpu(event->value));
> 
> What happens if input layer gets an
> unexpected event code or value?
> Or does something prevent it?
> 
> 
> 
> > +		virtinput_queue_evtbuf(vi, event);
> > +	}
> > +	virtqueue_kick(vq);
> > +	spin_unlock_irqrestore(&vi->lock, flags);
> > +}
> > +
> > +static int virtinput_send_status(struct virtio_input *vi,
> > +				 u16 type, u16 code, s32 value)
> > +{
> > +	struct virtio_input_event *stsbuf;
> > +	struct scatterlist sg[1];
> > +	unsigned long flags;
> > +	int rc;
> > +
> > +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > +	if (!stsbuf)
> > +		return -ENOMEM;
> > +
> > +	stsbuf->type  = cpu_to_le16(type);
> > +	stsbuf->code  = cpu_to_le16(code);
> > +	stsbuf->value = cpu_to_le32(value);
> > +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > +
> > +	spin_lock_irqsave(&vi->lock, flags);
> > +	rc = virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > +	virtqueue_kick(vi->sts);
> > +	spin_unlock_irqrestore(&vi->lock, flags);

I think locking is wrong here. This is basically input_dev->event()
which is called with input_dev->event_lock spinlock held, and it is
taking vi->lock. OTOH virtinput_recv_events() takes vi->lock and then
calls input_event(), which will try taking input_dev->event_lock. It is
bound to deadlock at some point.

I guess the easiest way would be to drop vi->lock() after fetching
virtio event and before calling input_event().

> > +
> > +	if (rc != 0)
> > +		kfree(stsbuf);
> > +	return rc;
> 
> This means that caller will get errors if it happens to call
> send_status at a rate that's faster than host's consumption of them.
> To me this looks wrong.
> Poking at input layer, it seems to simply discard errors.
> Is it always safe to discard status updates?
> If yes, some kind of comment to clarify the logic would
> make sense IMHO.
> 
> 
> 
> > +}
> > +
> > +static void virtinput_recv_status(struct virtqueue *vq)
> > +{
> > +	struct virtio_input *vi = vq->vdev->priv;
> > +	struct virtio_input_event *stsbuf;
> > +	unsigned long flags;
> > +	unsigned int len;
> > +
> > +	spin_lock_irqsave(&vi->lock, flags);
> > +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> > +		kfree(stsbuf);
> > +	spin_unlock_irqrestore(&vi->lock, flags);
> > +}
> > +
> > +static int virtinput_status(struct input_dev *idev, unsigned int type,
> > +			    unsigned int code, int value)
> > +{
> > +	struct virtio_input *vi = input_get_drvdata(idev);
> > +
> > +	return virtinput_send_status(vi, type, code, value);
> > +}
> > +
> > +static size_t virtinput_cfg_select(struct virtio_input *vi,
> > +				   u8 select, u8 subsel)
> > +{
> > +	u8 size;
> > +
> > +	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> > +	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> > +	return size;
> > +}
> > +
> > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > +			       unsigned long *bits, unsigned int bitcount)
> > +{
> > +	unsigned int bit;
> > +	size_t bytes;
> > +	u8 *virtio_bits;
> > +
> > +	bytes = virtinput_cfg_select(vi, select, subsel);
> > +	if (!bytes)
> > +		return;
> 
> How about limiting bytes to sizeof struct virtio_input_config->u?
> 
> > +	if (bitcount > bytes*8)
> > +		bitcount = bytes*8;
> 
> Space around * pls.
> 
> > +
> > +	/*
> > +	 * Bitmap in virtio config space is a simple stream of bytes,
> > +	 * with the first byte carrying bits 0-7, second bits 8-15 and
> > +	 * so on.
> > +	 */
> > +	virtio_bits = kzalloc(bytes, GFP_KERNEL);
> > +	if (!virtio_bits)
> > +		return;
> > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > +			   virtio_bits, bytes);
> > +	for (bit = 0; bit < bitcount; bit++) {
> > +		if (virtio_bits[bit / 8] & (1 << (bit % 8)))
> > +			__set_bit(bit, bits);
> > +	}
> > +	kfree(virtio_bits);
> > +
> > +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > +		__set_bit(subsel, vi->idev->evbit);
> > +}
> > +
> > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > +{
> > +	u32 mi, ma, re, fu, fl;
> > +
> > +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> > +	input_abs_set_res(vi->idev, abs, re);
> > +}
> > +
> > +static int virtinput_init_vqs(struct virtio_input *vi)
> > +{
> > +	struct virtqueue *vqs[2];
> > +	vq_callback_t *cbs[] = { virtinput_recv_events,
> > +				 virtinput_recv_status };
> > +	static const char * names[] = { "events", "status" };
> 
> No space between * and names expected
> 
> > +	int i, err, size;
> > +
> > +	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> > +	if (err)
> > +		return err;
> > +	vi->evt = vqs[0];
> > +	vi->sts = vqs[1];
> > +
> > +	size = virtqueue_get_vring_size(vi->evt);
> > +	if (size > ARRAY_SIZE(vi->evts))
> > +		size = ARRAY_SIZE(vi->evts);
> > +	for (i = 0; i < size; i++)
> > +		virtinput_queue_evtbuf(vi, &vi->evts[i]);
> > +	virtqueue_kick(vi->evt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int virtinput_probe(struct virtio_device *vdev)
> > +{
> > +	struct virtio_input *vi;
> > +	size_t size;
> > +	int abs, err;
> > +
> > +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > +		return -ENODEV;
> > +
> > +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> > +	if (!vi)
> > +		return -ENOMEM;
> > +
> > +	vdev->priv = vi;
> > +	vi->vdev = vdev;
> > +	spin_lock_init(&vi->lock);
> > +
> > +	err = virtinput_init_vqs(vi);
> > +	if (err)
> > +		goto err_init_vq;
> > +
> > +	vi->idev = input_allocate_device();
> > +	if (!vi->idev) {
> > +		err = -ENOMEM;
> > +		goto err_input_alloc;
> > +	}
> > +	input_set_drvdata(vi->idev, vi);
> > +
> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > +			   vi->name, min(size, sizeof(vi->name)));
> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > +			   vi->serial, min(size, sizeof(vi->serial)));
> > +	snprintf(vi->phys, sizeof(vi->phys),
> > +		 "virtio%d/input0", vdev->index);
> > +	vi->idev->name = vi->name;
> > +	vi->idev->phys = vi->phys;
> > +	vi->idev->uniq = vi->serial;
> > +
> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> > +	if (size >= 8) {
> 
> What does 8 mean here? Should be sizeof virtio_input_devids?
> 
> > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > +			     u.ids.bustype, &vi->idev->id.bustype);
> > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > +			     u.ids.vendor, &vi->idev->id.vendor);
> > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > +			     u.ids.product, &vi->idev->id.product);
> > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > +			     u.ids.version, &vi->idev->id.version);
> > +	} else {
> > +		vi->idev->id.bustype = BUS_VIRTUAL;
> > +	}
> > +
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> > +			   vi->idev->propbit, INPUT_PROP_CNT);
> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> > +	if (size)
> > +		__set_bit(EV_REP, vi->idev->evbit);
> > +
> > +	vi->idev->dev.parent = &vdev->dev;
> > +	vi->idev->event = virtinput_status;
> > +
> > +	/* device -> kernel */
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> > +			   vi->idev->keybit, KEY_CNT);
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> > +			   vi->idev->relbit, REL_CNT);
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> > +			   vi->idev->absbit, ABS_CNT);
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> > +			   vi->idev->mscbit, MSC_CNT);
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> > +			   vi->idev->swbit,  SW_CNT);
> > +
> > +	/* kernel -> device */
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> > +			   vi->idev->ledbit, LED_CNT);
> > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> > +			   vi->idev->sndbit, SND_CNT);
> > +
> > +	if (test_bit(EV_ABS, vi->idev->evbit)) {
> > +		for (abs = 0; abs < ABS_CNT; abs++) {
> > +			if (!test_bit(abs, vi->idev->absbit))
> > +				continue;
> > +			virtinput_cfg_abs(vi, abs);
> > +		}
> > +	}
> > +	virtio_device_ready(vdev);
> > +
> At this point you can already get interrupts.
> This will cause events to be forwarded.
> I'm guessing this is ok since you called
> input_allocate_device, but worth checking,
> and maybe adding a comment.

Yes, it is OK to send events though yet unregistered input device, as
long as it was allocated with input_allocate_device().

> 
> > +	err = input_register_device(vi->idev);
> > +	if (err)
> > +		goto err_input_register;
> > +
> > +	return 0;
> > +
> > +err_input_register:
> 
> > +	input_free_device(vi->idev);
> 
> At this point you can already get interrupts
> since you called virtio_device_ready, and
> getting events from a freed device likely won't
> DTRT.

Right. I guess you want to mark the virtio device ready only after
registering input device.

> 
> > +err_input_alloc:
> > +	vdev->config->del_vqs(vdev);
> > +err_init_vq:
> > +	kfree(vi);
> > +	return err;
> > +}
> > +
> > +static void virtinput_remove(struct virtio_device *vdev)
> > +{
> > +	struct virtio_input *vi = vdev->priv;
> > +
> > +	input_unregister_device(vi->idev);
> 
> same thing here, you might get an event at this point.
> You need to somehow block new events
> being sent to device while keeping
> device around.
> 
> Since you already do everything under a spinlock,
> it's probably easiest to add a flag discarding
> recv events. You can then check it in virtinput_recv_events
> before calling input_event.

Instead of checking the flag is it possible to pause virio device? Maybe
virtio_break_device()?

> 
> 
> 
> > +	vdev->config->del_vqs(vdev);
> > +	kfree(vi);
> > +}
> > +
> > +static unsigned int features[] = {
> > +};
> > +static struct virtio_device_id id_table[] = {
> > +	{ VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
> > +	{ 0 },
> > +};
> > +
> > +static struct virtio_driver virtio_input_driver = {
> > +	.driver.name         = KBUILD_MODNAME,
> > +	.driver.owner        = THIS_MODULE,
> > +	.feature_table       = features,
> > +	.feature_table_size  = ARRAY_SIZE(features),
> > +	.id_table            = id_table,
> > +	.probe               = virtinput_probe,
> > +	.remove              = virtinput_remove,
> 
> I note this driver doesn't seem to handle hybernation,
> that's probably a bug?
> 
> 
> > +};
> > +
> > +module_virtio_driver(virtio_input_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Virtio input device driver");
> > +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index 68ceb97..04b829e 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -430,6 +430,7 @@ header-y += virtio_blk.h
> >  header-y += virtio_config.h
> >  header-y += virtio_console.h
> >  header-y += virtio_ids.h
> > +header-y += virtio_input.h
> >  header-y += virtio_net.h
> >  header-y += virtio_pci.h
> >  header-y += virtio_ring.h
> > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> > index 284fc3a..5f60aa4 100644
> > --- a/include/uapi/linux/virtio_ids.h
> > +++ b/include/uapi/linux/virtio_ids.h
> > @@ -39,5 +39,6 @@
> >  #define VIRTIO_ID_9P		9 /* 9p virtio console */
> >  #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
> >  #define VIRTIO_ID_CAIF	       12 /* Virtio caif */
> > +#define VIRTIO_ID_INPUT        18 /* virtio input */
> >  
> >  #endif /* _LINUX_VIRTIO_IDS_H */
> > diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
> > new file mode 100644
> > index 0000000..7fceabd
> > --- /dev/null
> > +++ b/include/uapi/linux/virtio_input.h
> > @@ -0,0 +1,76 @@
> > +#ifndef _LINUX_VIRTIO_INPUT_H
> > +#define _LINUX_VIRTIO_INPUT_H
> > +/* 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:
> > + * 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 IBM 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
> > + * 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. */
> > +#include <linux/virtio_ids.h>
> > +#include <linux/virtio_config.h>
> > +
> > +enum virtio_input_config_select {
> > +	VIRTIO_INPUT_CFG_UNSET      = 0x00,
> > +	VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
> > +	VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
> > +	VIRTIO_INPUT_CFG_ID_DEVIDS  = 0x03,
> > +	VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
> > +	VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
> > +	VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
> > +};
> > +
> > +struct virtio_input_absinfo {
> > +	__virtio32  min;
> > +	__virtio32  max;
> > +	__virtio32  fuzz;
> > +	__virtio32  flat;
> > +	__virtio32  res;
> > +};
> > +
> > +struct virtio_input_devids {
> > +	__virtio16  bustype;
> > +	__virtio16  vendor;
> > +	__virtio16  product;
> > +	__virtio16  version;
> > +};
> > +
> 
> this padding bt two spaces looks weird.
> 
> > +struct virtio_input_config {
> > +	__u8    select;
> > +	__u8    subsel;
> > +	__u8    size;
> > +	__u8    reserved;
> > +	union {
> > +		char string[128];
> > +		__u8 bitmap[128];
> 
> I note that neither string nor bitmap are used by
> driver. What are they in aid of?

Also, what happens if we need more than 1024 bits to pass bitmap data?
We might get there with keyboards.

> 
> > +		struct virtio_input_absinfo abs;
> > +		struct virtio_input_devids ids;
> > +	} u;
> > +};
> > +
> > +struct virtio_input_event {
> > +	__le16 type;
> > +	__le16 code;
> > +	__le32 value;
> > +};
> > +
> > +#endif /* _LINUX_VIRTIO_INPUT_H */
> > -- 
> > 1.8.3.1

Thanks.

-- 
Dmitry

Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 09:23:37AM -0700, Dmitry Torokhov wrote:
> On Tue, Mar 24, 2015 at 11:36:31AM +0100, Michael S. Tsirkin wrote:
> > On Tue, Mar 24, 2015 at 08:32:01AM +0100, Gerd Hoffmann wrote:
> > > virtio-input is basically evdev-events-over-virtio, so this driver isn't
> > > much more than reading configuration from config space and forwarding
> > > incoming events to the linux input layer.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Looks pretty neat overall. I think I still see some
> > small issues, but it's getting there.
> > 
> > > ---
> > >  MAINTAINERS                       |   6 +
> > >  drivers/virtio/Kconfig            |  10 ++
> > >  drivers/virtio/Makefile           |   1 +
> > >  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/Kbuild         |   1 +
> > >  include/uapi/linux/virtio_ids.h   |   1 +
> > >  include/uapi/linux/virtio_input.h |  76 +++++++++
> > >  7 files changed, 408 insertions(+)
> > >  create mode 100644 drivers/virtio/virtio_input.c
> > >  create mode 100644 include/uapi/linux/virtio_input.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 358eb01..6f233dd 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -10442,6 +10442,12 @@ S:	Maintained
> > >  F:	drivers/vhost/
> > >  F:	include/uapi/linux/vhost.h
> > >  
> > > +VIRTIO INPUT DRIVER
> > > +M:	Gerd Hoffmann <kraxel@redhat.com>
> > > +S:	Maintained
> > > +F:	drivers/virtio/virtio_input.c
> > > +F:	include/uapi/linux/virtio_input.h
> > > +
> > >  VIA RHINE NETWORK DRIVER
> > >  M:	Roger Luethi <rl@hellgate.ch>
> > >  S:	Maintained
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index b546da5..cab9f3f 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
> > >  
> > >  	 If unsure, say M.
> > >  
> > > +config VIRTIO_INPUT
> > > +	tristate "Virtio input driver"
> > > +	depends on VIRTIO
> > > +	depends on INPUT
> > > +	---help---
> > > +	 This driver supports virtio input devices such as
> > > +	 keyboards, mice and tablets.
> > > +
> > > +	 If unsure, say M.
> > > +
> > >   config VIRTIO_MMIO
> > >  	tristate "Platform bus driver for memory mapped virtio devices"
> > >  	depends on HAS_IOMEM
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index d85565b..41e30e3 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > > +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > > diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> > > new file mode 100644
> > > index 0000000..cf112b2
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_input.c
> > > @@ -0,0 +1,313 @@
> > > +#include <linux/module.h>
> > > +#include <linux/virtio.h>
> > > +#include <linux/input.h>
> > > +
> > > +#include <uapi/linux/virtio_ids.h>
> > > +#include <uapi/linux/virtio_input.h>
> > > +
> > > +struct virtio_input {
> > > +	struct virtio_device       *vdev;
> > > +	struct input_dev           *idev;
> > > +	char                       name[64];
> > > +	char                       serial[64];
> > > +	char                       phys[64];
> > > +	struct virtqueue           *evt, *sts;
> > > +	struct virtio_input_event  evts[64];
> > > +	spinlock_t                 lock;
> > > +};
> > > +
> > > +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> > > +				   struct virtio_input_event *evtbuf)
> > > +{
> > > +	struct scatterlist sg[1];
> > > +
> > > +	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> > > +	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> > > +}
> > > +
> > > +static void virtinput_recv_events(struct virtqueue *vq)
> > > +{
> > > +	struct virtio_input *vi = vq->vdev->priv;
> > > +	struct virtio_input_event *event;
> > > +	unsigned long flags;
> > > +	unsigned int len;
> > > +
> > > +	spin_lock_irqsave(&vi->lock, flags);
> > > +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > > +		input_event(vi->idev,
> > > +			    le16_to_cpu(event->type),
> > > +			    le16_to_cpu(event->code),
> > > +			    le32_to_cpu(event->value));
> > 
> > What happens if input layer gets an
> > unexpected event code or value?
> > Or does something prevent it?
> > 
> > 
> > 
> > > +		virtinput_queue_evtbuf(vi, event);
> > > +	}
> > > +	virtqueue_kick(vq);
> > > +	spin_unlock_irqrestore(&vi->lock, flags);
> > > +}
> > > +
> > > +static int virtinput_send_status(struct virtio_input *vi,
> > > +				 u16 type, u16 code, s32 value)
> > > +{
> > > +	struct virtio_input_event *stsbuf;
> > > +	struct scatterlist sg[1];
> > > +	unsigned long flags;
> > > +	int rc;
> > > +
> > > +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > > +	if (!stsbuf)
> > > +		return -ENOMEM;
> > > +
> > > +	stsbuf->type  = cpu_to_le16(type);
> > > +	stsbuf->code  = cpu_to_le16(code);
> > > +	stsbuf->value = cpu_to_le32(value);
> > > +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > > +
> > > +	spin_lock_irqsave(&vi->lock, flags);
> > > +	rc = virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > > +	virtqueue_kick(vi->sts);
> > > +	spin_unlock_irqrestore(&vi->lock, flags);
> 
> I think locking is wrong here. This is basically input_dev->event()
> which is called with input_dev->event_lock spinlock held, and it is
> taking vi->lock. OTOH virtinput_recv_events() takes vi->lock and then
> calls input_event(), which will try taking input_dev->event_lock. It is
> bound to deadlock at some point.
> 
> I guess the easiest way would be to drop vi->lock() after fetching
> virtio event and before calling input_event().

Or just always use event_lock for event vq, leave vq->lock for
status vq only.

> > > +
> > > +	if (rc != 0)
> > > +		kfree(stsbuf);
> > > +	return rc;
> > 
> > This means that caller will get errors if it happens to call
> > send_status at a rate that's faster than host's consumption of them.
> > To me this looks wrong.
> > Poking at input layer, it seems to simply discard errors.
> > Is it always safe to discard status updates?
> > If yes, some kind of comment to clarify the logic would
> > make sense IMHO.
> > 
> > 
> > 
> > > +}
> > > +
> > > +static void virtinput_recv_status(struct virtqueue *vq)
> > > +{
> > > +	struct virtio_input *vi = vq->vdev->priv;
> > > +	struct virtio_input_event *stsbuf;
> > > +	unsigned long flags;
> > > +	unsigned int len;
> > > +
> > > +	spin_lock_irqsave(&vi->lock, flags);
> > > +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> > > +		kfree(stsbuf);
> > > +	spin_unlock_irqrestore(&vi->lock, flags);
> > > +}
> > > +
> > > +static int virtinput_status(struct input_dev *idev, unsigned int type,
> > > +			    unsigned int code, int value)
> > > +{
> > > +	struct virtio_input *vi = input_get_drvdata(idev);
> > > +
> > > +	return virtinput_send_status(vi, type, code, value);
> > > +}
> > > +
> > > +static size_t virtinput_cfg_select(struct virtio_input *vi,
> > > +				   u8 select, u8 subsel)
> > > +{
> > > +	u8 size;
> > > +
> > > +	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> > > +	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> > > +	return size;
> > > +}
> > > +
> > > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > > +			       unsigned long *bits, unsigned int bitcount)
> > > +{
> > > +	unsigned int bit;
> > > +	size_t bytes;
> > > +	u8 *virtio_bits;
> > > +
> > > +	bytes = virtinput_cfg_select(vi, select, subsel);
> > > +	if (!bytes)
> > > +		return;
> > 
> > How about limiting bytes to sizeof struct virtio_input_config->u?
> > 
> > > +	if (bitcount > bytes*8)
> > > +		bitcount = bytes*8;
> > 
> > Space around * pls.
> > 
> > > +
> > > +	/*
> > > +	 * Bitmap in virtio config space is a simple stream of bytes,
> > > +	 * with the first byte carrying bits 0-7, second bits 8-15 and
> > > +	 * so on.
> > > +	 */
> > > +	virtio_bits = kzalloc(bytes, GFP_KERNEL);
> > > +	if (!virtio_bits)
> > > +		return;
> > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > +			   virtio_bits, bytes);
> > > +	for (bit = 0; bit < bitcount; bit++) {
> > > +		if (virtio_bits[bit / 8] & (1 << (bit % 8)))
> > > +			__set_bit(bit, bits);
> > > +	}
> > > +	kfree(virtio_bits);
> > > +
> > > +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > > +		__set_bit(subsel, vi->idev->evbit);
> > > +}
> > > +
> > > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > > +{
> > > +	u32 mi, ma, re, fu, fl;
> > > +
> > > +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > > +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> > > +	input_abs_set_res(vi->idev, abs, re);
> > > +}
> > > +
> > > +static int virtinput_init_vqs(struct virtio_input *vi)
> > > +{
> > > +	struct virtqueue *vqs[2];
> > > +	vq_callback_t *cbs[] = { virtinput_recv_events,
> > > +				 virtinput_recv_status };
> > > +	static const char * names[] = { "events", "status" };
> > 
> > No space between * and names expected
> > 
> > > +	int i, err, size;
> > > +
> > > +	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> > > +	if (err)
> > > +		return err;
> > > +	vi->evt = vqs[0];
> > > +	vi->sts = vqs[1];
> > > +
> > > +	size = virtqueue_get_vring_size(vi->evt);
> > > +	if (size > ARRAY_SIZE(vi->evts))
> > > +		size = ARRAY_SIZE(vi->evts);
> > > +	for (i = 0; i < size; i++)
> > > +		virtinput_queue_evtbuf(vi, &vi->evts[i]);
> > > +	virtqueue_kick(vi->evt);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int virtinput_probe(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_input *vi;
> > > +	size_t size;
> > > +	int abs, err;
> > > +
> > > +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > > +		return -ENODEV;
> > > +
> > > +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> > > +	if (!vi)
> > > +		return -ENOMEM;
> > > +
> > > +	vdev->priv = vi;
> > > +	vi->vdev = vdev;
> > > +	spin_lock_init(&vi->lock);
> > > +
> > > +	err = virtinput_init_vqs(vi);
> > > +	if (err)
> > > +		goto err_init_vq;
> > > +
> > > +	vi->idev = input_allocate_device();
> > > +	if (!vi->idev) {
> > > +		err = -ENOMEM;
> > > +		goto err_input_alloc;
> > > +	}
> > > +	input_set_drvdata(vi->idev, vi);
> > > +
> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > +			   vi->name, min(size, sizeof(vi->name)));
> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > +			   vi->serial, min(size, sizeof(vi->serial)));
> > > +	snprintf(vi->phys, sizeof(vi->phys),
> > > +		 "virtio%d/input0", vdev->index);
> > > +	vi->idev->name = vi->name;
> > > +	vi->idev->phys = vi->phys;
> > > +	vi->idev->uniq = vi->serial;
> > > +
> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> > > +	if (size >= 8) {
> > 
> > What does 8 mean here? Should be sizeof virtio_input_devids?
> > 
> > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > +			     u.ids.bustype, &vi->idev->id.bustype);
> > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > +			     u.ids.vendor, &vi->idev->id.vendor);
> > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > +			     u.ids.product, &vi->idev->id.product);
> > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > +			     u.ids.version, &vi->idev->id.version);
> > > +	} else {
> > > +		vi->idev->id.bustype = BUS_VIRTUAL;
> > > +	}
> > > +
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> > > +			   vi->idev->propbit, INPUT_PROP_CNT);
> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> > > +	if (size)
> > > +		__set_bit(EV_REP, vi->idev->evbit);
> > > +
> > > +	vi->idev->dev.parent = &vdev->dev;
> > > +	vi->idev->event = virtinput_status;
> > > +
> > > +	/* device -> kernel */
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> > > +			   vi->idev->keybit, KEY_CNT);
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> > > +			   vi->idev->relbit, REL_CNT);
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> > > +			   vi->idev->absbit, ABS_CNT);
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> > > +			   vi->idev->mscbit, MSC_CNT);
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> > > +			   vi->idev->swbit,  SW_CNT);
> > > +
> > > +	/* kernel -> device */
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> > > +			   vi->idev->ledbit, LED_CNT);
> > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> > > +			   vi->idev->sndbit, SND_CNT);
> > > +
> > > +	if (test_bit(EV_ABS, vi->idev->evbit)) {
> > > +		for (abs = 0; abs < ABS_CNT; abs++) {
> > > +			if (!test_bit(abs, vi->idev->absbit))
> > > +				continue;
> > > +			virtinput_cfg_abs(vi, abs);
> > > +		}
> > > +	}
> > > +	virtio_device_ready(vdev);
> > > +
> > At this point you can already get interrupts.
> > This will cause events to be forwarded.
> > I'm guessing this is ok since you called
> > input_allocate_device, but worth checking,
> > and maybe adding a comment.
> 
> Yes, it is OK to send events though yet unregistered input device, as
> long as it was allocated with input_allocate_device().
> 
> > 
> > > +	err = input_register_device(vi->idev);
> > > +	if (err)
> > > +		goto err_input_register;
> > > +
> > > +	return 0;
> > > +
> > > +err_input_register:
> > 
> > > +	input_free_device(vi->idev);
> > 
> > At this point you can already get interrupts
> > since you called virtio_device_ready, and
> > getting events from a freed device likely won't
> > DTRT.
> 
> Right. I guess you want to mark the virtio device ready only after
> registering input device.

No that's broken since you can get events after this
point, and you won't be able to forward them.

> > 
> > > +err_input_alloc:
> > > +	vdev->config->del_vqs(vdev);
> > > +err_init_vq:
> > > +	kfree(vi);
> > > +	return err;
> > > +}
> > > +
> > > +static void virtinput_remove(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_input *vi = vdev->priv;
> > > +
> > > +	input_unregister_device(vi->idev);
> > 
> > same thing here, you might get an event at this point.
> > You need to somehow block new events
> > being sent to device while keeping
> > device around.
> > 
> > Since you already do everything under a spinlock,
> > it's probably easiest to add a flag discarding
> > recv events. You can then check it in virtinput_recv_events
> > before calling input_event.
> 
> Instead of checking the flag is it possible to pause virio device? Maybe
> virtio_break_device()?
> 
> > 
> > 
> > 
> > > +	vdev->config->del_vqs(vdev);
> > > +	kfree(vi);
> > > +}
> > > +
> > > +static unsigned int features[] = {
> > > +};
> > > +static struct virtio_device_id id_table[] = {
> > > +	{ VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
> > > +	{ 0 },
> > > +};
> > > +
> > > +static struct virtio_driver virtio_input_driver = {
> > > +	.driver.name         = KBUILD_MODNAME,
> > > +	.driver.owner        = THIS_MODULE,
> > > +	.feature_table       = features,
> > > +	.feature_table_size  = ARRAY_SIZE(features),
> > > +	.id_table            = id_table,
> > > +	.probe               = virtinput_probe,
> > > +	.remove              = virtinput_remove,
> > 
> > I note this driver doesn't seem to handle hybernation,
> > that's probably a bug?
> > 
> > 
> > > +};
> > > +
> > > +module_virtio_driver(virtio_input_driver);
> > > +MODULE_DEVICE_TABLE(virtio, id_table);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("Virtio input device driver");
> > > +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> > > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > > index 68ceb97..04b829e 100644
> > > --- a/include/uapi/linux/Kbuild
> > > +++ b/include/uapi/linux/Kbuild
> > > @@ -430,6 +430,7 @@ header-y += virtio_blk.h
> > >  header-y += virtio_config.h
> > >  header-y += virtio_console.h
> > >  header-y += virtio_ids.h
> > > +header-y += virtio_input.h
> > >  header-y += virtio_net.h
> > >  header-y += virtio_pci.h
> > >  header-y += virtio_ring.h
> > > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> > > index 284fc3a..5f60aa4 100644
> > > --- a/include/uapi/linux/virtio_ids.h
> > > +++ b/include/uapi/linux/virtio_ids.h
> > > @@ -39,5 +39,6 @@
> > >  #define VIRTIO_ID_9P		9 /* 9p virtio console */
> > >  #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
> > >  #define VIRTIO_ID_CAIF	       12 /* Virtio caif */
> > > +#define VIRTIO_ID_INPUT        18 /* virtio input */
> > >  
> > >  #endif /* _LINUX_VIRTIO_IDS_H */
> > > diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
> > > new file mode 100644
> > > index 0000000..7fceabd
> > > --- /dev/null
> > > +++ b/include/uapi/linux/virtio_input.h
> > > @@ -0,0 +1,76 @@
> > > +#ifndef _LINUX_VIRTIO_INPUT_H
> > > +#define _LINUX_VIRTIO_INPUT_H
> > > +/* 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:
> > > + * 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 IBM 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
> > > + * 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. */
> > > +#include <linux/virtio_ids.h>
> > > +#include <linux/virtio_config.h>
> > > +
> > > +enum virtio_input_config_select {
> > > +	VIRTIO_INPUT_CFG_UNSET      = 0x00,
> > > +	VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
> > > +	VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
> > > +	VIRTIO_INPUT_CFG_ID_DEVIDS  = 0x03,
> > > +	VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
> > > +	VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
> > > +	VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
> > > +};
> > > +
> > > +struct virtio_input_absinfo {
> > > +	__virtio32  min;
> > > +	__virtio32  max;
> > > +	__virtio32  fuzz;
> > > +	__virtio32  flat;
> > > +	__virtio32  res;
> > > +};
> > > +
> > > +struct virtio_input_devids {
> > > +	__virtio16  bustype;
> > > +	__virtio16  vendor;
> > > +	__virtio16  product;
> > > +	__virtio16  version;
> > > +};
> > > +
> > 
> > this padding bt two spaces looks weird.
> > 
> > > +struct virtio_input_config {
> > > +	__u8    select;
> > > +	__u8    subsel;
> > > +	__u8    size;
> > > +	__u8    reserved;
> > > +	union {
> > > +		char string[128];
> > > +		__u8 bitmap[128];
> > 
> > I note that neither string nor bitmap are used by
> > driver. What are they in aid of?
> 
> Also, what happens if we need more than 1024 bits to pass bitmap data?
> We might get there with keyboards.
> 
> > 
> > > +		struct virtio_input_absinfo abs;
> > > +		struct virtio_input_devids ids;
> > > +	} u;
> > > +};
> > > +
> > > +struct virtio_input_event {
> > > +	__le16 type;
> > > +	__le16 code;
> > > +	__le32 value;
> > > +};
> > > +
> > > +#endif /* _LINUX_VIRTIO_INPUT_H */
> > > -- 
> > > 1.8.3.1
> 
> Thanks.
> 
> -- 
> Dmitry

Re: [PATCH v3] Add virtio-input driver.

[Dmitry Torokhov]

On Tue, Mar 24, 2015 at 06:22:36PM +0100, Michael S. Tsirkin wrote:
> On Tue, Mar 24, 2015 at 09:23:37AM -0700, Dmitry Torokhov wrote:
> > On Tue, Mar 24, 2015 at 11:36:31AM +0100, Michael S. Tsirkin wrote:
> > > On Tue, Mar 24, 2015 at 08:32:01AM +0100, Gerd Hoffmann wrote:
> > > > virtio-input is basically evdev-events-over-virtio, so this driver isn't
> > > > much more than reading configuration from config space and forwarding
> > > > incoming events to the linux input layer.
> > > > 
> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > 
> > > Looks pretty neat overall. I think I still see some
> > > small issues, but it's getting there.
> > > 
> > > > ---
> > > >  MAINTAINERS                       |   6 +
> > > >  drivers/virtio/Kconfig            |  10 ++
> > > >  drivers/virtio/Makefile           |   1 +
> > > >  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
> > > >  include/uapi/linux/Kbuild         |   1 +
> > > >  include/uapi/linux/virtio_ids.h   |   1 +
> > > >  include/uapi/linux/virtio_input.h |  76 +++++++++
> > > >  7 files changed, 408 insertions(+)
> > > >  create mode 100644 drivers/virtio/virtio_input.c
> > > >  create mode 100644 include/uapi/linux/virtio_input.h
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 358eb01..6f233dd 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -10442,6 +10442,12 @@ S:	Maintained
> > > >  F:	drivers/vhost/
> > > >  F:	include/uapi/linux/vhost.h
> > > >  
> > > > +VIRTIO INPUT DRIVER
> > > > +M:	Gerd Hoffmann <kraxel@redhat.com>
> > > > +S:	Maintained
> > > > +F:	drivers/virtio/virtio_input.c
> > > > +F:	include/uapi/linux/virtio_input.h
> > > > +
> > > >  VIA RHINE NETWORK DRIVER
> > > >  M:	Roger Luethi <rl@hellgate.ch>
> > > >  S:	Maintained
> > > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > > index b546da5..cab9f3f 100644
> > > > --- a/drivers/virtio/Kconfig
> > > > +++ b/drivers/virtio/Kconfig
> > > > @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
> > > >  
> > > >  	 If unsure, say M.
> > > >  
> > > > +config VIRTIO_INPUT
> > > > +	tristate "Virtio input driver"
> > > > +	depends on VIRTIO
> > > > +	depends on INPUT
> > > > +	---help---
> > > > +	 This driver supports virtio input devices such as
> > > > +	 keyboards, mice and tablets.
> > > > +
> > > > +	 If unsure, say M.
> > > > +
> > > >   config VIRTIO_MMIO
> > > >  	tristate "Platform bus driver for memory mapped virtio devices"
> > > >  	depends on HAS_IOMEM
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index d85565b..41e30e3 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > > >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > > > +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > > > diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> > > > new file mode 100644
> > > > index 0000000..cf112b2
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_input.c
> > > > @@ -0,0 +1,313 @@
> > > > +#include <linux/module.h>
> > > > +#include <linux/virtio.h>
> > > > +#include <linux/input.h>
> > > > +
> > > > +#include <uapi/linux/virtio_ids.h>
> > > > +#include <uapi/linux/virtio_input.h>
> > > > +
> > > > +struct virtio_input {
> > > > +	struct virtio_device       *vdev;
> > > > +	struct input_dev           *idev;
> > > > +	char                       name[64];
> > > > +	char                       serial[64];
> > > > +	char                       phys[64];
> > > > +	struct virtqueue           *evt, *sts;
> > > > +	struct virtio_input_event  evts[64];
> > > > +	spinlock_t                 lock;
> > > > +};
> > > > +
> > > > +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> > > > +				   struct virtio_input_event *evtbuf)
> > > > +{
> > > > +	struct scatterlist sg[1];
> > > > +
> > > > +	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> > > > +	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> > > > +}
> > > > +
> > > > +static void virtinput_recv_events(struct virtqueue *vq)
> > > > +{
> > > > +	struct virtio_input *vi = vq->vdev->priv;
> > > > +	struct virtio_input_event *event;
> > > > +	unsigned long flags;
> > > > +	unsigned int len;
> > > > +
> > > > +	spin_lock_irqsave(&vi->lock, flags);
> > > > +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > > > +		input_event(vi->idev,
> > > > +			    le16_to_cpu(event->type),
> > > > +			    le16_to_cpu(event->code),
> > > > +			    le32_to_cpu(event->value));
> > > 
> > > What happens if input layer gets an
> > > unexpected event code or value?
> > > Or does something prevent it?
> > > 
> > > 
> > > 
> > > > +		virtinput_queue_evtbuf(vi, event);
> > > > +	}
> > > > +	virtqueue_kick(vq);
> > > > +	spin_unlock_irqrestore(&vi->lock, flags);
> > > > +}
> > > > +
> > > > +static int virtinput_send_status(struct virtio_input *vi,
> > > > +				 u16 type, u16 code, s32 value)
> > > > +{
> > > > +	struct virtio_input_event *stsbuf;
> > > > +	struct scatterlist sg[1];
> > > > +	unsigned long flags;
> > > > +	int rc;
> > > > +
> > > > +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > > > +	if (!stsbuf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	stsbuf->type  = cpu_to_le16(type);
> > > > +	stsbuf->code  = cpu_to_le16(code);
> > > > +	stsbuf->value = cpu_to_le32(value);
> > > > +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > > > +
> > > > +	spin_lock_irqsave(&vi->lock, flags);
> > > > +	rc = virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > > > +	virtqueue_kick(vi->sts);
> > > > +	spin_unlock_irqrestore(&vi->lock, flags);
> > 
> > I think locking is wrong here. This is basically input_dev->event()
> > which is called with input_dev->event_lock spinlock held, and it is
> > taking vi->lock. OTOH virtinput_recv_events() takes vi->lock and then
> > calls input_event(), which will try taking input_dev->event_lock. It is
> > bound to deadlock at some point.
> > 
> > I guess the easiest way would be to drop vi->lock() after fetching
> > virtio event and before calling input_event().
> 
> Or just always use event_lock for event vq, leave vq->lock for
> status vq only.
> 
> > > > +
> > > > +	if (rc != 0)
> > > > +		kfree(stsbuf);
> > > > +	return rc;
> > > 
> > > This means that caller will get errors if it happens to call
> > > send_status at a rate that's faster than host's consumption of them.
> > > To me this looks wrong.
> > > Poking at input layer, it seems to simply discard errors.
> > > Is it always safe to discard status updates?
> > > If yes, some kind of comment to clarify the logic would
> > > make sense IMHO.
> > > 
> > > 
> > > 
> > > > +}
> > > > +
> > > > +static void virtinput_recv_status(struct virtqueue *vq)
> > > > +{
> > > > +	struct virtio_input *vi = vq->vdev->priv;
> > > > +	struct virtio_input_event *stsbuf;
> > > > +	unsigned long flags;
> > > > +	unsigned int len;
> > > > +
> > > > +	spin_lock_irqsave(&vi->lock, flags);
> > > > +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> > > > +		kfree(stsbuf);
> > > > +	spin_unlock_irqrestore(&vi->lock, flags);
> > > > +}
> > > > +
> > > > +static int virtinput_status(struct input_dev *idev, unsigned int type,
> > > > +			    unsigned int code, int value)
> > > > +{
> > > > +	struct virtio_input *vi = input_get_drvdata(idev);
> > > > +
> > > > +	return virtinput_send_status(vi, type, code, value);
> > > > +}
> > > > +
> > > > +static size_t virtinput_cfg_select(struct virtio_input *vi,
> > > > +				   u8 select, u8 subsel)
> > > > +{
> > > > +	u8 size;
> > > > +
> > > > +	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> > > > +	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> > > > +	return size;
> > > > +}
> > > > +
> > > > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > > > +			       unsigned long *bits, unsigned int bitcount)
> > > > +{
> > > > +	unsigned int bit;
> > > > +	size_t bytes;
> > > > +	u8 *virtio_bits;
> > > > +
> > > > +	bytes = virtinput_cfg_select(vi, select, subsel);
> > > > +	if (!bytes)
> > > > +		return;
> > > 
> > > How about limiting bytes to sizeof struct virtio_input_config->u?
> > > 
> > > > +	if (bitcount > bytes*8)
> > > > +		bitcount = bytes*8;
> > > 
> > > Space around * pls.
> > > 
> > > > +
> > > > +	/*
> > > > +	 * Bitmap in virtio config space is a simple stream of bytes,
> > > > +	 * with the first byte carrying bits 0-7, second bits 8-15 and
> > > > +	 * so on.
> > > > +	 */
> > > > +	virtio_bits = kzalloc(bytes, GFP_KERNEL);
> > > > +	if (!virtio_bits)
> > > > +		return;
> > > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > > +			   virtio_bits, bytes);
> > > > +	for (bit = 0; bit < bitcount; bit++) {
> > > > +		if (virtio_bits[bit / 8] & (1 << (bit % 8)))
> > > > +			__set_bit(bit, bits);
> > > > +	}
> > > > +	kfree(virtio_bits);
> > > > +
> > > > +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > > > +		__set_bit(subsel, vi->idev->evbit);
> > > > +}
> > > > +
> > > > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > > > +{
> > > > +	u32 mi, ma, re, fu, fl;
> > > > +
> > > > +	virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.res, &re);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > > > +	input_set_abs_params(vi->idev, abs, mi, ma, fu, fl);
> > > > +	input_abs_set_res(vi->idev, abs, re);
> > > > +}
> > > > +
> > > > +static int virtinput_init_vqs(struct virtio_input *vi)
> > > > +{
> > > > +	struct virtqueue *vqs[2];
> > > > +	vq_callback_t *cbs[] = { virtinput_recv_events,
> > > > +				 virtinput_recv_status };
> > > > +	static const char * names[] = { "events", "status" };
> > > 
> > > No space between * and names expected
> > > 
> > > > +	int i, err, size;
> > > > +
> > > > +	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> > > > +	if (err)
> > > > +		return err;
> > > > +	vi->evt = vqs[0];
> > > > +	vi->sts = vqs[1];
> > > > +
> > > > +	size = virtqueue_get_vring_size(vi->evt);
> > > > +	if (size > ARRAY_SIZE(vi->evts))
> > > > +		size = ARRAY_SIZE(vi->evts);
> > > > +	for (i = 0; i < size; i++)
> > > > +		virtinput_queue_evtbuf(vi, &vi->evts[i]);
> > > > +	virtqueue_kick(vi->evt);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int virtinput_probe(struct virtio_device *vdev)
> > > > +{
> > > > +	struct virtio_input *vi;
> > > > +	size_t size;
> > > > +	int abs, err;
> > > > +
> > > > +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > > > +		return -ENODEV;
> > > > +
> > > > +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> > > > +	if (!vi)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	vdev->priv = vi;
> > > > +	vi->vdev = vdev;
> > > > +	spin_lock_init(&vi->lock);
> > > > +
> > > > +	err = virtinput_init_vqs(vi);
> > > > +	if (err)
> > > > +		goto err_init_vq;
> > > > +
> > > > +	vi->idev = input_allocate_device();
> > > > +	if (!vi->idev) {
> > > > +		err = -ENOMEM;
> > > > +		goto err_input_alloc;
> > > > +	}
> > > > +	input_set_drvdata(vi->idev, vi);
> > > > +
> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> > > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > > +			   vi->name, min(size, sizeof(vi->name)));
> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> > > > +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > > > +			   vi->serial, min(size, sizeof(vi->serial)));
> > > > +	snprintf(vi->phys, sizeof(vi->phys),
> > > > +		 "virtio%d/input0", vdev->index);
> > > > +	vi->idev->name = vi->name;
> > > > +	vi->idev->phys = vi->phys;
> > > > +	vi->idev->uniq = vi->serial;
> > > > +
> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_DEVIDS, 0);
> > > > +	if (size >= 8) {
> > > 
> > > What does 8 mean here? Should be sizeof virtio_input_devids?
> > > 
> > > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > > +			     u.ids.bustype, &vi->idev->id.bustype);
> > > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > > +			     u.ids.vendor, &vi->idev->id.vendor);
> > > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > > +			     u.ids.product, &vi->idev->id.product);
> > > > +		virtio_cread(vi->vdev, struct virtio_input_config,
> > > > +			     u.ids.version, &vi->idev->id.version);
> > > > +	} else {
> > > > +		vi->idev->id.bustype = BUS_VIRTUAL;
> > > > +	}
> > > > +
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> > > > +			   vi->idev->propbit, INPUT_PROP_CNT);
> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> > > > +	if (size)
> > > > +		__set_bit(EV_REP, vi->idev->evbit);
> > > > +
> > > > +	vi->idev->dev.parent = &vdev->dev;
> > > > +	vi->idev->event = virtinput_status;
> > > > +
> > > > +	/* device -> kernel */
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> > > > +			   vi->idev->keybit, KEY_CNT);
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> > > > +			   vi->idev->relbit, REL_CNT);
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> > > > +			   vi->idev->absbit, ABS_CNT);
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> > > > +			   vi->idev->mscbit, MSC_CNT);
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> > > > +			   vi->idev->swbit,  SW_CNT);
> > > > +
> > > > +	/* kernel -> device */
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> > > > +			   vi->idev->ledbit, LED_CNT);
> > > > +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> > > > +			   vi->idev->sndbit, SND_CNT);
> > > > +
> > > > +	if (test_bit(EV_ABS, vi->idev->evbit)) {
> > > > +		for (abs = 0; abs < ABS_CNT; abs++) {
> > > > +			if (!test_bit(abs, vi->idev->absbit))
> > > > +				continue;
> > > > +			virtinput_cfg_abs(vi, abs);
> > > > +		}
> > > > +	}
> > > > +	virtio_device_ready(vdev);
> > > > +
> > > At this point you can already get interrupts.
> > > This will cause events to be forwarded.
> > > I'm guessing this is ok since you called
> > > input_allocate_device, but worth checking,
> > > and maybe adding a comment.
> > 
> > Yes, it is OK to send events though yet unregistered input device, as
> > long as it was allocated with input_allocate_device().
> > 
> > > 
> > > > +	err = input_register_device(vi->idev);
> > > > +	if (err)
> > > > +		goto err_input_register;
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_input_register:
> > > 
> > > > +	input_free_device(vi->idev);
> > > 
> > > At this point you can already get interrupts
> > > since you called virtio_device_ready, and
> > > getting events from a freed device likely won't
> > > DTRT.
> > 
> > Right. I guess you want to mark the virtio device ready only after
> > registering input device.
> 
> No that's broken since you can get events after this
> point, and you won't be able to forward them.

Who cares? What makes these events needed compared to ones sent 1 ms
earlier before we had input device registered?

But I guess if you can call virtio_device_ready/virtio_device_broken
several times then the best option is putting them into input_dev->open
and input_dev->close callbacks.

Thanks.

-- 
Dmitry

Re: [PATCH v3] Add virtio-input driver.

[Michael S. Tsirkin]

On Tue, Mar 24, 2015 at 10:28:05AM -0700, Dmitry Torokhov wrote:
> > > > > +	err = input_register_device(vi->idev);
> > > > > +	if (err)
> > > > > +		goto err_input_register;
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err_input_register:
> > > > 
> > > > > +	input_free_device(vi->idev);
> > > > 
> > > > At this point you can already get interrupts
> > > > since you called virtio_device_ready, and
> > > > getting events from a freed device likely won't
> > > > DTRT.
> > > 
> > > Right. I guess you want to mark the virtio device ready only after
> > > registering input device.
> > 
> > No that's broken since you can get events after this
> > point, and you won't be able to forward them.
> 
> Who cares?

virtio cares: guest will crash if you attempt to
kick virtqueue before device ready call.

> What makes these events needed compared to ones sent 1 ms
> earlier before we had input device registered?
> 
> But I guess if you can call virtio_device_ready/virtio_device_broken
> several times then the best option is putting them into input_dev->open
> and input_dev->close callbacks.
> 
> Thanks.
> -- 
> Dmitry