Re: [PATCH 1/1] Add virtio-input driver.

On Thu, Mar 19, 2015 at 10:13:11AM +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 <redacted>

What worries me is how well are these events specified.
Will we be able to write drivers for non-linux guests?
Not sure where to discuss this - this seems like an inappropriate
thread.

Comments on linux driver below.

Does this return an error to userspace?
If so it's not a good idea I think, GFP_ATOMIC failures are
transient conditions and should not be reported
to userspace.
Can use GFP_KERNEL here?

+
+	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));
+	virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);

This can fail if queue is full. What prevents this from happening?

+	virtqueue_kick(vi->sts);

Also what prevents multiple virtinput_send_status calls
from racing with each other? Is there locking at a higher level?

+	return 0;
+}
+
+static void virtinput_recv_status(struct virtqueue *vq)
+{
+	struct virtio_input *vi = vq->vdev->priv;
+	struct virtio_input_event *stsbuf;
+	unsigned int len;
+
+	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)

looks like this get_buf can race with add above.
Need some locking.

Also pls avoid != NULL, removing it makes code less verbose.

+		kfree(stsbuf);
+	virtqueue_kick(vq);

Why are you kicking here?

+}
+
+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 cfg = 0;
+
+	bytes = virtinput_cfg_select(vi, select, subsel);
+	if (!bytes)
+		return;
+	if (bitcount > bytes*8)
+		bitcount = bytes*8;
+	if (select == VIRTIO_INPUT_CFG_EV_BITS)
+		set_bit(subsel, vi->idev->evbit);
+	for (bit = 0; bit < bitcount; bit++) {
+		if ((bit % 8) == 0)
+			virtio_cread(vi->vdev, struct virtio_input_config,
+				     u.bitmap[bit/8], &cfg);

coding style violations above. you need spaces around ops like / and *.
Please run checkpatch.pl

+		if (cfg & (1 << (bit % 8)))
+			set_bit(bit, bits);

what if not set? does something clear the mask?

+	}

doesn't above just implement bitmap_copy or bitmap_or?  Will it hurt to
just do virtio_cread_bytes into a temporary buffer and then invoke
bitmap ops? Looks like the buffer is at most 256 bytes:
too large to be on stack, but you can allocate it at probe time.

+}
+
+static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
+{
+	u32 size, mi, ma, fu, fl;
+
+	size = 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.fuzz, &fu);
+	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);

you read le field into u32 value.
Please run sparse on this code. you will get a ton
of warnings. Same error appears elsewhere.

+	input_set_abs_params(vi->idev, abs,
+			     le32_to_cpu(mi), le32_to_cpu(ma),
+			     le32_to_cpu(fu), le32_to_cpu(fl));
+}
+
+static int virtinput_init_vqs(struct virtio_input *vi)
+{
+	struct virtqueue *vqs[2];
+	vq_callback_t *cbs[] = { virtinput_recv_events,
+				 virtinput_recv_status };
+	const char *names[] = { "events", "status" };
+	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]);
+

you add a bunch of bufs but never kick apparently.
you really must kick otherwise device might not
see the buffers.

+	return 0;
+}
+
+static int virtinput_probe(struct virtio_device *vdev)
+{
+	struct virtio_input *vi;
+	size_t size;
+	int abs, err;

How about checking VERSION_1 and bailing out of not there?

+
+	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
+	if (!vi) {
+		err = -ENOMEM;
+		goto out1;
+	}
+	vdev->priv = vi;
+	vi->vdev = vdev;
+
+	err = virtinput_init_vqs(vi);
+	if (err)
+		goto out2;
+
+	vi->idev = input_allocate_device();
+	if (!vi->idev) {
+		err = -ENOMEM;
+		goto out3;
+	}
+	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);
+
+	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->name = vi->name;
+	vi->idev->phys = vi->phys;
+	vi->idev->id.bustype = BUS_VIRTUAL;
+	vi->idev->id.vendor  = 0x0001;
+	vi->idev->id.product = 0x0001;
+	vi->idev->id.version = 0x0100;

Add comments explaining why these #s make sense?

+	vi->idev->dev.parent = &vdev->dev;
+	vi->idev->dev.groups = dev_attr_groups;
+	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);
+		}
+	}
+
+	err = input_register_device(vi->idev);

Once you do this, virtinput_status can get called,
and that will kick, correct?
If so, you must call device_ready before this.

+	if (err)
+		goto out4;
+
+	return 0;
+
+out4:
+	input_free_device(vi->idev);
+out3:
+	vdev->config->del_vqs(vdev);
+out2:
+	kfree(vi);
+out1:
+	return err;

free on error is out of order with initialization.
Might lead to leaks or other bugs.
Also - can you name labels something sensible pls?
out is usually for exiting on success too...
E.g. out4 -> err_register etc.

+}
+
+static void virtinput_remove(struct virtio_device *vdev)
+{
+	struct virtio_input *vi = vdev->priv;
+
+	input_unregister_device(vi->idev);
+	vdev->config->reset(vdev);

You don't really need a reset if you just to del_vqs.
People do this if they want to prevent interrupts
without deleting vqs.

+	vdev->config->del_vqs(vdev);
+	kfree(vi);

free_device seems to be missing?