From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/ihidev: respect RESET_RESPONSE and adjsut POWER ON timeout
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org, mvs@openbsd.org
Date: Fri, 3 Jan 2025 09:31:03 +0100

On Fri, Jan 03, 2025 at 01:37:06AM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> i2c states that a device should response for RESET in less than 5
> seconds, and if it doesn't reponse as 0x00 0x00 it shouldn't be used.
> 
> Also, a device shall not respond back after receiving POWER ON, and must
> ensure that it transitions power on state in less than 1 second, not
> 100ms.
> 
> Without waiting the RESET response a HID report description from
> touchpad at Honor Magicbook 14 Art Snapdragon is reading as as series of
> 0xFF bytes every time, and with not enough waiting after power on likes
> 1 out of 10 reboot.
> 
> Feedback and help to find the right fix instead of qurik from mvs@
> 
> Additionally tested on Huawe Matebook X 2020, no regression.
> 
> Feedback? Tests? Ok?

Makes sense to me.

No regression found on the Samsung Galaxy Book4 Edge nor on the
Microsoft Surface Go4.

Two tiny comments inline only related to comment and style which you
might want to consider.

Otherwise, ok mglocker@ (maybe you want to wait a little longer for one
or two more test reports / other feedback's).
 
> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
> diff -u -p -u -p -r1.34 ihidev.c
> --- sys/dev/i2c/ihidev.c	2 Jan 2025 23:26:50 -0000	1.34
> +++ sys/dev/i2c/ihidev.c	3 Jan 2025 00:24:12 -0000
> @@ -55,6 +55,7 @@ enum {
>  
>  	/* pseudo commands */
>  	I2C_HID_REPORT_DESCR	= 0x100,
> +	I2C_HID_RESET_RESPONSE	= 0x101,
>  };
>  
>  static int I2C_HID_POWER_ON	= 0x0;
> @@ -516,6 +517,32 @@ ihidev_hid_command(struct ihidev_softc *
>  
>  		break;
>  	}
> +	case I2C_HID_RESET_RESPONSE: {
> +		int i;
> +		uint8_t buf[2] = { 0xff, 0xff };
> +
> +		DPRINTF(("%s: HID command I2C_HID_RESET_RESPONSE\n",
> +		    sc->sc_dev.dv_xname));
> +
> +		/*
> +		 * 7.2.1 states that a device should response for RESET
> +		 * in less than 5 seconds. It uses poll instead of
> +		 * tsleep because interrupts are blocked during autoconf.
> +		 */
> +		for (i = 0; i < 50; i++) {
> +			ihidev_sleep(sc, 100);
> +			res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
> +			    NULL, 0, buf, sizeof(buf), 0);

Line break could be easily avoided by just moving sc->sc_addr to the
next line.

> +			DPRINTF(("%s: read attempt %d: 0x%x, 0x%x, res: %d\n",
> +			    sc->sc_dev.dv_xname, i, buf[0], buf[1], res));
> +			if (!res)
> +				res = (buf[0] != 0x00 || buf[1] != 0x00);
> +			if (!res)
> +				break;
> +		}
> +
> +		break;
> +	}
>  	default:
>  		printf("%s: unknown command %d\n", sc->sc_dev.dv_xname,
>  		    hidcmd);
> @@ -529,14 +556,15 @@ ihidev_hid_command(struct ihidev_softc *
>  int
>  ihidev_poweron(struct ihidev_softc *sc)
>  {
> -	DPRINTF(("%s: resetting\n", sc->sc_dev.dv_xname));
> -
>  	if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_ON)) {
>  		printf("%s: failed to power on\n", sc->sc_dev.dv_xname);
>  		return (1);
>  	}
>  
> -	ihidev_sleep(sc, 100);
> +	/* 7.2.8 states that a device shall not respond back after
> +	 * receiving the command, and must ensure that it transitions to
> +	 * specified power state in less than 1 second */

After the receiving the power on command I assume?  Maybe worth to
mention that explicitly in the comment.

> +	ihidev_sleep(sc, 1000);
>  
>  	return 0;
>  }
> @@ -548,6 +576,8 @@ ihidev_reset(struct ihidev_softc *sc)
>  	if (ihidev_poweron(sc))
>  		return (1);
>  
> +	DPRINTF(("%s: resetting\n", sc->sc_dev.dv_xname));
> +
>  	if (ihidev_hid_command(sc, I2C_HID_CMD_RESET, 0)) {
>  		printf("%s: failed to reset hardware\n", sc->sc_dev.dv_xname);
>  
> @@ -557,7 +587,11 @@ ihidev_reset(struct ihidev_softc *sc)
>  		return (1);
>  	}
>  
> -	ihidev_sleep(sc, 100);
> +	if (ihidev_hid_command(sc, I2C_HID_RESET_RESPONSE, 0)) {
> +		printf("%s: unexpected reset response\n",
> +		    sc->sc_dev.dv_xname);
> +		return (1);
> +	}
>  
>  	return (0);
>  }
> @@ -571,8 +605,6 @@ ihidev_reset(struct ihidev_softc *sc)
>  int
>  ihidev_hid_desc_parse(struct ihidev_softc *sc)
>  {
> -	int retries = 3;
> -
>  	/* must be v01.00 */
>  	if (letoh16(sc->hid_desc.bcdVersion) != 0x0100) {
>  		printf("%s: bad HID descriptor bcdVersion (0x%x)\n",
> @@ -598,16 +630,8 @@ ihidev_hid_desc_parse(struct ihidev_soft
>  		return (1);
>  	}
>  
> -	while (retries-- > 0) {
> -		if (ihidev_reset(sc)) {
> -			if (retries == 0)
> -				return(1);
> -
> -			ihidev_sleep(sc, 10);
> -		}
> -		else
> -			break;
> -	}
> +	if (ihidev_reset(sc))
> +		return (1);
>  
>  	sc->sc_reportlen = letoh16(sc->hid_desc.wReportDescLength);
>  	sc->sc_report = malloc(sc->sc_reportlen, M_DEVBUF, M_WAITOK | M_ZERO);
> 
> -- 
> wbr, Kirill
>