Address a few issues with the iwi driver, namely:

+ do not release the dma-ble region used for downloading firmware.
  This should fix the problems that some people were seeing, due to
  memory becoming too fragmented which prevented subsequent allocations
  of a suitable contiguous region of memory;

+ document the firmware format and usage in if_iwivar.h

+ use a loop to allocate the four tx rings, instead of replicating
  the body of the loop.

+ add debugging code IWI_LOCK_ASSERT() to detect missing locks.
  These only do a printf, and should go away once we figure out why
  the driver sometimes freezes the system due to a (yet unidentified)
  race condition.

+ add a device_printf() in iwi_ioctl() in certain conditions
  (see comment in the code).  This helps preventing the race condition
  mentioned above, and makes the system survive. This printf will
  also go away once fixing this bug is completed.

+ change iwi_getfw() to return 0 on success, 1 on error, consistently
  with other functions.

+ fix the argument of a sizeof() in iwi_get_firmware()

+ use le32toh() to access little-endian fields

+ simplify error handling in iwi_load_firmware() and iwi_init_locked()

The bugs fixed by this commit (the freezing one especially) are serious
enough to call for a quick MFC

MFC after: 3 days

2 files changed, 147 insertions, 78 deletions

diff --git a/sys/dev/iwi/if_iwi.c b/sys/dev/iwi/if_iwi.c
index 1f7f573..9ebc86e 100644
--- a/sys/dev/iwi/if_iwi.c
+++ b/sys/dev/iwi/if_iwi.c
@@ -157,6 +157,7 @@ static void	iwi_stop_master(struct iwi_softc *);
 static int	iwi_reset(struct iwi_softc *);
 static int	iwi_load_ucode(struct iwi_softc *, const struct iwi_fw *);
 static int	iwi_load_firmware(struct iwi_softc *, const struct iwi_fw *);
+static void	iwi_release_fw_dma(struct iwi_softc *sc);
 static int	iwi_config(struct iwi_softc *);
 static int	iwi_get_firmware(struct iwi_softc *);
 static void	iwi_put_firmware(struct iwi_softc *);
@@ -331,32 +332,15 @@ iwi_attach(device_t dev)
 		goto fail;
 	}
 
-	error = iwi_alloc_tx_ring(sc, &sc->txq[0], IWI_TX_RING_COUNT,
-	    IWI_CSR_TX1_RIDX, IWI_CSR_TX1_WIDX);
-	if (error != 0) {
-		device_printf(dev, "could not allocate Tx ring 1\n");
-		goto fail;
-	}
-
-	error = iwi_alloc_tx_ring(sc, &sc->txq[1], IWI_TX_RING_COUNT,
-	    IWI_CSR_TX2_RIDX, IWI_CSR_TX2_WIDX);
-	if (error != 0) {
-		device_printf(dev, "could not allocate Tx ring 2\n");
-		goto fail;
-	}
-
-	error = iwi_alloc_tx_ring(sc, &sc->txq[2], IWI_TX_RING_COUNT,
-	    IWI_CSR_TX3_RIDX, IWI_CSR_TX3_WIDX);
-	if (error != 0) {
-		device_printf(dev, "could not allocate Tx ring 3\n");
-		goto fail;
-	}
-
-	error = iwi_alloc_tx_ring(sc, &sc->txq[3], IWI_TX_RING_COUNT,
-	    IWI_CSR_TX4_RIDX, IWI_CSR_TX4_WIDX);
-	if (error != 0) {
-		device_printf(dev, "could not allocate Tx ring 4\n");
-		goto fail;
+	for (i = 0; i < 4; i++) {
+		error = iwi_alloc_tx_ring(sc, &sc->txq[i], IWI_TX_RING_COUNT,
+		    IWI_CSR_TX1_RIDX + i * 4,
+		    IWI_CSR_TX1_WIDX + i * 4);
+		if (error != 0) {
+			device_printf(dev, "could not allocate Tx ring %d\n",
+				i+i);
+			goto fail;
+		}
 	}
 
 	if (iwi_alloc_rx_ring(sc, &sc->rxq, IWI_RX_RING_COUNT) != 0) {
@@ -496,6 +480,7 @@ iwi_detach(device_t dev)
 		ieee80211_ifdetach(ic);
 	}
 	iwi_put_firmware(sc);
+	iwi_release_fw_dma(sc);
 
 	iwi_free_cmd_ring(sc, &sc->cmdq);
 	iwi_free_tx_ring(sc, &sc->txq[0]);
@@ -970,6 +955,7 @@ iwi_newstate(struct ieee80211com *ic, enum ieee80211_state nstate, int arg)
 	struct ifnet *ifp = ic->ic_ifp;
 	struct iwi_softc *sc = ifp->if_softc;
 
+	IWI_LOCK_ASSERT(sc);
 	DPRINTF(("%s: %s -> %s flags 0x%x\n", __func__,
 		ieee80211_state_name[ic->ic_state],
 		ieee80211_state_name[nstate], sc->flags));
@@ -1216,6 +1202,7 @@ iwi_setcurchan(struct iwi_softc *sc, int chan)
 {
 	struct ieee80211com *ic = &sc->sc_ic;
 
+	IWI_LOCK_ASSERT(sc);
 	ic->ic_curchan = &ic->ic_channels[chan];
 	sc->curchan = chan;
 
@@ -1709,6 +1696,8 @@ iwi_cmd(struct iwi_softc *sc, uint8_t type, void *data, uint8_t len)
 {
 	struct iwi_cmd_desc *desc;
 
+	IWI_LOCK_ASSERT(sc);
+
 	if (sc->flags & IWI_FLAG_BUSY) {
 		device_printf(sc->sc_dev, "%s: cmd %d not sent, busy\n",
 			__func__, type);
@@ -1771,6 +1760,7 @@ iwi_tx_start(struct ifnet *ifp, struct mbuf *m0, struct ieee80211_node *ni,
 	int error, nsegs, hdrlen, i;
 	int ismcast, flags, xflags, staid;
 
+	IWI_LOCK_ASSERT(sc);
 	wh = mtod(m0, const struct ieee80211_frame *);
 	/* NB: only data frames use this path */
 	hdrlen = ieee80211_hdrsize(wh);
@@ -2080,11 +2070,20 @@ iwi_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 				 */
 				sc->sc_rfkill_timer = 0;
 			}
-			iwi_put_firmware(sc);
 		}
 		break;
 
 	default:
+		/*
+		 * XXX the driver has a tendency to freeze the machine
+		 * when initializing the interface. This seems due to
+		 * a race condition, whose origin is still unclear.
+		 * Adding a printf in this particular condition seems to cure
+		 * the symptom, so we do it until we find a proper fix.
+		 */
+		if (sc->flags & IWI_FLAG_BUSY)
+			device_printf(sc->sc_dev, "%s: flags %x cmd 0x%lx\n",
+				__func__, sc->flags, cmd);
 		error = ieee80211_ioctl(ic, cmd, data);
 	}
 
@@ -2107,6 +2106,8 @@ iwi_stop_master(struct iwi_softc *sc)
 	uint32_t tmp;
 	int ntries;
 
+	IWI_LOCK_ASSERT(sc);
+
 	/* disable interrupts */
 	CSR_WRITE_4(sc, IWI_CSR_INTR_MASK, 0);
 
@@ -2223,6 +2224,7 @@ iwi_getfw(struct iwi_fw *fw, const char *fwname,
  * This is necessary because we re-init the device sometimes
  * from a context where we cannot read from the filesystem
  * (e.g. from the taskqueue thread when rfkill is re-enabled).
+ * XXX return 0 on success, 1 on error.
  *
  * NB: the order of get'ing and put'ing images here is
  * intentional to support handling firmware images bundled
@@ -2306,33 +2308,38 @@ iwi_get_firmware(struct iwi_softc *sc)
 		/*
 		 * Check and setup combined image.
 		 */
-		if (fp->datasize < sizeof(hdr)) {
+		if (fp->datasize < sizeof(struct iwi_firmware_hdr)) {
 			device_printf(sc->sc_dev, "image '%s' too small\n",
 			    fp->name);
 			goto bad;
 		}
 		hdr = (const struct iwi_firmware_hdr *)fp->data;
-		if (fp->datasize < sizeof(*hdr) + hdr->bsize + hdr->usize + hdr->fsize) {
+		if (fp->datasize < sizeof(*hdr) + le32toh(hdr->bsize) + le32toh(hdr->usize)
+				+ le32toh(hdr->fsize)) {
 			device_printf(sc->sc_dev, "image '%s' too small (2)\n",
 			    fp->name);
 			goto bad;
 		}
 		sc->fw_boot.data = ((const char *) fp->data) + sizeof(*hdr);
-		sc->fw_boot.size = hdr->bsize;
+		sc->fw_boot.size = le32toh(hdr->bsize);
 		sc->fw_boot.name = fp->name;
 		sc->fw_uc.data = sc->fw_boot.data + sc->fw_boot.size;
-		sc->fw_uc.size = hdr->usize;
+		sc->fw_uc.size = le32toh(hdr->usize);
 		sc->fw_uc.name = fp->name;
 		sc->fw_fw.data = sc->fw_uc.data + sc->fw_uc.size;
-		sc->fw_fw.size = hdr->fsize;
+		sc->fw_fw.size = le32toh(hdr->fsize);
 		sc->fw_fw.name = fp->name;
 	}
+#if 0
+	device_printf(sc->sc_dev, "boot %d ucode %d fw %d bytes\n",
+		sc->fw_boot.size, sc->fw_uc.size, sc->fw_fw.size);
+#endif
 
 	sc->fw_mode = ic->ic_opmode;
-	return 1;
+	return 0;
 bad:
 	iwi_put_firmware(sc);
-	return 0;
+	return 1;
 }
 
 static void
@@ -2367,6 +2374,7 @@ iwi_load_ucode(struct iwi_softc *sc, const struct iwi_fw *fw)
 	size_t size = fw->size;
 	int i, ntries, error;
 
+	IWI_LOCK_ASSERT(sc);
 	error = 0;
 	CSR_WRITE_4(sc, IWI_CSR_RST, CSR_READ_4(sc, IWI_CSR_RST) |
 	    IWI_RST_STOP_MASTER);
@@ -2439,6 +2447,7 @@ iwi_load_firmware(struct iwi_softc *sc, const struct iwi_fw *fw)
 	uint32_t sentinel, ctl, src, dst, sum, len, mlen, tmp;
 	int ntries, error;
 
+	IWI_LOCK_ASSERT(sc);
 	/* copy firmware image to DMA memory */
 	memcpy(sc->fw_virtaddr, fw->data, fw->size);
 
@@ -2498,12 +2507,13 @@ iwi_load_firmware(struct iwi_softc *sc, const struct iwi_fw *fw)
 			break;
 		DELAY(100);
 	}
+	/* sync dma, just in case */
+	bus_dmamap_sync(sc->fw_dmat, sc->fw_map, BUS_DMASYNC_POSTWRITE);
 	if (ntries == 400) {
 		device_printf(sc->sc_dev,
 		    "timeout processing command blocks for %s firmware\n",
 		    fw->name);
-		error = EIO;
-		goto fail5;
+		return EIO;
 	}
 
 	/* we're done with command blocks processing */
@@ -2524,7 +2534,6 @@ iwi_load_firmware(struct iwi_softc *sc, const struct iwi_fw *fw)
 		    "initialization to complete\n", fw->name);
 	}
 
-fail5:
 	return error;
 }
 
@@ -2580,6 +2589,7 @@ iwi_config(struct iwi_softc *sc)
 	struct iwi_txpower power;
 	uint32_t data;
 	int error, i;
+	IWI_LOCK_ASSERT(sc);
 
 	IEEE80211_ADDR_COPY(ic->ic_myaddr, IF_LLADDR(ifp));
 	DPRINTF(("Setting MAC address to %6D\n", ic->ic_myaddr, ":"));
@@ -2707,6 +2717,8 @@ iwi_scan(struct iwi_softc *sc)
 	struct iwi_scan_ext scan;
 	int i, ix, start, scan_type, error;
 
+	IWI_LOCK_ASSERT(sc);
+
 	memset(&scan, 0, sizeof scan);
 
 	/* XXX different dwell times for different scan types */
@@ -2908,7 +2920,8 @@ iwi_auth_and_assoc(struct iwi_softc *sc)
 	struct iwi_rateset rs;
 	uint16_t capinfo;
 	int error;
-
+ 
+	IWI_LOCK_ASSERT(sc);
 	if (IEEE80211_IS_CHAN_2GHZ(ni->ni_chan)) {
 		memset(&config, 0, sizeof config);
 		config.bluetooth_coexistence = sc->bluetooth;
@@ -3077,6 +3090,66 @@ iwi_init(void *priv)
 	IWI_UNLOCK(sc);
 }
 
