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. > 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. > 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. > /* > - * 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. > - /* > - * 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? > - 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. -- Regards, Pavel Roskin -- 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
