| Message ID | 20240129223545.219878-1-jason.gerecke@wacom.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: wacom: Do not register input devices until after hid_hw_start | expand |
On Mon, Jan 29, 2024 at 02:35:45PM -0800, Gerecke, Jason wrote: > From: Jason Gerecke <killertofu@gmail.com> > > If a input device is opened before hid_hw_start is called, events may > not be received from the hardware. In the case of USB-backed devices, > for example, the hid_hw_start function is responsible for filling in > the URB which is submitted when the input device is opened. If a device > is opened prematurely, polling will never start because the device will > not have been in the correct state to send the URB. > > Because the wacom driver registers its input devices before calling > hid_hw_start, there is a window of time where a device can be opened > and end up in an inoperable state. Some ARM-based Chromebooks in particular > reliably trigger this bug. > > This commit splits the wacom_register_inputs function into two pieces. > One which is responsible for setting up the allocated inputs (and runs > prior to hid_hw_start so that devices are ready for any input events > they may end up receiving) and another which only registers the devices > (and runs after hid_hw_start to ensure devices can be immediately opened > without issue). Note that the functions to initialize the LEDs and remotes > are also moved after hid_hw_start to maintain their own dependency chains. > > Fixes: 7704ac937345 ("HID: wacom: implement generic HID handling for pen generic devices") > Cc: stable@vger.kernel.org # v3.18+ > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> Tested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks.
On Mon, 29 Jan 2024, Dmitry Torokhov wrote: > > From: Jason Gerecke <killertofu@gmail.com> > > > > If a input device is opened before hid_hw_start is called, events may > > not be received from the hardware. In the case of USB-backed devices, > > for example, the hid_hw_start function is responsible for filling in > > the URB which is submitted when the input device is opened. If a device > > is opened prematurely, polling will never start because the device will > > not have been in the correct state to send the URB. > > > > Because the wacom driver registers its input devices before calling > > hid_hw_start, there is a window of time where a device can be opened > > and end up in an inoperable state. Some ARM-based Chromebooks in particular > > reliably trigger this bug. > > > > This commit splits the wacom_register_inputs function into two pieces. > > One which is responsible for setting up the allocated inputs (and runs > > prior to hid_hw_start so that devices are ready for any input events > > they may end up receiving) and another which only registers the devices > > (and runs after hid_hw_start to ensure devices can be immediately opened > > without issue). Note that the functions to initialize the LEDs and remotes > > are also moved after hid_hw_start to maintain their own dependency chains. > > > > Fixes: 7704ac937345 ("HID: wacom: implement generic HID handling for pen generic devices") > > Cc: stable@vger.kernel.org # v3.18+ > > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > > Tested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Applied, thanks!
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index b613f11ed9498..2bc45b24075c3 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -2087,7 +2087,7 @@ static int wacom_allocate_inputs(struct wacom *wacom) return 0; } -static int wacom_register_inputs(struct wacom *wacom) +static int wacom_setup_inputs(struct wacom *wacom) { struct input_dev *pen_input_dev, *touch_input_dev, *pad_input_dev; struct wacom_wac *wacom_wac = &(wacom->wacom_wac); @@ -2106,10 +2106,6 @@ static int wacom_register_inputs(struct wacom *wacom) input_free_device(pen_input_dev); wacom_wac->pen_input = NULL; pen_input_dev = NULL; - } else { - error = input_register_device(pen_input_dev); - if (error) - goto fail; } error = wacom_setup_touch_input_capabilities(touch_input_dev, wacom_wac); @@ -2118,10 +2114,6 @@ static int wacom_register_inputs(struct wacom *wacom) input_free_device(touch_input_dev); wacom_wac->touch_input = NULL; touch_input_dev = NULL; - } else { - error = input_register_device(touch_input_dev); - if (error) - goto fail; } error = wacom_setup_pad_input_capabilities(pad_input_dev, wacom_wac); @@ -2130,7 +2122,34 @@ static int wacom_register_inputs(struct wacom *wacom) input_free_device(pad_input_dev); wacom_wac->pad_input = NULL; pad_input_dev = NULL; - } else { + } + + return 0; +} + +static int wacom_register_inputs(struct wacom *wacom) +{ + struct input_dev *pen_input_dev, *touch_input_dev, *pad_input_dev; + struct wacom_wac *wacom_wac = &(wacom->wacom_wac); + int error = 0; + + pen_input_dev = wacom_wac->pen_input; + touch_input_dev = wacom_wac->touch_input; + pad_input_dev = wacom_wac->pad_input; + + if (pen_input_dev) { + error = input_register_device(pen_input_dev); + if (error) + goto fail; + } + + if (touch_input_dev) { + error = input_register_device(touch_input_dev); + if (error) + goto fail; + } + + if (pad_input_dev) { error = input_register_device(pad_input_dev); if (error) goto fail; @@ -2383,6 +2402,20 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless) if (error) goto fail; + error = wacom_setup_inputs(wacom); + if (error) + goto fail; + + if (features->type == HID_GENERIC) + connect_mask |= HID_CONNECT_DRIVER; + + /* Regular HID work starts now */ + error = hid_hw_start(hdev, connect_mask); + if (error) { + hid_err(hdev, "hw start failed\n"); + goto fail; + } + error = wacom_register_inputs(wacom); if (error) goto fail; @@ -2397,16 +2430,6 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless) goto fail; } - if (features->type == HID_GENERIC) - connect_mask |= HID_CONNECT_DRIVER; - - /* Regular HID work starts now */ - error = hid_hw_start(hdev, connect_mask); - if (error) { - hid_err(hdev, "hw start failed\n"); - goto fail; - } - if (!wireless) { /* Note that if query fails it is not a hard failure */ wacom_query_tablet_data(wacom);