From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: uhidev fix for set report output command
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: tech@openbsd.org
Date: Mon, 6 Jan 2025 22:56:00 +0100

On Mon, Jan 06, 2025 at 10:37:59PM GMT, Mark Kettenis wrote:

> > Date: Mon, 6 Jan 2025 21:53:25 +0100
> > From: Marcus Glocker <marcus@nazgul.ch>
> > 
> > When I was trying to add LED support for ikbd(4), I noticed that the set
> > report output command in ihidev isn't working as expected, so my caps
> > lock LED never got lit.  While checking with kettenis@ on that topic,
> > he reported that he faced the same issue back then when trying to add
> > LED support to ikbd(4), and he is suspecting an issue in the ihidev set
> > report command.
> > 
> > After investigating further, it turned out that when we use the
> > wOutputRegister for the set report output command, the wCommandRegister
> > block needs to be omitted.  There is a clear statement in the Linux
> > driver about it:
> > 
> > https://github.com/torvalds/linux/blob/master/drivers/hid/i2c-hid/i2c-hid-core.c#L372
> > 
> > With the following patch, and the ikbd LED extension (which I would
> > share once this got fixed), I can control the caps lock LED on my Samsung
> > laptop.  I allowed myself to add some comment, since this isn't so
> > obvious to grasp, and not very well documented in the hid-over-i2c specs
> > IMO.
> 
> Can you share it right now such that I can test this on the x13s?
> 
> One diff that includes both changes is fine for that.

Sure.  As already mentioned in our previous discussion, I had to convert
the M_WAITOK to M_NOWAIT in that path to avoid assertwaitok() messages
when calling ihidev_set_report() from ikbd.  To be consistent, I also
did it for the get report command.


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	6 Jan 2025 21:50:08 -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,
@@ -453,6 +457,22 @@ ihidev_hid_command(struct ihidev_softc *
 			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
+			 * ...
+			 */
+			cmdlen = 5;
+			dataoff = 0;
 			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	6 Jan 2025 21:50:08 -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);