[PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Benjamin Tissoires]

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;
 
 	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

[PATCH 2/2] HID: logitech-hidpp: store the name of the device in struct hidpp

[Benjamin Tissoires]

If a disconnect occurs while getting the actual name of the device
(which can take several HID transactions), the name of the device will
be the hid name, provided by the Unifying Receiver.
This means that in some cases, the user space will see a different
name that what it usually sees when there is no disconnect.

We should store the name of the device in the struct hidpp. That way,
if a disconnect occurs while we are accessing the name,
hidpp_connect_event() can fail, and the input node is not created.

The input node will be created only if we have a connection which
lasts long enough to retrieve all the requested information:
name, protocol, and specific configuration.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index c0fb5fe..4bc8714 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -89,6 +89,7 @@ struct hidpp_device {
 	struct hid_device *hid_dev;
 	struct mutex send_mutex;
 	void *send_receive_buf;
+	char *name;
 	wait_queue_head_t wait;
 	bool answer_available;
 	u8 protocol_major;
@@ -1087,6 +1088,7 @@ static void hidpp_input_close(struct input_dev *dev)
 static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
 {
 	struct input_dev *input_dev = devm_input_allocate_device(&hdev->dev);
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 
 	if (!input_dev)
 		return NULL;
@@ -1095,7 +1097,7 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
 	input_dev->open = hidpp_input_open;
 	input_dev->close = hidpp_input_close;
 
-	input_dev->name = hdev->name;
+	input_dev->name = hidpp->name;
 	input_dev->phys = hdev->phys;
 	input_dev->uniq = hdev->uniq;
 	input_dev->id.bustype = hdev->bus;
@@ -1137,22 +1139,28 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	hid_info(hdev, "HID++ %u.%u device connected.\n",
 		 hidpp->protocol_major, hidpp->protocol_minor);
 
+	if (!hidpp->name || hidpp->name == hdev->name) {
+		name = hidpp_get_device_name(hidpp);
+		if (!name) {
+			hid_err(hdev,
+				"unable to retrieve the name of the device");
+			return;
+		}
+
+		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
+		kfree(name);
+		if (!devm_name)
+			return;
+
+		hidpp->name = devm_name;
+	}
+
 	input = hidpp_allocate_input(hdev);
 	if (!input) {
 		hid_err(hdev, "cannot allocate new input device: %d\n", ret);
 		return;
 	}
 
-	name = hidpp_get_device_name(hidpp);
-	if (!name) {
-		hid_err(hdev, "unable to retrieve the name of the device");
-	} else {
-		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
-		if (devm_name)
-			input->name = devm_name;
-		kfree(name);
-	}
-
 	hidpp_populate_input(hidpp, input, false);
 
 	ret = input_register_device(input);
@@ -1175,6 +1183,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return -ENOMEM;
 
 	hidpp->hid_dev = hdev;
+	hidpp->name = hdev->name;
 	hid_set_drvdata(hdev, hidpp);
 
 	hidpp->quirks = id->driver_data;
-- 
2.1.0

Re: [PATCH 2/2] HID: logitech-hidpp: store the name of the device in struct hidpp

[Peter Wu]

On Tuesday 16 December 2014 17:06:02 Benjamin Tissoires wrote:
> If a disconnect occurs while getting the actual name of the device
> (which can take several HID transactions), the name of the device will
> be the hid name, provided by the Unifying Receiver.
> This means that in some cases, the user space will see a different
> name that what it usually sees when there is no disconnect.
> 
> We should store the name of the device in the struct hidpp. That way,
> if a disconnect occurs while we are accessing the name,
> hidpp_connect_event() can fail, and the input node is not created.
> 
> The input node will be created only if we have a connection which
> lasts long enough to retrieve all the requested information:
> name, protocol, and specific configuration.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index c0fb5fe..4bc8714 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -89,6 +89,7 @@ struct hidpp_device {
>  	struct hid_device *hid_dev;
>  	struct mutex send_mutex;
>  	void *send_receive_buf;
> +	char *name;
>  	wait_queue_head_t wait;
>  	bool answer_available;
>  	u8 protocol_major;
> @@ -1087,6 +1088,7 @@ static void hidpp_input_close(struct input_dev *dev)
>  static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
>  {
>  	struct input_dev *input_dev = devm_input_allocate_device(&hdev->dev);
> +	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
>  
>  	if (!input_dev)
>  		return NULL;
> @@ -1095,7 +1097,7 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
>  	input_dev->open = hidpp_input_open;
>  	input_dev->close = hidpp_input_close;
>  
> -	input_dev->name = hdev->name;
> +	input_dev->name = hidpp->name;
>  	input_dev->phys = hdev->phys;
>  	input_dev->uniq = hdev->uniq;
>  	input_dev->id.bustype = hdev->bus;
> @@ -1137,22 +1139,28 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>  	hid_info(hdev, "HID++ %u.%u device connected.\n",
>  		 hidpp->protocol_major, hidpp->protocol_minor);
>  
> +	if (!hidpp->name || hidpp->name == hdev->name) {

Hm, is it ever possible that hidpp->name is NULL? probe sets the pointer
to an array (hdev->name). Defence in depth I guess, but perhaps a
comment could clarify this?

Otherwise the changes look OK. I have tested this situation:

 1. Insert receiver
 2. Return a HID++ version for the WTP.
 3. Return -9 (ResourceError) for the device name feature request (via
    the QEMU emulation).
 4. Observe that this fails.

So maybe you could add a comment for the above and add these tags:

Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Tested-by: Peter Wu <peter@lekensteyn.nl>

Kind regards,
Peter

> +		name = hidpp_get_device_name(hidpp);
> +		if (!name) {
> +			hid_err(hdev,
> +				"unable to retrieve the name of the device");
> +			return;
> +		}
> +
> +		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
> +		kfree(name);
> +		if (!devm_name)
> +			return;
> +
> +		hidpp->name = devm_name;
> +	}
> +
>  	input = hidpp_allocate_input(hdev);
>  	if (!input) {
>  		hid_err(hdev, "cannot allocate new input device: %d\n", ret);
>  		return;
>  	}
>  
> -	name = hidpp_get_device_name(hidpp);
> -	if (!name) {
> -		hid_err(hdev, "unable to retrieve the name of the device");
> -	} else {
> -		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
> -		if (devm_name)
> -			input->name = devm_name;
> -		kfree(name);
> -	}
> -
>  	hidpp_populate_input(hidpp, input, false);
>  
>  	ret = input_register_device(input);
> @@ -1175,6 +1183,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return -ENOMEM;
>  
>  	hidpp->hid_dev = hdev;
> +	hidpp->name = hdev->name;
>  	hid_set_drvdata(hdev, hidpp);
>  
>  	hidpp->quirks = id->driver_data;
> 

Re: [PATCH 2/2] HID: logitech-hidpp: store the name of the device in struct hidpp

[Benjamin Tissoires]

On Dec 17 2014 or thereabouts, Peter Wu wrote:
> On Tuesday 16 December 2014 17:06:02 Benjamin Tissoires wrote:
> > If a disconnect occurs while getting the actual name of the device
> > (which can take several HID transactions), the name of the device will
> > be the hid name, provided by the Unifying Receiver.
> > This means that in some cases, the user space will see a different
> > name that what it usually sees when there is no disconnect.
> > 
> > We should store the name of the device in the struct hidpp. That way,
> > if a disconnect occurs while we are accessing the name,
> > hidpp_connect_event() can fail, and the input node is not created.
> > 
> > The input node will be created only if we have a connection which
> > lasts long enough to retrieve all the requested information:
> > name, protocol, and specific configuration.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 31 ++++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index c0fb5fe..4bc8714 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -89,6 +89,7 @@ struct hidpp_device {
> >  	struct hid_device *hid_dev;
> >  	struct mutex send_mutex;
> >  	void *send_receive_buf;
> > +	char *name;
> >  	wait_queue_head_t wait;
> >  	bool answer_available;
> >  	u8 protocol_major;
> > @@ -1087,6 +1088,7 @@ static void hidpp_input_close(struct input_dev *dev)
> >  static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
> >  {
> >  	struct input_dev *input_dev = devm_input_allocate_device(&hdev->dev);
> > +	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> >  
> >  	if (!input_dev)
> >  		return NULL;
> > @@ -1095,7 +1097,7 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
> >  	input_dev->open = hidpp_input_open;
> >  	input_dev->close = hidpp_input_close;
> >  
> > -	input_dev->name = hdev->name;
> > +	input_dev->name = hidpp->name;
> >  	input_dev->phys = hdev->phys;
> >  	input_dev->uniq = hdev->uniq;
> >  	input_dev->id.bustype = hdev->bus;
> > @@ -1137,22 +1139,28 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >  	hid_info(hdev, "HID++ %u.%u device connected.\n",
> >  		 hidpp->protocol_major, hidpp->protocol_minor);
> >  
> > +	if (!hidpp->name || hidpp->name == hdev->name) {
> 
> Hm, is it ever possible that hidpp->name is NULL? probe sets the pointer

No, hidpp->name should never be a NULL reference.
I asked myself about that (i.e. having a NULL reference until the hidpp
calls get_name), but I thought that having a non consistent name would
just confuse other contributors when implementing other devices.
So I choose to have always the current name of the device.

> to an array (hdev->name). Defence in depth I guess, but perhaps a
> comment could clarify this?

That could clarify it, yes. Will send a v2.

> 
> Otherwise the changes look OK. I have tested this situation:
> 
>  1. Insert receiver
>  2. Return a HID++ version for the WTP.
>  3. Return -9 (ResourceError) for the device name feature request (via
>     the QEMU emulation).
>  4. Observe that this fails.

Hehe, I have been testing this by timely putting the device in the on
then off state. About 1 sec of ON is enough to trigger the various
failures :)

> 
> So maybe you could add a comment for the above and add these tags:
> 
> Reviewed-by: Peter Wu <peter@lekensteyn.nl>
> Tested-by: Peter Wu <peter@lekensteyn.nl>

Thanks! (and thanks for the other v3)

Cheers,
Benjamin

> 
> Kind regards,
> Peter
> 
> > +		name = hidpp_get_device_name(hidpp);
> > +		if (!name) {
> > +			hid_err(hdev,
> > +				"unable to retrieve the name of the device");
> > +			return;
> > +		}
> > +
> > +		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
> > +		kfree(name);
> > +		if (!devm_name)
> > +			return;
> > +
> > +		hidpp->name = devm_name;
> > +	}
> > +
> >  	input = hidpp_allocate_input(hdev);
> >  	if (!input) {
> >  		hid_err(hdev, "cannot allocate new input device: %d\n", ret);
> >  		return;
> >  	}
> >  
> > -	name = hidpp_get_device_name(hidpp);
> > -	if (!name) {
> > -		hid_err(hdev, "unable to retrieve the name of the device");
> > -	} else {
> > -		devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s", name);
> > -		if (devm_name)
> > -			input->name = devm_name;
> > -		kfree(name);
> > -	}
> > -
> >  	hidpp_populate_input(hidpp, input, false);
> >  
> >  	ret = input_register_device(input);
> > @@ -1175,6 +1183,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >  		return -ENOMEM;
> >  
> >  	hidpp->hid_dev = hdev;
> > +	hidpp->name = hdev->name;
> >  	hid_set_drvdata(hdev, hidpp);
> >  
> >  	hidpp->quirks = id->driver_data;
> > 
> 

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Benjamin Tissoires]

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

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Peter Wu]

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.
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Benjamin Tissoires]

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
> 

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Peter Wu]

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;
> 

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Peter Wu]

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;
> > 

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Benjamin Tissoires]

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;
> > > 
> 

Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs