[PATCH v3 1/2] xen/kbdif: Sync up with the canonical definition in Xen

[PATCH v3 1/2] xen/kbdif: Sync up with the canonical definition in Xen

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

This is the sync up with the canonical definition of the keyboard
protocol in Xen:
1. Add missing string constants for {feature|request}-raw-pointer
   to align with the rest of the interface file.

2. Add new XenStore feature fields, so it is possible to individually
   control set of exposed virtual devices for each guest OS:
     - set feature-disable-keyboard to 1 if no keyboard device needs
       to be created
     - set feature-disable-pointer to 1 if no pointer device needs
       to be created

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 include/xen/interface/io/kbdif.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/xen/interface/io/kbdif.h b/include/xen/interface/io/kbdif.h
index 2a9510ade701..808ef7d02a65 100644
--- a/include/xen/interface/io/kbdif.h
+++ b/include/xen/interface/io/kbdif.h
@@ -51,6 +51,18 @@
  * corresponding entries in XenStore and puts 1 as the value of the entry.
  * If a feature is not supported then 0 must be set or feature entry omitted.
  *
+ * feature-disable-keyboard
+ *      Values:         <uint>
+ *
+ *      If there is no need to expose a virtual keyboard device by the
+ *      frontend then this must be set to 1.
+ *
+ * feature-disable-pointer
+ *      Values:         <uint>
+ *
+ *      If there is no need to expose a virtual pointer device by the
+ *      frontend then this must be set to 1.
+ *
  * feature-abs-pointer
  *      Values:         <uint>
  *
@@ -63,6 +75,13 @@
  *      Backends, which support reporting of multi-touch events
  *      should set this to 1.
  *
+ * feature-raw-pointer
+ *      Values:        <uint>
+ *
+ *      Backends, which support reporting raw (unscaled) absolute coordinates
+ *      for pointer devices should set this to 1. Raw (unscaled) values have
+ *      a range of [0, 0x7fff].
+ *
  *------------------------- Pointer Device Parameters ------------------------
  *
  * width
@@ -98,6 +117,13 @@
  *
  *      Request backend to report multi-touch events.
  *
+ * request-raw-pointer
+ *      Values:         <uint>
+ *
+ *      Request backend to report raw unscaled absolute pointer coordinates.
+ *      This option is only valid if request-abs-pointer is also set.
+ *      Raw unscaled coordinates have the range [0, 0x7fff]
+ *
  *----------------------- Request Transport Parameters -----------------------
  *
  * event-channel
@@ -163,9 +189,13 @@
 
 #define XENKBD_DRIVER_NAME		"vkbd"
 
+#define XENKBD_FIELD_FEAT_DSBL_KEYBRD	"feature-disable-keyboard"
+#define XENKBD_FIELD_FEAT_DSBL_POINTER	"feature-disable-pointer"
 #define XENKBD_FIELD_FEAT_ABS_POINTER	"feature-abs-pointer"
+#define XENKBD_FIELD_FEAT_RAW_POINTER	"feature-raw-pointer"
 #define XENKBD_FIELD_FEAT_MTOUCH	"feature-multi-touch"
 #define XENKBD_FIELD_REQ_ABS_POINTER	"request-abs-pointer"
+#define XENKBD_FIELD_REQ_RAW_POINTER	"request-raw-pointer"
 #define XENKBD_FIELD_REQ_MTOUCH		"request-multi-touch"
 #define XENKBD_FIELD_RING_GREF		"page-gref"
 #define XENKBD_FIELD_EVT_CHANNEL	"event-channel"
-- 
2.17.0

[PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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.

Utilize keyboard and pointer device XenStore feature fields to
configure which virtual devices are created:
 - set "feature-disable-keyboard" to 1 if no keyboard device
   needs to be created
 - set "feature-disable-pointer" to 1 if no pointer device
   needs to be created
Keep old behavior by default.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
---

Changes since v2:
- based on XenStore kbdif features to control which devices are
  exposed instead of module parameters.

 drivers/input/misc/xen-kbdfront.c | 172 ++++++++++++++++++------------
 1 file changed, 101 insertions(+), 71 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index d91f3b1c5375..0f166e11c421 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -63,6 +63,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 +76,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 +103,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 +201,7 @@ static int xenkbd_probe(struct xenbus_device *dev,
 				  const struct xenbus_device_id *id)
 {
 	int ret, i;
-	unsigned int abs, touch;
+	bool with_mtouch, with_kbd, with_ptr;
 	struct xenkbd_info *info;
 	struct input_dev *kbd, *ptr, *mtouch;
 
@@ -211,93 +220,114 @@ 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;
-		}
-	}
+	/*
+	 * The below are reverse logic, e.g. if the feature is set, then
+	 * do not expose the corresponding virtual device.
+	 */
+	with_kbd = !xenbus_read_unsigned(dev->nodename,
+					 XENKBD_FIELD_FEAT_DSBL_KEYBRD, 0);
 
-	touch = xenbus_read_unsigned(dev->nodename,
-				     XENKBD_FIELD_FEAT_MTOUCH, 0);
-	if (touch) {
+	with_ptr = !xenbus_read_unsigned(dev->nodename,
+					 XENKBD_FIELD_FEAT_DSBL_POINTER, 0);
+
+	/* Direct logic: if set, then create multi-touch device. */
+	with_mtouch = xenbus_read_unsigned(dev->nodename,
+					   XENKBD_FIELD_FEAT_MTOUCH, 0);
+	if (with_mtouch) {
 		ret = xenbus_write(XBT_NIL, dev->nodename,
 				   XENKBD_FIELD_REQ_MTOUCH, "1");
 		if (ret) {
 			pr_warn("xenkbd: can't request multi-touch");
-			touch = 0;
+			with_mtouch = 0;
 		}
 	}
 
 	/* 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 (with_kbd) {
+		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 (with_ptr) {
+		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) {
+	if (with_mtouch) {
 		int num_cont, width, height;
 
 		mtouch = input_allocate_device();
-- 
2.17.0

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Dmitry Torokhov]

Hi Oleksandr,

On Mon, May 14, 2018 at 05:40:29PM +0300, Oleksandr Andrushchenko wrote:
> @@ -211,93 +220,114 @@ 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;
> -		}
> -	}
> +	/*
> +	 * The below are reverse logic, e.g. if the feature is set, then
> +	 * do not expose the corresponding virtual device.
> +	 */
> +	with_kbd = !xenbus_read_unsigned(dev->nodename,
> +					 XENKBD_FIELD_FEAT_DSBL_KEYBRD, 0);
>  
> -	touch = xenbus_read_unsigned(dev->nodename,
> -				     XENKBD_FIELD_FEAT_MTOUCH, 0);
> -	if (touch) {
> +	with_ptr = !xenbus_read_unsigned(dev->nodename,
> +					 XENKBD_FIELD_FEAT_DSBL_POINTER, 0);
> +
> +	/* Direct logic: if set, then create multi-touch device. */
> +	with_mtouch = xenbus_read_unsigned(dev->nodename,
> +					   XENKBD_FIELD_FEAT_MTOUCH, 0);
> +	if (with_mtouch) {
>  		ret = xenbus_write(XBT_NIL, dev->nodename,
>  				   XENKBD_FIELD_REQ_MTOUCH, "1");
>  		if (ret) {
>  			pr_warn("xenkbd: can't request multi-touch");
> -			touch = 0;
> +			with_mtouch = 0;
>  		}
>  	}

Does it make sense to still end up calling xenkbd_connect_backend() when
all interfaces (keyboard, pointer, and multitouch) are disabled? Should
we do:

	if (!(with_kbd || || with_ptr || with_mtouch))
		return -ENXIO;

?

Thanks.

-- 
Dmitry

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Oleksandr Andrushchenko]

On 05/16/2018 08:15 PM, Dmitry Torokhov wrote:
> Hi Oleksandr,
>
> On Mon, May 14, 2018 at 05:40:29PM +0300, Oleksandr Andrushchenko wrote:
>> @@ -211,93 +220,114 @@ 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;
>> -		}
>> -	}
>> +	/*
>> +	 * The below are reverse logic, e.g. if the feature is set, then
>> +	 * do not expose the corresponding virtual device.
>> +	 */
>> +	with_kbd = !xenbus_read_unsigned(dev->nodename,
>> +					 XENKBD_FIELD_FEAT_DSBL_KEYBRD, 0);
>>   
>> -	touch = xenbus_read_unsigned(dev->nodename,
>> -				     XENKBD_FIELD_FEAT_MTOUCH, 0);
>> -	if (touch) {
>> +	with_ptr = !xenbus_read_unsigned(dev->nodename,
>> +					 XENKBD_FIELD_FEAT_DSBL_POINTER, 0);
>> +
>> +	/* Direct logic: if set, then create multi-touch device. */
>> +	with_mtouch = xenbus_read_unsigned(dev->nodename,
>> +					   XENKBD_FIELD_FEAT_MTOUCH, 0);
>> +	if (with_mtouch) {
>>   		ret = xenbus_write(XBT_NIL, dev->nodename,
>>   				   XENKBD_FIELD_REQ_MTOUCH, "1");
>>   		if (ret) {
>>   			pr_warn("xenkbd: can't request multi-touch");
>> -			touch = 0;
>> +			with_mtouch = 0;
>>   		}
>>   	}
> Does it make sense to still end up calling xenkbd_connect_backend() when
> all interfaces (keyboard, pointer, and multitouch) are disabled? Should
> we do:
>
> 	if (!(with_kbd || || with_ptr || with_mtouch))
> 		return -ENXIO;
>
> ?
It does make sense. Then we probably need to move all xenbus_read_unsigned
calls to the very beginning of the .probe, so no memory allocations are made
which will be useless if we return -ENXIO, e.g. something like

static int xenkbd_probe(struct xenbus_device *dev,
                   const struct xenbus_device_id *id)
{
     int ret, i;
     bool with_mtouch, with_kbd, with_ptr;
     struct xenkbd_info *info;
     struct input_dev *kbd, *ptr, *mtouch;

<read with_mtouch, with_kbd, with_ptr here>

if (!(with_kbd | with_ptr | with_mtouch))
         return -ENXIO;

Does the above looks ok?
> Thanks.
>
Thank you,
Oleksandr

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Dmitry Torokhov]

On Wed, May 16, 2018 at 08:47:30PM +0300, Oleksandr Andrushchenko wrote:
> On 05/16/2018 08:15 PM, Dmitry Torokhov wrote:
> > Hi Oleksandr,
> > 
> > On Mon, May 14, 2018 at 05:40:29PM +0300, Oleksandr Andrushchenko wrote:
> > > @@ -211,93 +220,114 @@ 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;
> > > -		}
> > > -	}
> > > +	/*
> > > +	 * The below are reverse logic, e.g. if the feature is set, then
> > > +	 * do not expose the corresponding virtual device.
> > > +	 */
> > > +	with_kbd = !xenbus_read_unsigned(dev->nodename,
> > > +					 XENKBD_FIELD_FEAT_DSBL_KEYBRD, 0);
> > > -	touch = xenbus_read_unsigned(dev->nodename,
> > > -				     XENKBD_FIELD_FEAT_MTOUCH, 0);
> > > -	if (touch) {
> > > +	with_ptr = !xenbus_read_unsigned(dev->nodename,
> > > +					 XENKBD_FIELD_FEAT_DSBL_POINTER, 0);
> > > +
> > > +	/* Direct logic: if set, then create multi-touch device. */
> > > +	with_mtouch = xenbus_read_unsigned(dev->nodename,
> > > +					   XENKBD_FIELD_FEAT_MTOUCH, 0);
> > > +	if (with_mtouch) {
> > >   		ret = xenbus_write(XBT_NIL, dev->nodename,
> > >   				   XENKBD_FIELD_REQ_MTOUCH, "1");
> > >   		if (ret) {
> > >   			pr_warn("xenkbd: can't request multi-touch");
> > > -			touch = 0;
> > > +			with_mtouch = 0;
> > >   		}
> > >   	}
> > Does it make sense to still end up calling xenkbd_connect_backend() when
> > all interfaces (keyboard, pointer, and multitouch) are disabled? Should
> > we do:
> > 
> > 	if (!(with_kbd || || with_ptr || with_mtouch))
> > 		return -ENXIO;
> > 
> > ?
> It does make sense. Then we probably need to move all xenbus_read_unsigned
> calls to the very beginning of the .probe, so no memory allocations are made
> which will be useless if we return -ENXIO, e.g. something like
> 
> static int xenkbd_probe(struct xenbus_device *dev,
>                   const struct xenbus_device_id *id)
> {
>     int ret, i;
>     bool with_mtouch, with_kbd, with_ptr;
>     struct xenkbd_info *info;
>     struct input_dev *kbd, *ptr, *mtouch;
> 
> <read with_mtouch, with_kbd, with_ptr here>
> 
> if (!(with_kbd | with_ptr | with_mtouch))
>         return -ENXIO;
> 
> Does the above looks ok?

Yes. Another option is to keep the check where I suggested and do

	if (...) {
		ret = -ENXIO;
		goto error;
	}

Whichever you prefer is fine with me.

Thanks.

-- 
Dmitry

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Oleksandr Andrushchenko]

On 05/17/2018 12:08 AM, Dmitry Torokhov wrote:
> On Wed, May 16, 2018 at 08:47:30PM +0300, Oleksandr Andrushchenko wrote:
>> On 05/16/2018 08:15 PM, Dmitry Torokhov wrote:
>>> Hi Oleksandr,
>>>
>>> On Mon, May 14, 2018 at 05:40:29PM +0300, Oleksandr Andrushchenko wrote:
>>>> @@ -211,93 +220,114 @@ 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;
>>>> -		}
>>>> -	}
>>>> +	/*
>>>> +	 * The below are reverse logic, e.g. if the feature is set, then
>>>> +	 * do not expose the corresponding virtual device.
>>>> +	 */
>>>> +	with_kbd = !xenbus_read_unsigned(dev->nodename,
>>>> +					 XENKBD_FIELD_FEAT_DSBL_KEYBRD, 0);
>>>> -	touch = xenbus_read_unsigned(dev->nodename,
>>>> -				     XENKBD_FIELD_FEAT_MTOUCH, 0);
>>>> -	if (touch) {
>>>> +	with_ptr = !xenbus_read_unsigned(dev->nodename,
>>>> +					 XENKBD_FIELD_FEAT_DSBL_POINTER, 0);
>>>> +
>>>> +	/* Direct logic: if set, then create multi-touch device. */
>>>> +	with_mtouch = xenbus_read_unsigned(dev->nodename,
>>>> +					   XENKBD_FIELD_FEAT_MTOUCH, 0);
>>>> +	if (with_mtouch) {
>>>>    		ret = xenbus_write(XBT_NIL, dev->nodename,
>>>>    				   XENKBD_FIELD_REQ_MTOUCH, "1");
>>>>    		if (ret) {
>>>>    			pr_warn("xenkbd: can't request multi-touch");
>>>> -			touch = 0;
>>>> +			with_mtouch = 0;
>>>>    		}
>>>>    	}
>>> Does it make sense to still end up calling xenkbd_connect_backend() when
>>> all interfaces (keyboard, pointer, and multitouch) are disabled? Should
>>> we do:
>>>
>>> 	if (!(with_kbd || || with_ptr || with_mtouch))
>>> 		return -ENXIO;
>>>
>>> ?
>> It does make sense. Then we probably need to move all xenbus_read_unsigned
>> calls to the very beginning of the .probe, so no memory allocations are made
>> which will be useless if we return -ENXIO, e.g. something like
>>
>> static int xenkbd_probe(struct xenbus_device *dev,
>>                    const struct xenbus_device_id *id)
>> {
>>      int ret, i;
>>      bool with_mtouch, with_kbd, with_ptr;
>>      struct xenkbd_info *info;
>>      struct input_dev *kbd, *ptr, *mtouch;
>>
>> <read with_mtouch, with_kbd, with_ptr here>
>>
>> if (!(with_kbd | with_ptr | with_mtouch))
>>          return -ENXIO;
>>
>> Does the above looks ok?
> Yes. Another option is to keep the check where I suggested and do
>
> 	if (...) {
> 		ret = -ENXIO;
> 		goto error;
> 	}
>
> Whichever you prefer is fine with me.
I will go with the change you suggested and
I'll send v4 tomorrow then.
> Thanks.
>
Thank you,
Oleksandr

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Boris Ostrovsky]

On 05/17/2018 01:31 AM, Oleksandr Andrushchenko wrote:
> I will go with the change you suggested and
> I'll send v4 tomorrow then.



Please make sure your changes to kbdif.h are in Xen first. I believe you
submitted a patch there but I don't see it in the staging tree yet.

-boris

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Oleksandr Andrushchenko]

