| Message ID | 20180514144029.16019-2-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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();