From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: uhidev fix for set report output command
To: Miod Vallat <miod@online.fr>
Cc: tech@openbsd.org
Date: Tue, 7 Jan 2025 13:15:22 +0100

On Tue, Jan 07, 2025 at 11:01:31AM GMT, Miod Vallat wrote:

> > Feedback, OKs?
> 
> It is probably worth moving lines 434-448, which setup cmd[2], into the
> if, since setting dataoff to zero in the else part makes that assignment
> pointless.

Yes, thanks, good point.


Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
diff -u -p -u -p -r1.35 ihidev.c
--- sys/dev/i2c/ihidev.c	6 Jan 2025 02:13:55 -0000	1.35
+++ sys/dev/i2c/ihidev.c	7 Jan 2025 12:12:48 -0000
@@ -373,7 +373,11 @@ ihidev_hid_command(struct ihidev_softc *
 		 * rreq->data.
 		 */
 		report_len += report_id_len;
-		tmprep = malloc(report_len, M_DEVBUF, M_WAITOK | M_ZERO);
+		tmprep = malloc(report_len, M_DEVBUF, M_NOWAIT | M_ZERO);
+		if (tmprep == NULL) {
+			res = ENOMEM;
+			break;
+		}
 
 		/* type 3 id 8: 22 00 38 02 23 00 */
 		res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
@@ -431,28 +435,44 @@ ihidev_hid_command(struct ihidev_softc *
 			DPRINTF((" %.2x", ((uint8_t *)rreq->data)[i]));
 		DPRINTF(("\n"));
 
-		/*
-		 * 7.2.3.4 - "The protocol is optimized for Report < 15.  If a
-		 * report ID >= 15 is necessary, then the Report ID in the Low
-		 * Byte must be set to 1111 and a Third Byte is appended to the
-		 * protocol.  This Third Byte contains the entire/actual report
-		 * ID."
-		 */
-		dataoff = 4;
-		if (report_id >= 15) {
-			cmd[dataoff++] = report_id;
-			report_id = 15;
-		} else
-			cmdlen--;
+		if (rreq->type == I2C_HID_REPORT_TYPE_FEATURE) {
+			/*
+			 * 7.2.3.4 - "The protocol is optimized for Report < 15.
+			 * If a report ID >= 15 is necessary, then the Report
+			 * ID in the Low Byte must be set to 1111 and a Third
+			 * Byte is appended to the protocol.  This Third Byte
+			 * contains the entire/actual report ID."
+			 */
+			dataoff = 4;
+			if (report_id >= 15) {
+				cmd[dataoff++] = report_id;
+				report_id = 15;
+			} else
+				cmdlen--;
 
-		cmd[2] = report_id | rreq->type << 4;
+			cmd[2] = report_id | rreq->type << 4;
 
-		if (rreq->type == I2C_HID_REPORT_TYPE_FEATURE) {
 			cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister)
 			    & 0xff;
 			cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister)
 			    >> 8;
 		} else {
+			/*
+			 * When using the wOutputRegister for a set report
+			 * output command, the wCommandRegister command needs
+			 * to be omitted, and we write the report data straight
+			 * in to the wOutputRegister:
+			 *
+			 * cmd[0] = wOutputRegister (LSB)
+			 * cmd[1] = wOutputRegister (MSB)
+			 * cmd[2] = Length Field (LSB)
+			 * cmd[3] = Length Field (MSB)
+			 * cmd[4] = Report ID
+			 * cmd[5] = DATA
+			 * ...
+			 */
+			dataoff = 0;
+			cmdlen = 5;
 			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
 			    & 0xff;
 			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
@@ -464,7 +484,11 @@ ihidev_hid_command(struct ihidev_softc *
 		cmd[dataoff] = rreq->id;
 
 		finalcmd = malloc(cmdlen + rreq->len, M_DEVBUF,
-		    M_WAITOK | M_ZERO);
+		    M_NOWAIT | M_ZERO);
+		if (finalcmd == NULL) {
+			res = ENOMEM;
+			break;
+		}
 
 		memcpy(finalcmd, cmd, cmdlen);
 		memcpy(finalcmd + cmdlen, rreq->data, rreq->len);
Index: sys/dev/i2c/ikbd.c
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ikbd.c,v
diff -u -p -u -p -r1.2 ikbd.c
--- sys/dev/i2c/ikbd.c	3 Sep 2022 15:48:16 -0000	1.2
+++ sys/dev/i2c/ikbd.c	7 Jan 2025 12:12:48 -0000
@@ -36,6 +36,7 @@
 
 struct ikbd_softc {
 	struct ihidev	sc_hdev;
+#define sc_ledsize	sc_hdev.sc_osize
 	struct hidkbd	sc_kbd;
 	int		sc_spl;
 };
@@ -167,6 +168,15 @@ ikbd_enable(void *v, int on)
 void
 ikbd_set_leds(void *v, int leds)
 {
+	struct ikbd_softc *sc = v;
+	struct hidkbd *kbd = &sc->sc_kbd;
+	uint8_t res;
+
+	if (sc->sc_ledsize && hidkbd_set_leds(kbd, leds, &res) != 0) {
+		ihidev_set_report((struct device *)sc->sc_hdev.sc_parent,
+		    I2C_HID_REPORT_TYPE_OUTPUT, sc->sc_hdev.sc_report_id, &res,
+		    1);
+	}
 }
 
 int
@@ -180,6 +190,9 @@ ikbd_ioctl(void *v, u_long cmd, caddr_t 
 	case WSKBDIO_GTYPE:
 		/* XXX: should we set something else? */
 		*(u_int *)data = WSKBD_TYPE_USB;
+		return 0;
+	case WSKBDIO_SETLEDS:
+		ikbd_set_leds(v, *(int *)data);
 		return 0;
 	default:
 		rc = ihidev_ioctl(&sc->sc_hdev, cmd, data, flag, p);