Date: Thu, 7 May 2009 17:32:34 +0100 (BST) From: Gavin Atkinson <gavin@FreeBSD.org> To: FreeBSD-gnats-submit@FreeBSD.org Subject: arm/134338: [patch] Lock GPIO accesses on ixp425 Message-ID: <200905071632.n47GWYKf083347@buffy.york.ac.uk> Resent-Message-ID: <200905071640.n47Ge1xQ087813@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 134338
>Category: arm
>Synopsis: [patch] Lock GPIO accesses on ixp425
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-arm
>State: open
>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:
>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 <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/module.h>
+#include <sys/mutex.h>
#include <sys/time.h>
#include <sys/bus.h>
#include <sys/resource.h>
@@ -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<<config->gpin));
/* set interrupt type */
@@ -229,6 +234,8 @@
/* clear ISR */
GPIO_CONF_WRITE_4(sa, IXP425_GPIO_GPISR, (1<<config->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 <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/module.h>
+#include <sys/mutex.h>
#include <sys/bus.h>
#include <arm/xscale/ixp425/ixp425reg.h>
@@ -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 <sys/systm.h>
#include <sys/bus.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/module.h>
+#include <sys/mutex.h>
#include <sys/malloc.h>
#include <sys/rman.h>
#include <machine/bus.h>
@@ -51,6 +53,8 @@
#include <arm/xscale/ixp425/ixp425_intr.h>
#include <arm/xscale/ixp425/ixdp425reg.h>
+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 <sys/systm.h>
#include <sys/bus.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/module.h>
+#include <sys/mutex.h>
#include <sys/malloc.h>
#include <sys/rman.h>
#include <machine/bus.h>
@@ -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 <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/module.h>
+#include <sys/mutex.h>
#include <sys/bus.h>
#include <sys/uio.h>
@@ -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:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200905071632.n47GWYKf083347>