From: Mark Kettenis Subject: Re: uhidev fix for set report output command To: Marcus Glocker Cc: tech@openbsd.org Date: Tue, 07 Jan 2025 19:44:26 +0100 > Date: Tue, 7 Jan 2025 18:39:44 +0100 > From: Marcus Glocker > > 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? yes, ok kettenis@ > 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); >