Download raw body.
uhidev fix for set report output command
> Date: Tue, 7 Jan 2025 18:39:44 +0100
> From: Marcus Glocker <marcus@nazgul.ch>
>
> 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 <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.
> > >
> > > 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);
>
uhidev fix for set report output command