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.
Signed-off-by: Benoit Papillault <[email protected]>
Signed-off-by: Alban Browaeys <[email protected]>
---
drivers/net/wireless/rt2x00/rt2800pci.c | 86 +++++++++++++------------------
1 files changed, 36 insertions(+), 50 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index 7899789..46b06af 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -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);
}
--
1.7.0
On 02/25/10 05:19, Alban Browaeys wrote:
> 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.
>
> 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.
> drivers/net/wireless/rt2x00/rt2800pci.c | 86
> +++++++++++++------------------
> 1 files changed, 36 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c
> b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 7899789..46b06af 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -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++) {
Why this magical change to only try 256 times?
> 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");
>
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?
> /*
> * 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);
> }
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. :)
> >> + 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.
> >> + 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.
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
On 25/02/2010 18:48, Pavel Roskin wrote:
> 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).
>
>
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).
> 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.
>
>
>> + for (i=0; i<256; i++) {
>>
> checkpatch.pl complains about spacing. There should be spaces around
> "=" and"<"
>
>
>> + 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.
>> + 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
On 25/02/2010 21:53, Josef Bacik wrote:
> On Thu, Feb 25, 2010 at 09:46:22PM +0100, Ivo van Doorn wrote:
>
>> 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,
>
>
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
On 25/02/2010 21:21, Pavel Roskin wrote:
> On Thu, 2010-02-25 at 20:34 +0100, Alban Browaeys wrote:
>
>
>>> 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).
>>>
> 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.
>> 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.
>>
> There are no kernel messages, but ping stops working after 11 packets
> every time. Wireshark shows that no more pings are sent.
>
>
>> 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
>
>
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).
>>>> + 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.
>>
> 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
On 25/02/2010 21:46, Ivo van Doorn wrote:
> 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.
>>>> + 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).
>>>> + 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.
>>
> 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
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.
> + for (i=0; i<256; i++) {
checkpatch.pl complains about spacing. There should be spaces around
"=" and "<"
> + 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.
> + 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?
--
Regards,
Pavel Roskin
On Thu, 2010-02-25 at 20:34 +0100, Alban Browaeys wrote:
> > 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).
That's what I'm referring to:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/46727
> 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.
There are no kernel messages, but ping stops working after 11 packets
every time. Wireshark shows that no more pings are sent.
> 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
> >> + 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.
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.
--
Regards,
Pavel Roskin
On 25/02/2010 18:22, Gertjan van Wingerde wrote:
> On 02/25/10 05:19, Alban Browaeys wrote:
>
>> 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.
>>
>> 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.
>
>
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.
>> drivers/net/wireless/rt2x00/rt2800pci.c | 86
>> +++++++++++++------------------
>> 1 files changed, 36 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c
>> b/drivers/net/wireless/rt2x00/rt2800pci.c
>> index 7899789..46b06af 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> @@ -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++) {
>>
> 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.
>> 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");
>>
>>
> 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
On Thu, Feb 25, 2010 at 09:46:22PM +0100, Ivo van Doorn wrote:
> 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