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

> Date: Mon, 6 Jan 2025 22:56:00 +0100
> From: Marcus Glocker <marcus@nazgul.ch>
> 
> 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.

Yes, that works on the x13s as well.

> 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);
> 
>