On 30/06/2011, at 6:56 AM, Ted Unangst wrote:
> On Wed, 29 Jun 2011, Tom Murphy wrote:
>
>> /bsd: bnx0: Watchdog timeout occurred, resetting!
>> /bsd: splassert: assertwaitok: want -1 have 1
>> /bsd: Starting stack trace...
>> /bsd: assertwaitok() at assertwaitok+0x1c
>> /bsd: pool_get() at pool_get+0x95
>> /bsd: bnx_alloc_pkts() at bnx_alloc_pkts+0x31
>> /bsd: bnx_init_tx_chain() at bnx_init_tx_chain+0x13
>> /bsd: bnx_init() at bnx_init+0x18c
>> /bsd: bnx_watchdog() at bnx_watchdog+0x4d
>
> This driver is filled with bad juju. This changes all the waitoks to not
> ok, so they are interrupt safe. It already appears to handle the failure
> case. The rwlock is also totally unsafe and unnecessary.
the issue is that bnx_init is called from softclock when it looks like bnx
doesnt get any interrupts (so it doesnt do tx completions). i assumed bnx_init
was only called from the ioctl paths which have process context.
this diff is also unsafe because you still init the pool with the nointr
allocator, but you're trying to fix the code so bnx_alloc_pkts via bnx_init is
ok to call from interrupt context.
a simpler fix would be to have bnx_watchdog use the system workq to call
bnx_init to reset the chip.
it would also be worthwhile figuring out why this box is calling the watchdog
code on the chip.
dlg
>
> Index: if_bnx.c
> ===================================================================
> RCS file: /home/tedu/cvs/src/sys/dev/pci/if_bnx.c,v
> retrieving revision 1.95
> diff -u -r1.95 if_bnx.c
> --- if_bnx.c 22 Jun 2011 16:44:27 -0000 1.95
> +++ if_bnx.c 29 Jun 2011 20:56:09 -0000
> @@ -392,8 +392,8 @@
> void bnx_stats_update(struct bnx_softc *);
> void bnx_tick(void *);
>
> -struct rwlock bnx_tx_pool_lk = RWLOCK_INITIALIZER("bnxplinit");
> -struct pool *bnx_tx_pool = NULL;
> +int bnx_pool_inited;
> +struct pool bnx_tx_pool;
> void bnx_alloc_pkts(void *, void *);
>
>
/****************************************************************************
/
> @@ -759,6 +759,12 @@
> goto bnx_attach_fail;
> }
>
> + if (!bnx_pool_inited) {
> + pool_init(&bnx_tx_pool, sizeof(struct bnx_pkt),
> + 0, 0, 0, "bnxpkts", &pool_allocator_nointr);
> + bnx_pool_inited = 1;
> + }
> +
> mountroothook_establish(bnx_attachhook, sc);
> return;
>
> @@ -3727,13 +3733,13 @@
> int s;
>
> for (i = 0; i < 4; i++) { /* magic! */
> - pkt = pool_get(bnx_tx_pool, PR_WAITOK);
> + pkt = pool_get(&bnx_tx_pool, PR_NOWAIT);
> if (pkt == NULL)
> break;
>
> if (bus_dmamap_create(sc->bnx_dmatag,
> MCLBYTES * BNX_MAX_SEGMENTS, USABLE_TX_BD,
> - MCLBYTES, 0, BUS_DMA_WAITOK | BUS_DMA_ALLOCNOW,
> + MCLBYTES, 0, BUS_DMA_NOWAIT | BUS_DMA_ALLOCNOW,
> &pkt->pkt_dmamap) != 0)
> goto put;
>
> @@ -3760,7 +3766,7 @@
> stopping:
> bus_dmamap_destroy(sc->bnx_dmatag, pkt->pkt_dmamap);
> put:
> - pool_put(bnx_tx_pool, pkt);
> + pool_put(&bnx_tx_pool, pkt);
> }
>
>
/****************************************************************************
/
> @@ -3906,7 +3912,7 @@
> mtx_leave(&sc->tx_pkt_mtx);
>
> bus_dmamap_destroy(sc->bnx_dmatag, pkt->pkt_dmamap);
> - pool_put(bnx_tx_pool, pkt);
> + pool_put(&bnx_tx_pool, pkt);
>
> mtx_enter(&sc->tx_pkt_mtx);
> }
> @@ -4678,25 +4684,9 @@
> struct bnx_softc *sc = (struct bnx_softc *)xsc;
> struct ifnet *ifp = &sc->arpcom.ac_if;
> u_int32_t ether_mtu;
> - int txpl = 1;
> int s;
>
> DBPRINT(sc, BNX_VERBOSE_RESET, "Entering %s()\n", __FUNCTION__);
> -
> - if (rw_enter(&bnx_tx_pool_lk, RW_WRITE | RW_INTR) != 0)
> - return;
> - if (bnx_tx_pool == NULL) {
> - bnx_tx_pool = malloc(sizeof(*bnx_tx_pool), M_DEVBUF, M_WAITOK);
> - if (bnx_tx_pool != NULL) {
> - pool_init(bnx_tx_pool, sizeof(struct bnx_pkt),
> - 0, 0, 0, "bnxpkts", &pool_allocator_nointr);
> - } else
> - txpl = 0;
> - }
> - rw_exit(&bnx_tx_pool_lk);
> -
> - if (!txpl)
> - return;
>
> s = splnet();
