[PATCH 1/2] HID: wacom: Allow wacom_{get,set}_report to retry on -EAGAIN

[PATCH 1/2] HID: wacom: Allow wacom_{get,set}_report to retry on -EAGAIN

[Jason Gerecke]

If we get an -EAGAIN error within either of the wacom_{get,set}_report
functions, we should use one of our retries if available. Also, log an error
if the functions fail so that we can be aware of what's going on.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7abf52c..3cd74d9 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -35,7 +35,11 @@ static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
 	do {
 		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
 				HID_REQ_GET_REPORT);
-	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
+	} while ((retval == -ETIMEDOUT || retval == -EPIPE || retval == -EAGAIN) && --retries);
+
+	if (retval < 0)
+		dev_err(&hdev->dev, "wacom_get_report: ran out of retries "
+			"(last error = %d)\n", retval);
 
 	return retval;
 }
@@ -48,7 +52,11 @@ static int wacom_set_report(struct hid_device *hdev, u8 type, u8 *buf,
 	do {
 		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
 				HID_REQ_SET_REPORT);
-	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
+	} while ((retval == -ETIMEDOUT || retval == -EPIPE || retval == -EAGAIN) && --retries);
+
+	if (retval < 0)
+		dev_err(&hdev->dev, "wacom_set_report: ran out of retries "
+			"(last error = %d)\n", retval);
 
 	return retval;
 }
-- 
2.4.0

[PATCH 2/2] HID: wacom: Handle failing HID_DG_CONTACTMAX requests

[Jason Gerecke]

Hardware may not respond to a request for the HID_DG_CONTACTMAX feature and
we should be tolerant of such a failure. This is especially true when using
hid-replay where the hardware doesn't exist, but also for devices attached
to a flaky bus. This patch increases the number of allowable retries to
match other calls to 'wacom_get_report' and also provides a fallback which
forces 'touch_max = 16' (enough for any Wacom device seen so far).

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3cd74d9..c9a7e30 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -125,9 +125,17 @@ static void wacom_feature_mapping(struct hid_device *hdev,
 				break;
 			data[0] = field->report->id;
 			ret = wacom_get_report(hdev, HID_FEATURE_REPORT,
-						data, 2, 0);
-			if (ret == 2)
+						data, 2, WAC_CMD_RETRIES);
+			if (ret == 2) {
 				features->touch_max = data[1];
+			}
+			else {
+				features->touch_max = 16;
+				dev_warn(&hdev->dev, "wacom_feature_mapping: "
+					 "could not get HID_DG_CONTACTMAX, "
+					 "defaulting to %d\n",
+					  features->touch_max);
+			}
 			kfree(data);
 		}
 		break;
-- 
2.4.0

Re: [PATCH 1/2] HID: wacom: Allow wacom_{get,set}_report to retry on -EAGAIN

[Benjamin Tissoires]

Hi Jason,

On May 08 2015 or thereabouts, Jason Gerecke wrote:
> If we get an -EAGAIN error within either of the wacom_{get,set}_report
> functions, we should use one of our retries if available. Also, log an error
> if the functions fail so that we can be aware of what's going on.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/wacom_sys.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 7abf52c..3cd74d9 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -35,7 +35,11 @@ static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
>  	do {
>  		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
>  				HID_REQ_GET_REPORT);
> -	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
> +	} while ((retval == -ETIMEDOUT || retval == -EPIPE || retval == -EAGAIN) && --retries);

I am not sure we should retry if the error is different than -EAGAIN.
TIMEDOUT might be possible but IMO, EPIPE means there is something wrong
and the retry will likely fail too.

> +
> +	if (retval < 0)
> +		dev_err(&hdev->dev, "wacom_get_report: ran out of retries "

nitpick but you could use "hid_err(hdev" instead of a plain dev_err.

> +			"(last error = %d)\n", retval);
>  
>  	return retval;
>  }
> @@ -48,7 +52,11 @@ static int wacom_set_report(struct hid_device *hdev, u8 type, u8 *buf,
>  	do {
>  		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
>  				HID_REQ_SET_REPORT);
> -	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
> +	} while ((retval == -ETIMEDOUT || retval == -EPIPE || retval == -EAGAIN) && --retries);
> +
> +	if (retval < 0)
> +		dev_err(&hdev->dev, "wacom_set_report: ran out of retries "
> +			"(last error = %d)\n", retval);

