From: Marcus Glocker Subject: Re: uhidev fix for set report output command To: Mark Kettenis Cc: tech@openbsd.org Date: Tue, 7 Jan 2025 18:39:44 +0100 On Tue, Jan 07, 2025 at 01:31:09PM GMT, Mark Kettenis wrote: > > Date: Mon, 6 Jan 2025 21:53:25 +0100 > > From: Marcus Glocker > > > > 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. > > > > I couldn't find any other driver which uses the I2C_HID_REPORT_TYPE_OUTPUT > > set report command yet. Probably the reason why this kept undiscovered > > until now. > > > > Feedback, OKs? > > I looked a bit further into this and I think I understand what is > going on here. > > The LEDs are set using an Output report. The USB HID class spec has > the following to say about those: > > The Interrupt Out pipe is optional. If a device declares an > Interrupt Out endpoint then Output reports are transmitted by the > host to the device through the Interrupt Out endpoint. If no > Interrupt Out endpoint is declared then Output reports are > transmitted to a device through the Control endpoint, using > Set_Report(Output) requests. > > Our ukbd(4) driver always uses Set_Report(Output) requests. Not sure > if that is a bug or not, but there is a footnote in Appendix G that > suggests it isn't: > > If a device declares an Output report then support for > SetReport(Output) request is required. If an Ouput report is > defined, declaration of an Interrupt Out endpoint is optional, > however operating systems that do not support HID Interrupt Out > endpoints will route all Output reports through the control > endpoint using a SetReport(Output) request. > > Anyway, for HID-over-I2C this SetReport(Output) alternative isn't > implemented it seems. So instead we're supposed to just write the > Output reports to the register number defined by wOutputRegister in > the HID descriptor. > > So I think we should implement a separate function > (ihidev_send_report()?) that does this and remove the WOutputRegister > special handling from I2C_HID_CMD_SET_REPORT. All right. Were you thinking about something along those lines? Index: sys/dev/i2c/ihidev.c =================================================================== RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v diff -u -p -u -p -r1.36 ihidev.c --- sys/dev/i2c/ihidev.c 7 Jan 2025 15:25:18 -0000 1.36 +++ sys/dev/i2c/ihidev.c 7 Jan 2025 17:33:40 -0000 @@ -477,17 +477,8 @@ ihidev_hid_command(struct ihidev_softc * 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 { - cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister) - & 0xff; - cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister) - >> 8; - } + cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister) & 0xff; + cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister) >> 8; cmd[dataoff++] = report_len & 0xff; cmd[dataoff++] = report_len >> 8; @@ -966,4 +957,35 @@ ihidev_set_report(struct device *dev, in } return 0; +} + +int +ihidev_send_report(struct device *dev, int repid, void *data, int data_len) +{ + struct ihidev_softc *sc = (struct ihidev_softc *)dev; + uint8_t *finalcmd, cmd[5]; + int cmd_len, report_len, res; + + cmd_len = sizeof(cmd); + report_len = 2 + 1 + data_len; + + cmd[0] = htole16(sc->hid_desc.wOutputRegister) & 0xff; + cmd[1] = htole16(sc->hid_desc.wOutputRegister) >> 8; + cmd[2] = report_len & 0xff; + cmd[3] = report_len >> 8; + cmd[4] = repid; + + finalcmd = malloc(cmd_len + data_len, M_DEVBUF, M_NOWAIT | M_ZERO); + if (finalcmd == NULL) + return ENOMEM; + + memcpy(finalcmd, cmd, cmd_len); + memcpy(finalcmd + cmd_len, data, data_len); + + res = iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP, sc->sc_addr, + finalcmd, cmd_len + data_len, NULL, 0, 0); + + free(finalcmd, M_DEVBUF, cmd_len + data_len); + + return res; } Index: sys/dev/i2c/ihidev.h =================================================================== RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v diff -u -p -u -p -r1.10 ihidev.h --- sys/dev/i2c/ihidev.h 7 Jan 2025 15:25:18 -0000 1.10 +++ sys/dev/i2c/ihidev.h 7 Jan 2025 17:33:40 -0000 @@ -137,5 +137,6 @@ int ihidev_ioctl(struct ihidev *, u_long int ihidev_report_type_conv(int); int ihidev_set_report(struct device *, int, int, void *, int); int ihidev_get_report(struct device *, int, int, void *, int); +int ihidev_send_report(struct device *, int, void *, int); void ihidev_poll(void *); 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 17:33:40 -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,14 @@ 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_send_report((struct device *)sc->sc_hdev.sc_parent, + sc->sc_hdev.sc_report_id, &res, 1); + } } int @@ -180,6 +189,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);