linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures"
@ 2022-12-20  9:22 Bastien Nocera
  2022-12-20  9:22 ` [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary Bastien Nocera
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Bastien Nocera @ 2022-12-20  9:22 UTC (permalink / raw)
  To: linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

Now that we're in 2022, and the majority of desktop environments can and
should support touchpad gestures through libinput, remove the legacy
module parameter that made it possible to use gestures implemented in
firmware.

This will eventually allow simplifying the driver's initialisation code.

This reverts commit 9188dbaed68a4b23dc96eba165265c08caa7dc2a.
---
 drivers/hid/hid-logitech-hidpp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 08ad19097e9e..7f9187201913 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -32,11 +32,6 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
 MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
 
-static bool disable_raw_mode;
-module_param(disable_raw_mode, bool, 0644);
-MODULE_PARM_DESC(disable_raw_mode,
-	"Disable Raw mode reporting for touchpads and keep firmware gestures.");
-
 static bool disable_tap_to_click;
 module_param(disable_tap_to_click, bool, 0644);
 MODULE_PARM_DESC(disable_tap_to_click,
@@ -4355,11 +4350,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	    hidpp_application_equals(hdev, HID_GD_KEYBOARD))
 		hidpp->quirks |= HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS;
 
-	if (disable_raw_mode) {
-		hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
-		hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
-	}
-
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
 		ret = wtp_allocate(hdev, id);
 		if (ret)
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary
  2022-12-20  9:22 [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
@ 2022-12-20  9:22 ` Bastien Nocera
  2023-01-24 17:20   ` Bastien Nocera
  2022-12-20  9:22 ` [PATCH 3/3] HID: logitech-hidpp: Remove HIDPP_QUIRK_NO_HIDINPUT quirk Bastien Nocera
  2022-12-20 15:44 ` [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
  2 siblings, 1 reply; 7+ messages in thread
From: Bastien Nocera @ 2022-12-20  9:22 UTC (permalink / raw)
  To: linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

Don't stop and restart communication with the device unless we need to
modify the connect flags used because of a device quirk.
---
 drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 7f9187201913..b4e4a8c79c75 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	bool connected;
 	unsigned int connect_mask = HID_CONNECT_DEFAULT;
 	struct hidpp_ff_private_data data;
+	bool will_restart = false;
 
 	/* report_fixup needs drvdata to be set before we call hid_parse */
 	hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
@@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			return ret;
 	}
 
+	if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
+		will_restart = true;
+
 	INIT_WORK(&hidpp->work, delayed_work_cb);
 	mutex_init(&hidpp->send_mutex);
 	init_waitqueue_head(&hidpp->wait);
@@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * Plain USB connections need to actually call start and open
 	 * on the transport driver to allow incoming data.
 	 */
-	ret = hid_hw_start(hdev, 0);
+	ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
 		goto hid_hw_start_fail;
@@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			hidpp->wireless_feature_index = 0;
 		else if (ret)
 			goto hid_hw_init_fail;
+		ret = 0;
 	}
 
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
@@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	hidpp_connect_event(hidpp);
 
-	/* Reset the HID node state */
-	hid_device_io_stop(hdev);
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
+	if (will_restart) {
+		/* Reset the HID node state */
+		hid_device_io_stop(hdev);
+		hid_hw_close(hdev);
+		hid_hw_stop(hdev);
 
-	if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
-		connect_mask &= ~HID_CONNECT_HIDINPUT;
+		if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
+			connect_mask &= ~HID_CONNECT_HIDINPUT;
 
-	/* 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;
+		/* 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;
+		}
 	}
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] HID: logitech-hidpp: Remove HIDPP_QUIRK_NO_HIDINPUT quirk
  2022-12-20  9:22 [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
  2022-12-20  9:22 ` [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary Bastien Nocera
@ 2022-12-20  9:22 ` Bastien Nocera
  2022-12-20 15:44 ` [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
  2 siblings, 0 replies; 7+ messages in thread
From: Bastien Nocera @ 2022-12-20  9:22 UTC (permalink / raw)
  To: linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

HIDPP_QUIRK_NO_HIDINPUT isn't used by any devices but still happens to
work as HIDPP_QUIRK_DELAYED_INIT is defined to the same value. Remove
HIDPP_QUIRK_NO_HIDINPUT and use HIDPP_QUIRK_DELAYED_INIT everywhere
instead.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/hid/hid-logitech-hidpp.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b4e4a8c79c75..2092fb1be627 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -67,7 +67,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 /* bits 2..20 are reserved for classes */
 /* #define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21) disabled */
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
-#define HIDPP_QUIRK_NO_HIDINPUT			BIT(23)
+#define HIDPP_QUIRK_DELAYED_INIT		BIT(23)
 #define HIDPP_QUIRK_FORCE_OUTPUT_REPORTS	BIT(24)
 #define HIDPP_QUIRK_UNIFYING			BIT(25)
 #define HIDPP_QUIRK_HIDPP_WHEELS		BIT(26)
@@ -83,8 +83,6 @@ MODULE_PARM_DESC(disable_tap_to_click,
 					 HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL | \
 					 HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL)
 
-#define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT
-
 #define HIDPP_CAPABILITY_HIDPP10_BATTERY	BIT(0)
 #define HIDPP_CAPABILITY_HIDPP20_BATTERY	BIT(1)
 #define HIDPP_CAPABILITY_BATTERY_MILEAGE	BIT(2)
@@ -4205,7 +4203,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	if (hidpp->capabilities & HIDPP_CAPABILITY_HI_RES_SCROLL)
 		hi_res_scroll_enable(hidpp);
 
-	if (!(hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT) || hidpp->delayed_input)
+	if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
 		/* if the input nodes are already created, we can stop now */
 		return;
 
@@ -4436,7 +4434,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_hw_close(hdev);
 		hid_hw_stop(hdev);
 
-		if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
+		if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
 			connect_mask &= ~HID_CONNECT_HIDINPUT;
 
 		/* Now export the actual inputs and hidraw nodes to the world */
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures"
  2022-12-20  9:22 [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
  2022-12-20  9:22 ` [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary Bastien Nocera
  2022-12-20  9:22 ` [PATCH 3/3] HID: logitech-hidpp: Remove HIDPP_QUIRK_NO_HIDINPUT quirk Bastien Nocera
@ 2022-12-20 15:44 ` Bastien Nocera
  2 siblings, 0 replies; 7+ messages in thread
From: Bastien Nocera @ 2022-12-20 15:44 UTC (permalink / raw)
  To: linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> Now that we're in 2022, and the majority of desktop environments can
> and
> should support touchpad gestures through libinput, remove the legacy
> module parameter that made it possible to use gestures implemented in
> firmware.
> 
> This will eventually allow simplifying the driver's initialisation
> code.
> 
> This reverts commit 9188dbaed68a4b23dc96eba165265c08caa7dc2a.

Forgot the signed-off-by, resent as v2.

> ---
>  drivers/hid/hid-logitech-hidpp.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index 08ad19097e9e..7f9187201913 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -32,11 +32,6 @@ MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
>  MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
>  
> -static bool disable_raw_mode;
> -module_param(disable_raw_mode, bool, 0644);
> -MODULE_PARM_DESC(disable_raw_mode,
> -       "Disable Raw mode reporting for touchpads and keep firmware
> gestures.");
> -
>  static bool disable_tap_to_click;
>  module_param(disable_tap_to_click, bool, 0644);
>  MODULE_PARM_DESC(disable_tap_to_click,
> @@ -4355,11 +4350,6 @@ static int hidpp_probe(struct hid_device
> *hdev, const struct hid_device_id *id)
>             hidpp_application_equals(hdev, HID_GD_KEYBOARD))
>                 hidpp->quirks |=
> HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS;
>  
> -       if (disable_raw_mode) {
> -               hidpp->quirks &= ~HIDPP_QUIRK_CLASS_WTP;
> -               hidpp->quirks &= ~HIDPP_QUIRK_NO_HIDINPUT;
> -       }
> -
>         if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>                 ret = wtp_allocate(hdev, id);
>                 if (ret)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary
  2022-12-20  9:22 ` [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary Bastien Nocera
@ 2023-01-24 17:20   ` Bastien Nocera
  2023-01-25 10:18     ` Benjamin Tissoires
  0 siblings, 1 reply; 7+ messages in thread
From: Bastien Nocera @ 2023-01-24 17:20 UTC (permalink / raw)
  To: linux-input
  Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> Don't stop and restart communication with the device unless we need
> to
> modify the connect flags used because of a device quirk.

FIWW, Andreas Bergmeier told me off-list that this fixed their problem
with the Litra Glow not connecting properly.

Would be great to have reviews on this and my other HID++ patches.

Cheers

> ---
>  drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++-----------
> -
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index 7f9187201913..b4e4a8c79c75 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
>         bool connected;
>         unsigned int connect_mask = HID_CONNECT_DEFAULT;
>         struct hidpp_ff_private_data data;
> +       bool will_restart = false;
>  
>         /* report_fixup needs drvdata to be set before we call
> hid_parse */
>         hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
> @@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
>                         return ret;
>         }
>  
> +       if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> +               will_restart = true;
> +
>         INIT_WORK(&hidpp->work, delayed_work_cb);
>         mutex_init(&hidpp->send_mutex);
>         init_waitqueue_head(&hidpp->wait);
> @@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
>          * Plain USB connections need to actually call start and open
>          * on the transport driver to allow incoming data.
>          */
> -       ret = hid_hw_start(hdev, 0);
> +       ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
>         if (ret) {
>                 hid_err(hdev, "hw start failed\n");
>                 goto hid_hw_start_fail;
> @@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
>                         hidpp->wireless_feature_index = 0;
>                 else if (ret)
>                         goto hid_hw_init_fail;
> +               ret = 0;
>         }
>  
>         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
> @@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device
> *hdev, const struct hid_device_id *id)
>  
>         hidpp_connect_event(hidpp);
>  
> -       /* Reset the HID node state */
> -       hid_device_io_stop(hdev);
> -       hid_hw_close(hdev);
> -       hid_hw_stop(hdev);
> +       if (will_restart) {
> +               /* Reset the HID node state */
> +               hid_device_io_stop(hdev);
> +               hid_hw_close(hdev);
> +               hid_hw_stop(hdev);
>  
> -       if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> -               connect_mask &= ~HID_CONNECT_HIDINPUT;
> +               if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> +                       connect_mask &= ~HID_CONNECT_HIDINPUT;
>  
> -       /* 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;
> +               /* 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;
> +               }
>         }
>  
>         if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary
  2023-01-24 17:20   ` Bastien Nocera
@ 2023-01-25 10:18     ` Benjamin Tissoires
  2023-01-25 11:52       ` Bastien Nocera
  0 siblings, 1 reply; 7+ messages in thread
From: Benjamin Tissoires @ 2023-01-25 10:18 UTC (permalink / raw)
  To: Bastien Nocera
  Cc: linux-input, linux-kernel, Jiri Kosina,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

On Tue, Jan 24, 2023 at 6:20 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> > Don't stop and restart communication with the device unless we need
> > to
> > modify the connect flags used because of a device quirk.
>
> FIWW, Andreas Bergmeier told me off-list that this fixed their problem
> with the Litra Glow not connecting properly.
>
> Would be great to have reviews on this and my other HID++ patches.

Sigh. I reviewed the patches just now (well, v2 at least), and thought
I better give a shot at it before merging, and it turns out that this
patch breaks the Unifying receivers.

Without it, each device presented to the user space has a proper name:

logitech-hidpp-device 0003:046D:4041.001C: input,hidraw15: USB HID
v1.11 Keyboard [Logitech MX Master] on usb-0000:01:00.0-4/input2:5

But with it, I get:

logitech-hidpp-device 0003:046D:4041.0024: input,hidraw8: USB HID
v1.11 Keyboard [Logitech Wireless Device PID:4041] on
usb-0000:00:14.0-8.2.4/input2:5

This is because we present the device to the userspace before being
able to fetch the name from the receiver.

I think we should make that connect/disconnect a special case of the
receivers too. Or maybe if the bus is not Bluetooth or USB, do the
disconnect/reconnect.

Cheers,
Benjamin

>
> Cheers
>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 31 +++++++++++++++++++-----------
> > -
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index 7f9187201913..b4e4a8c79c75 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -4310,6 +4310,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> >         bool connected;
> >         unsigned int connect_mask = HID_CONNECT_DEFAULT;
> >         struct hidpp_ff_private_data data;
> > +       bool will_restart = false;
> >
> >         /* report_fixup needs drvdata to be set before we call
> > hid_parse */
> >         hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
> > @@ -4360,6 +4361,9 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> >                         return ret;
> >         }
> >
> > +       if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> > +               will_restart = true;
> > +
> >         INIT_WORK(&hidpp->work, delayed_work_cb);
> >         mutex_init(&hidpp->send_mutex);
> >         init_waitqueue_head(&hidpp->wait);
> > @@ -4374,7 +4378,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> >          * Plain USB connections need to actually call start and open
> >          * on the transport driver to allow incoming data.
> >          */
> > -       ret = hid_hw_start(hdev, 0);
> > +       ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
> >         if (ret) {
> >                 hid_err(hdev, "hw start failed\n");
> >                 goto hid_hw_start_fail;
> > @@ -4411,6 +4415,7 @@ static int hidpp_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> >                         hidpp->wireless_feature_index = 0;
> >                 else if (ret)
> >                         goto hid_hw_init_fail;
> > +               ret = 0;
> >         }
> >
> >         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
> > @@ -4425,19 +4430,21 @@ static int hidpp_probe(struct hid_device
> > *hdev, const struct hid_device_id *id)
> >
> >         hidpp_connect_event(hidpp);
> >
> > -       /* Reset the HID node state */
> > -       hid_device_io_stop(hdev);
> > -       hid_hw_close(hdev);
> > -       hid_hw_stop(hdev);
> > +       if (will_restart) {
> > +               /* Reset the HID node state */
> > +               hid_device_io_stop(hdev);
> > +               hid_hw_close(hdev);
> > +               hid_hw_stop(hdev);
> >
> > -       if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> > -               connect_mask &= ~HID_CONNECT_HIDINPUT;
> > +               if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
> > +                       connect_mask &= ~HID_CONNECT_HIDINPUT;
> >
> > -       /* 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;
> > +               /* 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;
> > +               }
> >         }
> >
> >         if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary
  2023-01-25 10:18     ` Benjamin Tissoires
@ 2023-01-25 11:52       ` Bastien Nocera
  0 siblings, 0 replies; 7+ messages in thread
From: Bastien Nocera @ 2023-01-25 11:52 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: linux-input, linux-kernel, Jiri Kosina,
	Peter F . Patel-Schneider, Filipe Laíns,
	Nestor Lopez Casado

On Wed, 2023-01-25 at 11:18 +0100, Benjamin Tissoires wrote:
> On Tue, Jan 24, 2023 at 6:20 PM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Tue, 2022-12-20 at 10:22 +0100, Bastien Nocera wrote:
> > > Don't stop and restart communication with the device unless we
> > > need
> > > to
> > > modify the connect flags used because of a device quirk.
> > 
> > FIWW, Andreas Bergmeier told me off-list that this fixed their
> > problem
> > with the Litra Glow not connecting properly.
> > 
> > Would be great to have reviews on this and my other HID++ patches.
> 
> Sigh. I reviewed the patches just now (well, v2 at least), and
> thought
> I better give a shot at it before merging, and it turns out that this
> patch breaks the Unifying receivers.
> 
> Without it, each device presented to the user space has a proper
> name:
> 
> logitech-hidpp-device 0003:046D:4041.001C: input,hidraw15: USB HID
> v1.11 Keyboard [Logitech MX Master] on usb-0000:01:00.0-4/input2:5
> 
> But with it, I get:
> 
> logitech-hidpp-device 0003:046D:4041.0024: input,hidraw8: USB HID
> v1.11 Keyboard [Logitech Wireless Device PID:4041] on
> usb-0000:00:14.0-8.2.4/input2:5
> 
> This is because we present the device to the userspace before being
> able to fetch the name from the receiver.
> 
> I think we should make that connect/disconnect a special case of the
> receivers too. Or maybe if the bus is not Bluetooth or USB, do the
> disconnect/reconnect.

From what I can tell, this would mean restarting the connection in case
hidpp_unifying_init() did anything, right?

I'll test this out and update the patch.

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4547e9580101..e0c28257f598 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4392,8 +4392,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
        /* Allow incoming packets */
        hid_device_io_start(hdev);
 
-       if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
-               hidpp_unifying_init(hidpp);
+       if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) {
+               if (hidpp_unifying_init(hidpp) == 0)
+                       will_restart = true;
+       }
 
        connected = hidpp_root_get_protocol_version(hidpp) == 0;
        atomic_set(&hidpp->connected, connected);


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-25 11:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-20  9:22 [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera
2022-12-20  9:22 ` [PATCH 2/3] HID: logitech-hidpp: Don't restart communication if not necessary Bastien Nocera
2023-01-24 17:20   ` Bastien Nocera
2023-01-25 10:18     ` Benjamin Tissoires
2023-01-25 11:52       ` Bastien Nocera
2022-12-20  9:22 ` [PATCH 3/3] HID: logitech-hidpp: Remove HIDPP_QUIRK_NO_HIDINPUT quirk Bastien Nocera
2022-12-20 15:44 ` [PATCH 1/3] Revert "HID: logitech-hidpp: add a module parameter to keep firmware gestures" Bastien Nocera

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).