On Thursday 15 November 2007, Mattias Nissler wrote: > > On Wed, 2007-11-14 at 21:40 -0500, Will Dyson wrote: > > On Nov 14, 2007 4:13 PM, John W. Linville <linville@xxxxxxxxxxxxx> wrote: > > > > > > http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/ivd/rt2x00.git;a=commit;h=d37cabfb5f60a3bb56585a74fd3f140ba2960fe0 > > > > > > > > The patch is in the wireless-2.6/everything tree, but not Linus's tree. > > > > > > Most of the patch seems like a no-op, except this bit: > > > > > > if (is_rts_frame(frame_control) || is_cts_frame(frame_control)) { > > > __set_bit(ENTRY_TXD_BURST, &desc.flags); > > > - if (is_rts_frame(frame_control)) > > > + if (is_rts_frame(frame_control)) { > > > __set_bit(ENTRY_TXD_RTS_FRAME, &desc.flags); > > > + __set_bit(ENTRY_TXD_ACK, &desc.flags); > > > + } else > > > + __clear_bit(ENTRY_TXD_ACK, &desc.flags); > > > if (control->rts_cts_rate) > > > tx_rate = control->rts_cts_rate; > > > } > > > > > > Is this correct? I'm not sure about the actual meaning of TXD_W0_ACK > > > (which keys off ENTRY_TXD_ACK)... > > > > Adding Mattias (the patch's author), Ivo and the rt2x00 list to the CC. > > > > TXD_W0_ACK seems to mean that the firmware should expect an ack for > > the packet represented by that tx descriptor. That is how it is being > > used (and looking at the vendor driver confirms it). > > Correct. > > > > > The rest of the patch moves the logic for setting this bit (or not) to > > a central location, so that the interesting bit is not repeated in > > each chip-specific driver file. > > Not quite. Thing is that we only have one ieee80211_tx_control > structure, which we received from mac80211 for the original frame. Some > parameters, e.g. the transmission queue are valid for both the > rts/cts-to-self frame and the data frame. So we use the same control > structure when setting up both frames. Before the patch, the driver > incorrectly assumed that the IEEE80211_TXCTL_NO_ACK flag determines > whether to expect an ACK, which is simply incorrect for rts/cts frames. Perhaps we should test if clearing the ENTRY_TXD_ACK unconditionally for cts and rts frames would help. (patch located at the bottom). In fact I wonder if clearing that flag would fix the rt61pci to run txdone for all frames (opposed to skipping an occasional entry). > > Although now that I really look at the patch, I wonder why the > > IEEE80211_TXCTL_NO_ACK bit is not already set correctly for RTS and > > CTS-to-self frames. It doesn't look like any other driver does this > > kind of calculation, so perhaps the problem solved by this patch is > > also present elsewhere? > > > > That depends on how the driver/hardware generates rts/cts-to-self > frames. One way to clean this up would be to change mac80211 to generate > a new tx control structure in ieee80211_ctstoself_get and > ieee80211_rts_get for the rts/cts-to-self frame. But IMHO that's just > adding overhead. Agreed, we should use the same tx_control structure, and just check what flags apply to rts and cts frames. Ivo --- diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c index 3ab1fb9..2e3e822 100644 --- a/drivers/net/wireless/rt2x00/rt2x00dev.c +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c @@ -639,11 +639,11 @@ void rt2x00lib_write_tx_desc(struct rt2x00_dev *rt2x00dev, */ if (is_rts_frame(frame_control) || is_cts_frame(frame_control)) { __set_bit(ENTRY_TXD_BURST, &desc.flags); - if (is_rts_frame(frame_control)) { + __clear_bit(ENTRY_TXD_ACK, &desc.flags); + + if (is_rts_frame(frame_control)) __set_bit(ENTRY_TXD_RTS_FRAME, &desc.flags); - __set_bit(ENTRY_TXD_ACK, &desc.flags); - } else - __clear_bit(ENTRY_TXD_ACK, &desc.flags); + if (control->rts_cts_rate) tx_rate = control->rts_cts_rate; } - 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
