Re: [PATCH 2/2] xen pvfb: Para-virtual framebuffer, keyboard and pointer driver

Jeremy Fitzhardinge [off-list ref] writes:

Markus Armbruster wrote:

quoted

This is a pair of Xen para-virtual frontend device drivers:
drivers/video/xen-fbfront.c provides a framebuffer, and
drivers/input/xen-kbdfront provides keyboard and mouse.
  

Unless they're actually inter-dependent, could you post this as two
separate patches?  I don't know anything about these parts of the
kernel, so it would be nice to make it very obvious which changes are
fb vs mouse/keyboard.

I could do that do that, but the intermediate step (one driver, not
the other) is somewhat problematic: the backend in dom0 needs both
drivers, and will refuse to complete device initialization unless
they're both present.

(I guess input/* vs video/* should make it obvious, but it looks like
input has a config dependency on fb, so I'll avoid making too many
presumptions...)

Framebuffer: fbif.h xen-fbfront.c
Keyboard/mouse: kbdif.h xen-kbdfront.h

I added the config dependency because having one without the other
doesn't make sense, as explained above.

Still want it split into two separate patches?

(Couple of comments below)

   J

quoted

The backends run in dom0 user space.

Signed-off-by: Markus Armbruster <redacted>

---

[...]

quoted

diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
new file mode 100644
index 0000000..84f65cf
--- /dev/null
+++ b/drivers/input/xen-kbdfront.c

@@ -0,0 +1,337 @@

[...]

quoted

+static int __devinit xenkbd_probe(struct xenbus_device *dev,
+				  const struct xenbus_device_id *id)
+{

[...]

quoted

+	if (ret < 0)
+		goto error;
+
+	return 0;
+
+ error_nomem:
+	ret = -ENOMEM;
+	xenbus_dev_fatal(dev, ret, "allocating device memory");
+ error:
+	xenkbd_remove(dev);
  

This is happy if dev->info is only partially initialized?

It's designed that way.  dev->info is initialized so that
xenkbd_remove() does nothing.  Then stuff is stored into dev->info
only when it's sufficiently initialized for xenkbd_remove() to clean
it up.

quoted

+	return ret;
+}
+
+static int xenkbd_resume(struct xenbus_device *dev)
+{
+	struct xenkbd_info *info = dev->dev.driver_data;
+
+	xenkbd_disconnect_backend(info);
+	memset(info->page, 0, PAGE_SIZE);
+	return xenkbd_connect_backend(dev, info);
+}
+
+static int xenkbd_remove(struct xenbus_device *dev)
+{
+	struct xenkbd_info *info = dev->dev.driver_data;
+
+	xenkbd_disconnect_backend(info);
+	input_unregister_device(info->kbd);
+	input_unregister_device(info->ptr);
  

Does this free kdb and ptr?

Yes.  xenkbd_probe() initializes info->kbd and info->ptr to null, and
changes that to the device only after input_register_device()
succeeds.  If something goes wrong between input_allocate_device() and
input_register_device(), xenkbd_probe() frees the device with
input_free_device().  This is how input_register_device() wants to be
used according to its function comment:

    /**
     * input_register_device - register device with input core
     * @dev: device to be registered
     *
     * This function registers device with input core. The device must be
     * allocated with input_allocate_device() and all it's capabilities
     * set up before registering.
     * If function fails the device must be freed with input_free_device().
     * Once device has been successfully registered it can be unregistered
     * with input_unregister_device(); input_free_device() should not be
     * called in this case.
     */

There's another bug here: must not call input_unregister_device() when
the device is still null.  Man, I remember checking cleanup multiple
times when this stuff went into Xen (i.e. quite some time ago), and I
still missed this one.  Going to check cleanup *again*.

quoted

+	free_page((unsigned long)info->page);
+	kfree(info);
+	return 0;
+}

[...]

Thanks!