[PATCH 0/4] HID: wacom: fixes for next

[PATCH 0/4] HID: wacom: fixes for next

[Benjamin Tissoires]

Hi,

well, these are non critical but nonetheless interesting fixes for
Wacom I was working on last week. I was waiting for the current v4.10
fix to be sent by Jason before pushing those out.

Cheers,
Benjamin

Benjamin Tissoires (4):
  HID: wacom: release the resources before leaving despite devm
  HID: wacom: remove warning while disconnecting devices
  HID: wacom: do not attempt to switch mode while in probe
  HID: wacom: do not shout an error on LED control

 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c | 42 ++++++++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 12 deletions(-)

-- 
2.9.3

[PATCH 1/4] HID: wacom: release the resources before leaving despite devm

[Benjamin Tissoires]

In the general case, the resources are properly released by devm without
needing to do anything. However, when unplugging the wireless receiver,
the kernel segfaults from time to time while calling devres_release_all().

I think in that case the resources attempt to access hid_get_drvdata(hdev)
which has been set to null while leaving wacom_remove().

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 23b272a..734100a 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2495,6 +2495,8 @@ static void wacom_remove(struct hid_device *hdev)
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 
+	wacom_release_resources(wacom);
+
 	hid_set_drvdata(hdev, NULL);
 }
 
-- 
2.9.3

[PATCH 2/4] HID: wacom: remove warning while disconnecting devices

[Benjamin Tissoires]

When the LED class gets removed, it actually tries to reset the LED.
However, the device being disconnected, the set_report fails.

Previously, the attempt to cut lose this last event was through unsetting
the HID drvdata, but it was not working properly. Simply reset the LED
groups to NULL makes a more efficient solution.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 734100a..4a2c88d 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -757,9 +757,6 @@ static int wacom_led_control(struct wacom *wacom)
 	unsigned char report_id = WAC_CMD_LED_CONTROL;
 	int buf_size = 9;
 
-	if (!hid_get_drvdata(wacom->hdev))
-		return -ENODEV;
-
 	if (!wacom->led.groups)
 		return -ENOTSUPP;
 
@@ -2495,6 +2492,8 @@ static void wacom_remove(struct hid_device *hdev)
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 
+	/* make sure we don't trigger the LEDs */
+	wacom_led_groups_release(wacom);
 	wacom_release_resources(wacom);
 
 	hid_set_drvdata(hdev, NULL);
-- 
2.9.3

[PATCH 3/4] HID: wacom: do not attempt to switch mode while in probe

[Benjamin Tissoires]

The Intuos Pro seems to not like when we set the features right after
being powered up. Instead of waiting during probe, we can schedule the
switch mode and LED control in a deferred worker so that we don't have the
5 secs of delay from USB when the device is not accessible.

The USB timeout delays were really a pain because if you happen to unplug
the tablet while it is still waiting, you are just adding 5 second timeouts
to the USB stack. Which means that a new plug of the same tablet will also
gets delayed, and will also attempt to access the hardware while in
.probe(). So the tablet doesn't appear in the dmesg, the user unplug/replug
it to make it appearing... and so on so forth.

Really, this is for the best :)

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 020956c..3f650ce6 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -165,6 +165,7 @@ struct wacom {
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
 	struct work_struct remote_work;
+	struct delayed_work init_work;
 	struct wacom_remote *remote;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4a2c88d..6acb422 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -497,11 +497,11 @@ static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed,
  * from the tablet, it is necessary to switch the tablet out of this
  * mode and into one which sends the full range of tablet data.
  */
-static int wacom_query_tablet_data(struct hid_device *hdev,
-		struct wacom_features *features)
+static int _wacom_query_tablet_data(struct wacom *wacom)
 {
-	struct wacom *wacom = hid_get_drvdata(hdev);
+	struct hid_device *hdev = wacom->hdev;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_features *features = &wacom_wac->features;
 
 	if (hdev->bus == BUS_BLUETOOTH)
 		return wacom_bt_query_tablet_data(hdev, 1, features);
@@ -1437,11 +1437,23 @@ static int wacom_initialize_leds(struct wacom *wacom)
 			"cannot create sysfs group err: %d\n", error);
 		return error;
 	}
