On Monday 06 March 2006 00:09, you wrote: > Hi, Michael: > > It seems to me that the today's wireless-2.6 git contains bcm43xx which > does not free txb's correctly, if I understand it right. > > Consider a situation where a txb with two skb's is sent down. > The dma_tx_fragment will save the pointer to meta->txb of the first > fragment. If fragments are freed in order, ieee80211_txb_free frees both > skb's when the first fragment is processed. This may result in reuse > of the second skb's memory.
Yes, but I think the fix is to save the txb when transmitting
the last fragment rather than the first.
> - if (cur_frag == 0) {
> - /* Save the txb pointer for freeing in xmitstatus IRQ */
> - meta->txb = txb;
> - }
> -
> /* Add a device specific TX header. */
> assert(skb_headroom(skb) >= sizeof(struct bcm43xx_txhdr));
> /* Reserve enough headroom for the device tx header. */
> @@ -809,14 +782,12 @@ int bcm43xx_dma_tx(struct bcm43xx_privat
>
> for (i = 0; i < txb->nr_frags; i++) {
> skb = txb->fragments[i];
> - /* We do not free the skb, as it is freed as
> - * part of the txb freeing.
> - */
> - mark_skb_mustfree(skb, 0);
> - dma_tx_fragment(ring, skb, txb, i);
> + txb->fragments[i] = NULL; /* Take skb from ieee80211_txb_free */
> + dma_tx_fragment(ring, skb, i);
> //TODO: handle failure of dma_tx_fragment
> }
>
> + ieee80211_txb_free(txb);
This seems even more wrong to me than the current code.
This free's all fragments while transmission is in progress.
The txb must be free'd after the xmitstatus for the last
fragment of the txb has been received. (and that is currently
indeed a bug. We free the txb on the first frag).
> return 0;
> }
>
> diff -urp -X dontdiff
> linux-2.6-w20060224/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
> linux-2.6-w20060224-wlan/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
> --- linux-2.6-w20060224/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
> 2006-02-24 18:49:28.000000000 -0800
> +++ linux-2.6-w20060224-wlan/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
> 2006-03-05 14:45:28.000000000 -0800
> @@ -115,10 +115,6 @@ struct bcm43xx_dmadesc_meta {
> struct sk_buff *skb;
> /* DMA base bus-address of the descriptor buffer. */
> dma_addr_t dmaaddr;
> - /* Pointer to our txb (can be NULL).
> - * This should be freed in completion IRQ.
> - */
> - struct ieee80211_txb *txb;
> };
We must keep the txb pointer, because it must be freed later
by the xmitstatus IRQ.
My fix for the bug would rather look like the following:
(pseudo patch)
- if (cur_frag == 0) {
+ if (cur_frag == last_frag) {
/* Save the txb pointer for freeing in xmitstatus IRQ */
meta->txb = txb;
}
--
Greetings Michael.
pgpQXUDrQG0M2.pgp
Description: PGP signature
