[rt2x00-users] [PATCH 3/5] rt2x00: Cleanup rt2x00usb_watchdog_reset_tx

[Ivo Van Doorn]

On Tue, Aug 17, 2010 at 9:38 AM, Helmut Schaa
<helmut.schaa at googlemail.com> wrote:
> Am Saturday 14 August 2010 schrieb Ivo van Doorn:
>> rt2x00usb_watchdog_reset_tx performs the same task
>> as rt2x00usb_kill_tx_queue, with the only difference
>> is that it waits for all entries to be returned to
>> the driver and for all frames the status has been
>> reported to mac80211.
>>
>> We can easily split this task by calling rt2x00usb_kill_tx_queue,
>> sleep for a short period and invoke the TX status reporting
>> function.
>>
>> Additionally rather then calling rt2x00usb_work_txdone() for
>> status reporting we let the driver perform the TX status reporting
>> first. If this is not sufficient then rt2x00usb_work_txdone() will
>> still be used to cleanup the mess.
>>
>> Signed-off-by: Ivo van Doorn <IvDoorn at gmail.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2x00usb.c |   48 +++++++++++++++++-------------
>>  1 files changed, 27 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c
>> index 17e42ea..84365c4 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00usb.c
>> @@ -283,11 +283,10 @@ EXPORT_SYMBOL_GPL(rt2x00usb_kill_tx_queue);
>>
>>  static void rt2x00usb_watchdog_reset_tx(struct data_queue *queue)
>>  {
>> -     struct queue_entry *entry;
>> -     struct queue_entry_priv_usb *entry_priv;
>> +     struct rt2x00_dev *rt2x00dev = queue->rt2x00dev;
>>       unsigned short threshold = queue->threshold;
>>
>> -     WARNING(queue->rt2x00dev, "TX queue %d timed out, invoke reset", queue->qid);
>> +     WARNING(rt2x00dev, "TX queue %d timed out, invoke reset", queue->qid);
>>
>>       /*
>>        * Temporarily disable the TX queue, this will force mac80211
>> @@ -297,28 +296,35 @@ static void rt2x00usb_watchdog_reset_tx(struct data_queue *queue)
>>        * queue from being enabled during the txdone handler.
>>        */
>>       queue->threshold = queue->limit;
>> -     ieee80211_stop_queue(queue->rt2x00dev->hw, queue->qid);
>> +     ieee80211_stop_queue(rt2x00dev->hw, queue->qid);
>>
>>       /*
>> -      * Reset all currently uploaded TX frames.
>> +      * Kill all entries in the queue, afterwards we need to
>> +      * wait a bit for all URBs to be cancelled.
>>        */
>> -     while (!rt2x00queue_empty(queue)) {
>> -             entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
>> -             entry_priv = entry->priv_data;
>> -             usb_kill_urb(entry_priv->urb);
>> +     rt2x00usb_kill_tx_queue(queue);
>> +     msleep(1);
>
> This looks a bit fishy. We cant be sure that the urbs are really canceled
> after the msleep. Cant we add a parameter "sync" to rt2x00usb_kill_tx_queue
> that waits for each urb to complete?

Good point, I wouldn't do it with sync argument though. I would just always
wait for the URB to be returned. Sounds much safer anyway. :)

Ivo