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

[Markus Armbruster <armbru@redhat.com>]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> Markus Armbruster wrote:
>> 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
>
>> The backends run in dom0 user space.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> ---
[...]
>> 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 @@
[...]
>> +static int __devinit xenkbd_probe(struct xenbus_device *dev,
>> +				  const struct xenbus_device_id *id)
>> +{
[...]
>> +	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.

>> +	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*.

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

Thanks!