Hi, "Jason R Thorpe" <thor...@netbsd.org> writes:
> Module Name: src > Committed By: thorpej > Date: Sun Dec 22 16:44:35 UTC 2019 > > Modified Files: > src/sys/dev/i2c: ihidev.c ihidev.h > > Log Message: > The hid-over-i2c spec specifies that compliant devices use level-sensitive > interrupts. However, it's not safe to do i2c bus access in hard interrupt > context, and we must read the event data off the device in order to clear > the interrupt condition. > > Address this by using acpi_intr_mask() to mask off the interrupt source > while a softint is pending to service the events, re-enabling it once > servicing is completed. > > While here, re-factor the interrupt setup / tear-down code a bit to > eventually once day simplify supporting the FDT bindings for hid-over-i2c. This change freezes an amd64 kernel boot on my laptop. And I cannot enter into DDB with ALt+Ctrl+ESC. My laptop is HP Spectre x360 and it has twi ims(4) touch/pen screen. With "userconf disable ihidev", the kernel boots fine as of 2019-12-24T08:00 (UTC). Could you take a look at my problem? > To generate a diff of this commit: > cvs rdiff -u -r1.9 -r1.10 src/sys/dev/i2c/ihidev.c > cvs rdiff -u -r1.1 -r1.2 src/sys/dev/i2c/ihidev.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > > Modified files: > > Index: src/sys/dev/i2c/ihidev.c > diff -u src/sys/dev/i2c/ihidev.c:1.9 src/sys/dev/i2c/ihidev.c:1.10 > --- src/sys/dev/i2c/ihidev.c:1.9 Tue Oct 1 18:00:08 2019 > +++ src/sys/dev/i2c/ihidev.c Sun Dec 22 16:44:35 2019 > @@ -1,4 +1,4 @@ > -/* $NetBSD: ihidev.c,v 1.9 2019/10/01 18:00:08 chs Exp $ */ > +/* $NetBSD: ihidev.c,v 1.10 2019/12/22 16:44:35 thorpej Exp $ */ > /* $OpenBSD ihidev.c,v 1.13 2017/04/08 02:57:23 deraadt Exp $ */ > > /*- > @@ -54,7 +54,7 @@ > */ > > #include <sys/cdefs.h> > -__KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1.9 2019/10/01 18:00:08 chs Exp $"); > +__KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1.10 2019/12/22 16:44:35 thorpej Exp > $"); > > #include <sys/param.h> > #include <sys/systm.h> > @@ -71,6 +71,7 @@ __KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1 > # include "acpica.h" > #endif > #if NACPICA > 0 > +#include <dev/acpi/acpivar.h> > #include <dev/acpi/acpi_intr.h> > #endif > > @@ -109,10 +110,14 @@ static int ihidev_detach(device_t, int); > CFATTACH_DECL_NEW(ihidev, sizeof(struct ihidev_softc), > ihidev_match, ihidev_attach, ihidev_detach, NULL); > > +static bool ihiddev_intr_init(struct ihidev_softc *); > +static void ihiddev_intr_fini(struct ihidev_softc *); > + > static bool ihidev_suspend(device_t, const pmf_qual_t *); > static bool ihidev_resume(device_t, const pmf_qual_t *); > static int ihidev_hid_command(struct ihidev_softc *, int, void *, bool); > static int ihidev_intr(void *); > +static void ihidev_softintr(void *); > static int ihidev_reset(struct ihidev_softc *, bool); > static int ihidev_hid_desc_parse(struct ihidev_softc *); > > @@ -200,20 +205,9 @@ ihidev_attach(device_t parent, device_t > repsz)); > } > sc->sc_ibuf = kmem_zalloc(sc->sc_isize, KM_SLEEP); > -#if NACPICA > 0 > - { > - char buf[100]; > - > - sc->sc_ih = acpi_intr_establish(self, sc->sc_phandle, IPL_TTY, > - false, ihidev_intr, sc, device_xname(self)); > - if (sc->sc_ih == NULL) { > - aprint_error_dev(self, "can't establish interrupt\n"); > - return; > - } > - aprint_normal_dev(self, "interrupting at %s\n", > - acpi_intr_string(sc->sc_ih, buf, sizeof(buf))); > + if (! ihiddev_intr_init(sc)) { > + return; > } > -#endif > > iha.iaa = ia; > iha.parent = sc; > @@ -260,10 +254,7 @@ ihidev_detach(device_t self, int flags) > struct ihidev_softc *sc = device_private(self); > > mutex_enter(&sc->sc_intr_lock); > -#if NACPICA > 0 > - if (sc->sc_ih != NULL) > - acpi_intr_disestablish(sc->sc_ih); > -#endif > + ihiddev_intr_fini(sc); > if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, > &I2C_HID_POWER_OFF, true)) > aprint_error_dev(sc->sc_dev, "failed to power down\n"); > @@ -649,31 +640,110 @@ ihidev_hid_desc_parse(struct ihidev_soft > return (0); > } > > +static bool > +ihiddev_intr_init(struct ihidev_softc *sc) > +{ > +#if NACPICA > 0 > + ACPI_HANDLE hdl = (void *)(uintptr_t)sc->sc_phandle; > + struct acpi_resources res; > + ACPI_STATUS rv; > + char buf[100]; > + > + rv = acpi_resource_parse(sc->sc_dev, hdl, "_CRS", &res, > + &acpi_resource_parse_ops_quiet); > + if (ACPI_FAILURE(rv)) { > + aprint_error_dev(sc->sc_dev, "can't parse '_CRS'\n"); > + return false; > + } > + > + const struct acpi_irq * const irq = acpi_res_irq(&res, 0); > + if (irq == NULL) { > + aprint_error_dev(sc->sc_dev, "no IRQ resource\n"); > + acpi_resource_cleanup(&res); > + return false; > + } > + > + sc->sc_intr_type = > + irq->ar_type == ACPI_EDGE_SENSITIVE ? IST_EDGE : IST_LEVEL; > + > + acpi_resource_cleanup(&res); > + > + sc->sc_ih = acpi_intr_establish(sc->sc_dev, sc->sc_phandle, IPL_TTY, > + false, ihidev_intr, sc, device_xname(sc->sc_dev)); > + if (sc->sc_ih == NULL) { > + aprint_error_dev(sc->sc_dev, "can't establish interrupt\n"); > + return false; > + } > + aprint_normal_dev(sc->sc_dev, "interrupting at %s\n", > + acpi_intr_string(sc->sc_ih, buf, sizeof(buf))); > + > + sc->sc_sih = softint_establish(SOFTINT_SERIAL, ihidev_softintr, sc); > + if (sc->sc_sih == NULL) { > + aprint_error_dev(sc->sc_dev, > + "can't establish soft interrupt\n"); > + return false; > + } > + > + return true; > +#else > + aprint_error_dev(sc->sc_dev, "can't establish interrupt\n"); > + return false; > +#endif > +} > + > +static void > +ihiddev_intr_fini(struct ihidev_softc *sc) > +{ > +#if NACPICA > 0 > + if (sc->sc_ih != NULL) { > + acpi_intr_disestablish(sc->sc_ih); > + } > + if (sc->sc_sih != NULL) { > + softint_disestablish(sc->sc_sih); > + } > +#endif > +} > + > static int > ihidev_intr(void *arg) > { > - struct ihidev_softc *sc = arg; > + struct ihidev_softc * const sc = arg; > + > + mutex_enter(&sc->sc_intr_lock); > + > + /* > + * Schedule our soft interrupt handler. If we're using a level- > + * triggered interrupt, we have to mask it off while we wait > + * for service. > + */ > + softint_schedule(sc->sc_sih); > + if (sc->sc_intr_type == IST_LEVEL) { > +#if NACPICA > 0 > + acpi_intr_mask(sc->sc_ih); > +#endif > + } > + > + mutex_exit(&sc->sc_intr_lock); > + > + return 1; > +} > + > +static void > +ihidev_softintr(void *arg) > +{ > + struct ihidev_softc * const sc = arg; > struct ihidev *scd; > u_int psize; > int res, i; > u_char *p; > u_int rep = 0; > > - /* > - * XXX: force I2C_F_POLL for now to avoid dwiic interrupting > - * while we are interrupting > - */ > - > - mutex_enter(&sc->sc_intr_lock); > - iic_acquire_bus(sc->sc_tag, I2C_F_POLL); > - > + iic_acquire_bus(sc->sc_tag, 0); > res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr, NULL, 0, > - sc->sc_ibuf, sc->sc_isize, I2C_F_POLL); > - > - iic_release_bus(sc->sc_tag, I2C_F_POLL); > - mutex_exit(&sc->sc_intr_lock); > + sc->sc_ibuf, sc->sc_isize, 0); > + iic_release_bus(sc->sc_tag, 0); > if (res != 0) > - return 1; > + goto out; > > /* > * 6.1.1 - First two bytes are the packet length, which must be less > @@ -683,7 +753,7 @@ ihidev_intr(void *arg) > if (!psize || psize > sc->sc_isize) { > DPRINTF(("%s: %s: invalid packet size (%d vs. %d)\n", > sc->sc_dev.dv_xname, __func__, psize, sc->sc_isize)); > - return (1); > + goto out; > } > > /* 3rd byte is the report id */ > @@ -695,22 +765,30 @@ ihidev_intr(void *arg) > if (rep >= sc->sc_nrepid) { > aprint_error_dev(sc->sc_dev, "%s: bad report id %d\n", > __func__, rep); > - return (1); > + goto out; > } > > - DPRINTF(("%s: ihidev_intr: hid input (rep %d):", sc->sc_dev.dv_xname, > - rep)); > + DPRINTF(("%s: %s: hid input (rep %d):", sc->sc_dev.dv_xname, > + __func__, rep)); > for (i = 0; i < sc->sc_isize; i++) > DPRINTF((" %.2x", sc->sc_ibuf[i])); > DPRINTF(("\n")); > > scd = sc->sc_subdevs[rep]; > if (scd == NULL || !(scd->sc_state & IHIDEV_OPEN)) > - return (1); > + goto out; > > scd->sc_intr(scd, p, psize); > > - return 1; > + out: > + /* > + * If our interrupt is level-triggered, re-enable it now. > + */ > + if (sc->sc_intr_type == IST_LEVEL) { > +#if NACPICA > 0 > + acpi_intr_unmask(sc->sc_ih); > +#endif > + } > } > > static int > > Index: src/sys/dev/i2c/ihidev.h > diff -u src/sys/dev/i2c/ihidev.h:1.1 src/sys/dev/i2c/ihidev.h:1.2 > --- src/sys/dev/i2c/ihidev.h:1.1 Sun Dec 10 17:05:54 2017 > +++ src/sys/dev/i2c/ihidev.h Sun Dec 22 16:44:35 2019 > @@ -1,4 +1,4 @@ > -/* $NetBSD: ihidev.h,v 1.1 2017/12/10 17:05:54 bouyer Exp $ */ > +/* $NetBSD: ihidev.h,v 1.2 2019/12/22 16:44:35 thorpej Exp $ */ > /* $OpenBSD ihidev.h,v 1.4 2016/01/31 18:24:35 jcs Exp $ */ > > /*- > @@ -100,7 +100,9 @@ struct ihidev_softc { > uint64_t sc_phandle; > > void * sc_ih; > + void * sc_sih; > kmutex_t sc_intr_lock; > + int sc_intr_type; > > u_int sc_hid_desc_addr; > union { > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3