Re: [PATCH] Add intr_mask() / intr_unmask() interface to mask / unmask individual interrupt sources

[Jason Thorpe <thorpej%me.com@localhost>]

+Andrew for his comments as well.

> On Dec 1, 2019, at 3:16 AM, Jason Thorpe <thorpej%me.com@localhost> wrote:
> 
> Ok, so I thought about this some more, and there is a problem there, but not the one you mentioned.
> 
> intr_mask() is intended to be callable from an interrupt handler, but you can't take the cpu_lock in that case, because that's a MUTEX_DEFAULT mutex.  The code can be tweaked to address this issue, but yes, places where the mask count is checked do need to be protected in a block that disables interrupts.
> 
> I'll post a follow-up patch shortly.

Well, FSVO "shortly".  Anyway, I *think* this is the final patch.  The main change between this one and the previous is that this one also uses an xcall to perform the "hwunmask" operation.  I added the relevant details in code comments, so please make sure to read them.

Again, to recap:

1- Adds intr_mask() and intr_unmask() functions that take as an argument the cookie returned by intr_establish().

2- intr_mask() calls can be nested (i.e. there is a mask count).

3- intr_mask() can be called from interrupt context; intr_unmask() cannot (softint context is OK).

4- Whereas spl*() masks off a logical interrupt level (e.g. IPL_NET), intr_mask() masks off an individual interrupt source / line.

5- Wrappers for ACPI are included (following the existing ACPI interrupt code layering model).

6- Adapts the hid-over-i2c driver, so as to avoid using i2c in interrupt context.  I don't have such a device.  Please ping me off-list if you have one and would be willing to test a kernel with these changes.

7- Making all of this stuff work with Xen is up to the people who maintain our Xen code.  As of right now, it doesn't, and I don't know what will happen if code on a Xen system tries to call intr_mask().

diff --git a/sys/arch/amd64/amd64/genassym.cf b/sys/arch/amd64/amd64/genassym.cf
index 168cae5cb8b5..270a9118d840 100644
--- a/sys/arch/amd64/amd64/genassym.cf
+++ b/sys/arch/amd64/amd64/genassym.cf
@@ -320,7 +320,8 @@ define	IS_FLAGS	offsetof(struct intrsource, is_flags)
 define	IS_PIN		offsetof(struct intrsource, is_pin)
 define	IS_TYPE		offsetof(struct intrsource, is_type)
 define	IS_MAXLEVEL	offsetof(struct intrsource, is_maxlevel)
-define	IS_LWP			offsetof(struct intrsource, is_lwp)
+define	IS_LWP		offsetof(struct intrsource, is_lwp)
+define	IS_MASK_COUNT	offsetof(struct intrsource, is_mask_count)
 
 define	IPL_NONE		IPL_NONE
 define	IPL_PREEMPT		IPL_PREEMPT
