[PATCH v11 12/18] HID: asus: size feature reports from the HID report descriptor

[Denis Benato]

From: Denis Benato <benato.denis96@xxxxxxxxx>

The ASUS vendor protocol uses the same feature report IDs across very
different devices: on N-KEY keyboards and ROG Ally report 0x5A/0x5E are
64-byte reports, while on XG Mobile stations report 0x5E is a 300-byte
report. The driver currently hardcodes transfer lengths (16 bytes for
the handshake, FEATURE_KBD_REPORT_SIZE for reads, ROG_XGM_REPORT_SIZE
for the XG Mobile LED), which mismatches what the hardware expects and
would break if a new device used yet another size for the same report
ID.

Derive the transfer length of every feature report from the HID report
descriptor of the device at hand:

- add asus_report_len(), which resolves a report ID to its struct
  hid_report and returns hid_report_len();
- asus_kbd_set_report() now zero-pads short payloads to the exact
  report size and rejects payloads that do not fit the report;
- add asus_kbd_get_report() and use it from asus_kbd_init() and
  asus_kbd_get_functions() so reads are sized per report as well;
- asus_xgm_led_set() sends a minimal payload and checks the transfer
  result against the descriptor length instead of the hardcoded
  ROG_XGM_REPORT_SIZE, which is removed along with the fixed-size
  buffers in asus_kbd_set_fn_lock() and asus_kbd_set_brightness().

Captured USB traffic of the same commands issued by Windows shows they
are always written at the full report length, which this change now
matches.

Signed-off-by: Denis Benato <denis.benato@xxxxxxxxx>
---
 drivers/hid/hid-asus.c | 148 +++++++++++++++++++++++++++++++++--------
 1 file changed, 120 insertions(+), 28 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index ea27495c3df6..fe2a326ccd86 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -63,8 +63,6 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define FEATURE_KBD_LED_REPORT_ID1 0x5d
 #define FEATURE_KBD_LED_REPORT_ID2 0x5e
 
-#define ROG_XGM_REPORT_SIZE 300
-
 #define ROG_ALLY_REPORT_SIZE 64
 #define ROG_ALLY_X_MIN_MCU 313
 #define ROG_ALLY_MIN_MCU 319
@@ -658,6 +656,51 @@ static void ally_probe_release_role(int ep)
 	spin_unlock_irqrestore(&ally_data_lock, flags);
 }
 
