[PATCH v2 17/33] HID: logitech-hidpp: return negative error codes from hidpp_send_message_sync()

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Low-level synchronous send helpers previously returned 0 on success,
negative error numbers on transport/timeout errors, and positive values
on device protocol errors. Callers across the driver were burdened with
checking if (ret > 0) to convert protocol errors into -EPROTO or -EIO,
leading to scattered, inconsistent error translation.

Translate HID++ 1.0 and HID++ 2.0 error reports directly into standard
negative error codes (-EBUSY, -EIO, -EPROTO) in the core synchronous
send helper, simplify retry handling in hidpp_send_message_sync() to
check ret == -EBUSY, and remove redundant protocol error conversion
boilerplate across all callers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 158 +++++++++------------------------------
 1 file changed, 37 insertions(+), 121 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3745d95f5bba..cf42626ab98d 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -287,10 +287,9 @@ static int __hidpp_send_report(struct hid_device *hdev,
  *
  * Must be called with hidpp->send_mutex locked
  *
- * Same return protocol than hidpp_send_message_sync():
- * - success on 0
- * - negative error means transport error
- * - positive value means protocol error
+ * Same return protocol as hidpp_send_message_sync():
+ * - 0 on success
+ * - negative error code on failure
  */
 static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	struct hidpp_report *message,
@@ -331,6 +330,18 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	    response->rap.sub_id == HIDPP_ERROR) {
 		ret = response->rap.params[1];
 		dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+		switch (ret) {
+		case HIDPP_ERROR_BUSY:
+			ret = -EBUSY;
+			break;
+		case HIDPP_ERROR_RESOURCE_ERROR:
+		case HIDPP_ERROR_UNKNOWN_DEVICE:
+			ret = -EIO;
+			break;
+		default:
+			ret = -EPROTO;
+			break;
+		}
 		goto out;
 	}
 
@@ -339,6 +350,14 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	    response->fap.feature_index == HIDPP20_ERROR) {
 		ret = response->fap.params[1];
 		dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
+		switch (ret) {
+		case HIDPP20_ERROR_BUSY:
+			ret = -EBUSY;
+			break;
+		default:
+			ret = -EPROTO;
+			break;
+		}
 		goto out;
 	}
 
@@ -350,11 +369,8 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_message_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_message_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 	struct hidpp_report *message,
@@ -367,18 +383,10 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 
 	do {
 		ret = __do_hidpp_send_message_sync(hidpp, message, response);
-		if (ret <= 0)
+		if (ret != -EBUSY)
 			break;
 
-		if (response->report_id == REPORT_ID_HIDPP_SHORT &&
-		    ret != HIDPP_ERROR_BUSY)
-			break;
-		if ((response->report_id == REPORT_ID_HIDPP_LONG ||
-		     response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
-		    ret != HIDPP20_ERROR_BUSY)
-			break;
-
-		dbg_hid("%s:got busy hidpp error %02X, retrying\n", __func__, ret);
+		dbg_hid("%s:got busy hidpp error, retrying\n", __func__);
 	} while (--max_retries);
 
 	mutex_unlock(&hidpp->send_mutex);
@@ -387,11 +395,8 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_fap_command_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_fap_command_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 	u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count,
@@ -426,11 +431,8 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_rap_command_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_rap_command_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
 	u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count,
@@ -988,22 +990,14 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
 			CMD_ROOT_GET_PROTOCOL_VERSION | LINUX_KERNEL_SW_ID,
 			ping_data, sizeof(ping_data), &response);
 
-	if (ret == HIDPP_ERROR_INVALID_SUBID) {
+	if (ret == -EPROTO &&
+	    response.rap.sub_id == HIDPP_ERROR &&
+	    response.rap.params[1] == HIDPP_ERROR_INVALID_SUBID) {
 		hidpp->protocol_major = 1;
 		hidpp->protocol_minor = 0;
 		goto print_version;
 	}
 
-	/* the device might not be connected */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR ||
-	    ret == HIDPP_ERROR_UNKNOWN_DEVICE)
-		return -EIO;
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1091,12 +1085,6 @@ static int hidpp_devicenametype_get_count(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_GET_DEVICE_NAME_TYPE_GET_COUNT, NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1115,12 +1103,6 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME, &char_index, 1,
 		&response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1273,14 +1255,6 @@ static int hidpp20_batterylevel_get_battery_capacity(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1302,11 +1276,6 @@ static int hidpp20_batterylevel_get_battery_info(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY,
 					  NULL, 0, &response);
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1456,12 +1425,6 @@ static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
 					  NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1604,14 +1567,6 @@ static int hidpp20_unifiedbattery_get_capabilities(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_UNIFIED_BATTERY_GET_CAPABILITIES,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1694,14 +1649,6 @@ static int hidpp20_unifiedbattery_get_status(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_UNIFIED_BATTERY_GET_STATUS,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1958,13 +1905,8 @@ static bool hidpp20_get_adc_measurement_1f20(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT,
 					  NULL, 0, &response);
-
-	if (ret) {
-		if (ret > 0)
-			hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-				__func__, ret);
+	if (ret)
 		return false;
-	}
 
 	*status = hidpp20_map_adc_measurement_1f20(params, voltage);
 	return true;
@@ -2138,11 +2080,6 @@ static int hidpp_solar_request_battery_event(struct hidpp_device *hidpp)
 					  hidpp->battery.solar_feature_index,
 					  CMD_SOLAR_SET_LIGHT_MEASURE,
 					  params, 2, &response);
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -2230,12 +2167,6 @@ static int hidpp_touchpad_fw_items_set(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_TOUCHPAD_FW_ITEMS_SET, &items->state, 1, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -2300,12 +2231,6 @@ static int hidpp_touchpad_get_raw_info(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_TOUCHPAD_GET_RAW_INFO, NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -3493,13 +3418,8 @@ static int g920_get_config(struct hidpp_device *hidpp,
 					  HIDPP_FF_GET_INFO,
 					  NULL, 0,
 					  &response);
-	if (ret) {
-		if (ret < 0)
-			return ret;
-		hid_err(hidpp->hid_dev,
-			"%s: received protocol error 0x%02x\n", __func__, ret);
-		return -EPROTO;
-	}
+	if (ret)
+		return ret;
 
 	data->num_effects = response.fap.params[0];
 
@@ -3702,8 +3622,6 @@ static int hidpp20_reprog_controls_get_count(struct hidpp_device *hidpp)
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, NULL, 0,
 					  &response);
-	if (ret > 0)
-		return -EPROTO;
 	if (ret)
 		return ret;
 
@@ -3721,8 +3639,6 @@ static int hidpp20_reprog_controls_get_cid_info(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, &index,
 					  sizeof(index), &response);
-	if (ret > 0)
-		return -EPROTO;
 	if (ret)
 		return ret;
 

-- 
2.56.0.542.g3ef89b7020-goog