| Message ID | 20170117143547.30488-14-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 17, 2017 at 3:35 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > The current custom solution for the G920 is not the best because > hid_hw_start() is not called at the end of the .probe(). > It means that any configuration retrieved after the initial hid_hw_start > would not be exposed to user space without races. > > We can simply force hid_hw_start to just enable the transport layer by > not using a connect_mask. This way, we can have a common path between > USB, Unifying and Bluetooth devices. > > Tested with a G502 (plain USB), a T650 and many other unifying devices, > and the T651 over Bluetooth. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------ > 1 file changed, 48 insertions(+), 40 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index a5d37a4..f5889ff 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > if (!hidpp_validate_device(hdev)) > return hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > + /* > + * HID++ needs to read incoming report while in .probe(). > + * However, this doesn't work for plain USB devices until the hdev > + * status is set with HID_STAT_ADDED (device fully registered once > + * with HID). > + * So we ask for it to be reprobed later. > + */ > + if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE && > + !(hdev->status & HID_STAT_ADDED)) Looks like this test breaks the T651 (bluetooth) after all. I seem to have better success with: if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED)) But that also means that the solution will not work if there is only one USB interface in the device :/ Cheers, Benjamin > + return -EPROBE_DEFER; > + > hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device), > GFP_KERNEL); > if (!hidpp) > @@ -2853,24 +2864,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > hid_warn(hdev, "Cannot allocate sysfs group for %s\n", > hdev->name); > > - if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) > - connect_mask &= ~HID_CONNECT_HIDINPUT; > - > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { > - ret = hid_hw_start(hdev, connect_mask); > - if (ret) { > - hid_err(hdev, "hw start failed\n"); > - goto hid_hw_start_fail; > - } > - ret = hid_hw_open(hdev); > - if (ret < 0) { > - dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n", > - __func__, ret); > - hid_hw_stop(hdev); > - goto hid_hw_start_fail; > - } > + /* > + * Plain USB connections need to actually call start and open > + * on the transport driver to allow incoming data. > + */ > + ret = hid_hw_start(hdev, 0); > + if (ret) { > + hid_err(hdev, "hw start failed\n"); > + goto hid_hw_start_fail; > } > > + ret = hid_hw_open(hdev); > + if (ret < 0) { > + dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n", > + __func__, ret); > + hid_hw_stop(hdev); > + goto hid_hw_open_fail; > + } > > /* Allow incoming packets */ > hid_device_io_start(hdev); > @@ -2880,7 +2890,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > if (!connected) { > ret = -ENODEV; > hid_err(hdev, "Device not connected"); > - goto hid_hw_open_failed; > + goto hid_hw_init_fail; > } > > hid_info(hdev, "HID++ %u.%u device connected.\n", > @@ -2893,37 +2903,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { > ret = wtp_get_config(hidpp); > if (ret) > - goto hid_hw_open_failed; > + goto hid_hw_init_fail; > } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { > ret = g920_get_config(hidpp); > if (ret) > - goto hid_hw_open_failed; > + goto hid_hw_init_fail; > } > > - /* Block incoming packets */ > - hid_device_io_stop(hdev); > + hidpp_connect_event(hidpp); > > - if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { > - ret = hid_hw_start(hdev, connect_mask); > - if (ret) { > - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); > - goto hid_hw_start_fail; > - } > - } > + /* Reset the HID node state */ > + hid_device_io_stop(hdev); > + hid_hw_close(hdev); > + hid_hw_stop(hdev); > > - /* Allow incoming packets */ > - hid_device_io_start(hdev); > + if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) > + connect_mask &= ~HID_CONNECT_HIDINPUT; > > - hidpp_connect_event(hidpp); > + /* Now export the actual inputs and hidraw nodes to the world */ > + ret = hid_hw_start(hdev, connect_mask); > + if (ret) { > + hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); > + goto hid_hw_start_fail; > + } > > return ret; > > -hid_hw_open_failed: > - hid_device_io_stop(hdev); > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { > - hid_hw_close(hdev); > - hid_hw_stop(hdev); > - } > +hid_hw_init_fail: > + hid_hw_close(hdev); > +hid_hw_open_fail: > + hid_hw_stop(hdev); > hid_hw_start_fail: > sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group); > cancel_work_sync(&hidpp->work); > @@ -2942,10 +2951,9 @@ static void hidpp_remove(struct hid_device *hdev) > > sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group); > > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) > hidpp_ff_deinit(hdev); > - hid_hw_close(hdev); > - } > + > hid_hw_stop(hdev); > cancel_work_sync(&hidpp->work); > mutex_destroy(&hidpp->send_mutex); > -- > 2.9.3 > -- 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, 18 Jan 2017, Benjamin Tissoires wrote: > > The current custom solution for the G920 is not the best because > > hid_hw_start() is not called at the end of the .probe(). > > It means that any configuration retrieved after the initial hid_hw_start > > would not be exposed to user space without races. > > > > We can simply force hid_hw_start to just enable the transport layer by > > not using a connect_mask. This way, we can have a common path between > > USB, Unifying and Bluetooth devices. > > > > Tested with a G502 (plain USB), a T650 and many other unifying devices, > > and the T651 over Bluetooth. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------ > > 1 file changed, 48 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index a5d37a4..f5889ff 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > > if (!hidpp_validate_device(hdev)) > > return hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > > > + /* > > + * HID++ needs to read incoming report while in .probe(). > > + * However, this doesn't work for plain USB devices until the hdev > > + * status is set with HID_STAT_ADDED (device fully registered once > > + * with HID). > > + * So we ask for it to be reprobed later. > > + */ > > + if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE && > > + !(hdev->status & HID_STAT_ADDED)) > > Looks like this test breaks the T651 (bluetooth) after all. I seem to > have better success with: > if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED)) > > But that also means that the solution will not work if there is only > one USB interface in the device :/ Benjamin, do you want at least a subset of this patchset to be queued before you figure this out, or should I put the whole thing on hold? (not gone through it fully yet). Thanks,
On Thu, Jan 19, 2017 at 11:56 AM, Jiri Kosina <jikos@kernel.org> wrote: > On Wed, 18 Jan 2017, Benjamin Tissoires wrote: > >> > The current custom solution for the G920 is not the best because >> > hid_hw_start() is not called at the end of the .probe(). >> > It means that any configuration retrieved after the initial hid_hw_start >> > would not be exposed to user space without races. >> > >> > We can simply force hid_hw_start to just enable the transport layer by >> > not using a connect_mask. This way, we can have a common path between >> > USB, Unifying and Bluetooth devices. >> > >> > Tested with a G502 (plain USB), a T650 and many other unifying devices, >> > and the T651 over Bluetooth. >> > >> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> > --- >> > drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------ >> > 1 file changed, 48 insertions(+), 40 deletions(-) >> > >> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c >> > index a5d37a4..f5889ff 100644 >> > --- a/drivers/hid/hid-logitech-hidpp.c >> > +++ b/drivers/hid/hid-logitech-hidpp.c >> > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) >> > if (!hidpp_validate_device(hdev)) >> > return hid_hw_start(hdev, HID_CONNECT_DEFAULT); >> > >> > + /* >> > + * HID++ needs to read incoming report while in .probe(). >> > + * However, this doesn't work for plain USB devices until the hdev >> > + * status is set with HID_STAT_ADDED (device fully registered once >> > + * with HID). >> > + * So we ask for it to be reprobed later. >> > + */ >> > + if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE && >> > + !(hdev->status & HID_STAT_ADDED)) >> >> Looks like this test breaks the T651 (bluetooth) after all. I seem to >> have better success with: >> if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED)) >> >> But that also means that the solution will not work if there is only >> one USB interface in the device :/ > > Benjamin, > > do you want at least a subset of this patchset to be queued before you > figure this out, or should I put the whole thing on hold? (not gone > through it fully yet). > I think the first 11 patches (until "HID: logitech-hidpp: add a sysfs file to tell we support power_supply" - included) should be good to go while we get the test results and confirmation from others. The last part of the series is really for non-unifying devices, but they are not handled currently by upower, so it won't be a conflict to have them now or later. However, I'd like to get inputs from Bastien to see if that works for him (I provided a test kenrel build for him to do the tests). I have some doubts with the T650 as it appears in the main section of the battery info instead of a separate device if you connect it while the power panel is opened... Cheers, Benjamin -- 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/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index a5d37a4..f5889ff 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) if (!hidpp_validate_device(hdev)) return hid_hw_start(hdev, HID_CONNECT_DEFAULT); + /* + * HID++ needs to read incoming report while in .probe(). + * However, this doesn't work for plain USB devices until the hdev + * status is set with HID_STAT_ADDED (device fully registered once + * with HID). + * So we ask for it to be reprobed later. + */ + if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE && + !(hdev->status & HID_STAT_ADDED)) + return -EPROBE_DEFER; + hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device), GFP_KERNEL); if (!hidpp) @@ -2853,24 +2864,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_warn(hdev, "Cannot allocate sysfs group for %s\n", hdev->name); - if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) - connect_mask &= ~HID_CONNECT_HIDINPUT; - - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { - ret = hid_hw_start(hdev, connect_mask); - if (ret) { - hid_err(hdev, "hw start failed\n"); - goto hid_hw_start_fail; - } - ret = hid_hw_open(hdev); - if (ret < 0) { - dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n", - __func__, ret); - hid_hw_stop(hdev); - goto hid_hw_start_fail; - } + /* + * Plain USB connections need to actually call start and open + * on the transport driver to allow incoming data. + */ + ret = hid_hw_start(hdev, 0); + if (ret) { + hid_err(hdev, "hw start failed\n"); + goto hid_hw_start_fail; } + ret = hid_hw_open(hdev); + if (ret < 0) { + dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n", + __func__, ret); + hid_hw_stop(hdev); + goto hid_hw_open_fail; + } /* Allow incoming packets */ hid_device_io_start(hdev); @@ -2880,7 +2890,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) if (!connected) { ret = -ENODEV; hid_err(hdev, "Device not connected"); - goto hid_hw_open_failed; + goto hid_hw_init_fail; } hid_info(hdev, "HID++ %u.%u device connected.\n", @@ -2893,37 +2903,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { ret = wtp_get_config(hidpp); if (ret) - goto hid_hw_open_failed; + goto hid_hw_init_fail; } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { ret = g920_get_config(hidpp); if (ret) - goto hid_hw_open_failed; + goto hid_hw_init_fail; } - /* Block incoming packets */ - hid_device_io_stop(hdev); + hidpp_connect_event(hidpp); - if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { - ret = hid_hw_start(hdev, connect_mask); - if (ret) { - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); - goto hid_hw_start_fail; - } - } + /* Reset the HID node state */ + hid_device_io_stop(hdev); + hid_hw_close(hdev); + hid_hw_stop(hdev); - /* Allow incoming packets */ - hid_device_io_start(hdev); + if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) + connect_mask &= ~HID_CONNECT_HIDINPUT; - hidpp_connect_event(hidpp); + /* Now export the actual inputs and hidraw nodes to the world */ + ret = hid_hw_start(hdev, connect_mask); + if (ret) { + hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); + goto hid_hw_start_fail; + } return ret; -hid_hw_open_failed: - hid_device_io_stop(hdev); - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { - hid_hw_close(hdev); - hid_hw_stop(hdev); - } +hid_hw_init_fail: + hid_hw_close(hdev); +hid_hw_open_fail: + hid_hw_stop(hdev); hid_hw_start_fail: sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group); cancel_work_sync(&hidpp->work); @@ -2942,10 +2951,9 @@ static void hidpp_remove(struct hid_device *hdev) sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group); - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { + if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) hidpp_ff_deinit(hdev); - hid_hw_close(hdev); - } + hid_hw_stop(hdev); cancel_work_sync(&hidpp->work); mutex_destroy(&hidpp->send_mutex);
The current custom solution for the G920 is not the best because hid_hw_start() is not called at the end of the .probe(). It means that any configuration retrieved after the initial hid_hw_start would not be exposed to user space without races. We can simply force hid_hw_start to just enable the transport layer by not using a connect_mask. This way, we can have a common path between USB, Unifying and Bluetooth devices. Tested with a G502 (plain USB), a T650 and many other unifying devices, and the T651 over Bluetooth. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 40 deletions(-)