Re: [PATCH 1/2] rt2x00 : hw support txdone implementation. (without fallback change)

[Alban Browaeys <prahal@yahoo.com>]

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