dito

Cheers,
Benjamin

>  
>  	return retval;
>  }
> -- 
> 2.4.0
> 

Re: [PATCH 2/2] HID: wacom: Handle failing HID_DG_CONTACTMAX requests

[Benjamin Tissoires]

On May 08 2015 or thereabouts, Jason Gerecke wrote:
> Hardware may not respond to a request for the HID_DG_CONTACTMAX feature and
> we should be tolerant of such a failure. This is especially true when using
> hid-replay where the hardware doesn't exist, but also for devices attached
> to a flaky bus. This patch increases the number of allowable retries to
> match other calls to 'wacom_get_report' and also provides a fallback which
> forces 'touch_max = 16' (enough for any Wacom device seen so far).
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---

Just a small nitpick given that I already asked for a v2 on 1/2:

>  drivers/hid/wacom_sys.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 3cd74d9..c9a7e30 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -125,9 +125,17 @@ static void wacom_feature_mapping(struct hid_device *hdev,
>  				break;
>  			data[0] = field->report->id;
>  			ret = wacom_get_report(hdev, HID_FEATURE_REPORT,
> -						data, 2, 0);
> -			if (ret == 2)
> +						data, 2, WAC_CMD_RETRIES);
> +			if (ret == 2) {
>  				features->touch_max = data[1];
> +			}
> +			else {

the else should be on the line above :)

> +				features->touch_max = 16;
> +				dev_warn(&hdev->dev, "wacom_feature_mapping: "

hid_warn(hdev...

> +					 "could not get HID_DG_CONTACTMAX, "
> +					 "defaulting to %d\n",
> +					  features->touch_max);
> +			}
>  			kfree(data);
>  		}
>  		break;
> -- 
> 2.4.0
> 

Cheers,
Benjamin

[PATCH v2 1/2] HID: wacom: Have wacom_{get,set}_report retry on -EAGAIN, not -EPIPE

[Jason Gerecke]

Retrying on -EPIPE makes very little sense since this typically indicates
a problem that will not just disappear on its own. For instance, the USB
documentation states that it will be sent if the endpoint is stalled or
the device has disconnected. Instead, we should retry if -EAGAIN is
received since this indicates a temporary error condition such as a busy
bus.

In addition to adjusting the conditions we retry under, we also log an
error on failure so that we can be aware of what's going on.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changed in v2:
 - Renamed from "HID: wacom: Allow wacom_{get,set}_report to retry on -EAGAIN"
 - Changed retry conditions so that we no longer retry on -EPIPE
 - Use 'hid_err' instead of 'dev_err'

 drivers/hid/wacom_sys.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7abf52c..109312f 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -35,7 +35,11 @@ static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
 	do {
 		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
 				HID_REQ_GET_REPORT);
-	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
+	} while ((retval == -ETIMEDOUT || retval == -EAGAIN) && --retries);
+
+	if (retval < 0)
+		hid_err(hdev, "wacom_get_report: ran out of retries "
+			"(last error = %d)\n", retval);
 
 	return retval;
 }
@@ -48,7 +52,11 @@ static int wacom_set_report(struct hid_device *hdev, u8 type, u8 *buf,
 	do {
 		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
 				HID_REQ_SET_REPORT);
-	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
+	} while ((retval == -ETIMEDOUT || retval == -EAGAIN) && --retries);
+
+	if (retval < 0)
+		hid_err(hdev, "wacom_set_report: ran out of retries "
+			"(last error = %d)\n", retval);
 
 	return retval;
 }
-- 
2.4.1

[PATCH v2 2/2] HID: wacom: Handle failing HID_DG_CONTACTMAX requests

[Jason Gerecke]

