From: Oleksandr Andrushchenko <[email protected]>
It is now only possible to control if multi-touch virtual device
is created or not (via the corresponding XenStore entries),
but keyboard and pointer devices are always created.
In some cases this is not desirable. For example, if virtual
keyboard device is exposed to Android then the latter won't
automatically show on-screen keyboard as it expects that a
physical keyboard device can be used for typing.
Make it possible to configure which virtual devices are created
with module parameters:
- no_ptr_dev=1 if no pointer device needs to be created
- no_kbd_dev=1 if no keyboard device needs to be created
Keep old behavior by default.
Signed-off-by: Oleksandr Andrushchenko <[email protected]>
Suggested-by: Andrii Chepurnyi <[email protected]>
Tested-by: Andrii Chepurnyi <[email protected]>
---
drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
1 file changed, 92 insertions(+), 67 deletions(-)
diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index d91f3b1c5375..a3306aad40b0 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
MODULE_PARM_DESC(ptr_size,
"Pointing device width, height in pixels (default 800,600)");
+static unsigned int no_ptr_dev;
+module_param(no_ptr_dev, uint, 0);
+MODULE_PARM_DESC(no_ptr_dev,
+ "If set then no virtual pointing device exposed to the guest");
+
+static unsigned int no_kbd_dev;
+module_param(no_kbd_dev, uint, 0);
+MODULE_PARM_DESC(no_kbd_dev,
+ "If set then no virtual keyboard device exposed to the guest");
+
static int xenkbd_remove(struct xenbus_device *);
static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *);
static void xenkbd_disconnect_backend(struct xenkbd_info *);
@@ -63,6 +73,9 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *);
static void xenkbd_handle_motion_event(struct xenkbd_info *info,
struct xenkbd_motion *motion)
{
+ if (unlikely(!info->ptr))
+ return;
+
input_report_rel(info->ptr, REL_X, motion->rel_x);
input_report_rel(info->ptr, REL_Y, motion->rel_y);
if (motion->rel_z)
@@ -73,6 +86,9 @@ static void xenkbd_handle_motion_event(struct xenkbd_info *info,
static void xenkbd_handle_position_event(struct xenkbd_info *info,
struct xenkbd_position *pos)
{
+ if (unlikely(!info->ptr))
+ return;
+
input_report_abs(info->ptr, ABS_X, pos->abs_x);
input_report_abs(info->ptr, ABS_Y, pos->abs_y);
if (pos->rel_z)
@@ -97,6 +113,9 @@ static void xenkbd_handle_key_event(struct xenkbd_info *info,
return;
}
+ if (unlikely(!dev))
+ return;
+
input_event(dev, EV_KEY, key->keycode, value);
input_sync(dev);
}
@@ -192,7 +211,7 @@ static int xenkbd_probe(struct xenbus_device *dev,
const struct xenbus_device_id *id)
{
int ret, i;
- unsigned int abs, touch;
+ unsigned int touch;
struct xenkbd_info *info;
struct input_dev *kbd, *ptr, *mtouch;
@@ -211,24 +230,6 @@ static int xenkbd_probe(struct xenbus_device *dev,
if (!info->page)
goto error_nomem;
- /* Set input abs params to match backend screen res */
- abs = xenbus_read_unsigned(dev->otherend,
- XENKBD_FIELD_FEAT_ABS_POINTER, 0);
- ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend,
- XENKBD_FIELD_WIDTH,
- ptr_size[KPARAM_X]);
- ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend,
- XENKBD_FIELD_HEIGHT,
- ptr_size[KPARAM_Y]);
- if (abs) {
- ret = xenbus_write(XBT_NIL, dev->nodename,
- XENKBD_FIELD_REQ_ABS_POINTER, "1");
- if (ret) {
- pr_warn("xenkbd: can't request abs-pointer\n");
- abs = 0;
- }
- }
-
touch = xenbus_read_unsigned(dev->nodename,
XENKBD_FIELD_FEAT_MTOUCH, 0);
if (touch) {
@@ -241,60 +242,84 @@ static int xenkbd_probe(struct xenbus_device *dev,
}
/* keyboard */
- kbd = input_allocate_device();
- if (!kbd)
- goto error_nomem;
- kbd->name = "Xen Virtual Keyboard";
- kbd->phys = info->phys;
- kbd->id.bustype = BUS_PCI;
- kbd->id.vendor = 0x5853;
- kbd->id.product = 0xffff;
-
- __set_bit(EV_KEY, kbd->evbit);
- for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
- __set_bit(i, kbd->keybit);
- for (i = KEY_OK; i < KEY_MAX; i++)
- __set_bit(i, kbd->keybit);
-
- ret = input_register_device(kbd);
- if (ret) {
- input_free_device(kbd);
- xenbus_dev_fatal(dev, ret, "input_register_device(kbd)");
- goto error;
+ if (!no_kbd_dev) {
+ kbd = input_allocate_device();
+ if (!kbd)
+ goto error_nomem;
+ kbd->name = "Xen Virtual Keyboard";
+ kbd->phys = info->phys;
+ kbd->id.bustype = BUS_PCI;
+ kbd->id.vendor = 0x5853;
+ kbd->id.product = 0xffff;
+
+ __set_bit(EV_KEY, kbd->evbit);
+ for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
+ __set_bit(i, kbd->keybit);
+ for (i = KEY_OK; i < KEY_MAX; i++)
+ __set_bit(i, kbd->keybit);
+
+ ret = input_register_device(kbd);
+ if (ret) {
+ input_free_device(kbd);
+ xenbus_dev_fatal(dev, ret, "input_register_device(kbd)");
+ goto error;
+ }
+ info->kbd = kbd;
}
- info->kbd = kbd;
/* pointing device */
- ptr = input_allocate_device();
- if (!ptr)
- goto error_nomem;
- ptr->name = "Xen Virtual Pointer";
- ptr->phys = info->phys;
- ptr->id.bustype = BUS_PCI;
- ptr->id.vendor = 0x5853;
- ptr->id.product = 0xfffe;
-
- if (abs) {
- __set_bit(EV_ABS, ptr->evbit);
- input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
- input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
- } else {
- input_set_capability(ptr, EV_REL, REL_X);
- input_set_capability(ptr, EV_REL, REL_Y);
- }
- input_set_capability(ptr, EV_REL, REL_WHEEL);
+ if (!no_ptr_dev) {
+ unsigned int abs;
+
+ /* Set input abs params to match backend screen res */
+ abs = xenbus_read_unsigned(dev->otherend,
+ XENKBD_FIELD_FEAT_ABS_POINTER, 0);
+ ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend,
+ XENKBD_FIELD_WIDTH,
+ ptr_size[KPARAM_X]);
+ ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend,
+ XENKBD_FIELD_HEIGHT,
+ ptr_size[KPARAM_Y]);
+ if (abs) {
+ ret = xenbus_write(XBT_NIL, dev->nodename,
+ XENKBD_FIELD_REQ_ABS_POINTER, "1");
+ if (ret) {
+ pr_warn("xenkbd: can't request abs-pointer\n");
+ abs = 0;
+ }
+ }
- __set_bit(EV_KEY, ptr->evbit);
- for (i = BTN_LEFT; i <= BTN_TASK; i++)
- __set_bit(i, ptr->keybit);
+ ptr = input_allocate_device();
+ if (!ptr)
+ goto error_nomem;
+ ptr->name = "Xen Virtual Pointer";
+ ptr->phys = info->phys;
+ ptr->id.bustype = BUS_PCI;
+ ptr->id.vendor = 0x5853;
+ ptr->id.product = 0xfffe;
+
+ if (abs) {
+ __set_bit(EV_ABS, ptr->evbit);
+ input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
+ input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
+ } else {
+ input_set_capability(ptr, EV_REL, REL_X);
+ input_set_capability(ptr, EV_REL, REL_Y);
+ }
+ input_set_capability(ptr, EV_REL, REL_WHEEL);
- ret = input_register_device(ptr);
- if (ret) {
- input_free_device(ptr);
- xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
- goto error;
+ __set_bit(EV_KEY, ptr->evbit);
+ for (i = BTN_LEFT; i <= BTN_TASK; i++)
+ __set_bit(i, ptr->keybit);
+
+ ret = input_register_device(ptr);
+ if (ret) {
+ input_free_device(ptr);
+ xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
+ goto error;
+ }
+ info->ptr = ptr;
}
- info->ptr = ptr;
/* multi-touch device */
if (touch) {
--
2.17.0
On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> It is now only possible to control if multi-touch virtual device
> is created or not (via the corresponding XenStore entries),
> but keyboard and pointer devices are always created.
Why don't you want to go that route for keyboard and mouse, too?
Or does this really make no sense?
> In some cases this is not desirable. For example, if virtual
> keyboard device is exposed to Android then the latter won't
> automatically show on-screen keyboard as it expects that a
> physical keyboard device can be used for typing.
>
> Make it possible to configure which virtual devices are created
> with module parameters:
> - no_ptr_dev=1 if no pointer device needs to be created
> - no_kbd_dev=1 if no keyboard device needs to be created
> Keep old behavior by default.
>
> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
> Suggested-by: Andrii Chepurnyi <[email protected]>
> Tested-by: Andrii Chepurnyi <[email protected]>
> ---
> drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
> 1 file changed, 92 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index d91f3b1c5375..a3306aad40b0 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
> MODULE_PARM_DESC(ptr_size,
> "Pointing device width, height in pixels (default 800,600)");
>
> +static unsigned int no_ptr_dev;
> +module_param(no_ptr_dev, uint, 0);
Use type invbool instead?
> +MODULE_PARM_DESC(no_ptr_dev,
> + "If set then no virtual pointing device exposed to the guest");
> +
> +static unsigned int no_kbd_dev;
> +module_param(no_kbd_dev, uint, 0);
invbool?
Juergen
On 04/19/2018 02:25 PM, Juergen Gross wrote:
> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <[email protected]>
>>
>> It is now only possible to control if multi-touch virtual device
>> is created or not (via the corresponding XenStore entries),
>> but keyboard and pointer devices are always created.
> Why don't you want to go that route for keyboard and mouse, too?
> Or does this really make no sense?
Well, I would prefer not to touch anything outside Linux and
this driver. And these settings seem to be implementation specific.
So, this is why introduce Linux module parameters and don't extend
the kbdif protocol.
>> In some cases this is not desirable. For example, if virtual
>> keyboard device is exposed to Android then the latter won't
>> automatically show on-screen keyboard as it expects that a
>> physical keyboard device can be used for typing.
>>
>> Make it possible to configure which virtual devices are created
>> with module parameters:
>> - no_ptr_dev=1 if no pointer device needs to be created
>> - no_kbd_dev=1 if no keyboard device needs to be created
>> Keep old behavior by default.
>>
>> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>> Suggested-by: Andrii Chepurnyi <[email protected]>
>> Tested-by: Andrii Chepurnyi <[email protected]>
>> ---
>> drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>> 1 file changed, 92 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
>> index d91f3b1c5375..a3306aad40b0 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>> MODULE_PARM_DESC(ptr_size,
>> "Pointing device width, height in pixels (default 800,600)");
>>
>> +static unsigned int no_ptr_dev;
>> +module_param(no_ptr_dev, uint, 0);
> Use type invbool instead?
Hm, better bool then? invbool will require parameter name change to
something like "with_ptr_dev" which might confuse, e.g.
default was to go with pointer device, now we have with_ptr_dev
module parameter: do I now need to set it to preserve the old behavior?
The answer is no (because of invbool), but you have to dig for it.
Will bool work for you?
>> +MODULE_PARM_DESC(no_ptr_dev,
>> + "If set then no virtual pointing device exposed to the guest");
>> +
>> +static unsigned int no_kbd_dev;
>> +module_param(no_kbd_dev, uint, 0);
> invbool?
>
>
> Juergen
On 19/04/18 13:44, Oleksandr Andrushchenko wrote:
> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <[email protected]>
>>>
>>> It is now only possible to control if multi-touch virtual device
>>> is created or not (via the corresponding XenStore entries),
>>> but keyboard and pointer devices are always created.
>> Why don't you want to go that route for keyboard and mouse, too?
>> Or does this really make no sense?
> Well, I would prefer not to touch anything outside Linux and
> this driver. And these settings seem to be implementation specific.
> So, this is why introduce Linux module parameters and don't extend
> the kbdif protocol.
>>> In some cases this is not desirable. For example, if virtual
>>> keyboard device is exposed to Android then the latter won't
>>> automatically show on-screen keyboard as it expects that a
>>> physical keyboard device can be used for typing.
>>>
>>> Make it possible to configure which virtual devices are created
>>> with module parameters:
>>> - no_ptr_dev=1 if no pointer device needs to be created
>>> - no_kbd_dev=1 if no keyboard device needs to be created
>>> Keep old behavior by default.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <[email protected]>
>>> Suggested-by: Andrii Chepurnyi <[email protected]>
>>> Tested-by: Andrii Chepurnyi <[email protected]>
>>> ---
>>> drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>>> 1 file changed, 92 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>> b/drivers/input/misc/xen-kbdfront.c
>>> index d91f3b1c5375..a3306aad40b0 100644
>>> --- a/drivers/input/misc/xen-kbdfront.c
>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>>> MODULE_PARM_DESC(ptr_size,
>>> "Pointing device width, height in pixels (default 800,600)");
>>> +static unsigned int no_ptr_dev;
>>> +module_param(no_ptr_dev, uint, 0);
>> Use type invbool instead?
> Hm, better bool then? invbool will require parameter name change to
> something like "with_ptr_dev" which might confuse, e.g.
> default was to go with pointer device, now we have with_ptr_dev
> module parameter: do I now need to set it to preserve the old behavior?
> The answer is no (because of invbool), but you have to dig for it.
>
> Will bool work for you?
As long as the default won't change from today: yes.
Juergen
On 04/19/2018 03:52 PM, Juergen Gross wrote:
> On 19/04/18 13:44, Oleksandr Andrushchenko wrote:
>> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <[email protected]>
>>>>
>>>> It is now only possible to control if multi-touch virtual device
>>>> is created or not (via the corresponding XenStore entries),
>>>> but keyboard and pointer devices are always created.
>>> Why don't you want to go that route for keyboard and mouse, too?
>>> Or does this really make no sense?
>> Well, I would prefer not to touch anything outside Linux and
>> this driver. And these settings seem to be implementation specific.
>> So, this is why introduce Linux module parameters and don't extend
>> the kbdif protocol.
>>>> In some cases this is not desirable. For example, if virtual
>>>> keyboard device is exposed to Android then the latter won't
>>>> automatically show on-screen keyboard as it expects that a
>>>> physical keyboard device can be used for typing.
>>>>
>>>> Make it possible to configure which virtual devices are created
>>>> with module parameters:
>>>> - no_ptr_dev=1 if no pointer device needs to be created
>>>> - no_kbd_dev=1 if no keyboard device needs to be created
>>>> Keep old behavior by default.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko
>>>> <[email protected]>
>>>> Suggested-by: Andrii Chepurnyi <[email protected]>
>>>> Tested-by: Andrii Chepurnyi <[email protected]>
>>>> ---
>>>> drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>>>> 1 file changed, 92 insertions(+), 67 deletions(-)
>>>>
>>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>>> b/drivers/input/misc/xen-kbdfront.c
>>>> index d91f3b1c5375..a3306aad40b0 100644
>>>> --- a/drivers/input/misc/xen-kbdfront.c
>>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>>>> MODULE_PARM_DESC(ptr_size,
>>>> "Pointing device width, height in pixels (default 800,600)");
>>>> +static unsigned int no_ptr_dev;
>>>> +module_param(no_ptr_dev, uint, 0);
>>> Use type invbool instead?
>> Hm, better bool then? invbool will require parameter name change to
>> something like "with_ptr_dev" which might confuse, e.g.
>> default was to go with pointer device, now we have with_ptr_dev
>> module parameter: do I now need to set it to preserve the old behavior?
>> The answer is no (because of invbool), but you have to dig for it.
>>
>> Will bool work for you?
> As long as the default won't change from today: yes.
Ok, so I'll send v2 with the following changes:
diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
index a3306aad40b0..d8cca212f737 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
MODULE_PARM_DESC(ptr_size,
"Pointing device width, height in pixels (default 800,600)");
-static unsigned int no_ptr_dev;
-module_param(no_ptr_dev, uint, 0);
+static bool no_ptr_dev;
+module_param(no_ptr_dev, bool, 0);
MODULE_PARM_DESC(no_ptr_dev,
"If set then no virtual pointing device exposed to the guest");
-static unsigned int no_kbd_dev;
-module_param(no_kbd_dev, uint, 0);
+static bool no_kbd_dev;
+module_param(no_kbd_dev, bool, 0);
MODULE_PARM_DESC(no_kbd_dev,
"If set then no virtual keyboard device exposed to the guest");
>
> Juergen
On Thu, Apr 19, 2018 at 9:01 AM, Oleksandr Andrushchenko
<[email protected]> wrote:
>
> Ok, so I'll send v2 with the following changes:
>
> diff --git a/drivers/input/misc/xen-kbdfront.c
> b/drivers/input/misc/xen-kbdfront.c
> index a3306aad40b0..d8cca212f737 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
> MODULE_PARM_DESC(ptr_size,
> "Pointing device width, height in pixels (default 800,600)");
>
> -static unsigned int no_ptr_dev;
> -module_param(no_ptr_dev, uint, 0);
> +static bool no_ptr_dev;
> +module_param(no_ptr_dev, bool, 0);
> MODULE_PARM_DESC(no_ptr_dev,
> "If set then no virtual pointing device exposed to the guest");
>
> -static unsigned int no_kbd_dev;
> -module_param(no_kbd_dev, uint, 0);
> +static bool no_kbd_dev;
> +module_param(no_kbd_dev, bool, 0);
> MODULE_PARM_DESC(no_kbd_dev,
> "If set then no virtual keyboard device exposed to the guest");
I prefer direct logic over inverse logic. Maybe just use kbd_dev,
default to true, but allow it to be set off?
static bool kbd_dev = true;
module_param(kbd_dev, bool, 0);
Regards,
Jason
On 04/19/2018 04:10 PM, Jason Andryuk wrote:
> On Thu, Apr 19, 2018 at 9:01 AM, Oleksandr Andrushchenko
> <[email protected]> wrote:
>> Ok, so I'll send v2 with the following changes:
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c
>> b/drivers/input/misc/xen-kbdfront.c
>> index a3306aad40b0..d8cca212f737 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
>> MODULE_PARM_DESC(ptr_size,
>> "Pointing device width, height in pixels (default 800,600)");
>>
>> -static unsigned int no_ptr_dev;
>> -module_param(no_ptr_dev, uint, 0);
>> +static bool no_ptr_dev;
>> +module_param(no_ptr_dev, bool, 0);
>> MODULE_PARM_DESC(no_ptr_dev,
>> "If set then no virtual pointing device exposed to the guest");
>>
>> -static unsigned int no_kbd_dev;
>> -module_param(no_kbd_dev, uint, 0);
>> +static bool no_kbd_dev;
>> +module_param(no_kbd_dev, bool, 0);
>> MODULE_PARM_DESC(no_kbd_dev,
>> "If set then no virtual keyboard device exposed to the guest");
> I prefer direct logic over inverse logic. Maybe just use kbd_dev,
> default to true, but allow it to be set off?
>
> static bool kbd_dev = true;
> module_param(kbd_dev, bool, 0);
I have no preference here, either way works for me
Juergen, what do you think about the above?
> Regards,
> Jason
On 19/04/18 15:12, Oleksandr Andrushchenko wrote:
> On 04/19/2018 04:10 PM, Jason Andryuk wrote:
>> On Thu, Apr 19, 2018 at 9:01 AM, Oleksandr Andrushchenko
>> <[email protected]> wrote:
>>> Ok, so I'll send v2 with the following changes:
>>>
>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>> b/drivers/input/misc/xen-kbdfront.c
>>> index a3306aad40b0..d8cca212f737 100644
>>> --- a/drivers/input/misc/xen-kbdfront.c
>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>> @@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
>>> MODULE_PARM_DESC(ptr_size,
>>> "Pointing device width, height in pixels (default 800,600)");
>>>
>>> -static unsigned int no_ptr_dev;
>>> -module_param(no_ptr_dev, uint, 0);
>>> +static bool no_ptr_dev;
>>> +module_param(no_ptr_dev, bool, 0);
>>> MODULE_PARM_DESC(no_ptr_dev,
>>> "If set then no virtual pointing device exposed to the guest");
>>>
>>> -static unsigned int no_kbd_dev;
>>> -module_param(no_kbd_dev, uint, 0);
>>> +static bool no_kbd_dev;
>>> +module_param(no_kbd_dev, bool, 0);
>>> MODULE_PARM_DESC(no_kbd_dev,
>>> "If set then no virtual keyboard device exposed to the guest");
>> I prefer direct logic over inverse logic. Maybe just use kbd_dev,
>> default to true, but allow it to be set off?
>>
>> static bool kbd_dev = true;
>> module_param(kbd_dev, bool, 0);
> I have no preference here, either way works for me
> Juergen, what do you think about the above?
I really have no preference here. What should be taken into account is
that boolean parameters don't need a value, meaning "true" in that case.
This would make no sense for "kbd_dev" as it wouldn't change the
default.
Juergen
On 04/19/2018 04:19 PM, Juergen Gross wrote:
> On 19/04/18 15:12, Oleksandr Andrushchenko wrote:
>> On 04/19/2018 04:10 PM, Jason Andryuk wrote:
>>> On Thu, Apr 19, 2018 at 9:01 AM, Oleksandr Andrushchenko
>>> <[email protected]> wrote:
>>>> Ok, so I'll send v2 with the following changes:
>>>>
>>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>>> b/drivers/input/misc/xen-kbdfront.c
>>>> index a3306aad40b0..d8cca212f737 100644
>>>> --- a/drivers/input/misc/xen-kbdfront.c
>>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>>> @@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
>>>> MODULE_PARM_DESC(ptr_size,
>>>> "Pointing device width, height in pixels (default 800,600)");
>>>>
>>>> -static unsigned int no_ptr_dev;
>>>> -module_param(no_ptr_dev, uint, 0);
>>>> +static bool no_ptr_dev;
>>>> +module_param(no_ptr_dev, bool, 0);
>>>> MODULE_PARM_DESC(no_ptr_dev,
>>>> "If set then no virtual pointing device exposed to the guest");
>>>>
>>>> -static unsigned int no_kbd_dev;
>>>> -module_param(no_kbd_dev, uint, 0);
>>>> +static bool no_kbd_dev;
>>>> +module_param(no_kbd_dev, bool, 0);
>>>> MODULE_PARM_DESC(no_kbd_dev,
>>>> "If set then no virtual keyboard device exposed to the guest");
>>> I prefer direct logic over inverse logic. Maybe just use kbd_dev,
>>> default to true, but allow it to be set off?
>>>
>>> static bool kbd_dev = true;
>>> module_param(kbd_dev, bool, 0);
>> I have no preference here, either way works for me
>> Juergen, what do you think about the above?
> I really have no preference here. What should be taken into account is
> that boolean parameters don't need a value, meaning "true" in that case.
> This would make no sense for "kbd_dev" as it wouldn't change the
> default.
Then I'll go with the diff above, e.g. boolean no_{kbd|ptr})dev
> Juergen
On Thu, Apr 19, 2018 at 02:44:19PM +0300, Oleksandr Andrushchenko wrote:
> On 04/19/2018 02:25 PM, Juergen Gross wrote:
> > On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
> > > From: Oleksandr Andrushchenko <[email protected]>
> > >
> > > It is now only possible to control if multi-touch virtual device
> > > is created or not (via the corresponding XenStore entries),
> > > but keyboard and pointer devices are always created.
> > Why don't you want to go that route for keyboard and mouse, too?
> > Or does this really make no sense?
> Well, I would prefer not to touch anything outside Linux and
> this driver. And these settings seem to be implementation specific.
> So, this is why introduce Linux module parameters and don't extend
> the kbdif protocol.
Why do you consider this implementation specific? How other guests
decide to forego creation of relative pointer device or keyboard-like
device?
You already have "features" for absolute pointing device and multitouch,
so please extend the protocol properly so you indeed do not code
something implementation-specific (i.e. module parameters).
Thanks.
--
Dmitry
On 04/23/2018 09:53 PM, Dmitry Torokhov wrote:
> On Thu, Apr 19, 2018 at 02:44:19PM +0300, Oleksandr Andrushchenko wrote:
>> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <[email protected]>
>>>>
>>>> It is now only possible to control if multi-touch virtual device
>>>> is created or not (via the corresponding XenStore entries),
>>>> but keyboard and pointer devices are always created.
>>> Why don't you want to go that route for keyboard and mouse, too?
>>> Or does this really make no sense?
>> Well, I would prefer not to touch anything outside Linux and
>> this driver. And these settings seem to be implementation specific.
>> So, this is why introduce Linux module parameters and don't extend
>> the kbdif protocol.
> Why do you consider this implementation specific? How other guests
> decide to forego creation of relative pointer device or keyboard-like
> device?
>
> You already have "features" for absolute pointing device and multitouch,
> so please extend the protocol properly so you indeed do not code
> something implementation-specific (i.e. module parameters).
Ok, but in order to preserve the default behavior, e.g.
pointer and keyboard devices are always created now, I'll have
to have reverse features in the protocol:
- feature-no-pointer
- feature-no-keyboard
The above may be set as a part of frontend's configuration and
if missed are considered to be set to false.
>
> Thanks.
>
On Tue, Apr 24, 2018 at 08:55:19AM +0300, Oleksandr Andrushchenko wrote:
> On 04/23/2018 09:53 PM, Dmitry Torokhov wrote:
> > On Thu, Apr 19, 2018 at 02:44:19PM +0300, Oleksandr Andrushchenko wrote:
> > > On 04/19/2018 02:25 PM, Juergen Gross wrote:
> > > > On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
> > > > > From: Oleksandr Andrushchenko <[email protected]>
> > > > >
> > > > > It is now only possible to control if multi-touch virtual device
> > > > > is created or not (via the corresponding XenStore entries),
> > > > > but keyboard and pointer devices are always created.
> > > > Why don't you want to go that route for keyboard and mouse, too?
> > > > Or does this really make no sense?
> > > Well, I would prefer not to touch anything outside Linux and
> > > this driver. And these settings seem to be implementation specific.
> > > So, this is why introduce Linux module parameters and don't extend
> > > the kbdif protocol.
> > Why do you consider this implementation specific? How other guests
> > decide to forego creation of relative pointer device or keyboard-like
> > device?
> >
> > You already have "features" for absolute pointing device and multitouch,
> > so please extend the protocol properly so you indeed do not code
> > something implementation-specific (i.e. module parameters).
> Ok, but in order to preserve the default behavior, e.g.
> pointer and keyboard devices are always created now, I'll have
> to have reverse features in the protocol:
> ?- feature-no-pointer
> ?- feature-no-keyboard
> The above may be set as a part of frontend's configuration and
> if missed are considered to be set to false.
I think you can have them as "feature-pointer" and "feature-keyboard"
(no negation), but assume not present considered enabled. I.e.
kbd = xenbus_read_unsigned(..., XENKBD_FIELD_FEAT_KEYBOARD, 1);
if (kbd) {
...
}
Thanks.
--
Dmitry
On 04/26/2018 10:16 PM, Dmitry Torokhov wrote:
> On Tue, Apr 24, 2018 at 08:55:19AM +0300, Oleksandr Andrushchenko wrote:
>> On 04/23/2018 09:53 PM, Dmitry Torokhov wrote:
>>> On Thu, Apr 19, 2018 at 02:44:19PM +0300, Oleksandr Andrushchenko wrote:
>>>> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>>>>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>>>>> From: Oleksandr Andrushchenko <[email protected]>
>>>>>>
>>>>>> It is now only possible to control if multi-touch virtual device
>>>>>> is created or not (via the corresponding XenStore entries),
>>>>>> but keyboard and pointer devices are always created.
>>>>> Why don't you want to go that route for keyboard and mouse, too?
>>>>> Or does this really make no sense?
>>>> Well, I would prefer not to touch anything outside Linux and
>>>> this driver. And these settings seem to be implementation specific.
>>>> So, this is why introduce Linux module parameters and don't extend
>>>> the kbdif protocol.
>>> Why do you consider this implementation specific? How other guests
>>> decide to forego creation of relative pointer device or keyboard-like
>>> device?
>>>
>>> You already have "features" for absolute pointing device and multitouch,
>>> so please extend the protocol properly so you indeed do not code
>>> something implementation-specific (i.e. module parameters).
>> Ok, but in order to preserve the default behavior, e.g.
>> pointer and keyboard devices are always created now, I'll have
>> to have reverse features in the protocol:
>> - feature-no-pointer
>> - feature-no-keyboard
>> The above may be set as a part of frontend's configuration and
>> if missed are considered to be set to false.
> I think you can have them as "feature-pointer" and "feature-keyboard"
> (no negation), but assume not present considered enabled. I.e.
>
> kbd = xenbus_read_unsigned(..., XENKBD_FIELD_FEAT_KEYBOARD, 1);
> if (kbd) {
> ...
Thank you for your comments,
could you please take a look at the patch [1] where I am trying
to change the corresponding Xen protocol to fit the requirements?
As we agreed I have to change the protocol first, so this patch is no
longer valid
> }
>
> Thanks.
Thank you,
Oleksandr
[1] https://www.spinics.net/lists/linux-input/msg56094.html