[rt2x00-users] [PATCH 1/5] rt2x00: Move direct access to queue->entries to rt2x00queue.c

[Ivo Van Doorn]

On Tue, Aug 17, 2010 at 8:41 PM, Gertjan van Wingerde
<gwingerde at gmail.com> wrote:
> On 08/14/10 21:01, Ivo van Doorn wrote:
>> All access to queue->entries through the Q_INDEX/Q_INDEX_DONE
>> variables must be done using spinlock protection. It is best
>> to manage this completely from rt2x00queue.c.
>>
>> For safely looping through all entries in the queue, the function
>> rt2x00queue_for_each_entry is added which will walk from Q_INDEX_DONE
>> to Q_INDEX in a safe manner.
>>
>> This also fixes rt2x00usb which walked the entries list from
>> 0 to length to kill each entry (killing entries must be done
>> from Q_INDEX_DONE to Q_INDEX to enforce TX status reporting to
>> occur in the correct order.
>>
>> Signed-off-by: IvDoorn at gmail.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800pci.c   |    4 +-
>>  drivers/net/wireless/rt2x00/rt2x00queue.c |   36 +++++++++++
>>  drivers/net/wireless/rt2x00/rt2x00queue.h |   12 ++++
>>  drivers/net/wireless/rt2x00/rt2x00usb.c   |   98 +++++++++--------------------
>>  4 files changed, 81 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
>> index af1c691..a5e5870 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> @@ -629,7 +629,7 @@ static void rt2800pci_write_tx_desc(struct queue_entry *entry,
>>  static void rt2800pci_kick_tx_queue(struct data_queue *queue)
>>  {
>>       struct rt2x00_dev *rt2x00dev = queue->rt2x00dev;
>> -     unsigned int idx = queue->index[Q_INDEX];
>> +     struct queue_entry *entry = rt2x00queue_get_entry(queue, Q_INDEX);
>>       unsigned int qidx = 0;
>>
>>       if (queue->qid == QID_MGMT)
>> @@ -637,7 +637,7 @@ static void rt2800pci_kick_tx_queue(struct data_queue *queue)
>>       else
>>               qidx = queue->qid;
>>
>> -     rt2800_register_write(rt2x00dev, TX_CTX_IDX(qidx), idx);
>> +     rt2800_register_write(rt2x00dev, TX_CTX_IDX(qidx), entry->entry_idx);
>>  }
>>
>>  static void rt2800pci_kill_tx_queue(struct data_queue *queue)
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> index 189eaf7..2486963 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> @@ -625,6 +625,42 @@ int rt2x00queue_update_beacon(struct rt2x00_dev *rt2x00dev,
>>       return 0;
>>  }
>>
>> +void rt2x00queue_for_each_entry(struct data_queue *queue,
>> +                             void (*fn)(struct queue_entry *entry))
>> +{
>> +     unsigned long irqflags;
>> +     unsigned int index;
>> +     unsigned int index_done;
>> +     unsigned int i;
>> +
>> +     /*
>> +      * Only protect the range we are going to loop over,
>> +      * if during our loop a extra entry is set to pending
>> +      * it should not be kicked during this run, since it
>> +      * is part of another TX operation.
>> +      */
>> +     spin_lock_irqsave(&queue->lock, irqflags);
>> +     index = queue->index[Q_INDEX];
>> +     index_done = queue->index[Q_INDEX_DONE];
>> +     spin_unlock_irqrestore(&queue->lock, irqflags);
>
> Maybe it is just me, but this locking looks suspicious to me.
> Why is it enough to just lock around the queue->index access, and don't
> we need to lock the entire queue?
>
> I know this is how it has been before, but I've always felt uncomfortable
> with it. Can you explain why this is sufficient?

The goal has always been to reduce the locking around the queue as
much as possible,
the reason why it is safe, is that mac80211 is feeding the frames on
one thread (or at leat
one thread per hardware queue for QoS). So the INDEX pointer is moved
from a single
location. The INDEX_DONE is controlled from the hardware, and we also
have a single
thread which is moving that.

Also if we lock around the entire queue, while we have the issue for
sleeping & non-sleeping
threads which want to obtain the lock (also within the locking some
threads might want to
sleep). So what lock do we need then? Spinlocks will fail on USB,
mutex will fail as well
since some calls from mac80211 are atomic.

Ivo