diff --git a/sys/arch/amd64/amd64/vector.S b/sys/arch/amd64/amd64/vector.S
index 2ef3a7ba3b08..a0ac3fed6866 100644
--- a/sys/arch/amd64/amd64/vector.S
+++ b/sys/arch/amd64/amd64/vector.S
@@ -387,6 +387,8 @@ IDTVEC(handle_ ## name ## num)						;\
 	sti								;\
 	incl	CPUVAR(IDEPTH)						;\
 	movq	IS_HANDLERS(%r14),%rbx					;\
+	cmpl	$0,IS_MASK_COUNT(%r14)	/* source currently masked? */	;\
+	jne	7f			/* yes, hold it */		;\
 6:									\
 	movl	IH_LEVEL(%rbx),%r12d					;\
 	cmpl	%r13d,%r12d						;\
@@ -399,6 +401,8 @@ IDTVEC(handle_ ## name ## num)						;\
 	testq	%rbx,%rbx						;\
 	jnz	6b							;\
 5:									\
+	cmpl	$0,IS_MASK_COUNT(%r14)	/* source now masked? */	;\
+	jne	7f			/* yes, deal */			;\
 	cli								;\
 	unmask(num)			/* unmask it in hardware */	;\
 	late_ack(num)							;\
diff --git a/sys/arch/i386/i386/genassym.cf b/sys/arch/i386/i386/genassym.cf
index 479c5319fc2f..0002e23d6a5f 100644
--- a/sys/arch/i386/i386/genassym.cf
+++ b/sys/arch/i386/i386/genassym.cf
@@ -319,6 +319,7 @@ define	IS_PIN			offsetof(struct intrsource, is_pin)
 define	IS_TYPE			offsetof(struct intrsource, is_type)
 define	IS_MAXLEVEL		offsetof(struct intrsource, is_maxlevel)
 define	IS_LWP			offsetof(struct intrsource, is_lwp)
+define	IS_MASK_COUNT		offsetof(struct intrsource, is_mask_count)
 
 define	IPL_NONE		IPL_NONE
 define	IPL_PREEMPT		IPL_PREEMPT
diff --git a/sys/arch/i386/i386/vector.S b/sys/arch/i386/i386/vector.S
index 5ef99df2713c..3c5a625cef49 100644
--- a/sys/arch/i386/i386/vector.S
+++ b/sys/arch/i386/i386/vector.S
@@ -408,6 +408,8 @@ IDTVEC(intr_ ## name ## num)						;\
 	IDEPTH_INCR							;\
 	sti								;\
 	movl	IS_HANDLERS(%ebp),%ebx					;\
+	cmpl	$0,IS_MASK_COUNT(%ebp)	/* source currently masked? */	;\
+	jne	7f			/* yes, hold it */		;\
 6:									\
 	movl	IH_LEVEL(%ebx),%edi					;\
 	cmpl	%esi,%edi						;\
@@ -420,6 +422,8 @@ IDTVEC(intr_ ## name ## num)						;\
 	addl	$4,%esp			/* toss the arg */		;\
 	testl	%ebx,%ebx						;\
 	jnz	6b							;\
+	cmpl	$0,IS_MASK_COUNT(%ebp)	/* source now masked? */	;\
+	jne	7f			/* yes, deal */			;\
 	cli								;\
 	unmask(num)			/* unmask it in hardware */	;\
 	late_ack(num)							;\
diff --git a/sys/arch/x86/acpi/acpi_machdep.c b/sys/arch/x86/acpi/acpi_machdep.c
index 41f39d2b52f0..3519fdf7ba79 100644
--- a/sys/arch/x86/acpi/acpi_machdep.c
+++ b/sys/arch/x86/acpi/acpi_machdep.c
@@ -294,6 +294,18 @@ acpi_md_intr_establish(uint32_t InterruptNumber, int ipl, int type,
 	return ih;
 }
 
+void
+acpi_md_intr_mask(void *ih)
+{
+	intr_mask(ih);
+}
+
+void
+acpi_md_intr_unmask(void *ih)
+{
+	intr_unmask(ih);
+}
+
 void
 acpi_md_intr_disestablish(void *ih)
 {
diff --git a/sys/arch/x86/include/acpi_machdep.h b/sys/arch/x86/include/acpi_machdep.h
index 86a7bd36d337..a2277cd5811f 100644
--- a/sys/arch/x86/include/acpi_machdep.h
+++ b/sys/arch/x86/include/acpi_machdep.h
@@ -72,6 +72,8 @@ void		acpi_md_OsEnableInterrupt(void);
 
 void *		acpi_md_intr_establish(uint32_t, int, int, int (*)(void *),
 		    void *, bool, const char *);
+void		acpi_md_intr_mask(void *);
+void		acpi_md_intr_unmask(void *);
 void		acpi_md_intr_disestablish(void *);
 
 int		acpi_md_sleep(int);
diff --git a/sys/arch/x86/include/intr.h b/sys/arch/x86/include/intr.h
index 6fb4e9545338..f8d6e69cd24e 100644
--- a/sys/arch/x86/include/intr.h
+++ b/sys/arch/x86/include/intr.h
@@ -1,7 +1,7 @@
 /*	$NetBSD: intr.h,v 1.60 2019/02/14 08:18:25 cherry Exp $	*/
 
 /*-
- * Copyright (c) 1998, 2001, 2006, 2007, 2008 The NetBSD Foundation, Inc.
+ * Copyright (c) 1998, 2001, 2006, 2007, 2008, 2019 The NetBSD Foundation, Inc.
  * All rights reserved.
  *
  * This code is derived from software contributed to The NetBSD Foundation
@@ -95,6 +95,16 @@ struct intrsource {
 	u_long ipl_evt_mask2[NR_EVENT_CHANNELS];
 #endif
 	struct evcnt is_evcnt;		/* interrupt counter per cpu */
+	/*
+	 * is_mask_count requires special handling; it can only be modifed
+	 * or examined on the CPU that owns the interrupt source, and such
+	 * references need to be protected by disabling interrupts.  This
+	 * is because intr_mask() can be called from an interrupt handler.
+	 * is_distribute_pending does not require such special handling
+	 * because intr_unmask() cannot be called from an interrupt handler.
+	 */
+	u_int is_mask_count;		/* masked? (nested) [see above] */
+	int is_distribute_pending;	/* ci<->ci move pending [cpu_lock] */
 	int is_flags;			/* see below */
 	int is_type;			/* level, edge */
 	int is_idtvec;
@@ -215,6 +225,8 @@ void x86_nmi(void);
 void *intr_establish_xname(int, struct pic *, int, int, int, int (*)(void *),
 			   void *, bool, const char *);
 void *intr_establish(int, struct pic *, int, int, int, int (*)(void *), void *, bool);
+void intr_mask(struct intrhand *);
+void intr_unmask(struct intrhand *);
 void intr_disestablish(struct intrhand *);
 void intr_add_pcibus(struct pcibus_attach_args *);
 const char *intr_string(intr_handle_t, char *, size_t);
diff --git a/sys/arch/x86/x86/intr.c b/sys/arch/x86/x86/intr.c
index 3722436e5886..07d678137e39 100644
--- a/sys/arch/x86/x86/intr.c
+++ b/sys/arch/x86/x86/intr.c
@@ -1,11 +1,11 @@
 /*	$NetBSD: intr.c,v 1.146 2019/06/17 06:38:30 msaitoh Exp $	*/
 
 /*
- * Copyright (c) 2007, 2008, 2009 The NetBSD Foundation, Inc.
+ * Copyright (c) 2007, 2008, 2009, 2019 The NetBSD Foundation, Inc.
  * All rights reserved.
  *
  * This code is derived from software contributed to The NetBSD Foundation
- * by Andrew Doran.
+ * by Andrew Doran, and by Jason R. Thorpe.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -739,6 +739,34 @@ intr_append_intrsource_xname(struct intrsource *isp, const char *xname)
 	strlcat(isp->is_xname, xname, sizeof(isp->is_xname));
 }
 
+/*
+ * Called on bound CPU to handle calling pic_hwunmask from contexts
+ * that are not already running on the bound CPU.
+ *
+ * => caller (on initiating CPU) holds cpu_lock on our behalf
+ * => arg1: struct intrhand *ih
+ */
+static void
+intr_hwunmask_xcall(void *arg1, void *arg2)
+{
+	struct intrhand * const ih = arg1;
+	struct cpu_info * const ci = ih->ih_cpu;
+
+	KASSERT(ci == curcpu() || !mp_online);
+
+	const u_long psl = x86_read_psl();
+	x86_disable_intr();
+
+	struct intrsource * const source = ci->ci_isources[ih->ih_slot];
+	struct pic * const pic = source->is_pic;
+
+	if (source->is_mask_count == 0) {
+		(*pic->pic_hwunmask)(pic, ih->ih_pin);
+	}
+
+	x86_write_psl(psl);
+}
+
 /*
  * Handle per-CPU component of interrupt establish.
  *
@@ -955,7 +983,12 @@ intr_establish_xname(int legacy_irq, struct pic *pic, int pin, int type,
 
 	/* All set up, so add a route for the interrupt and unmask it. */
 	(*pic->pic_addroute)(pic, ci, pin, idt_vec, type);
-	(*pic->pic_hwunmask)(pic, pin);
+	if (ci == curcpu() || !mp_online) {
+		intr_hwunmask_xcall(ih, NULL);
+	} else {
+		where = xc_unicast(0, intr_hwunmask_xcall, ih, NULL, ci);
+		xc_wait(where);
+	}
 	mutex_exit(&cpu_lock);
 
 	if (bootverbose || cpu_index(ci) != 0)
@@ -976,6 +1009,118 @@ intr_establish(int legacy_irq, struct pic *pic, int pin, int type, int level,
 	    level, handler, arg, known_mpsafe, "unknown");
 }
 
+/*
+ * Called on bound CPU to handle intr_mask() / intr_unmask().
+ *
+ * => caller (on initiating CPU) holds cpu_lock on our behalf
+ * => arg1: struct intrhand *ih
+ * => arg2: true -> mask, false -> unmask.
+ */
+static void
+intr_mask_xcall(void *arg1, void *arg2)
+{
+	struct intrhand * const ih = arg1;
+	const uintptr_t mask = (uintptr_t)arg2;
+	struct cpu_info * const ci = ih->ih_cpu;
+	bool force_pending = false;
+
+	KASSERT(ci == curcpu() || !mp_online);
+
+	/*
+	 * We need to disable interrupts to hold off the interrupt
+	 * vectors.
+	 */
+	const u_long psl = x86_read_psl();
+	x86_disable_intr();
+
+	struct intrsource * const source = ci->ci_isources[ih->ih_slot];
+	struct pic * const pic = source->is_pic;
+
+	if (mask) {
+		source->is_mask_count++;
+		KASSERT(source->is_mask_count != 0);
+		if (source->is_mask_count == 1) {
+			(*pic->pic_hwmask)(pic, ih->ih_pin);
+		}
+	} else {
+		KASSERT(source->is_mask_count != 0);
+		if (--source->is_mask_count == 0) {
+			/*
+			 * If this interrupt source is being moved, don't
+			 * unmask it at the hw.
+			 */
+			if (! source->is_distribute_pending)
+				(*pic->pic_hwunmask)(pic, ih->ih_pin);
+			force_pending = true;
+		}
+	}
+
+	/* Re-enable interrupts. */
+	x86_write_psl(psl);
+
+	if (force_pending) {
+		/* Force processing of any pending interrupts. */
+		splx(splhigh());
+	}
+}
+
+static void
+intr_mask_internal(struct intrhand * const ih, const bool mask)
+{
+
+	/*
+	 * Call out to the remote CPU to update its interrupt state.
+	 * Only make RPCs if the APs are up and running.
+	 */
+	mutex_enter(&cpu_lock);
+	struct cpu_info * const ci = ih->ih_cpu;
+	void * const mask_arg = (void *)(uintptr_t)mask;
+	if (ci == curcpu() || !mp_online) {
+		intr_mask_xcall(ih, mask_arg);
+	} else {
+		const uint64_t where =
+		    xc_unicast(0, intr_mask_xcall, ih, mask_arg, ci);
+		xc_wait(where);
+	}
+	mutex_exit(&cpu_lock);
+}
+
+void
+intr_mask(struct intrhand *ih)
+{
+
+	if (cpu_intr_p()) {
+		/*
+		 * Special case of calling intr_mask() from an interrupt
+		 * handler: we MUST be called from the bound CPU for this
+		 * interrupt (presumably from a handler we're about to
+		 * mask).
+		 *
+		 * We can't take the cpu_lock in this case, and we must
+		 * therefore be extra careful.
+		 */
+		struct cpu_info * const ci = ih->ih_cpu;
+		KASSERT(ci == curcpu() || !mp_online);
+		intr_mask_xcall(ih, (void *)(uintptr_t)true);
+		return;
+	}
+
+	intr_mask_internal(ih, true);
+}
+
+void
+intr_unmask(struct intrhand *ih)
+{
+
+	/*
+	 * This is not safe to call from an interrupt context because
+	 * we don't want to accidentally unmask an interrupt source
+	 * that's masked because it's being serviced.
+	 */
+	KASSERT(!cpu_intr_p());
+	intr_mask_internal(ih, false);
+}
+
 /*
  * Called on bound CPU to handle intr_disestablish().
  *
@@ -1036,7 +1181,7 @@ intr_disestablish_xcall(void *arg1, void *arg2)
 	if (source->is_handlers == NULL)
 		(*pic->pic_delroute)(pic, ci, ih->ih_pin, idtvec,
 		    source->is_type);
-	else
+	else if (source->is_mask_count == 0)
 		(*pic->pic_hwunmask)(pic, ih->ih_pin);
 
 	/* Re-enable interrupts. */
@@ -1851,10 +1996,14 @@ intr_set_affinity(struct intrsource *isp, const kcpuset_t *cpuset)
 		return err;
 	}
 
+	/* Prevent intr_unmask() from reenabling the source at the hw. */
+	isp->is_distribute_pending = true;
+
 	pin = isp->is_pin;
 	(*pic->pic_hwmask)(pic, pin); /* for ci_ipending check */
-	while (oldci->ci_ipending & (1 << oldslot))
+	while (oldci->ci_ipending & (1 << oldslot)) {
 		(void)kpause("intrdist", false, 1, &cpu_lock);
+	}
 
 	kpreempt_disable();
 
@@ -1889,9 +2038,16 @@ intr_set_affinity(struct intrsource *isp, const kcpuset_t *cpuset)
 	isp->is_active_cpu = newci->ci_cpuid;
 	(*pic->pic_addroute)(pic, newci, pin, idt_vec, isp->is_type);
 
-	kpreempt_enable();
+	isp->is_distribute_pending = false;
+	if (newci == curcpu() || !mp_online) {
+		intr_hwunmask_xcall(ih, NULL);
+	} else {
+		uint64_t where;
+		where = xc_unicast(0, intr_hwunmask_xcall, ih, NULL, newci);
+		xc_wait(where);
+	}
 
-	(*pic->pic_hwunmask)(pic, pin);
+	kpreempt_enable();
 
 	return err;
 }
diff --git a/sys/dev/acpi/acpi_intr.h b/sys/dev/acpi/acpi_intr.h
index 5d2d8d47c0a3..6001204a8d2b 100644
--- a/sys/dev/acpi/acpi_intr.h
+++ b/sys/dev/acpi/acpi_intr.h
@@ -28,7 +28,15 @@
  * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
  * POSSIBILITY OF SUCH DAMAGE.
  */
+
+#ifndef _DEV_ACPI_ACPI_INTR_H_
+#define	_DEV_ACPI_ACPI_INTR_H_
+
 void *		acpi_intr_establish(device_t, uint64_t, int, bool,
 		    int (*intr)(void *), void *, const char *);
+void		acpi_intr_mask(void *);
+void		acpi_intr_unmask(void *);
 void 		acpi_intr_disestablish(void *);
 const char * 	acpi_intr_string(void *, char *, size_t len);
+
+#endif /* _DEV_ACPI_ACPI_INTR_H_ */
diff --git a/sys/dev/acpi/acpi_util.c b/sys/dev/acpi/acpi_util.c
index aac787211eee..fdec7ad89be9 100644
--- a/sys/dev/acpi/acpi_util.c
+++ b/sys/dev/acpi/acpi_util.c
@@ -590,6 +590,22 @@ end:
 	return aih;
 }
 
+void
+acpi_intr_mask(void *c)
+{
+	struct acpi_irq_handler * const aih = c;
+
+	acpi_md_intr_mask(aih->aih_ih);
+}
+
+void
+acpi_intr_unmask(void *c)
+{
+	struct acpi_irq_handler * const aih = c;
+
+	acpi_md_intr_unmask(aih->aih_ih);
+}
+
 void
 acpi_intr_disestablish(void *c)
 {
diff --git a/sys/dev/i2c/ihidev.c b/sys/dev/i2c/ihidev.c
index 1e0956295795..8d7cb2a51b0d 100644
--- a/sys/dev/i2c/ihidev.c
+++ b/sys/dev/i2c/ihidev.c
@@ -71,6 +71,7 @@ __KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1.7 2018/11/16 23:05:50 jmcneill Exp $");
 #  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 *);
 
@@ -204,18 +209,9 @@ ihidev_attach(device_t parent, device_t self, void *aux)
 		    repsz));
 	}
 	sc->sc_ibuf = kmem_zalloc(sc->sc_isize, KM_NOSLEEP);
-#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");
-		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;
@@ -262,10 +258,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");
@@ -651,31 +644,110 @@ ihidev_hid_desc_parse(struct ihidev_softc *sc)
 	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
@@ -685,7 +757,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 */
@@ -697,22 +769,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
diff --git a/sys/dev/i2c/ihidev.h b/sys/dev/i2c/ihidev.h
index 6685c307fe6e..911cfea6259c 100644
--- a/sys/dev/i2c/ihidev.h
+++ b/sys/dev/i2c/ihidev.h
@@ -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 {

-- thorpej