[rt2x00-users] [PATCH 1/5] rt2x00: Move direct access to queue->entries to rt2x00queue.c
Gertjan van Wingerde
gwingerde at gmail.com
Tue Aug 17 18:41:21 UTC 2010
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?
> +
> + /*
> + * Start from the TX done pointer, this guarentees that we will
> + * send out all frames in the correct order.
> + */
> + if (index_done < index) {
> + for (i = index_done; i < index; i++)
> + fn(&queue->entries[i]);
> + } else {
> + for (i = index_done; i < queue->limit; i++)
> + fn(&queue->entries[i]);
> +
> + for (i = 0; i < index; i++)
> + fn(&queue->entries[i]);
> + }
> +}
> +EXPORT_SYMBOL_GPL(rt2x00queue_for_each_entry);
> +
> struct data_queue *rt2x00queue_get_queue(struct rt2x00_dev *rt2x00dev,
> const enum data_queue_qid queue)
> {
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
> index 2d3bf84..4de6596 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
> @@ -571,6 +571,18 @@ struct data_queue_desc {
> queue_loop(__entry, (__dev)->tx, queue_end(__dev))
>
> /**
> + * rt2x00queue_for_each_entry - Loop through all entries in the queue
> + * @queue: Pointer to @data_queue
> + * @fn: The function to call for each &struct queue_entry
> + *
> + * This will walk through all entries in the queue, in chronological
> + * order. This means it will start at the current Q_INDEX_DONE pointer
> + * and will walk through the queue until it reaches the Q_INDEX pointer.
> + */
> +void rt2x00queue_for_each_entry(struct data_queue *queue,
> + void (*fn)(struct queue_entry *entry));
> +
> +/**
> * rt2x00queue_empty - Check if the queue is empty.
> * @queue: Queue to check if empty.
> */
> diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c
> index 52ed3f4..17e42ea 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00usb.c
> @@ -232,88 +232,52 @@ static inline void rt2x00usb_kick_tx_entry(struct queue_entry *entry)
> struct queue_entry_priv_usb *entry_priv = entry->priv_data;
> u32 length;
>
> - if (test_and_clear_bit(ENTRY_DATA_PENDING, &entry->flags)) {
> - /*
> - * USB devices cannot blindly pass the skb->len as the
> - * length of the data to usb_fill_bulk_urb. Pass the skb
> - * to the driver to determine what the length should be.
> - */
> - length = rt2x00dev->ops->lib->get_tx_data_len(entry);
> + if (!test_and_clear_bit(ENTRY_DATA_PENDING, &entry->flags))
> + return;
> +
> + /*
> + * USB devices cannot blindly pass the skb->len as the
> + * length of the data to usb_fill_bulk_urb. Pass the skb
> + * to the driver to determine what the length should be.
> + */
> + length = rt2x00dev->ops->lib->get_tx_data_len(entry);
>
> - usb_fill_bulk_urb(entry_priv->urb, usb_dev,
> - usb_sndbulkpipe(usb_dev, entry->queue->usb_endpoint),
> - entry->skb->data, length,
> - rt2x00usb_interrupt_txdone, entry);
> + usb_fill_bulk_urb(entry_priv->urb, usb_dev,
> + usb_sndbulkpipe(usb_dev, entry->queue->usb_endpoint),
> + entry->skb->data, length,
> + rt2x00usb_interrupt_txdone, entry);
>
> - usb_submit_urb(entry_priv->urb, GFP_ATOMIC);
> - }
> + usb_submit_urb(entry_priv->urb, GFP_ATOMIC);
> }
>
> void rt2x00usb_kick_tx_queue(struct data_queue *queue)
> {
> - 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);
> -
> - /*
> - * Start from the TX done pointer, this guarentees that we will
> - * send out all frames in the correct order.
> - */
> - if (index_done < index) {
> - for (i = index_done; i < index; i++)
> - rt2x00usb_kick_tx_entry(&queue->entries[i]);
> - } else {
> - for (i = index_done; i < queue->limit; i++)
> - rt2x00usb_kick_tx_entry(&queue->entries[i]);
> -
> - for (i = 0; i < index; i++)
> - rt2x00usb_kick_tx_entry(&queue->entries[i]);
> - }
> + rt2x00queue_for_each_entry(queue, rt2x00usb_kick_tx_entry);
> }
> EXPORT_SYMBOL_GPL(rt2x00usb_kick_tx_queue);
>
> -void rt2x00usb_kill_tx_queue(struct data_queue *queue)
> +static inline void rt2x00usb_kill_tx_entry(struct queue_entry *entry)
> {
> - struct queue_entry_priv_usb *entry_priv;
> - struct queue_entry_priv_usb_bcn *bcn_priv;
> - unsigned int i;
> - bool kill_guard;
> + struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
> + struct queue_entry_priv_usb *entry_priv = entry->priv_data;
> + struct queue_entry_priv_usb_bcn *bcn_priv = entry->priv_data;
>
> - /*
> - * When killing the beacon queue, we must also kill
> - * the beacon guard byte.
> - */
> - kill_guard =
> - (queue->qid == QID_BEACON) &&
> - (test_bit(DRIVER_REQUIRE_BEACON_GUARD, &queue->rt2x00dev->flags));
> + if (!test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
> + return;
> +
> + usb_kill_urb(entry_priv->urb);
>
> /*
> - * Cancel all entries.
> + * Kill guardian urb (if required by driver).
> */
> - for (i = 0; i < queue->limit; i++) {
> - entry_priv = queue->entries[i].priv_data;
> - usb_kill_urb(entry_priv->urb);
> + if ((entry->queue->qid == QID_BEACON) &&
> + (test_bit(DRIVER_REQUIRE_BEACON_GUARD, &rt2x00dev->flags)))
> + usb_kill_urb(bcn_priv->guardian_urb);
> +}
>
> - /*
> - * Kill guardian urb (if required by driver).
> - */
> - if (kill_guard) {
> - bcn_priv = queue->entries[i].priv_data;
> - usb_kill_urb(bcn_priv->guardian_urb);
> - }
> - }
> +void rt2x00usb_kill_tx_queue(struct data_queue *queue)
> +{
> + rt2x00queue_for_each_entry(queue, rt2x00usb_kill_tx_entry);
> }
> EXPORT_SYMBOL_GPL(rt2x00usb_kill_tx_queue);
>
More information about the users
mailing list