From: Kirill A. Korinsky Subject: Re: sys/ihidev: prevent crash on interrupt storm To: Vitaliy Makkoveev Cc: Martin Pieuchot , OpenBSD tech Date: Wed, 25 Dec 2024 01:17:56 +0100 On Tue, 24 Dec 2024 23:26:30 +0100, Vitaliy Makkoveev wrote: > > > On 25 Dec 2024, at 00:09, Kirill A. Korinsky wrote: > > > > > > Thanks for the suggestion. I tried to implement it but discovered that > > tsleep inside attach sleeps for 1 microsecond. I tried using delay(9) > > instead, to sleep for 10 seconds, but no interrupt arrived while it was > > waiting. My guess that it waits until autoconfiguration is over, but I'm not > > sure. > > > > Extracting part of the logic from attach to some function that is called > > from intr and by the timer could be a solution, but this function uses > > config_found_sm and it requires i2c_attach_args. All of that seems like the > > wrong way. Am I mistaken? > > > > My HID touchpad 04f3:3282 doesn’t throw interrupt reset. We had no > reports before, so I suspect the most devices do the same. > > delay(9) is arch specific, IIRC it doesn’t interrupts on x86. This machine has three HID devices on I2C and only one is an issue. I think some folks are using machines with the Snapdragon X Elite platform without any issues. So this is definitely device specific. > > At least you should check err in the for loop, otherwise it could > be overwritten with the EWOULDBLOCK even in the non error case. Also > sc_st check and sleep should be serialised with sc_st modification > within interrupt handler, otherwise the wakeup could be lost. > > I propose you to try something like below: > > sc->sc_st = I2C_HID_RESET_WAIT; > for (i = 0; i < 3; ++i) { > ihidev_hid_command(sc, I2C_HID_CMD_RESET); > delay(10 * 1000 * 1000); > if (sc->sc_st != I2C_HID_RESET_WAIT) > break; > } > if (sc->sc_st != I2C_HID_RESET_DONE) { > if (sc->sc_st == I2C_HID_RESET_WAIT) > printf("timeout"); > else > printf("other error"); > goto error; > } > > Just tried, doesn't work neither. Here that I have in dmesg: ihidev0 at iic1 addr 0x3aihidev0: HID command I2C_HID_CMD_DESCR at 0x1 ihidev0: HID descriptor: 1e 00 00 01 95 00 02 00 03 00 20 00 04 00 20 00 05 00 06 00 9f 04 43 53 01 01 00 00 00 00 ihidev0: HID command I2C_HID_CMD_SET_POWER(0) ihidev0: HID command I2C_HID_CMD_RESET ihidev0: HID command I2C_HID_CMD_RESET ihidev0: HID command I2C_HID_CMD_RESET ihidev0: timeout ihidev0: marked as broken ihidev1 at iic3 addr 0x5dihidev1: HID command I2C_HID_CMD_DESCR at 0x1 ihidev1: HID descriptor: 1e 00 00 01 df 02 02 00 03 00 2a 00 04 00 20 00 05 00 06 00 cc 35 04 01 01 01 00 00 00 00 ihidev1: HID command I2C_HID_CMD_SET_POWER(0) ihidev1: HID command I2C_HID_CMD_RESET ihidev1: HID command I2C_HID_CMD_RESET ihidev1: HID command I2C_HID_CMD_RESET ihidev1: timeout ihidev1: marked as broken ihidev2 at iic4 addr 0x38ihidev2: HID command I2C_HID_CMD_DESCR at 0x1 ihidev2: HID descriptor: 1e 00 00 01 dd 01 02 00 03 00 42 00 04 00 42 00 05 00 06 00 08 28 62 56 02 45 00 00 00 00 ihidev2: HID command I2C_HID_CMD_SET_POWER(0) ihidev2: HID command I2C_HID_CMD_RESET ihidev2: HID command I2C_HID_CMD_RESET ihidev2: HID command I2C_HID_CMD_RESET ihidev2: timeout ihidev2: marked as broken ihidev1: unexpected state 0x5, marked as failed ihidev2: unexpected state 0x5, marked as failed ihidev0: unexpected state 0x5, marked as failed from another hand, I just start from scratch and use different approach (diff is attached), which works on this machine as: ihidev0 at iic1 addr 0x3a gpio 384, vendor 0x49f product 0x5343, QTEC0001 ihidev0: 5 report ids ikbd0 at ihidev0 reportid 1: 8 variable keys, 6 key codes icc0 at ihidev0 reportid 3: 573 usages, 20 keys, array hid at ihidev0 reportid 5 not configured ihidev1 at iic3 addr 0x5d gpio 896, vendor 0x35cc product 0x104, QTEC0002 ihidev1: 14 report ids imt0 at ihidev1ihidev1: failed fetching report ims0 at ihidev1 reportid 1: 3 buttons, Z dir hid at ihidev1 reportid 5 not configured hid at ihidev1 reportid 6 not configured hid at ihidev1 reportid 7 not configured icc1 at ihidev1 reportid 8: 768 usages, 20 keys, array hid at ihidev1 reportid 14 not configured ihidev2 at iic4 addr 0x38 gpio 51, vendor 0x2808 product 0x5662, MSFT0001 ihidev2: 16 report ids ims1 at ihidev2 reportid 1: 1 button, tip hid at ihidev2 reportid 2 not configured hid at ihidev2 reportid 5 not configured hid at ihidev2 reportid 6 not configured hid at ihidev2 reportid 16 not configured BTW, if I add if (ihidev_poweron(sc)) return; after ihidev_hid_command(sc, I2C_HID_REPORT_DESCR, 0) it reads ihidev1 also as multitouch touchpad. 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); } -- wbr, Kirill