Re: ihidev: always register interrupt, stop polling if it fires
On Fri, Jul 19, 2019 at 02:16:18PM -0500, joshua stein wrote:
> The fast polling of ihidev may cause a problem during suspend/resume
> because dwiic may be in an unknown state, so add a DVACT_QUIESCE
> handler to properly shut it down.
>
> Even if polling is requested, register the interrupt handler too.
> If it ever fires, stop polling and just use the interrupt. This is
> what led me to discover that ihidev's interrupt starts firing after
> a suspend/resume cycle (but I still have no idea why it doesn't work
> before that).
>
>
Seems to be a sensible workaround, but you might want to remove that
printf in the interrupt handler unless you're absolutely sure it's safe
there...
-ml
> Index: dev/acpi/dwiic_acpi.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/dwiic_acpi.c,v
> retrieving revision 1.9
> diff -u -p -u -p -r1.9 dwiic_acpi.c
> --- dev/acpi/dwiic_acpi.c 16 Jul 2019 19:12:32 - 1.9
> +++ dev/acpi/dwiic_acpi.c 19 Jul 2019 19:11:21 -
> @@ -457,8 +457,9 @@ dwiic_acpi_found_ihidev(struct dwiic_sof
>
> aml_freevalue(&res);
>
> - if (!sc->sc_poll_ihidev &&
> - !(crs.irq_int == 0 && crs.gpio_int_node == NULL))
> + if (sc->sc_poll_ihidev)
> + ia.ia_poll = 1;
> + if (!(crs.irq_int == 0 && crs.gpio_int_node == NULL))
> ia.ia_intr = &crs;
>
> if (config_found(sc->sc_iic, &ia, dwiic_i2c_print)) {
> Index: dev/i2c/i2cvar.h
> ===
> RCS file: /cvs/src/sys/dev/i2c/i2cvar.h,v
> retrieving revision 1.16
> diff -u -p -u -p -r1.16 i2cvar.h
> --- dev/i2c/i2cvar.h 23 Apr 2016 09:40:28 - 1.16
> +++ dev/i2c/i2cvar.h 19 Jul 2019 19:11:21 -
> @@ -113,6 +113,7 @@ struct i2c_attach_args {
> char*ia_name; /* chip name */
> void*ia_cookie; /* pass extra info from bus to dev */
> void*ia_intr; /* interrupt info */
> + int ia_poll;/* to force polling */
> };
>
> /*
> Index: dev/i2c/ihidev.c
> ===
> RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
> retrieving revision 1.19
> diff -u -p -u -p -r1.19 ihidev.c
> --- dev/i2c/ihidev.c 8 Apr 2019 17:50:45 - 1.19
> +++ dev/i2c/ihidev.c 19 Jul 2019 19:11:21 -
> @@ -63,6 +63,7 @@ static int I2C_HID_POWER_OFF= 0x1;
> int ihidev_match(struct device *, void *, void *);
> void ihidev_attach(struct device *, struct device *, void *);
> int ihidev_detach(struct device *, int);
> +int ihidev_activate(struct device *, int);
>
> int ihidev_hid_command(struct ihidev_softc *, int, void *);
> int ihidev_intr(void *);
> @@ -80,7 +81,7 @@ struct cfattach ihidev_ca = {
> ihidev_match,
> ihidev_attach,
> ihidev_detach,
> - NULL
> + ihidev_activate,
> };
>
> struct cfdriver ihidev_cd = {
> @@ -128,7 +129,7 @@ ihidev_attach(struct device *parent, str
> printf(", can't establish interrupt");
> }
>
> - if (sc->sc_ih == NULL) {
> + if (ia->ia_poll) {
> printf(" (polling)");
> sc->sc_poll = 1;
> sc->sc_fastpoll = 1;
> @@ -227,6 +228,39 @@ ihidev_detach(struct device *self, int f
> return (0);
> }
>
> +int
> +ihidev_activate(struct device *self, int act)
> +{
> + struct ihidev_softc *sc = (struct ihidev_softc *)self;
> +
> + DPRINTF(("%s(%d)\n", __func__, act));
> +
> + switch (act) {
> + case DVACT_QUIESCE:
> + sc->sc_dying = 1;
> + if (sc->sc_poll && timeout_initialized(&sc->sc_timer)) {
> + DPRINTF(("%s: canceling polling\n",
> + sc->sc_dev.dv_xname));
> + timeout_del_barrier(&sc->sc_timer);
> + }
> + if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER,
> + &I2C_HID_POWER_OFF))
> + printf("%s: failed to power down\n",
> + sc->sc_dev.dv_xname);
> + break;
> + case DVACT_WAKEUP:
> + ihidev_reset(sc);
> + sc->sc_dying = 0;
> + if (sc->sc_poll && timeout_initialized(&sc->sc_timer))
> + timeout_add(&sc->sc_timer, 2000);
> + break;
> + }
> +
> + config_activate_children(self, act);
> +
> + return 0;
> +}
> +
> void
> ihidev_sleep(struct ihidev_softc *sc, int ms)
> {
> @@ -580,6 +614,16 @@ ihidev_hid_desc_parse(struct ihidev_soft
> return (0);
> }
>
> +void
> +ihidev_poll(void *arg)
> +{
> + struct ihidev_softc *sc = arg;
> +
> + sc->sc_frompoll = 1;
> + ihidev_intr(sc);
> + sc->sc_frompoll = 0;
> +}
> +
> int
> ihidev_intr(void *arg)
> {
> @@ -589,6 +633,16 @@ ihidev_intr(void *arg)
> u_char *p;
> u_int rep = 0;
>
> + if (sc->sc_dying)
> + retur
ihidev: always register interrupt, stop polling if it fires
The fast polling of ihidev may cause a problem during suspend/resume
because dwiic may be in an unknown state, so add a DVACT_QUIESCE
handler to properly shut it down.
Even if polling is requested, register the interrupt handler too.
If it ever fires, stop polling and just use the interrupt. This is
what led me to discover that ihidev's interrupt starts firing after
a suspend/resume cycle (but I still have no idea why it doesn't work
before that).
Index: dev/acpi/dwiic_acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/dwiic_acpi.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 dwiic_acpi.c
--- dev/acpi/dwiic_acpi.c 16 Jul 2019 19:12:32 - 1.9
+++ dev/acpi/dwiic_acpi.c 19 Jul 2019 19:11:21 -
@@ -457,8 +457,9 @@ dwiic_acpi_found_ihidev(struct dwiic_sof
aml_freevalue(&res);
- if (!sc->sc_poll_ihidev &&
- !(crs.irq_int == 0 && crs.gpio_int_node == NULL))
+ if (sc->sc_poll_ihidev)
+ ia.ia_poll = 1;
+ if (!(crs.irq_int == 0 && crs.gpio_int_node == NULL))
ia.ia_intr = &crs;
if (config_found(sc->sc_iic, &ia, dwiic_i2c_print)) {
Index: dev/i2c/i2cvar.h
===
RCS file: /cvs/src/sys/dev/i2c/i2cvar.h,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 i2cvar.h
--- dev/i2c/i2cvar.h23 Apr 2016 09:40:28 - 1.16
+++ dev/i2c/i2cvar.h19 Jul 2019 19:11:21 -
@@ -113,6 +113,7 @@ struct i2c_attach_args {
char*ia_name; /* chip name */
void*ia_cookie; /* pass extra info from bus to dev */
void*ia_intr; /* interrupt info */
+ int ia_poll;/* to force polling */
};
/*
Index: dev/i2c/ihidev.c
===
RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
retrieving revision 1.19
diff -u -p -u -p -r1.19 ihidev.c
--- dev/i2c/ihidev.c8 Apr 2019 17:50:45 - 1.19
+++ dev/i2c/ihidev.c19 Jul 2019 19:11:21 -
@@ -63,6 +63,7 @@ static int I2C_HID_POWER_OFF = 0x1;
intihidev_match(struct device *, void *, void *);
void ihidev_attach(struct device *, struct device *, void *);
intihidev_detach(struct device *, int);
+intihidev_activate(struct device *, int);
intihidev_hid_command(struct ihidev_softc *, int, void *);
intihidev_intr(void *);
@@ -80,7 +81,7 @@ struct cfattach ihidev_ca = {
ihidev_match,
ihidev_attach,
ihidev_detach,
- NULL
+ ihidev_activate,
};
struct cfdriver ihidev_cd = {
@@ -128,7 +129,7 @@ ihidev_attach(struct device *parent, str
printf(", can't establish interrupt");
}
- if (sc->sc_ih == NULL) {
+ if (ia->ia_poll) {
printf(" (polling)");
sc->sc_poll = 1;
sc->sc_fastpoll = 1;
@@ -227,6 +228,39 @@ ihidev_detach(struct device *self, int f
return (0);
}
+int
+ihidev_activate(struct device *self, int act)
+{
+ struct ihidev_softc *sc = (struct ihidev_softc *)self;
+
+ DPRINTF(("%s(%d)\n", __func__, act));
+
+ switch (act) {
+ case DVACT_QUIESCE:
+ sc->sc_dying = 1;
+ if (sc->sc_poll && timeout_initialized(&sc->sc_timer)) {
+ DPRINTF(("%s: canceling polling\n",
+ sc->sc_dev.dv_xname));
+ timeout_del_barrier(&sc->sc_timer);
+ }
+ if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER,
+ &I2C_HID_POWER_OFF))
+ printf("%s: failed to power down\n",
+ sc->sc_dev.dv_xname);
+ break;
+ case DVACT_WAKEUP:
+ ihidev_reset(sc);
+ sc->sc_dying = 0;
+ if (sc->sc_poll && timeout_initialized(&sc->sc_timer))
+ timeout_add(&sc->sc_timer, 2000);
+ break;
+ }
+
+ config_activate_children(self, act);
+
+ return 0;
+}
+
void
ihidev_sleep(struct ihidev_softc *sc, int ms)
{
@@ -580,6 +614,16 @@ ihidev_hid_desc_parse(struct ihidev_soft
return (0);
}
+void
+ihidev_poll(void *arg)
+{
+ struct ihidev_softc *sc = arg;
+
+ sc->sc_frompoll = 1;
+ ihidev_intr(sc);
+ sc->sc_frompoll = 0;
+}
+
int
ihidev_intr(void *arg)
{
@@ -589,6 +633,16 @@ ihidev_intr(void *arg)
u_char *p;
u_int rep = 0;
+ if (sc->sc_dying)
+ return 1;
+
+ if (sc->sc_poll && !sc->sc_frompoll) {
+ printf("%s: received interrupt while polling, disabling "
+ "polling\n", sc->sc_dev.dv_xname);
+ sc->sc_poll = 0;
+ timeout_del_barrier(&sc->sc_timer);
+ }
+
/*
* XXX: force I2C_F_POLL for now to avoid dwiic interrupting