-	wacom_led_control(wacom);
 
 	return 0;
 }
 
+static void wacom_init_work(struct work_struct *work)
+{
+	struct wacom *wacom = container_of(work, struct wacom, init_work.work);
+
+	_wacom_query_tablet_data(wacom);
+	wacom_led_control(wacom);
+}
+
+static void wacom_query_tablet_data(struct wacom *wacom)
+{
+	schedule_delayed_work(&wacom->init_work, msecs_to_jiffies(1000));
+}
+
 static enum power_supply_property wacom_battery_props[] = {
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -2115,7 +2127,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 
 	if (!wireless) {
 		/* Note that if query fails it is not a hard failure */
-		wacom_query_tablet_data(hdev, features);
+		wacom_query_tablet_data(wacom);
 	}
 
 	/* touch only Bamboo doesn't support pen */
@@ -2445,6 +2457,7 @@ static int wacom_probe(struct hid_device *hdev,
 	wacom->usbdev = dev;
 	wacom->intf = intf;
 	mutex_init(&wacom->lock);
+	INIT_DELAYED_WORK(&wacom->init_work, wacom_init_work);
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 	INIT_WORK(&wacom->remote_work, wacom_remote_work);
@@ -2486,6 +2499,7 @@ static void wacom_remove(struct hid_device *hdev)
 
 	hid_hw_stop(hdev);
 
+	cancel_delayed_work_sync(&wacom->init_work);
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
 	cancel_work_sync(&wacom->remote_work);
@@ -2503,12 +2517,11 @@ static void wacom_remove(struct hid_device *hdev)
 static int wacom_resume(struct hid_device *hdev)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
-	struct wacom_features *features = &wacom->wacom_wac.features;
 
 	mutex_lock(&wacom->lock);
 
 	/* switch to wacom mode first */
-	wacom_query_tablet_data(hdev, features);
+	_wacom_query_tablet_data(wacom);
 	wacom_led_control(wacom);
 
 	mutex_unlock(&wacom->lock);
-- 
2.9.3

[PATCH 4/4] HID: wacom: do not shout an error on LED control

[Benjamin Tissoires]

At plug, the tablet seems to output a -EPIPE when first accessing the
LED. The weird part is that the command is taken into account by the
tablet, but we shout an error in the dmesg.

Cut off the error so that users are happier.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 6acb422..6acf3a3 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -803,8 +803,12 @@ static int wacom_led_control(struct wacom *wacom)
 		buf[4] = wacom->led.img_lum;
 	}
 
-	retval = wacom_set_report(wacom->hdev, HID_FEATURE_REPORT, buf, buf_size,
-				  WAC_CMD_RETRIES);
+	/*
+	 * we do not use wacom_set_report because -EPIPE happens but is
+	 * not fatal, so do not shout something at the user.
+	 */
+	retval = hid_hw_raw_request(wacom->hdev, buf[0], buf, buf_size,
+				    HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	kfree(buf);
 
 	return retval;
-- 
2.9.3

Re: [PATCH 4/4] HID: wacom: do not shout an error on LED control

[Jason Gerecke]

I'm not super comfortable with hiding this error. If the tablet reacts
appropriately, I wouldn't think we'd get an EPIPE. Although its not
fatal, it still might be a symptom of something deeper...

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....



On Fri, Jan 20, 2017 at 7:20 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> At plug, the tablet seems to output a -EPIPE when first accessing the
> LED. The weird part is that the command is taken into account by the
> tablet, but we shout an error in the dmesg.
>
> Cut off the error so that users are happier.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/wacom_sys.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 6acb422..6acf3a3 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -803,8 +803,12 @@ static int wacom_led_control(struct wacom *wacom)
>                 buf[4] = wacom->led.img_lum;
>         }
>
> -       retval = wacom_set_report(wacom->hdev, HID_FEATURE_REPORT, buf, buf_size,
> -                                 WAC_CMD_RETRIES);
> +       /*
> +        * we do not use wacom_set_report because -EPIPE happens but is
> +        * not fatal, so do not shout something at the user.
> +        */
> +       retval = hid_hw_raw_request(wacom->hdev, buf[0], buf, buf_size,
> +                                   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>         kfree(buf);
>
>         return retval;
> --
> 2.9.3
>

Re: [PATCH 0/4] HID: wacom: fixes for next

[Jason Gerecke]

Patches 1/3 look reasonable to me, though I've not run into the bugs
they aim to fix. For those:

Acked-by: Jason Gerecke <jason.gerecke@wacom.com>

As for patch 4, I have some additional reservations about hiding the
message... We can discuss that further in its thread.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....



On Fri, Jan 20, 2017 at 7:20 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi,
>
> well, these are non critical but nonetheless interesting fixes for
> Wacom I was working on last week. I was waiting for the current v4.10
> fix to be sent by Jason before pushing those out.
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (4):
>   HID: wacom: release the resources before leaving despite devm
>   HID: wacom: remove warning while disconnecting devices
>   HID: wacom: do not attempt to switch mode while in probe
>   HID: wacom: do not shout an error on LED control
>
>  drivers/hid/wacom.h     |  1 +
>  drivers/hid/wacom_sys.c | 42 ++++++++++++++++++++++++++++++------------
>  2 files changed, 31 insertions(+), 12 deletions(-)
>
> --
> 2.9.3
>

Re: [PATCH 0/4] HID: wacom: fixes for next

[Benjamin Tissoires]

On Jan 20 2017 or thereabouts, Jason Gerecke wrote:
> Patches 1/3 look reasonable to me, though I've not run into the bugs
> they aim to fix. For those:
> 
> Acked-by: Jason Gerecke <jason.gerecke@wacom.com>
> 
> As for patch 4, I have some additional reservations about hiding the
> message... We can discuss that further in its thread.
> 

Yeah, I am fine dropping patch 4. Thanks for the review/tests Jason!

Cheers,
Benjamin

> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one  /
> (That is to say, eight) to the two,     /
> But you can’t take seven from three,    /
> So you look at the sixty-fours....
> 
> 
> 
> On Fri, Jan 20, 2017 at 7:20 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > Hi,
> >
> > well, these are non critical but nonetheless interesting fixes for
> > Wacom I was working on last week. I was waiting for the current v4.10
> > fix to be sent by Jason before pushing those out.
> >
> > Cheers,
> > Benjamin
> >
> > Benjamin Tissoires (4):
> >   HID: wacom: release the resources before leaving despite devm
> >   HID: wacom: remove warning while disconnecting devices
> >   HID: wacom: do not attempt to switch mode while in probe
> >   HID: wacom: do not shout an error on LED control
> >
> >  drivers/hid/wacom.h     |  1 +
> >  drivers/hid/wacom_sys.c | 42 ++++++++++++++++++++++++++++++------------
> >  2 files changed, 31 insertions(+), 12 deletions(-)
> >
> > --
> > 2.9.3
> >

Re: [PATCH 0/4] HID: wacom: fixes for next

[Jiri Kosina]

On Mon, 23 Jan 2017, Benjamin Tissoires wrote:

> > Patches 1/3 look reasonable to me, though I've not run into the bugs
> > they aim to fix. For those:
> > 
> > Acked-by: Jason Gerecke <jason.gerecke@wacom.com>
> > 
> > As for patch 4, I have some additional reservations about hiding the
> > message... We can discuss that further in its thread.
> > 
> 
> Yeah, I am fine dropping patch 4. Thanks for the review/tests Jason!

I've applied patches 1-3. Thanks,

-- 
Jiri Kosina
SUSE Labs