+/*
+ * release dma resources for the firmware
+ */
+static void
+iwi_release_fw_dma(struct iwi_softc *sc)
+{
+	if (sc->fw_flags & IWI_FW_HAVE_PHY)
+		bus_dmamap_unload(sc->fw_dmat, sc->fw_map);
+	if (sc->fw_flags & IWI_FW_HAVE_MAP)
+		bus_dmamem_free(sc->fw_dmat, sc->fw_virtaddr, sc->fw_map);
+	if (sc->fw_flags & IWI_FW_HAVE_DMAT)
+		bus_dma_tag_destroy(sc->fw_dmat);
+
+	sc->fw_flags = 0;
+	sc->fw_dma_size = 0;
+	sc->fw_dmat = NULL;
+	sc->fw_map = NULL;
+	sc->fw_physaddr = 0;
+	sc->fw_virtaddr = NULL;
+}
+
+/*
+ * allocate the dma descriptor for the firmware.
+ * Return 0 on success, 1 on error.
+ * Must be called unlocked, protected by IWI_FLAG_FW_LOADING.
+ */
+static int
+iwi_init_fw_dma(struct iwi_softc *sc, int size)
+{
+	if (sc->fw_dma_size > size)
+		return 0;
+	if (bus_dma_tag_create(bus_get_dma_tag(sc->sc_dev), 4, 0,
+	    BUS_SPACE_MAXADDR_32BIT, BUS_SPACE_MAXADDR, NULL, NULL,
+	    size, 1, size, 0, NULL, NULL, &sc->fw_dmat) != 0) {
+		device_printf(sc->sc_dev,
+		    "could not create firmware DMA tag\n");
+		goto error;
+	}
+	sc->fw_flags |= IWI_FW_HAVE_DMAT;
+	if (bus_dmamem_alloc(sc->fw_dmat, &sc->fw_virtaddr, 0,
+	    &sc->fw_map) != 0) {
+		device_printf(sc->sc_dev,
+		    "could not allocate firmware DMA memory\n");
+		goto error;
+	}
+	sc->fw_flags |= IWI_FW_HAVE_MAP;
+	if (bus_dmamap_load(sc->fw_dmat, sc->fw_map, sc->fw_virtaddr,
+	    size, iwi_dma_map_addr, &sc->fw_physaddr, 0) != 0) {
+		device_printf(sc->sc_dev, "could not load firmware DMA map\n");
+		goto error;
+	}
+	sc->fw_flags |= IWI_FW_HAVE_PHY;
+	sc->fw_dma_size = size;
+	return 0;
+
+error:
+	iwi_release_fw_dma(sc);
+	return 1;
+}
+
 static void
 iwi_init_locked(void *priv, int force)
 {
@@ -3087,8 +3160,11 @@ iwi_init_locked(void *priv, int force)
 	int i;
 	IWI_LOCK_DECL;
 
-	if (sc->flags & IWI_FLAG_FW_LOADING)
+	IWI_LOCK_ASSERT(sc);
+	if (sc->flags & IWI_FLAG_FW_LOADING) {
+		device_printf(sc->sc_dev, "%s: already loading\n", __func__);
 		return;		/* XXX: condvar? */
+	}
 
 	iwi_stop(sc);
 
@@ -3100,53 +3176,34 @@ iwi_init_locked(void *priv, int force)
 	sc->flags |= IWI_FLAG_FW_LOADING;
 
 	IWI_UNLOCK(sc);
-	if (!iwi_get_firmware(sc)) {
+	if (iwi_get_firmware(sc)) {
 		IWI_LOCK(sc);
 		goto fail;
 	}
 
 	/* allocate DMA memory for mapping firmware image */
-	if (sc->fw_boot.size > sc->fw_dma_size)
-		sc->fw_dma_size = sc->fw_boot.size;
-	if (sc->fw_fw.size > sc->fw_dma_size)
-		sc->fw_dma_size = sc->fw_fw.size;
-	if (sc->fw_uc.size > sc->fw_dma_size)
-		sc->fw_dma_size = sc->fw_uc.size;
-
-	if (bus_dma_tag_create(bus_get_dma_tag(sc->sc_dev), 4, 0, 
-	    BUS_SPACE_MAXADDR_32BIT, BUS_SPACE_MAXADDR, NULL, NULL, 
-	    sc->fw_dma_size, 1, sc->fw_dma_size, 0, NULL, NULL, 
-	    &sc->fw_dmat) != 0) {
-		device_printf(sc->sc_dev,
-		    "could not create firmware DMA tag\n");
+	i = sc->fw_fw.size;
+	if (sc->fw_boot.size > i)
+		i = sc->fw_boot.size;
+	/* XXX do we dma the ucode as well ? */
+	if (sc->fw_uc.size > i)
+		i = sc->fw_uc.size;
+	if (iwi_init_fw_dma(sc, i)) {
 		IWI_LOCK(sc);
 		goto fail;
 	}
-	if (bus_dmamem_alloc(sc->fw_dmat, &sc->fw_virtaddr, 0,
-	    &sc->fw_map) != 0) {
-		device_printf(sc->sc_dev,
-		    "could not allocate firmware DMA memory\n");
-		IWI_LOCK(sc);
-		goto fail2;
-	}
-	if (bus_dmamap_load(sc->fw_dmat, sc->fw_map, sc->fw_virtaddr,
-	    sc->fw_dma_size, iwi_dma_map_addr, &sc->fw_physaddr, 0) != 0) {
-		device_printf(sc->sc_dev, "could not load firmware DMA map\n");
-		IWI_LOCK(sc);
-		goto fail3;
-	}
 	IWI_LOCK(sc);
 
 	if (iwi_load_firmware(sc, &sc->fw_boot) != 0) {
 		device_printf(sc->sc_dev,
 		    "could not load boot firmware %s\n", sc->fw_boot.name);
-		goto fail4;
+		goto fail;
 	}
 
 	if (iwi_load_ucode(sc, &sc->fw_uc) != 0) {
 		device_printf(sc->sc_dev,
 		    "could not load microcode %s\n", sc->fw_uc.name);
-		goto fail4;
+		goto fail;
 	}
 
 	iwi_stop_master(sc);
@@ -3181,15 +3238,10 @@ iwi_init_locked(void *priv, int force)
 	if (iwi_load_firmware(sc, &sc->fw_fw) != 0) {
 		device_printf(sc->sc_dev,
 		    "could not load main firmware %s\n", sc->fw_fw.name);
-		goto fail4;
+		goto fail;
 	}
 	sc->flags |= IWI_FLAG_FW_INITED;
 
-	bus_dmamap_sync(sc->fw_dmat, sc->fw_map, BUS_DMASYNC_POSTWRITE);
-	bus_dmamap_unload(sc->fw_dmat, sc->fw_map);
-	bus_dmamem_free(sc->fw_dmat, sc->fw_virtaddr, sc->fw_map);
-	bus_dma_tag_destroy(sc->fw_dmat);
-
 	if (iwi_config(sc) != 0) {
 		device_printf(sc->sc_dev, "device configuration failed\n");
 		goto fail;
@@ -3213,10 +3265,6 @@ iwi_init_locked(void *priv, int force)
 	sc->flags &= ~IWI_FLAG_FW_LOADING;
 	return;
 
-fail4:	bus_dmamap_sync(sc->fw_dmat, sc->fw_map, BUS_DMASYNC_POSTWRITE);
-	bus_dmamap_unload(sc->fw_dmat, sc->fw_map);
-fail3:	bus_dmamem_free(sc->fw_dmat, sc->fw_virtaddr, sc->fw_map);
-fail2:	bus_dma_tag_destroy(sc->fw_dmat);
 fail:	ifp->if_flags &= ~IFF_UP;
 	sc->flags &= ~IWI_FLAG_FW_LOADING;
 	iwi_stop(sc);
@@ -3230,6 +3278,7 @@ iwi_stop(void *priv)
 	struct ieee80211com *ic = &sc->sc_ic;
 	struct ifnet *ifp = ic->ic_ifp;
 
+	IWI_LOCK_ASSERT(sc);	/* XXX: pretty sure this triggers */
 	if (sc->sc_softled) {
 		callout_stop(&sc->sc_ledtimer);
 		sc->sc_blinking = 0;
diff --git a/sys/dev/iwi/if_iwivar.h b/sys/dev/iwi/if_iwivar.h
index 6b5571e..ce292a9 100644
--- a/sys/dev/iwi/if_iwivar.h
+++ b/sys/dev/iwi/if_iwivar.h
@@ -149,7 +149,22 @@ struct iwi_softc {
 	int			mem_rid;
 	int			irq_rid;
 
+	/*
+	 * The card needs external firmware images to work, which is made of a
+	 * bootloader, microcode and firmware proper. In version 3.00 and
+	 * above, all pieces are contained in a single image, preceded by a
+	 * struct iwi_firmware_hdr indicating the size of the 3 pieces.
+	 * Old firmware < 3.0 has separate boot and ucode, so we need to
+	 * load all of them explicitly.
+	 * To avoid issues related to fragmentation, we keep the block of
+	 * dma-ble memory around until detach time, and reallocate it when
+	 * it becomes too small. fw_dma_size is the size currently allocated.
+	 */
 	int			fw_dma_size;
+	uint32_t		fw_flags;	/* allocation status */
+#define	IWI_FW_HAVE_DMAT	0x01
+#define	IWI_FW_HAVE_MAP		0x02
+#define	IWI_FW_HAVE_PHY		0x04
 	bus_dma_tag_t		fw_dmat;
 	bus_dmamap_t		fw_map;
 	bus_addr_t		fw_physaddr;
@@ -216,6 +231,11 @@ struct iwi_softc {
  *	and must be kept in sync.
  */
 #define	IWI_LOCK_DECL	int	__waslocked = 0
+//#define	IWI_LOCK_ASSERT(sc)	mtx_assert(&(sc)->sc_mtx, MA_OWNED)
+#define IWI_LOCK_ASSERT(sc)	do {				\
+	if (!mtx_owned(&(sc)->sc_mtx))	\
+		printf("%s iwi_lock not held\n", __func__);		\
+} while (0)
 #define IWI_LOCK(sc)	do {				\
 	if (!(__waslocked = mtx_owned(&(sc)->sc_mtx)))	\
 		mtx_lock(&(sc)->sc_mtx);		\