On 25/02/2010 22:00, Pavel Roskin wrote:
On Thu, 2010-02-25 at 20:17 +0100, 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.
This text is hard to understand without context. Please use a
description for the changes contained in the patch, not for the
circumstances around it. The same applies to the subject. A space
before ":" is unnecessary.
Fixing it.
It replaces the software only implementation by an implementation
supporting HW encryption.
So, I guess rt2800pci_txdone() would not work correctly if hardware
encryption is used? Perhaps that should be explained.
Explain that HW encryption is not working with current code ? Well it
cannot work at all . At least txdone
read WCID (WIRLESS_CLI_ID as written in rt2800pci_write_tx_desc as the
key index:
rt2x00_set_field32(&word, TXWI_W1_WIRELESS_CLI_ID,
test_bit(ENTRY_TXD_ENCRYPT, &txdesc->flags) ?
txdesc->key_idx : 0xff);
then in rt2800pci_txdone we read it as the queue entry index:
index = rt2x00_get_field32(reg, TX_STA_FIFO_WCID) - 1;
As Josef also found out this leads to a mess:
"that doesn't even work most of the time. I seperated out all of the
TX_STA_FIFO
reading stuff and either TX_STA_FIFO_VALID would be 0,
TX_STA_FIFO_TX_ACK_REQUIRED would be 0, or TX_STA_FIFO_WCID would be
254, which
is way higher than the queue limit. So basically it gives us crap
statistics."
http://article.gmane.org/gmane.linux.kernel.wireless.general/46713
It fixes the mixes of usage of WCID behing set to the key_idx and
then getting used as the entry index in the queue.
Maybe WCID could be expanded at least once? "behing" must be a typo.
WCID is TX_STA_FIFO_WCID in the code. Do you mean I should use
TX_STA_FIFO_WCID in the comment ?
Or WIRELESS_CLI_ID as to what it means (and the constant used in
write_tx_desc). Both are the same
one is used for writing it , the other for reading it. Out of knowing
which one to use in the comment I used WCID.
/*
- * 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.
Please spell check your comments.
I think using a different way for terminating the loop is a completely
separate issue not related to the hardware crypto support. It should be
explained why the old code needs to be changed.
/*
* 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;
I'm concerned that you are killing a valid check here. pid should be
between 1 and QID_RX (inclusively).
It looks like you are replacing the existing code with your code instead
of improving it with a new check. That alone could be a reason to
reject your patch.
Thank you I did not noticed that (the check was pid < 1 back when the
patch was made. This rewrite of the check is more
of a merge artifact.
http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2009-August/000210.html
- /*
- * Catch up.
- * Just report any entries we missed as failed.
- */
- WARNING(rt2x00dev,
- "TX status report missed for entry %d\n",
- entry_done->entry_idx);
I'm concerned that you are removing this check. Is this condition
impossible now or we just cannot detect it anymore?
Both.
- 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);
Maybe you could avoid renaming and redefining variables to make your
patch more readable? Alternatively, you could do the renaming first in
a separate patch. You can use interdiff to create a difference between
patches i.e. subtract the cleanups from the main patch.
Renaming will be removed.
--
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
