From ga9@buffy.york.ac.uk Thu May 7 16:32:42 2009 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DAF901065670 for ; Thu, 7 May 2009 16:32:41 +0000 (UTC) (envelope-from ga9@buffy.york.ac.uk) Received: from mail-gw0.york.ac.uk (mail-gw0.york.ac.uk [144.32.128.245]) by mx1.freebsd.org (Postfix) with ESMTP id 8BA968FC16 for ; Thu, 7 May 2009 16:32:41 +0000 (UTC) (envelope-from ga9@buffy.york.ac.uk) Received: from mail-gw7.york.ac.uk (mail-gw7.york.ac.uk [144.32.129.30]) by mail-gw0.york.ac.uk (8.13.6/8.13.6) with ESMTP id n47GWZW6004658 for ; Thu, 7 May 2009 17:32:35 +0100 (BST) Received: from buffy-128.york.ac.uk ([144.32.128.160] helo=buffy.york.ac.uk) by mail-gw7.york.ac.uk with esmtps (TLSv1:AES256-SHA:256) (Exim 4.68) (envelope-from ) id 1M26WQ-0001uS-UV for FreeBSD-gnats-submit@freebsd.org; Thu, 07 May 2009 17:32:35 +0100 Received: from buffy.york.ac.uk (localhost [127.0.0.1]) by buffy.york.ac.uk (8.14.3/8.14.3) with ESMTP id n47GWY3t083348 for ; Thu, 7 May 2009 17:32:34 +0100 (BST) (envelope-from ga9@buffy.york.ac.uk) Received: (from ga9@localhost) by buffy.york.ac.uk (8.14.3/8.14.3/Submit) id n47GWYKf083347; Thu, 7 May 2009 17:32:34 +0100 (BST) (envelope-from ga9) Message-Id: <200905071632.n47GWYKf083347@buffy.york.ac.uk> Date: Thu, 7 May 2009 17:32:34 +0100 (BST) From: Gavin Atkinson Reply-To: Gavin Atkinson To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: [patch] Lock GPIO accesses on ixp425 X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 134338 >Category: arm >Synopsis: [patch] Lock GPIO accesses on ixp425 >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-arm >State: patched >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Thu May 07 16:40:01 UTC 2009 >Closed-Date: >Last-Modified: Wed Dec 08 00:31:24 UTC 2010 >Originator: Gavin Atkinson >Release: FreeBSD 7.1-STABLE amd64 >Organization: >Environment: System: FreeBSD buffy.york.ac.uk 7.1-STABLE FreeBSD 7.1-STABLE #5: Fri Feb 13 11:25:58 GMT 2009 root@buffy.york.ac.uk:/usr/obj/usr/src/sys/GENERIC amd64 >Description: There are several points in the code where GPIO registers are read, various operations are performed with the result, then GPIO registers are written back to. The problem here is that there is nothing preventing other access occuring between the read and the write. Some parts of the code (IIC) go one step further and use Giant as a lock around these accesses, but as it is not done globally, this makes little difference. I stumbled on this while writing a driver for the NSLU LEDs, while hammering the driver I saw interrupts being missed. Whilst I have no absolute proof that this is the cause, I've not been able to recreate it with this patch in place. I've gone for a seperate spin lock for the GPIO pins as it is used in various contexts where a spin lock seems to be the correct choice. It's also implemented as a global lock rather than being stored in a softc or similar - until there is a GPIO driver, I can't see any better solution. Something like this is probably also needed for the other arm platforms, however I do not have the hardware required to test these. >How-To-Repeat: (ab)use code paths that toggle GPIO lines (e.g. use IIC heavily while USB generates interrupts) >Fix: --- ixp425-gpio-locking.diff begins here --- Index: src-head/sys/arm/xscale/ixp425/avila_ata.c =================================================================== RCS file: /home/ncvs/src/sys/arm/xscale/ixp425/avila_ata.c,v retrieving revision 1.6 diff -u -r1.6 avila_ata.c --- src-head/sys/arm/xscale/ixp425/avila_ata.c 20 Dec 2008 03:26:09 -0000 1.6 +++ src-head/sys/arm/xscale/ixp425/avila_ata.c 7 May 2009 16:16:12 -0000 @@ -44,7 +44,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -151,6 +153,7 @@ u_int16_t *, bus_size_t); static void ata_bs_wm_2_s(void *, bus_space_handle_t, bus_size_t, const u_int16_t *, bus_size_t); +extern struct mtx ixp425_gpio_mtx; static int ata_avila_probe(device_t dev) @@ -218,6 +221,8 @@ rman_set_bustag(&sc->sc_alt_ata, &sc->sc_expbus_tag); rman_set_bushandle(&sc->sc_alt_ata, sc->sc_alt_ioh); + mtx_lock(&ixp425_gpio_mtx); + GPIO_CONF_WRITE_4(sa, IXP425_GPIO_GPOER, GPIO_CONF_READ_4(sa, IXP425_GPIO_GPOER) | (1<gpin)); /* set interrupt type */ @@ -229,6 +234,8 @@ /* clear ISR */ GPIO_CONF_WRITE_4(sa, IXP425_GPIO_GPISR, (1<gpin)); + mtx_unlock(&ixp425_gpio_mtx); + /* configure CS1/3 window, leaving timing unchanged */ EXP_BUS_WRITE_4(sc, sc->sc_16bit_off, EXP_BUS_READ_4(sc, sc->sc_16bit_off) | Index: src-head/sys/arm/xscale/ixp425/avila_led.c =================================================================== RCS file: /home/ncvs/src/sys/arm/xscale/ixp425/avila_led.c,v retrieving revision 1.2 diff -u -r1.2 avila_led.c --- src-head/sys/arm/xscale/ixp425/avila_led.c 20 Dec 2008 03:26:09 -0000 1.2 +++ src-head/sys/arm/xscale/ixp425/avila_led.c 7 May 2009 16:16:28 -0000 @@ -28,7 +28,9 @@ #include #include #include +#include #include +#include #include #include @@ -46,18 +48,22 @@ struct cdev *sc_led; }; +extern struct mtx ixp425_gpio_mtx; + static void led_func(void *arg, int onoff) { struct led_avila_softc *sc = arg; uint32_t reg; + mtx_lock(&ixp425_gpio_mtx); reg = GPIO_CONF_READ_4(sc, IXP425_GPIO_GPOUTR); if (onoff) reg &= ~GPIO_LED_STATUS_BIT; else reg |= GPIO_LED_STATUS_BIT; GPIO_CONF_WRITE_4(sc, IXP425_GPIO_GPOUTR, reg); + mtx_unlock(&ixp425_gpio_mtx); } static int @@ -78,8 +84,10 @@ sc->sc_gpio_ioh = sa->sc_gpio_ioh; /* Configure LED GPIO pin as output */ + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_WRITE_4(sc, IXP425_GPIO_GPOER, GPIO_CONF_READ_4(sc, IXP425_GPIO_GPOER) &~ GPIO_LED_STATUS_BIT); + mtx_unlock(&ixp425_gpio_mtx); sc->sc_led = led_create(led_func, sc, "gpioled"); Index: src-head/sys/arm/xscale/ixp425/ixdp425_pci.c =================================================================== RCS file: /home/ncvs/src/sys/arm/xscale/ixp425/ixdp425_pci.c,v retrieving revision 1.2 diff -u -r1.2 ixdp425_pci.c --- src-head/sys/arm/xscale/ixp425/ixdp425_pci.c 20 Mar 2008 15:54:19 -0000 1.2 +++ src-head/sys/arm/xscale/ixp425/ixdp425_pci.c 7 May 2009 16:16:43 -0000 @@ -40,7 +40,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -51,6 +53,8 @@ #include #include +extern struct mtx ixp425_gpio_mtx; + void ixp425_md_attach(device_t dev) { @@ -58,7 +62,7 @@ struct ixppcib_softc *pci_sc = device_get_softc(dev); uint32_t reg; - + mtx_lock(&ixp425_gpio_mtx); /* PCI Reset Assert */ reg = GPIO_CONF_READ_4(sc, IXP425_GPIO_GPOUTR); reg &= ~(1U << GPIO_PCI_RESET); @@ -130,6 +134,7 @@ reg = GPIO_CONF_READ_4(sc, IXP425_GPIO_GPOUTR); reg |= 1U << GPIO_PCI_RESET; GPIO_CONF_WRITE_4(sc, IXP425_GPIO_GPOUTR, reg | (1U << GPIO_PCI_RESET)); + mtx_unlock(&ixp425_gpio_mtx); pci_sc->sc_irq_rman.rm_type = RMAN_ARRAY; pci_sc->sc_irq_rman.rm_descr = "IXP425 PCI IRQs"; CTASSERT(PCI_INT_D < PCI_INT_A); Index: src-head/sys/arm/xscale/ixp425/ixp425.c =================================================================== RCS file: /home/ncvs/src/sys/arm/xscale/ixp425/ixp425.c,v retrieving revision 1.17 diff -u -r1.17 ixp425.c --- src-head/sys/arm/xscale/ixp425/ixp425.c 10 Mar 2009 19:15:35 -0000 1.17 +++ src-head/sys/arm/xscale/ixp425/ixp425.c 7 May 2009 16:17:00 -0000 @@ -43,7 +43,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -65,6 +67,7 @@ uint32_t intr_steer2 = 0; struct ixp425_softc *ixp425_softc = NULL; +struct mtx ixp425_gpio_mtx; static int ixp425_probe(device_t); static void ixp425_identify(driver_t *, device_t); @@ -253,6 +256,7 @@ KASSERT(ixp425_softc == NULL, ("%s called twice?", __func__)); ixp425_softc = sc; + mtx_init(&ixp425_gpio_mtx, "GPIO register mutex", NULL, MTX_SPIN); intr_enabled = 0; ixp425_set_intrmask(); ixp425_set_intrsteer(); Index: src-head/sys/arm/xscale/ixp425/ixp425_iic.c =================================================================== RCS file: /home/ncvs/src/sys/arm/xscale/ixp425/ixp425_iic.c,v retrieving revision 1.4 diff -u -r1.4 ixp425_iic.c --- src-head/sys/arm/xscale/ixp425/ixp425_iic.c 20 Dec 2008 03:26:09 -0000 1.4 +++ src-head/sys/arm/xscale/ixp425/ixp425_iic.c 7 May 2009 16:17:16 -0000 @@ -29,7 +29,9 @@ #include #include #include +#include #include +#include #include #include @@ -60,6 +62,8 @@ static struct ixpiic_softc *ixpiic_sc = NULL; +extern struct mtx ixp425_gpio_mtx; + static int ixpiic_probe(device_t dev) { @@ -79,10 +83,12 @@ sc->sc_iot = sa->sc_iot; sc->sc_gpio_ioh = sa->sc_gpio_ioh; + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_SET(sc, IXP425_GPIO_GPOER, GPIO_I2C_SCL_BIT | GPIO_I2C_SDA_BIT); GPIO_CONF_CLR(sc, IXP425_GPIO_GPOUTR, GPIO_I2C_SCL_BIT | GPIO_I2C_SDA_BIT); + mtx_unlock(&ixp425_gpio_mtx); /* add generic bit-banging code */ if ((sc->iicbb = device_add_child(dev, "iicbb", -1)) == NULL) @@ -106,11 +112,11 @@ struct ixpiic_softc *sc = ixpiic_sc; uint32_t reg; - mtx_lock(&Giant); + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_SET(sc, IXP425_GPIO_GPOER, GPIO_I2C_SCL_BIT); reg = GPIO_CONF_READ_4(sc, IXP425_GPIO_GPINR); - mtx_unlock(&Giant); + mtx_unlock(&ixp425_gpio_mtx); return (reg & GPIO_I2C_SCL_BIT); } @@ -120,11 +126,11 @@ struct ixpiic_softc *sc = ixpiic_sc; uint32_t reg; - mtx_lock(&Giant); + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_SET(sc, IXP425_GPIO_GPOER, GPIO_I2C_SDA_BIT); reg = GPIO_CONF_READ_4(sc, IXP425_GPIO_GPINR); - mtx_unlock(&Giant); + mtx_unlock(&ixp425_gpio_mtx); return (reg & GPIO_I2C_SDA_BIT); } @@ -133,13 +139,13 @@ { struct ixpiic_softc *sc = ixpiic_sc; - mtx_lock(&Giant); + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_CLR(sc, IXP425_GPIO_GPOUTR, GPIO_I2C_SDA_BIT); if (val) GPIO_CONF_SET(sc, IXP425_GPIO_GPOER, GPIO_I2C_SDA_BIT); else GPIO_CONF_CLR(sc, IXP425_GPIO_GPOER, GPIO_I2C_SDA_BIT); - mtx_unlock(&Giant); + mtx_unlock(&ixp425_gpio_mtx); DELAY(I2C_DELAY); } @@ -148,13 +154,13 @@ { struct ixpiic_softc *sc = ixpiic_sc; - mtx_lock(&Giant); + mtx_lock(&ixp425_gpio_mtx); GPIO_CONF_CLR(sc, IXP425_GPIO_GPOUTR, GPIO_I2C_SCL_BIT); if (val) GPIO_CONF_SET(sc, IXP425_GPIO_GPOER, GPIO_I2C_SCL_BIT); else GPIO_CONF_CLR(sc, IXP425_GPIO_GPOER, GPIO_I2C_SCL_BIT); - mtx_unlock(&Giant); + mtx_unlock(&ixp425_gpio_mtx); DELAY(I2C_DELAY); } --- ixp425-gpio-locking.diff ends here --- >Release-Note: >Audit-Trail: State-Changed-From-To: open->patched State-Changed-By: thompsa State-Changed-When: Wed Dec 8 00:29:07 UTC 2010 State-Changed-Why: Equivalent patch committed in r215319 http://www.freebsd.org/cgi/query-pr.cgi?pr=134338 >Unformatted: