rt2x00 : hw support txdone implementation. (without fallback change)
PATCH 1/2 rt2x00 : hw support txdone implementation. (without fallback change)
This is an implementation that support WCID being the key_index coming from benoit without the change in the meaning of the tx fallback flag.
Ivo: in previous patch I forgot about your comment months ago against the fallback meaning change. This version thus avoid this change.
--- drivers/net/wireless/rt2x00/rt2800pci.c | 86 +++++++++++++------------------ 1 files changed, 36 insertions(+), 50 deletions(-)
drivers/net/wireless/rt2x00/rt2800pci.c +36 -50
@@ -920,76 +920,61 @@ static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) {struct data_queue *queue;struct queue_entry *entry; -struct queue_entry *entry_done; -struct queue_entry_priv_pci *entry_priv; +__le32 *txwi;struct txdone_entry_desc txdesc;u32 word;u32 reg; -u32 old_reg; -unsigned int type; -unsigned int index; -u16 mcs, real_mcs; - +int i; +int wcid, ack, pid, tx_wcid, tx_ack, tx_pid; +u16 mcs, tx_mcs; +/* -* During each loop we will compare the freshly read -* TX_STA_FIFO register value with the value read from -* the previous loop. If the 2 values are equal then -* we should stop processing because the chance it -* quite big that the device has been unplugged and -* we risk going into an endless loop. +* To avoid an endlees loop, we only read the TX_STA_FIFO register up +* to 256 times (this is enought to get all values from the FIFO). In +* normal situation, the loop is terminated when we reach a value with +* TX_STA_FIFO_VALID bit is 0. */ - old_reg =0 ; - -while (1 ) { + +for (i=0 ; i<256 ; i++) {rt2800_register_read (rt2x00dev,TX_STA_FIFO , ®);if (!rt2x00_get_field32 (reg,TX_STA_FIFO_VALID ))break ; -if (old_reg == reg) -break ; - old_reg = reg; + wcid =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ); + ack =rt2x00_get_field32 (reg,TX_STA_FIFO_TX_ACK_REQUIRED ); + pid =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE );/* * Skip this entry when it contains an invalid * queue identication number. */ - type =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE ) -1 ; -if (type >=QID_RX ) +if (pid <1 )continue ; - queue =rt2x00queue_get_queue (rt2x00dev, type); + queue =rt2x00queue_get_queue (rt2x00dev, pid -1 );if (unlikely (!queue))continue ;/* -* Skip this entry when it contains an invalid -* index number. +* Inside each queue, we process each entry in a chronological +* order. We first check that the queue is not empty. */ - index =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ) -1 ; -if (unlikely (index >= queue->limit)) +if (queue->length ==0 )continue ; + entry =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - entry = &queue->entries[index]; - entry_priv = entry->priv_data; -rt2x00_desc_read ((__le32 *)entry->skb->data,0 , &word); - - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); -while (entry != entry_done) { -/* -* Catch up. -* Just report any entries we missed as failed. -*/ -WARNING (rt2x00dev, -"TX status report missed for entry %d\n" , - entry_done->entry_idx); - - txdesc.flags =0 ; -__set_bit (TXDONE_UNKNOWN , &txdesc.flags); - txdesc.retry =0 ; - -rt2x00lib_txdone (entry_done, &txdesc); - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - } +/* Check if we got a match by looking at WCID/ACK/PID +* fields */ + txwi = (__le32 *)(entry->skb->data - + rt2x00dev->hw->extra_tx_headroom); + +rt2x00_desc_read (txwi,1 , &word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );/* * Obtain the status about this packet.
@@ -1010,10 +995,11 @@ static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev)* we have mcs = tx_mcs - 1. So the number of * retry is (tx_mcs - mcs). */ - mcs =rt2x00_get_field32 (word,TXWI_W0_MCS ); - real_mcs =rt2x00_get_field32 (reg,TX_STA_FIFO_MCS ); + mcs =rt2x00_get_field32 (reg,TX_STA_FIFO_MCS ); +rt2x00_desc_read (txwi,0 , &word); + tx_mcs =rt2x00_get_field32 (word,TXWI_W0_MCS );__set_bit (TXDONE_FALLBACK , &txdesc.flags); - txdesc.retry = mcs -min (mcs, real_mcs); + txdesc.retry = tx_mcs -min (tx_mcs, mcs);rt2x00lib_txdone (entry, &txdesc); }
Signature · 1 line
1.7.0
Quoted 9 lines · Alban Browaeys
This is an implementation that support WCID being the
key_indexcoming from benoit without the change in the meaning of the tx fallback flag.Ivo: in previous patch I forgot about your comment months ago against the fallback meaning change. This version thus avoid this change.
Signed-off-by Benoit Papillault <[email protected]>Signed-off-by Alban Browaeys <[email protected]>---
Your commit message does not seem to explain what this change is for, and why this change is being made (and the original patch submission doesn't explain this as well)
What problem with the original code is being fixed with your patch?
Please include a commit message that explains these things.
Quoted 36 lines
drivers/net/wireless/rt2x00/rt2800pci.c | 86 +++++++++++++------------------ 1 files changed, 36 insertions(+), 50 deletions(-)drivers/net/wireless/rt2x00/rt2800pci.c +5
-13@@ -920,76 +920,61 @@ static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) {struct data_queue *queue;struct queue_entry *entry; -struct queue_entry *entry_done; -struct queue_entry_priv_pci *entry_priv; +__le32 *txwi;struct txdone_entry_desc txdesc;u32 word;u32 reg; -u32 old_reg; -unsigned int type; -unsigned int index; -u16 mcs, real_mcs; - +int i; +int wcid, ack, pid, tx_wcid, tx_ack, tx_pid; +u16 mcs, tx_mcs; +/* -* During each loop we will compare the freshly read -* TX_STA_FIFO register value with the value read from -* the previous loop. If the 2 values are equal then -* we should stop processing because the chance it -* quite big that the device has been unplugged and -* we risk going into an endless loop.
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev · line 939
Quoted 10 lines
drivers/net/wireless/rt2x00/rt2800pci.c +6
-3+* To avoid an endlees loop, we only read the TX_STA_FIFO register up +* to 256 times (this is enought to get all values from the FIFO). In +* normal situation, the loop is terminated when we reach a value with +* TX_STA_FIFO_VALID bit is 0. */ - old_reg =0 ; - -while (1 ) { + +for (i=0 ; i<256 ; i++) {
Why this magical change to only try 256 times?
Quoted 61 lines
drivers/net/wireless/rt2x00/rt2800pci.c +12
-31rt2800_register_read (rt2x00dev,TX_STA_FIFO , ®);if (!rt2x00_get_field32 (reg,TX_STA_FIFO_VALID ))break ; -if (old_reg == reg) -break ; - old_reg = reg; + wcid =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ); + ack =rt2x00_get_field32 (reg,TX_STA_FIFO_TX_ACK_REQUIRED ); + pid =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE );/* * Skip this entry when it contains an invalid * queue identication number. */ - type =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE ) -1 ; -if (type >=QID_RX ) +if (pid <1 )continue ; - queue =rt2x00queue_get_queue (rt2x00dev, type); + queue =rt2x00queue_get_queue (rt2x00dev, pid -1 );if (unlikely (!queue))continue ;/* -* Skip this entry when it contains an invalid -* index number. +* Inside each queue, we process each entry in a chronological +* order. We first check that the queue is not empty. */ - index =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ) -1 ; -if (unlikely (index >= queue->limit)) +if (queue->length ==0 )continue ; + entry =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - entry = &queue->entries[index]; - entry_priv = entry->priv_data; -rt2x00_desc_read ((__le32 *)entry->skb->data,0 , &word); - - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); -while (entry != entry_done) { -/* -* Catch up. -* Just report any entries we missed as failed. -*/ -WARNING (rt2x00dev, -"TX status report missed for entry %d\n" , - entry_done->entry_idx); - - txdesc.flags =0 ; -__set_bit (TXDONE_UNKNOWN , &txdesc.flags); - txdesc.retry =0 ; - -rt2x00lib_txdone (entry_done, &txdesc); - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - } +/* Check if we got a match by looking at WCID/ACK/PID +* fields */ + txwi = (__le32 *)(entry->skb->data -
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 978
Quoted 10 lines
drivers/net/wireless/rt2x00/rt2800pci.c +9
-0+ rt2x00dev->hw->extra_tx_headroom); + +rt2x00_desc_read (txwi,1 , &word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );
You're patch seems to remove the protection we had to detect transmitted frames for which we didn't get a TX status confirmation. This had been added in the past for the other rt2x00 drivers to handle real issues with the rt2x00 devices. Why is this being removed?
Quoted 18 lines
drivers/net/wireless/rt2x00/rt2800pci.c +0
-0/* * Obtain the status about this packet.
drivers/net/wireless/rt2x00/rt2800pci.c +4
-3@@ -1010,10 +995,11 @@ static void rt2800pci_txdone(struct rt2x00_dev*rt2x00dev) * we have mcs = tx_mcs - 1. So the number of * retry is (tx_mcs - mcs). */ - mcs =rt2x00_get_field32 (word,TXWI_W0_MCS ); - real_mcs =rt2x00_get_field32 (reg,TX_STA_FIFO_MCS ); + mcs =rt2x00_get_field32 (reg,TX_STA_FIFO_MCS ); +rt2x00_desc_read (txwi,0 , &word); + tx_mcs =rt2x00_get_field32 (word,TXWI_W0_MCS );__set_bit (TXDONE_FALLBACK , &txdesc.flags); - txdesc.retry = mcs -min (mcs, real_mcs); + txdesc.retry = tx_mcs -min (tx_mcs, mcs);rt2x00lib_txdone (entry, &txdesc);}
Quoted 12 lines · Gertjan van Wingerde
Quoted 10 lines · Alban Browaeys
This is an implementation that support WCID being the
key_indexcoming from benoit without the change in the meaning of the tx fallback flag.Ivo: in previous patch I forgot about your comment months ago against the fallback meaning change. This version thus avoid this change.
Signed-off-by Benoit Papillault<[email protected]>Signed-off-by Alban Browaeys<[email protected]>---
Quoted 8 lines
Your commit message does not seem to explain what this change is for, and why this change is being made (and the original patch submission doesn't explain this as well)
What problem with the original code is being fixed with your patch?Please include a commit message that explains these things.
Doing? Sending it asap. The previous version was mine and relied on WCID behing the entry idx. But it got broken by another commit that restore WCID behing the key idx.
Thus
index = rt2x00_get_field32(reg, TX_STA_FIFO_WCID) - 1;
cannot work. As it will grab the key idx instead of the entry idx.
This implementation is Benoit Papillault one that support hw crypt and WCID behing key idx.
Quoted 40 lines
Quoted 40 lines
drivers/net/wireless/rt2x00/rt2800pci.c | 86 +++++++++++++------------------ 1 files changed, 36 insertions(+), 50 deletions(-)drivers/net/wireless/rt2x00/rt2800pci.c +9
-13@@ -920,76 +920,61 @@ static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) {struct data_queue *queue;struct queue_entry *entry; -struct queue_entry *entry_done; -struct queue_entry_priv_pci *entry_priv; +__le32 *txwi;struct txdone_entry_desc txdesc;u32 word;u32 reg; -u32 old_reg; -unsigned int type; -unsigned int index; -u16 mcs, real_mcs; - +int i; +int wcid, ack, pid, tx_wcid, tx_ack, tx_pid; +u16 mcs, tx_mcs; +/* -* During each loop we will compare the freshly read -* TX_STA_FIFO register value with the value read from -* the previous loop. If the 2 values are equal then -* we should stop processing because the chance it -* quite big that the device has been unplugged and -* we risk going into an endless loop. +* To avoid an endlees loop, we only read the TX_STA_FIFO register up +* to 256 times (this is enought to get all values from the FIFO). In +* normal situation, the loop is terminated when we reach a value with +* TX_STA_FIFO_VALID bit is 0.
Quoted 9 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 942
Quoted 7 lines
drivers/net/wireless/rt2x00/rt2800pci.c +2
-3*/ - old_reg =0 ; - -while (1 ) { + +for (i=0 ; i<256 ; i++) {Why this magical change to only try 256 times?
see comment above "to 256 times (this is enought to get all values from the FIFO)". I seems to make more sense that the previous infinite loop that relied on the comparison of old_reg and reg to detect if we were to exit.
Quoted 68 lines
Quoted 68 lines
drivers/net/wireless/rt2x00/rt2800pci.c +19
-31rt2800_register_read (rt2x00dev,TX_STA_FIFO ,®);if (!rt2x00_get_field32 (reg,TX_STA_FIFO_VALID ))break ; -if (old_reg == reg) -break ; - old_reg = reg; + wcid =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ); + ack =rt2x00_get_field32 (reg,TX_STA_FIFO_TX_ACK_REQUIRED ); + pid =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE );/* * Skip this entry when it contains an invalid * queue identication number. */ - type =rt2x00_get_field32 (reg,TX_STA_FIFO_PID_TYPE ) -1 ; -if (type>=QID_RX ) +if (pid<1 )continue ; - queue =rt2x00queue_get_queue (rt2x00dev, type); + queue =rt2x00queue_get_queue (rt2x00dev, pid -1 );if (unlikely (!queue))continue ;/* -* Skip this entry when it contains an invalid -* index number. +* Inside each queue, we process each entry in a chronological +* order. We first check that the queue is not empty. */ - index =rt2x00_get_field32 (reg,TX_STA_FIFO_WCID ) -1 ; -if (unlikely (index>= queue->limit)) +if (queue->length ==0 )continue ; + entry =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - entry =&queue->entries[index]; - entry_priv = entry->priv_data; -rt2x00_desc_read ((__le32 *)entry->skb->data,0 ,&word); - - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); -while (entry != entry_done) { -/* -* Catch up. -* Just report any entries we missed as failed. -*/ -WARNING (rt2x00dev, -"TX status report missed for entry %d\n" , - entry_done->entry_idx); - - txdesc.flags =0 ; -__set_bit (TXDONE_UNKNOWN ,&txdesc.flags); - txdesc.retry =0 ; - -rt2x00lib_txdone (entry_done,&txdesc); - entry_done =rt2x00queue_get_entry (queue,Q_INDEX_DONE ); - } +/* Check if we got a match by looking at WCID/ACK/PID +* fields */ + txwi = (__le32 *)(entry->skb->data - + rt2x00dev->hw->extra_tx_headroom); + +rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); +
Quoted 10 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 978
Quoted 4 lines
drivers/net/wireless/rt2x00/rt2800pci.c +2
-0+if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );You're patch seems to remove the protection we had to detect transmitted frames for which we didn't get a TX status confirmation. This had been added in the past for the other rt2x00 drivers to handle real issues with the rt2x00 devices. Why is this being removed?
This protection relied on the fact that we could get the current entry idx (which could only be shipped in WCID) to compare it to Q_INDEX_DONE and loop over the diff.
This cannot work with WCID behing the key idx as thus we have to rely on the fact that the current entry index is the current one (thus the diff is always void and looping over this void diff is pointless).
Best regards and thanks for the comments.
Alban
Hello!
I have tested both patches on the real hardware, and I don't see any regressions. Unfortunately, the issue with interrupts is still there, so ping stops after 11 packets, which limited my ability to test the change extensively. That said, I was able to use wpa_supplicant and dhcp to get an IP address using the patched driver in the station mode.
wpa_supplicant worked most of the time. I believe the occasional failures are due to a preexisting memory corruption issue (I reported earlier that addr3 can be corrupted in probe requests).
Unfortunately, the patches include corrupt whitespace, so they had to be applied by "patch -l". Also, there are trailing tabs in two places. That's not a big deal, but it's better avoided. Please consider using git or stgit to send patches.
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 938
Quoted 1 line
drivers/net/wireless/rt2x00/rt2800pci.c +1
-0+for (i=0 ; i<256 ; i++) {
checkpatch.pl complains about spacing. There should be spaces around "=" and "<"
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 969
Quoted 2 lines
drivers/net/wireless/rt2x00/rt2800pci.c +2
-0+ txwi = (__le32 *)(entry->skb->data - + rt2x00dev->hw->extra_tx_headroom);
I really don't see any point in introducing wrong code in one patch and fixing it in another. I would just join the patches.
When bisecting for a problem, landing at a broken commit can lead to a lot of wasted time.
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 7 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 , &word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );
Can we make this sanity check optional?
Signature · 2 lines
Regards, Pavel Roskin
Quoted 8 lines · Pavel Roskin
Hello!
I have tested both patches on the real hardware, and I don't see any regressions. Unfortunately, the issue with interrupts is still there, so ping stops after 11 packets, which limited my ability to test the change extensively. That said, I was able to use
wpa_supplicantand dhcp to get an IP address using the patched driver in the station mode.
Quoted 5 lines
wpa_supplicantworked most of the time. I believe the occasional failures are due to a preexisting memory corruption issue (I reported earlier that addr3 can be corrupted in probe requests).
Really interesting. I had access to an Access Point that leading to
such state of affait a week ago (but not for long enough to decipher the issue).
All I could tell is the rt2x00mac_config was constantly called and as this function kills
RX well I ended up with RX off all the time after a few initial pings.
Does any message comes out with mac80211 and rt2x00 debug on ?
As I cannot reproduce with both of my 3 different access points I am kind of interested
by such a setup that break.
Does http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=f82ab894fdac70954a50507921947facce8d8321 helps ? It is my next patch in the pipe . Is supposed to only take care of reseting the DMA engine and MCU one thus should only prevents messages about failure to send mcu request after boot.
But if you have such an error you could have incorrect initialization of the radio thus more issues afterwards.
Note that station mode is not properly working as master (benoit decipher we were not sending beacons).
Quoted 12 lines
Unfortunately, the patches include corrupt whitespace, so they had to be applied by "patch -l". Also, there are trailing tabs in two places. That's not a big deal, but it's better avoided. Please consider using git or stgit to send patches.
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 938
Quoted 2 lines
drivers/net/wireless/rt2x00/rt2800pci.c +1
-0+for (i=0 ; i<256 ; i++) {checkpatch.pl complains about spacing. There should be spaces around "=" and"<"
Quoted 10 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 969
Quoted 3 lines
drivers/net/wireless/rt2x00/rt2800pci.c +2
-0+ txwi = (__le32 *)(entry->skb->data - + rt2x00dev->hw->extra_tx_headroom);I really don't see any point in introducing wrong code in one patch and fixing it in another. I would just join the patches.
When bisecting for a problem, landing at a broken commit can lead to a lot of wasted time.
Changes done in just sent patch version.
Quoted 11 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 8 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );Can we make this sanity check optional?
Is this a showstopper ? Do you mean only enabling this message telling something totally unexpected happened in debug mode ? The sanity of the queue is pretty critical for operation.
Best regards
Alban
Quoted 3 lines · Alban Browaeys
Quoted 3 lines
wpa_supplicantworked most of the time. I believe the occasional failures are due to a preexisting memory corruption issue (I reported earlier that addr3 can be corrupted in probe requests).
That's what I'm referring to:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/46727
Quoted 10 lines
Really interesting. I had access to an Access Point that leading to such state of affait a week ago (but not for long enough to decipher the issue).
All I could tell is thert2x00mac_configwas constantly called and as this function kills
RX well I ended up with RX off all the time after a few initial pings. Does any message comes out with mac80211 and rt2x00 debug on ? As I cannot reproduce with both of my 3 different access points I am kind of interested
by such a setup that break.
There are no kernel messages, but ping stops working after 11 packets every time. Wireshark shows that no more pings are sent.
Quoted 3 lines
Does
http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=f82ab894fdac70954a50507921947facce8d8321
helps ?
It's already in the kernel.
This patch should help, but it was criticized and we are waiting for a better version:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/46713
Quoted 11 lines
Quoted 11 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 8 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );Can we make this sanity check optional?
Quoted 4 lines
Is this a showstopper ? Do you mean only enabling this message telling something totally
unexpected happened in debug mode ? The sanity of the queue is pretty critical for operation.
I'm not a maintainer, I'm just trying to help with the driver, so I cannot declare anything a showstopper. It was just an idea. We can make it optional later.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 11 lines · Pavel Roskin
Quoted 4 lines · Alban Browaeys
Quoted 4 lines
wpa_supplicantworked most of the time. I believe the occasional failures are due to a preexisting memory corruption issue (I reported earlier that addr3 can be corrupted in probe requests).That's what I'm referring to:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/46727
Those mcu request errors are critical. Once the mcu get stuck access to the registers returns garbage.
Quoted 19 lines
Quoted 11 lines
Really interesting. I had access to an Access Point that leading to such state of affait a week ago (but not for long enough to decipher the issue).
All I could tell is thert2x00mac_configwas constantly called and as this function kills
RX well I ended up with RX off all the time after a few initial pings. Does any message comes out with mac80211 and rt2x00 debug on ? As I cannot reproduce with both of my 3 different access points I am kind of interested
by such a setup that break.There are no kernel messages, but ping stops working after 11 packets every time. Wireshark shows that no more pings are sent.
Quoted 4 lines
Does
http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=f82ab894fdac70954a50507921947facce8d8321
helps ?
Quoted 7 lines
It's already in the kernel.
This patch should help, but it was criticized and we are waiting for a better version:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/46713
As I explain in http://article.gmane.org/gmane.linux.kernel.wireless.general/47177 DMA interrupts are not of any use to us if we want to report success/failure. So this is a dead end (in fact my first test 2 years ago was to do exactly that . Rely on DMA0, etc
interrutps). Back then due to l2pad issue packet were never reaching
tx sta fifo but always dma done interrupts. Thus nothing was
received on the other side but the box reported success to send.
And it is not already in the kernel
http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=f82ab894fdac70954a50507921947facce8d8321
Try it it exactly handles the mcu request errors and corruption and prevents following access to the registers from returning garbage. One need to reboot (once the mcu is trashed it need to be powered down).
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 9 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );Can we make this sanity check optional?
Quoted 10 lines
Quoted 5 lines
Is this a showstopper ? Do you mean only enabling this message telling something totally
unexpected happened in debug mode ? The sanity of the queue is pretty critical for operation.I'm not a maintainer, I'm just trying to help with the driver, so I cannot declare anything a showstopper. It was just an idea. We can make it optional later.
Anyway this can wait for you to have operational driver. I struggle to cleanupfor submission around 60 patches,
the hardest to maintain behing those I did not coded liek this one but
I am slowly integrating
the meaning of each of those).
If you want to take a look :
http://git.popipo.fr/?p=rt2x00.git;a=shortlog;h=refs/heads/rt3070v2-next
http://git.prahal.homelinux.net/?p=rt2x00.git;a=shortlog;h=refs/heads/rtt3070v2-next-debug
the second tree behing my debug version (thus more patches but more printk and less ready for submission upstream).
Best regards
Alban
Hi,
Overall this patch has great similarities to something which Josef (CC added) has posted earlier. The patch in question was not merged due to some issues, but he is working on an updated. You might want to synchronize your work with him. :)
Quoted 4 lines
Quoted 4 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 938
Quoted 2 lines
drivers/net/wireless/rt2x00/rt2800pci.c +1
-0+for (i=0 ; i<256 ; i++) {checkpatch.pl complains about spacing. There should be spaces around "=" and"<"
Also I prefer the:
while (!rt2x00queue_empty(queue)) {version from Josef's patch.
Quoted 11 lines
Quoted 11 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 8 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );Can we make this sanity check optional?
Quoted 4 lines
Is this a showstopper ? Do you mean only enabling this message telling something totally
unexpected happened in debug mode ? The sanity of the queue is pretty critical for operation.
I don't think this should be a showstopper, in fact how often would this error be printed? Is it regularly, like the rt61pci bug where not all TX done events were raised, or is it really a very rare case?
Ivo
Quoted 7 lines · Ivo van Doorn
Hi,
Overall this patch has great similarities to something which Josef (CC added) has posted earlier. The patch in question was not merged due to some issues, but he is working on an updated. You might want to synchronize your work with him. :)
I was just looking at this patch series, but I couldn't get it to apply at all. Alban, does your patch series depend on other patches not in wireless-testing? It may have just been the whitespaces or something, cause the context looked right, it just wouldn't apply anything. Anyway I'm happy to work together, or I can just send you what I have and I'll be the guinea pig, since this really isn't my area of expertise. Thanks,
Josef
Quoted 10 lines · Josef Bacik
Quoted 8 lines · Ivo van Doorn
Hi,
Overall this patch has great similarities to something which Josef (CC added) has posted earlier. The patch in question was not merged due to some issues, but he is working on an updated. You might want to synchronize your work with him. :)
Quoted 8 lines
I was just looking at this patch series, but I couldn't get it to apply at all. Alban, does your patch series depend on other patches not in wireless-testing? It may have just been the whitespaces or something, cause the context looked right, it just wouldn't apply anything. Anyway I'm happy to work together, or I can just send you what I have and I'll be the guinea pig, since this really isn't my area of expertise. Thanks,
Well I cannot agree on the replacement from TX_STA_FIFO by DMA DONE interrupts handlers. As the latter prevents from having any success/failure status.
The txdone was borked and could not worked since WirelessCliId was read as a queue entry index and written as an encryption key. What you noticed too though with the weird values you got.
Could you try http://article.gmane.org/gmane.linux.kernel.wireless.general/47183 probably and also "pull" from
http://git.prahal.homelinux.net/?p=rt2x00.git;a=shortlog;h=refs/heads/rtt3070v2-next-debug.
It is not a clean set of patches though I am doing my best to get it ready at least for to be sent to the ML.
The last commits adds a lot of printk (this is a debug branch) to try to decipher a weird issue with long range issues.
Best regards
Alban
Quoted 8 lines · Ivo van Doorn
Hi,
Overall this patch has great similarities to something which Josef (CC added) has posted earlier. The patch in question was not merged due to some issues, but he is working on an updated. You might want to synchronize your work with him. :)
The patch :
http://article.gmane.org/gmane.linux.kernel.wireless.general/46713
if so it has nothing in common. The patch from benoit papillault you rejected a year ago due to another part of the patch that was modifying the meaning of the fallback flag si the one I am reworking. I just removed this fallback part.
Also it relies on DMA<number> interrupt which are triggered even if the packet is rejected by the hw engine. Only tx_sta_fifo triggered tells the thing was sent in the end. So it ignores the errors from the hw, tells success always to mac layer and
remove any sanity check about the packet descriptor.
With this one there is no way to tell success, failure, fallback or else. A DMA<number> interrupt without TX_STA_FIFO to follow it means an incorrect txdesc. Uses to be padding issues. COuld also be wrong headroom calculation in write_tx_desc.
If we handle txdone in DMA<number> interrupts we are killing any chances to get status (success, failure).
The other TBTT fix is indeed a great step in the right direction (I ignored beacons issues for a long time as not top priority and as I knew few about it but it is a great fix :
http://article.gmane.org/gmane.linux.kernel.wireless.general/42949
Both of those patches were not sent to the rt2x00 ML so I missed them.
Quoted 11 lines
Quoted 6 lines
Quoted 6 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 938
Quoted 3 lines
drivers/net/wireless/rt2x00/rt2800pci.c +1
-0+for (i=0 ; i<256 ; i++) {checkpatch.pl complains about spacing. There should be spaces around "=" and"<"
Also I prefer the:
while (!rt2x00queue_empty(queue)) {version from Josef's patch.
Note that this one construct requires to use DMA<number> interrupt to call txdone.
I should not go this way knowing that it prevents any success/failure status handling.
Also I found a year ago that emptying the queue at each packet processing is killing packets.
We tag as unsuccessfull packets that have not yet been acknowledged by a tx sta fifo status (as dma<number> interrupts and adhoc tx sta fifo interrupt are not synchronized).
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/net/wireless/rt2x00/rt2800pci.c static void rt2800pci_txdone(struct rt2x00_dev *rt2x00dev) · line 977
Quoted 9 lines
drivers/net/wireless/rt2x00/rt2800pci.c +7
-0+rt2x00_desc_read (txwi,1 ,&word); + tx_wcid =rt2x00_get_field32 (word,TXWI_W1_WIRELESS_CLI_ID ); + tx_ack =rt2x00_get_field32 (word,TXWI_W1_ACK ); + tx_pid =rt2x00_get_field32 (word,TXWI_W1_PACKETID ); + +if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) +WARNING (rt2x00dev,"invalid TX_STA_FIFO content\n" );Can we make this sanity check optional?
Quoted 10 lines
Quoted 5 lines
Is this a showstopper ? Do you mean only enabling this message telling something totally
unexpected happened in debug mode ? The sanity of the queue is pretty critical for operation.I don't think this should be a showstopper, in fact how often would this error be printed? Is it regularly, like the rt61pci bug where not all TX done events were raised, or is it really a very rare case?
It should never happens except while hacking the driver and making mistake in the descriptor writing or such. Which well happens in the development process . Or for testers.
BR
Alban