Hardware may not respond to a request for the HID_DG_CONTACTMAX feature and
we should be tolerant of such a failure. This is especially true when using
hid-replay where the hardware doesn't exist, but also for devices attached
to a flaky bus. This patch increases the number of allowable retries to
match other calls to 'wacom_get_report' and also provides a fallback which
forces 'touch_max = 16' (enough for any Wacom device seen so far).

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changed in v2:
 - Fixed placement of 'else'
 - Use 'hid_warn' instead of 'dev_warn'

 drivers/hid/wacom_sys.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 109312f..eea18a6 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -125,9 +125,16 @@ static void wacom_feature_mapping(struct hid_device *hdev,
 				break;
 			data[0] = field->report->id;
 			ret = wacom_get_report(hdev, HID_FEATURE_REPORT,
-						data, 2, 0);
-			if (ret == 2)
+						data, 2, WAC_CMD_RETRIES);
+			if (ret == 2) {
 				features->touch_max = data[1];
+			} else {
+				features->touch_max = 16;
+				hid_warn(hdev, "wacom_feature_mapping: "
+					 "could not get HID_DG_CONTACTMAX, "
+					 "defaulting to %d\n",
+					  features->touch_max);
+			}
 			kfree(data);
 		}
 		break;
-- 
2.4.1

Re: [PATCH v2 1/2] HID: wacom: Have wacom_{get,set}_report retry on -EAGAIN, not -EPIPE

[Benjamin Tissoires]

On May 21 2015 or thereabouts, Jason Gerecke wrote:
> Retrying on -EPIPE makes very little sense since this typically indicates
> a problem that will not just disappear on its own. For instance, the USB
> documentation states that it will be sent if the endpoint is stalled or
> the device has disconnected. Instead, we should retry if -EAGAIN is
> received since this indicates a temporary error condition such as a busy
> bus.
> 
> In addition to adjusting the conditions we retry under, we also log an
> error on failure so that we can be aware of what's going on.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---

The series is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks for the respin Jason!

Cheers,
Benjamin

> Changed in v2:
>  - Renamed from "HID: wacom: Allow wacom_{get,set}_report to retry on -EAGAIN"
>  - Changed retry conditions so that we no longer retry on -EPIPE
>  - Use 'hid_err' instead of 'dev_err'
> 
>  drivers/hid/wacom_sys.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 7abf52c..109312f 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -35,7 +35,11 @@ static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
>  	do {
>  		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
>  				HID_REQ_GET_REPORT);
> -	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
> +	} while ((retval == -ETIMEDOUT || retval == -EAGAIN) && --retries);
> +
> +	if (retval < 0)
> +		hid_err(hdev, "wacom_get_report: ran out of retries "
> +			"(last error = %d)\n", retval);
>  
>  	return retval;
>  }
> @@ -48,7 +52,11 @@ static int wacom_set_report(struct hid_device *hdev, u8 type, u8 *buf,
>  	do {
>  		retval = hid_hw_raw_request(hdev, buf[0], buf, size, type,
>  				HID_REQ_SET_REPORT);
> -	} while ((retval == -ETIMEDOUT || retval == -EPIPE) && --retries);
> +	} while ((retval == -ETIMEDOUT || retval == -EAGAIN) && --retries);
> +
> +	if (retval < 0)
> +		hid_err(hdev, "wacom_set_report: ran out of retries "
> +			"(last error = %d)\n", retval);
>  
>  	return retval;
>  }
> -- 
> 2.4.1
> 

Re: [PATCH v2 1/2] HID: wacom: Have wacom_{get,set}_report retry on -EAGAIN, not -EPIPE

[Jiri Kosina]

On Thu, 21 May 2015, Jason Gerecke wrote:

> Retrying on -EPIPE makes very little sense since this typically indicates
> a problem that will not just disappear on its own. For instance, the USB
> documentation states that it will be sent if the endpoint is stalled or
> the device has disconnected. Instead, we should retry if -EAGAIN is
> received since this indicates a temporary error condition such as a busy
> bus.
> 
> In addition to adjusting the conditions we retry under, we also log an
> error on failure so that we can be aware of what's going on.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

I have applied the series to for-4.2/wacom. Thanks,

-- 
Jiri Kosina
SUSE Labs