From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/ihidev: prevent crash on interrupt storm
To: otto@bsdbox.dev, mpi@grenadille.net, tech@openbsd.org
Date: Wed, 25 Dec 2024 01:30:09 +0100

On Wed, 25 Dec 2024 01:17:56 +0100,
Kirill A. Korinsky <kirill@korins.ky> wrote:
> 
> So, the diff:
> 
> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
> diff -u -p -r1.33 ihidev.c
> --- sys/dev/i2c/ihidev.c	18 Oct 2024 12:53:49 -0000	1.33
> +++ sys/dev/i2c/ihidev.c	25 Dec 2024 00:13:30 -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;
> @@ -137,6 +138,21 @@ ihidev_attach(struct device *parent, str
>  	    letoh16(sc->hid_desc.wVendorID), letoh16(sc->hid_desc.wProductID),
>  	    (char *)ia->ia_cookie);
>  
> +	if (ihidev_reset(sc))
> +		return;
> +
> +	if (ihidev_hid_command(sc, I2C_HID_RESET_RESPONSE, 0)) {
> +		printf("%s: unexpected reset response\n",
> +		    sc->sc_dev.dv_xname);
> +		return;
> +	}
> +
> +	if (ihidev_hid_command(sc, I2C_HID_REPORT_DESCR, 0)) {
> +		printf("%s: failed fetching HID report\n",
> +		    sc->sc_dev.dv_xname);
> +		return;
> +	}
> +
>  	sc->sc_nrepid = ihidev_maxrepid(sc->sc_report, sc->sc_reportlen);
>  	if (sc->sc_nrepid < 0)
>  		return;
> @@ -515,6 +531,28 @@ 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));
> +
> +		for (i = 0; i < 100; i++) {
> +			res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
> +			    NULL, 0, buf, sizeof(buf), 0);
> +			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)
> +				break;
> +			ihidev_sleep(sc, 100);
> +		}
> +
> +		if (!res)
> +			res = (buf[0] != 0x00 || buf[1] != 0x00);
> +
> +		break;
> +	}
>  	default:
>  		printf("%s: unknown command %d\n", sc->sc_dev.dv_xname,
>  		    hidcmd);
> @@ -528,8 +566,6 @@ 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);
> @@ -547,6 +583,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);
>  
> @@ -570,8 +608,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",
> @@ -597,25 +633,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;
> -	}
> -
>  	sc->sc_reportlen = letoh16(sc->hid_desc.wReportDescLength);
>  	sc->sc_report = malloc(sc->sc_reportlen, M_DEVBUF, M_WAITOK | M_ZERO);
> -
> -	if (ihidev_hid_command(sc, I2C_HID_REPORT_DESCR, 0)) {
> -		printf("%s: failed fetching HID report\n",
> -		    sc->sc_dev.dv_xname);
> -		return (1);
> -	}
>  
>  	return (0);
>  }
> 
> 

BTW on Huwaei Matebook X 2020 it also works without any regression.

-- 
wbr, Kirill