[Madwifi-devel] ath_rx_tasklet/ath_rxbuf_init possible potential small bug
Status: Beta
Brought to you by:
otaku
From: David B. <dbe...@ea...> - 2004-10-28 18:22:33
|
I think I just noticed a small bug in the madwifi rx path. I grabbed the latest CVS code to make sure it is still the same. Here is the situation: In ath_rx_tasklet, the main loop attempts to read as many completed rx packets as possible. For each one, it grabs the skb from the rx descriptor, decides what to do with it, and then puts another skb into the rx descriptor. To allocate another skb the ath_rxbuf_init() call is used at the bottom of the do/while loop. The problem is that if the ath_rxbuf_init() call fails, the loop stops, leaving the rx descriptor without an skb to receive packets into. This doesn't cause a crash. Instead what happens is that every time this rx descriptor is checked, if there isn't an skb attached, it is skipped by the ath_rx_tasklet. That can be seen in as the if() statement with a continue at the top of the loop. I expect the hardware skips it simply because there isn't any buffer attached to the descriptor. Essentially this becomes a lost buffer descriptor. The problem will only show up if the dev_alloc_skb() call, in ath_rxbuf_init fails, which probably doesn't happen except in extreme low memory conditions. Another odd behavior here is that whenever dev_alloc_skb fails, the ath_rx_tasklet do/while loop will exit even if there are more packets to receive. If I'm reading the code right, the solution is fairly simple. Create a rx descriptor freelist. Whenever a skb alloc fails, put the descriptor on the freelist, instead of back on the rxbuf list. Attempt to allocate new skb's for the freelist at another time. Perhaps during tx completion handling, and at the top of the ath_rx_tasklet. Would use a spinlock to keep things safe. If I'm reading this right, the only time this problem would show up is if there were packets being received while the system was under a heavy enough load that there was no more memory available for the skb's. Since I expect that there would be other weird behavior at that point, this probably isn't much of a bug, but thought I'd mention it anyways. David Beberman > |