Jeremy Fitzhardinge <jeremy@xxxxxxxx> 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@xxxxxxxxxx>
>>
>> ---
[...]
>> 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!
-
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
