| Message ID | 1418767562-4136-1-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Tue, Dec 16, 2014 at 5:06 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > If wtp_connect() fails, that means most of the time that the device has > been disconnected. Subsequent attempts to contact the device will fail > too, so it's simpler to bail out earlier. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- Jiri, Peter, the logitech patches are queuing up really fast. To keep track of them, I made a bundle on patchwork: https://patchwork.kernel.org/bundle/bentiss/hid-logitech-hidpp/ (/me discovered a new tool to play with) Right now, the patch "HID: logitech-hidpp: detect HID++ 2.0 errors too" is waiting for Logitech's approval, and the 2 of this series need review. Peter, please tell me if I missed one patch. Cheers, Benjamin > drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index d008d71..c0fb5fe 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) > return 0; > }; > > -static void wtp_connect(struct hid_device *hdev, bool connected) > +static int wtp_connect(struct hid_device *hdev, bool connected) > { > struct hidpp_device *hidpp = hid_get_drvdata(hdev); > struct wtp_data *wd = hidpp->private_data; > int ret; > > if (!connected) > - return; > + return 0; > > if (!wd->x_size) { > ret = wtp_get_config(hidpp); > if (ret) { > hid_err(hdev, "Can not get wtp config: %d\n", ret); > - return; > + return ret; > } > } > > - hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > + return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > true, true); > } > > @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) > struct input_dev *input; > char *name, *devm_name; > > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) > - wtp_connect(hdev, connected); > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { > + ret = wtp_connect(hdev, connected); > + if (ret) > + return; > + } > > if (!connected || hidpp->delayed_input) > return; > -- > 2.1.0 > > -- > 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 -- 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 Tuesday 16 December 2014 17:06:01 Benjamin Tissoires wrote: > If wtp_connect() fails, that means most of the time that the device has > been disconnected. Subsequent attempts to contact the device will fail > too, so it's simpler to bail out earlier. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index d008d71..c0fb5fe 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) > return 0; > }; > > -static void wtp_connect(struct hid_device *hdev, bool connected) > +static int wtp_connect(struct hid_device *hdev, bool connected) > { > struct hidpp_device *hidpp = hid_get_drvdata(hdev); > struct wtp_data *wd = hidpp->private_data; > int ret; > > if (!connected) > - return; > + return 0; "0" is success, what about -1 or -ENODEV here to signal an error? The following line (in hidpp_connect_event) returns on !connected, but that is no reason to return 0 here. ("No connection" sounds like an error condition to me as "[wtp_]connect" cannot do something meaningful.) Whether or not you change the return value is up to you. This patch is Reviewed-by: Peter Wu <peter@lekensteyn.nl> Kind regards, Peter > if (!wd->x_size) { > ret = wtp_get_config(hidpp); > if (ret) { > hid_err(hdev, "Can not get wtp config: %d\n", ret); > - return; > + return ret; > } > } > > - hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > + return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > true, true); > } > > @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) > struct input_dev *input; > char *name, *devm_name; > > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) > - wtp_connect(hdev, connected); > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { > + ret = wtp_connect(hdev, connected); > + if (ret) > + return; > + } > > if (!connected || hidpp->delayed_input) > return; > -- 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
Hi Benjamin, On Tuesday 16 December 2014 17:13:05 Benjamin Tissoires wrote: > the logitech patches are queuing up really fast. > To keep track of them, I made a bundle on patchwork: > https://patchwork.kernel.org/bundle/bentiss/hid-logitech-hidpp/ > (/me discovered a new tool to play with) > > Right now, the patch "HID: logitech-hidpp: detect HID++ 2.0 errors > too" is waiting for Logitech's approval, and the 2 of this series need > review. > > Peter, please tell me if I missed one patch. Nice, it would be even better if a bundle could be bookmarked, or if a group could set review flags in this bundle :-) There are no missing patches from my side. All changes (based on jikos/hid, branch for-next) are at https://git.lekensteyn.nl/peter/linux/log/?h=logitech-hidpp and are tested in QEMU with an emulated device and a real device (with T400/T650/M525 paired). I noticed that all devices would immediately get an input device (even if they were off), except for the T650. This apparently happens because the touchpad configuration cannot be retrieved or when the touchpad cannot be put in raw reporting mode. I cannot think of something to "fix" this though.
Sorry for the rapid mail, I forgot to mention something. wtp_connect won't work on non-HID++ devices. What about moving it down, between the generic routines (reading protocol and name) and hidpp_allocate_input? Then the connected parameter can also be dropped. Kind regards, Peter On Wednesday 17 December 2014 00:33:55 Peter Wu wrote: > On Tuesday 16 December 2014 17:06:01 Benjamin Tissoires wrote: > > If wtp_connect() fails, that means most of the time that the device has > > been disconnected. Subsequent attempts to contact the device will fail > > too, so it's simpler to bail out earlier. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index d008d71..c0fb5fe 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) > > return 0; > > }; > > > > -static void wtp_connect(struct hid_device *hdev, bool connected) > > +static int wtp_connect(struct hid_device *hdev, bool connected) > > { > > struct hidpp_device *hidpp = hid_get_drvdata(hdev); > > struct wtp_data *wd = hidpp->private_data; > > int ret; > > > > if (!connected) > > - return; > > + return 0; > > "0" is success, what about -1 or -ENODEV here to signal an error? The > following line (in hidpp_connect_event) returns on !connected, but that > is no reason to return 0 here. > > ("No connection" sounds like an error condition to me as "[wtp_]connect" > cannot do something meaningful.) > > Whether or not you change the return value is up to you. This patch is > Reviewed-by: Peter Wu <peter@lekensteyn.nl> > > Kind regards, > Peter > > > if (!wd->x_size) { > > ret = wtp_get_config(hidpp); > > if (ret) { > > hid_err(hdev, "Can not get wtp config: %d\n", ret); > > - return; > > + return ret; > > } > > } > > > > - hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > > + return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > > true, true); > > } > > > > @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) > > struct input_dev *input; > > char *name, *devm_name; > > > > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) > > - wtp_connect(hdev, connected); > > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { > > + ret = wtp_connect(hdev, connected); > > + if (ret) > > + return; > > + } > > > > if (!connected || hidpp->delayed_input) > > return; > > -- 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 Dec 17 2014 or thereabouts, Peter Wu wrote: > Hi Benjamin, > > On Tuesday 16 December 2014 17:13:05 Benjamin Tissoires wrote: > > the logitech patches are queuing up really fast. > > To keep track of them, I made a bundle on patchwork: > > https://patchwork.kernel.org/bundle/bentiss/hid-logitech-hidpp/ > > (/me discovered a new tool to play with) > > > > Right now, the patch "HID: logitech-hidpp: detect HID++ 2.0 errors > > too" is waiting for Logitech's approval, and the 2 of this series need > > review. > > > > Peter, please tell me if I missed one patch. > > Nice, it would be even better if a bundle could be bookmarked, or if a > group could set review flags in this bundle :-) > > There are no missing patches from my side. All changes (based on > jikos/hid, branch for-next) are at > https://git.lekensteyn.nl/peter/linux/log/?h=logitech-hidpp > and are tested in QEMU with an emulated device and a real device (with > T400/T650/M525 paired). Thanks. The only problem with publishing these kind of tree is that at some point you will want to rebase it, and this will break people who pulled your tree. I found Jiri's name scheme really good (with a tag for the current version). This allows to push several branch based on different revisions without breaking the others. But I am a little bit digressing here :) > > I noticed that all devices would immediately get an input device (even > if they were off), except for the T650. This apparently happens because > the touchpad configuration cannot be retrieved or when the touchpad > cannot be put in raw reporting mode. I cannot think of something to > "fix" this though. That's the design, unfortunately. Ideally, I would have prefer having a consistant way of setting up devices: when the receiver is plugged, create the input nodes and done. Unfortunately, this does not apply to touchpads and mice in raw mode as we need to query the devices for their capabilities and axis ranges. We then need to deffer the creation upon the connection. Unfortunately, we can not do the same for the normal DJ devices. If you do so, you will lose the very first input reports while the device is set up, and while the userspace is ready to read from it. This is *really* problematic for keyboards, especially when you use it to enter your computer encryption password. You lose the first few chars, and the password fails, and it's a mess. So in the end, I came up with this hybrid solution. For a few selected and tested devices, we deffer the input creation. For the rest of the world, we try to create them at the earliest in order not losing events. To sum up, this is really unfortunate :) Cheers, Benjamin > -- > Kind regards, > Peter > https://lekensteyn.nl > -- 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 Dec 17 2014 or thereabouts, Peter Wu wrote: > Sorry for the rapid mail, I forgot to mention something. > > wtp_connect won't work on non-HID++ devices. What about moving it down, > between the generic routines (reading protocol and name) and > hidpp_allocate_input? Then the connected parameter can also be dropped. No, this will not work. wtp_connect sets the device in the raw report mode. If we call it after hidpp_allocate_input, this will work on the first connect. Then, if you switch off/on the device, the connect_event will be called, but the device will not be set in the raw mode. We really need to unconditionally call wtp_connect at each connect_event. > > Kind regards, > Peter > > On Wednesday 17 December 2014 00:33:55 Peter Wu wrote: > > On Tuesday 16 December 2014 17:06:01 Benjamin Tissoires wrote: > > > If wtp_connect() fails, that means most of the time that the device has > > > been disconnected. Subsequent attempts to contact the device will fail > > > too, so it's simpler to bail out earlier. > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > --- > > > drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > > index d008d71..c0fb5fe 100644 > > > --- a/drivers/hid/hid-logitech-hidpp.c > > > +++ b/drivers/hid/hid-logitech-hidpp.c > > > @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) > > > return 0; > > > }; > > > > > > -static void wtp_connect(struct hid_device *hdev, bool connected) > > > +static int wtp_connect(struct hid_device *hdev, bool connected) > > > { > > > struct hidpp_device *hidpp = hid_get_drvdata(hdev); > > > struct wtp_data *wd = hidpp->private_data; > > > int ret; > > > > > > if (!connected) > > > - return; > > > + return 0; > > > > "0" is success, what about -1 or -ENODEV here to signal an error? The > > following line (in hidpp_connect_event) returns on !connected, but that > > is no reason to return 0 here. 0 is fine here. Maybe I over thought the API, but the connect_event is sent whenever the device is connected or disconnected. This allows a subdriver to do things on connect and on disconnect. For instance, you could delete the input node on disconnect. This is not something we want though, so the disconnect event is just discarded. But this disconnect event is not an error, it is just a discarded event, so returning success is fine. > > > > ("No connection" sounds like an error condition to me as "[wtp_]connect" > > cannot do something meaningful.) > > > > Whether or not you change the return value is up to you. This patch is > > Reviewed-by: Peter Wu <peter@lekensteyn.nl> Thanks for the review! Cheers, Benjamin > > > > Kind regards, > > Peter > > > > > if (!wd->x_size) { > > > ret = wtp_get_config(hidpp); > > > if (ret) { > > > hid_err(hdev, "Can not get wtp config: %d\n", ret); > > > - return; > > > + return ret; > > > } > > > } > > > > > > - hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > > > + return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, > > > true, true); > > > } > > > > > > @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) > > > struct input_dev *input; > > > char *name, *devm_name; > > > > > > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) > > > - wtp_connect(hdev, connected); > > > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { > > > + ret = wtp_connect(hdev, connected); > > > + if (ret) > > > + return; > > > + } > > > > > > if (!connected || hidpp->delayed_input) > > > return; > > > > -- 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 Tue, 16 Dec 2014, Benjamin Tissoires wrote: > If wtp_connect() fails, that means most of the time that the device has > been disconnected. Subsequent attempts to contact the device will fail > too, so it's simpler to bail out earlier. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> I have applied this one to for-3.20/logitech. I am postponing 2/2, expecting v2 with an updated comment. Thanks,
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index d008d71..c0fb5fe 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) return 0; }; -static void wtp_connect(struct hid_device *hdev, bool connected) +static int wtp_connect(struct hid_device *hdev, bool connected) { struct hidpp_device *hidpp = hid_get_drvdata(hdev); struct wtp_data *wd = hidpp->private_data; int ret; if (!connected) - return; + return 0; if (!wd->x_size) { ret = wtp_get_config(hidpp); if (ret) { hid_err(hdev, "Can not get wtp config: %d\n", ret); - return; + return ret; } } - hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, + return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index, true, true); } @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) struct input_dev *input; char *name, *devm_name; - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) - wtp_connect(hdev, connected); + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { + ret = wtp_connect(hdev, connected); + if (ret) + return; + } if (!connected || hidpp->delayed_input) return;
If wtp_connect() fails, that means most of the time that the device has been disconnected. Subsequent attempts to contact the device will fail too, so it's simpler to bail out earlier. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)