+/**
+ * asus_find_report() - Find a report of the given type by its ID
+ * @hdev: HID device
+ * @type: report type (HID_INPUT_REPORT, HID_OUTPUT_REPORT or
+ * HID_FEATURE_REPORT)
+ * @report_id: report ID to look for
+ *
+ * Return: the report with the given ID, or NULL if the device does not
+ * declare such a report
+ */
+static struct hid_report *asus_find_report(struct hid_device *hdev,
+					   unsigned int type, u8 report_id)
+{
+	struct hid_report *report;
+
+	list_for_each_entry(report, &hdev->report_enum[type].report_list, list) {
+		if (report->id == report_id)
+			return report;
+	}
+
+	return NULL;
+}
+
+/**
+ * asus_report_len() - Length of a feature report for the device
+ * @hdev: HID device
+ * @report_id: report ID of the feature report
+ *
+ * The same ASUS report ID can have different sizes depending on the device:
+ * report 0x5E is a 64-byte report on laptops but a 300-byte report on XG
+ * Mobile stations. Always size transfers from the report descriptor of the
+ * device at hand instead of hardcoding lengths.
+ *
+ * Return: the length of the feature report in bytes, or 0 if the device
+ * does not declare the report
+ */
+static size_t asus_report_len(struct hid_device *hdev, u8 report_id)
+{
+	struct hid_report *report;
+
+	report = asus_find_report(hdev, HID_FEATURE_REPORT, report_id);
+
+	return report ? hid_report_len(report) : 0;
+}
+
 static const u8 asus_report_id_init[] = {
 	FEATURE_KBD_REPORT_ID,
 	FEATURE_KBD_LED_REPORT_ID1,
@@ -5370,18 +5413,64 @@ static int asus_raw_event(struct hid_device *hdev,
 
 static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t buf_size)
 {
-	u8 *dmabuf __free(kfree) = kmemdup(buf, buf_size, GFP_KERNEL);
+	size_t hw_len = asus_report_len(hdev, buf[0]);
+
+	/* The payload (including the report ID) must fit the device report. */
+	if (!hw_len || buf_size > hw_len)
+		return -EINVAL;
+
+	u8 *dmabuf __free(kfree) = kzalloc(hw_len, GFP_KERNEL);
 	if (!dmabuf)
 		return -ENOMEM;
 
+	memcpy(dmabuf, buf, buf_size);
+
 	/*
-	 * The report ID should be set from the incoming buffer due to LED and key
-	 * interfaces having different pages
+	 * The report ID should be set from the incoming buffer due to LED and
+	 * key interfaces having different pages. Short payloads are zero-padded
+	 * to the exact report size the hardware expects for this report ID.
 	 */
-	return hid_hw_raw_request(hdev, buf[0], dmabuf, buf_size, HID_FEATURE_REPORT,
+	return hid_hw_raw_request(hdev, buf[0], dmabuf, hw_len, HID_FEATURE_REPORT,
 				  HID_REQ_SET_REPORT);
 }
 
+/**
+ * asus_kbd_get_report() - Read a report sized from the descriptor
+ * @hdev: HID device
+ * @report_id: report ID to read
+ * @buf: buffer to store the report in
+ * @buf_size: size of @buf
+ *
+ * The transfer is performed at the exact report size the device declares
+ * for @report_id. The device may answer with a short read, and only the
+ * bytes that fit in @buf are copied into it.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+static int asus_kbd_get_report(struct hid_device *hdev, u8 report_id,
+			       u8 *buf, size_t buf_size)
+{
+	size_t hw_len = asus_report_len(hdev, report_id);
+	int ret;
+
+	if (!hw_len)
+		return -EINVAL;
+
+	u8 *dmabuf __free(kfree) = kzalloc(hw_len, GFP_KERNEL);
+	if (!dmabuf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(hdev, report_id, dmabuf, hw_len,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0)
+		return ret;
+
+	/* The device may return less than requested (short read). */
+	memcpy(buf, dmabuf, min(buf_size, hw_len));
+
+	return 0;
+}
+
 static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
 {
 	/*
@@ -5390,8 +5479,13 @@ static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
 	 */
 	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
 		     0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+	size_t hw_len = asus_report_len(hdev, report_id);
+	size_t cmp_len;
 	int ret;
 
+	if (!hw_len)
+		return -EINVAL;
+
 	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
 	if (ret < 0) {
 		hid_err(hdev, "Asus handshake %02x failed to send: %d\n",
@@ -5399,19 +5493,20 @@ static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
 		return ret;
 	}
 
-	u8 *readbuf __free(kfree) = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+	u8 *readbuf __free(kfree) = kzalloc(hw_len, GFP_KERNEL);
 	if (!readbuf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(hdev, report_id, readbuf,
-				 FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
-				 HID_REQ_GET_REPORT);
+	ret = asus_kbd_get_report(hdev, report_id, readbuf, hw_len);
 	if (ret < 0) {
 		hid_warn(hdev, "Asus handshake %02x failed to receive ack: %d\n",
 			 report_id, ret);
-	} else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
-		hid_warn(hdev, "Asus handshake %02x returned invalid response: %*ph\n",
-			 report_id, FEATURE_KBD_REPORT_SIZE, readbuf);
+	} else {
+		cmp_len = min_t(size_t, sizeof(buf), hw_len);
+		if (memcmp(readbuf, buf, cmp_len) != 0) {
+			hid_warn(hdev, "Asus handshake %02x returned invalid response: %*ph\n",
+				 report_id, (int)cmp_len, readbuf);
+		}
 	}
 
 	/*
@@ -5426,7 +5521,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
 				  u8 report_id)
 {
 	const u8 buf[] = { report_id, 0x05, 0x20, 0x31, 0x00, 0x08 };
-	u8 *readbuf;
+	size_t hw_len = asus_report_len(hdev, report_id);
 	int ret;
 
 	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
@@ -5435,23 +5530,22 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
 		return ret;
 	}
 
-	readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+	if (!hw_len)
+		return -EINVAL;
+
+	u8 *readbuf __free(kfree) = kzalloc(hw_len, GFP_KERNEL);
 	if (!readbuf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(hdev, report_id, readbuf,
-				 FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
-				 HID_REQ_GET_REPORT);
+	ret = asus_kbd_get_report(hdev, report_id, readbuf, hw_len);
 	if (ret < 0) {
 		hid_err(hdev, "Asus failed to request functions: %d\n", ret);
-		kfree(readbuf);
 		return ret;
 	}
 
 	*kbd_func = readbuf[6];
 
-	kfree(readbuf);
-	return ret;
+	return 0;
 }
 
 static int asus_kbd_disable_oobe(struct hid_device *hdev)
@@ -5476,7 +5570,7 @@ static int asus_kbd_disable_oobe(struct hid_device *hdev)
 
 static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
 {
-	const u8 buf[FEATURE_KBD_REPORT_SIZE] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
+	const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xd0, 0x4e, !!enabled };
 	int ret;
 
 	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
@@ -5486,7 +5580,7 @@ static void asus_kbd_set_fn_lock(struct hid_device *hdev, bool enabled)
 
 static void asus_kbd_set_brightness(struct hid_device *hdev, u8 brightness)
 {
-	const u8 buf[FEATURE_KBD_REPORT_SIZE] = {
+	const u8 buf[] = {
 		FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, brightness
 	};
 	int ret;
@@ -5957,17 +6051,15 @@ static int asus_battery_probe(struct hid_device *hdev)
 
 static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
 {
-	const u8 buf[ROG_XGM_REPORT_SIZE] = {
-		FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
-	};
+	const u8 buf[] = { FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00 };
 	struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
 	int ret;
 
-	ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
+	ret = asus_kbd_set_report(xgm->hdev, buf, sizeof(buf));
 	if (ret < 0) {
 		hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
 		return ret;
-	} else if (ret != ROG_XGM_REPORT_SIZE) {
+	} else if (ret != (int)asus_report_len(xgm->hdev, FEATURE_KBD_LED_REPORT_ID2)) {
 		hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
 		return -EIO;
 	}
-- 
2.47.3