On 05/17/2018 04:08 PM, Boris Ostrovsky wrote:
> On 05/17/2018 01:31 AM, Oleksandr Andrushchenko wrote:
>> I will go with the change you suggested and
>> I'll send v4 tomorrow then.
>
>
> Please make sure your changes to kbdif.h are in Xen first. I believe you
> submitted a patch there but I don't see it in the staging tree yet.
Sure, I already have Release ack for the one which is not
in Xen tree yet, hope Konrad can apply it today,
so tomorrow Xen and Linux are both ok (or by the time
I send v4).
> -boris
Thank you,
Oleksandr

Re: [PATCH v3 2/2] Input: xen-kbdfront - allow better run-time configuration

[Konrad Rzeszutek Wilk]

On Thu, May 17, 2018 at 04:09:04PM +0300, Oleksandr Andrushchenko wrote:
> On 05/17/2018 04:08 PM, Boris Ostrovsky wrote:
> > On 05/17/2018 01:31 AM, Oleksandr Andrushchenko wrote:
> > > I will go with the change you suggested and
> > > I'll send v4 tomorrow then.
> > 
> > 
> > Please make sure your changes to kbdif.h are in Xen first. I believe you
> > submitted a patch there but I don't see it in the staging tree yet.
> Sure, I already have Release ack for the one which is not
> in Xen tree yet, hope Konrad can apply it today,
> so tomorrow Xen and Linux are both ok (or by the time

It is checked in (on the Xen tree).
> I send v4).
> > -boris
> Thank you,
> Oleksandr