Re: CVS commit: src/sys/dev/i2c

[Ryo ONODERA <ryo%tetera.org@localhost>]

Hi,

"Jason R Thorpe" <thorpej%netbsd.org@localhost> 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 // ryo%tetera.org@localhost
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB  FD1B F404 27FA C7D1 15F3