On 10/12/2010 11:43 AM, Ben Greear wrote:
On 10/12/2010 11:40 AM, Luis R. Rodriguez wrote:
On Tue, Oct 12, 2010 at 11:35 AM, Ben Greear<greearb@xxxxxxxxxxxxxxx>
wrote:
This code looks weird to me. One of the paprd branches
deletes the skb, the other doesn't appear to. Neither
null out bf->bf_mpdu, which would appear to leave a dangling
pointer in at least the dev_kfree_skb_any() branch.
ath_tx_complete frees it's skb in all cases, so another
bf->bf_mpdu dangling pointer issue.
Maybe at the least we should null out bf->bf_mpdu when
skb is consumed?
You're reading my mind, that was what I was going to test today. Still
doing e-mail sweep though.
I'm trying to put together a patch now..but the paprd branch makes
no sense at all.
Shouldn't it also free the skb in the complete(&sc->paprd_complete) branch?
I don't see anything that uses the skb, and nothing that frees it.
if (bf->bf_state.bfs_paprd) {
if (time_after(jiffies,
bf->bf_state.bfs_paprd_timestamp +
msecs_to_jiffies(ATH_PAPRD_TIMEOUT)))
dev_kfree_skb_any(skb);
else
complete(&sc->paprd_complete);
I tried this patch, but the memory poision warnings continue.
Might still be a useful patch though...
[greearb@build-32 mac80211]$ git diff
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 8656491..f43a004 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -398,7 +398,7 @@ void ath_paprd_calibrate(struct work_struct *work)
"Timeout waiting for paprd training on "
"TX chain %d\n",
chain);
- goto fail_paprd;
+ break;
}
if (!ar9003_paprd_is_done(ah))
@@ -416,7 +416,6 @@ void ath_paprd_calibrate(struct work_struct *work)
ath_paprd_activate(sc);
}
-fail_paprd:
ath9k_ps_restore(sc);
}
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 942be55..d39b4b5 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1919,17 +1919,17 @@ static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
dma_unmap_single(sc->dev, bf->bf_dmacontext, skb->len, DMA_TO_DEVICE);
if (bf->bf_state.bfs_paprd) {
- if (time_after(jiffies,
- bf->bf_state.bfs_paprd_timestamp +
- msecs_to_jiffies(ATH_PAPRD_TIMEOUT)))
- dev_kfree_skb_any(skb);
- else
- complete(&sc->paprd_complete);
+ /* ath_paprd_calibrate owns the skb. */
+ complete(&sc->paprd_complete);
} else {
- ath_tx_complete(sc, skb, bf->aphy, tx_flags);
+ /* stat_tx must be called first, it references skb. */
ath_debug_stat_tx(sc, txq, bf, ts);
+ ath_tx_complete(sc, skb, bf->aphy, tx_flags);
}
+ /* At this point, skb is consumed one way or another */
+ bf->bf_mpdu = NULL;
+
/*
* Return the list of ath_buf of this mpdu to free queue
*/
Thanks,
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
