Download raw body.
sys/ihidev: prevent crash on interrupt storm
On Tue, 24 Dec 2024 23:26:30 +0100,
Vitaliy Makkoveev <otto@bsdbox.dev> wrote:
>
> > On 25 Dec 2024, at 00:09, Kirill A. Korinsky <kirill@korins.ky> 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
sys/ihidev: prevent crash on interrupt storm