[PATCH v2] HID: Off-by-one error in CP2112 HID driver affecting SMBus block read output

[Luis Yanes <melus0@yahoo.es>]

Just following up on this bug report and proposed fix. I would appreciate feedback on
whether this approach is acceptable or if there is a preferred way to handle this quirk.

After thoroughly reviewing the driver code and rereading AN495 I still don't know why
these chips (tried 5 of them so far) are showing the previously stated observed behaviour,
because the driver logic seems compliant with the AN495 CP2112 interface specification.

But performing an SMBus block read using the CP2112 HID USB-to-SMBus bridge with these
chips the last byte of a 32-byte response is consistently incorrect unless an extra byte
was requested.

A flag could be added at driver load time to enable a quirk fix for affected chips but
the end user first would need to notice or know that their adapter is affected.

I have not been able to figure out a way to detect those affected chips that will produce
corrupted output without notice or warning other than directly checking the serial number.

So have written a quirks patch against v5.15.195 that applies to a given serial number
range or device version and can be enabled for those known to be affected with minimal
impact on the rest.

Alternatively I think that at least some sort of warning notice always should be included.

Signed-off-by: Luis Yanes <melus0@yahoo.es>
--- a/drivers/hid-cp2112.c
+++ b/drivers/hid-cp2112.c
@@ -26,6 +26,14 @@
 #include <linux/nls.h>
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
+#include <linux/usb.h>
+
+#ifndef CP2112_QUIRKS
+#define CP2112_SN_RANGE_QUIRKS_START	"00670000"
+#define CP2112_SN_RANGE_QUIRKS_END	"00670FFF"
+#define CP2112_DEVICE_VERSION_QUIRK	2
+#define CP2112_QUIRK_SMBUS_BLOCK33 BIT(0)
+#endif
 
 #define CP2112_REPORT_MAX_LENGTH		64
 #define CP2112_GPIO_CONFIG_LENGTH		5
@@ -170,6 +178,7 @@
 	struct delayed_work gpio_poll_worker;
 	unsigned long irq_mask;
 	u8 gpio_prev_state;
+	unsigned long quirks;
 };
 
 static int gpio_push_pull = 0xFF;
@@ -642,6 +651,7 @@
 	__le16 word;
 	ssize_t count;
 	size_t read_length = 0;
+	size_t max_length;
 	unsigned int retries;
 	int ret;
 
@@ -702,8 +712,14 @@
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		if (I2C_SMBUS_READ == read_write) {
+			read_length = I2C_SMBUS_BLOCK_MAX ;
+			/* Apply quirk: allow one extra byte for CP2112 block read */
+			if (dev->quirks & CP2112_QUIRK_SMBUS_BLOCK33) {
+				read_length += 1;
+				hid_info(hdev, "Quirk: using block read size %lu\n", read_length);
+			}
 			count = cp2112_write_read_req(buf, addr,
-						      I2C_SMBUS_BLOCK_MAX,
+						      read_length,
 						      command, NULL, 0);
 		} else {
 			count = cp2112_write_req(buf, addr, command,
@@ -796,7 +812,14 @@
 		memcpy(data->block + 1, buf, read_length);
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		if (read_length > I2C_SMBUS_BLOCK_MAX) {
+		max_length = I2C_SMBUS_BLOCK_MAX;
+
+		if (dev->quirks & CP2112_QUIRK_SMBUS_BLOCK33) {
+			max_length += 1;
+			hid_info(hdev, "Quirk: allowing block read up to %lu bytes\n", max_length);
+		}
+
+		if (read_length > max_length) {
 			ret = -EPROTO;
 			goto power_normal;
 		}
@@ -1235,6 +1258,9 @@
 	struct gpio_irq_chip *girq;
 	int ret;
 
+	struct usb_device *udev;
+	char serial[64] = {0};
+
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
@@ -1282,6 +1308,26 @@
 	hid_info(hdev, "Part Number: 0x%02X Device Version: 0x%02X\n",
 		 buf[1], buf[2]);
 
+	if (buf[2] == CP2112_DEVICE_VERSION_QUIRK) {
+		dev->quirks |= CP2112_QUIRK_SMBUS_BLOCK33;
+		hid_info(hdev, "Enabled SMBus block+1 quirk for device version 0x%02X\n", buf[2]);
+	}
+
+	udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
+
+	if (udev->descriptor.iSerialNumber) {
+		ret = usb_string(udev, udev->descriptor.iSerialNumber, serial, sizeof(serial));
+		if (ret > 0)
+			hid_info(hdev, "CP2112 device serial number: %s\n", serial);
+	}
+
+	if (strlen(serial) == 8 &&
+		strncmp(serial, CP2112_SN_RANGE_QUIRKS_START, 8) >= 0 &&
+		strncmp(serial, CP2112_SN_RANGE_QUIRKS_END, 8) <= 0) {
+			dev->quirks |= CP2112_QUIRK_SMBUS_BLOCK33;
+			hid_info(hdev, "Enabled SMBus block+1 quirk for serial %s\n", serial);
+	}
+
 	ret = cp2112_hid_get(hdev, CP2112_SMBUS_CONFIG, (u8 *)&config,
 			     sizeof(config), HID_FEATURE_REPORT);
 	if (ret != sizeof(config)) {

--