On Tue, 2012-12-11 at 00:18 +0200, Arik Nemtsov wrote:

On Mon, Dec 10, 2012 at 11:50 PM, Luciano Coelho [off-list ref] wrote:

quoted

quoted

we can't use normal since we need to assign the exact same HW queue
base to existing interfaces (in case of resume from suspend).
Otherwise we'll have a problem, as mac80211 doesn't flush internal
packets during suspend. So if we assign a new HW queues
to an existing vif we risk stopping/starting the wrong vifs using the
ieee80211_queue_xxx APIs.

Isn't the only difference of using _NORMAL that mac80211 won't call the
iterator function when the driver already knows about the interface?

The other way around - the iterator won't be called for interfaces not
yet added to the driver that already exist in mac80211.

Right, I see it in the code.

Using the iterator as is, we gain information on which interfaces
mac80211 knows, regardless of the lower driver. So if the current
interface already exists, it means this is recovery/resume.

Ok.

Actually Johannes came up with this little bit of black magic.

Macumba! :)

quoted

quoted

quoted

quoted

+     if (q_base >= WLCORE_NUM_MAC_ADDRESSES)
+             return -EBUSY;
+
+     wlvif->hw_queue_base = q_base * NUM_TX_QUEUES;
+     wl1271_debug(DEBUG_MAC80211, "allocating hw queue base: %d",
+                  wlvif->hw_queue_base);
+
+     for (i = 0; i < NUM_TX_QUEUES; i++) {
+             wl->queue_stop_reasons[wlvif->hw_queue_base + i] = 0;
+             /* register hw queues in mac80211 */
+             vif->hw_queue[i] = wlvif->hw_queue_base + i;
+     }
+
+adjust_cab_queue:
+     /* the last places are reserved for cab queues per interface */
+     if (wlvif->bss_type == BSS_TYPE_AP_BSS)
+             vif->cab_queue = NUM_TX_QUEUES * WLCORE_NUM_MAC_ADDRESSES +
+                              wlvif->hw_queue_base / NUM_TX_QUEUES;
+     else
+             vif->cab_queue = IEEE80211_INVAL_HW_QUEUE;

Why not keep the reservation per-vif in the same order (ie. the cab
queue comes together with the other queues for that vif)? It doesn't
matter that you don't use the cab_queue for the non AP vifs, since
you're allocating them anyway.

Keeping them together would make this code clearer.  Just make a macro
NUM_TX_QUEUES_PER_VIF that includes the space for cab_queue.

I did it on purpose - that way the functions wlcore_wake_queues and
friends can loop on the "real" vifs and stop/start them.

I don't see the difference.  They can still do it all the same.

It's a matter of convenience. It was easier for me to picture the real
HW queues as contiguous and throw the fake ones off to the side.
The off-channel HW queue is also fake (but global) and gets treated the same..

Okay, that's fine.  It is easier for me to picture the queues (fake or
not) contiguous per-vif. :) The off-channel queue is special because it
is global.  Anyway, no need to argue, both views are fine and directed
at the same point, so let's keep it as it is. ;)

quoted

quoted

quoted

quoted

diff --git a/drivers/net/wireless/ti/wlcore/tx.c b/drivers/net/wireless/ti/wlcore/tx.c
index d799fc9..dbc1721 100644
--- a/drivers/net/wireless/ti/wlcore/tx.c
+++ b/drivers/net/wireless/ti/wlcore/tx.c

[...]

quoted

@@ -1189,44 +1192,45 @@ u32 wl1271_tx_min_rate_get(struct wl1271 *wl, u32 rate_set)
 }
 EXPORT_SYMBOL_GPL(wl1271_tx_min_rate_get);

-void wlcore_stop_queue_locked(struct wl1271 *wl, u8 queue,
-                           enum wlcore_queue_stop_reason reason)
+void wlcore_stop_queue_locked(struct wl1271 *wl, struct wl12xx_vif *wlvif,
+                           u8 queue, enum wlcore_queue_stop_reason reason)
 {
-     bool stopped = !!wl->queue_stop_reasons[queue];
+     int hwq = wlvif->hw_queue_base + wl1271_tx_get_mac80211_queue(queue);

It would look nicer if we moved the "wlvif->hw_queue_base + " to the
wl1271_tx_get_mac80211_queue() function.  After all, the function call
without adding the hw_queue_base doesn't mean anything anymore.

well a rename is necessary as well then, since this is not a simple
translation between mac8021 and wlcore jargon for queues..

I don't think we need to rename.  I think wl1271_tx_get_mac80211_queue()
suits very well to include the hw_queue_base.  It would be a more
complete translation, actually.

I won't press the issue :)

:)

quoted

quoted

quoted

quoted

@@ -1235,49 +1239,62 @@ out:
 void wlcore_stop_queues(struct wl1271 *wl,
                      enum wlcore_queue_stop_reason reason)
 {
-     int i;
+     int i, q_base;
+     unsigned long flags;

-     for (i = 0; i < NUM_TX_QUEUES; i++)
-             wlcore_stop_queue(wl, i, reason);
-}
-EXPORT_SYMBOL_GPL(wlcore_stop_queues);
+     spin_lock_irqsave(&wl->wl_lock, flags);

-void wlcore_wake_queues(struct wl1271 *wl,
-                     enum wlcore_queue_stop_reason reason)
-{
-     int i;
+     for (q_base = 0; q_base < WLCORE_NUM_MAC_ADDRESSES; q_base++)
+             for (i = 0; i < NUM_TX_QUEUES; i++) {
+                     int h = q_base * NUM_TX_QUEUES + i;
+                     WARN_ON(test_and_set_bit(reason,
+                                              &wl->queue_stop_reasons[h]));
+             }

This looks a bit hacky.  You'll "stop queues" even for vifs that were
not added, won't you? Also, you can do with a single for-loop here ;)

there's no harm in setting the bit for queues that were not added, as
we zero out all stop reasons when a vif is added. we need to zero-out
the reasons anyway in case someone removes a stopped vif..

Yeah, I know there's no harm, but it still looks a bit ugly.  It's
probably not efficient to iterate over the existing vifs to set the stop
reasons, but that would look nicer.

Note that "existing" is tricky here as sometimes vifs are removed from
the driver (suspend/recovery) but still exist in mac80211.

It's the same deal as above, isn't it?

So for instance we want to stop all vifs during recovery, and mark all
with stop reasons. That's also the reason I used the mac80211 global
API for stopping/waking the queues.

A "global" ieee80211_iterate_active_interfaces_atomic() would do the
same thing. ;)

But I agree that it's simpler to set all possible vifs as stopped here.

quoted

In any case, if you want a quick and dirty way of doing it without
caring about the actual vifs, it could be like this:

        for (i = 0; i < WLCORE_NUM_MAC_ADDRESSES * NUM_TX_QUEUES; i++)
                WARN_ON(test_and_set_bit(reason,
                                         &wl->queue_stop_reasons[i]));

Your double-loop would make more sense if the cabs were included in the
same block per vif, as I suggested, though.

The compiler optimizes away the difference between my code and yours,
and I thought my version was a bit clearer, but yours is ok as well.

I'm not worried about optimization, the compiler is smarter than me. :)
But I think my version is easier to read, especially if we add a comment
like "/* stop all possible queues */".

quoted

quoted

quoted

I'm also not sure about the WARN_ON.  If one of the queues is already
stopped, should we really warn? I see that, currently, the reasons to
stop all queues are not used to stop a single queue, but still. :)

you're not supposed to use the global API if you've already used a
per-vif stop/start API. Generally there are reasons we stop a vif for,
and other reasons we stop all vifs for. There should not be an overlap
between reasons as that doesn't make sense.

Yes, this makes my statement below more significant. :)

quoted

I guess we could convert it to WARN_ON_ONCE though.

Yes, didn't I mention I would convert all WARN_ONs to WARN_ON_ONCEs? Oh,
well, I meant it anyway. ;)

:)

quoted

quoted

quoted

[...]

quoted

-void wlcore_reset_stopped_queues(struct wl1271 *wl)
+void wlcore_wake_queues(struct wl1271 *wl,
+                     enum wlcore_queue_stop_reason reason)
 {
-     int i;
+     int i, q_base;
      unsigned long flags;

      spin_lock_irqsave(&wl->wl_lock, flags);

-     for (i = 0; i < NUM_TX_QUEUES; i++) {
-             if (!wl->queue_stop_reasons[i])
-                     continue;
+     for (q_base = 0; q_base < WLCORE_NUM_MAC_ADDRESSES; q_base++)
+             for (i = 0; i < NUM_TX_QUEUES; i++) {
+                     int h = q_base * NUM_TX_QUEUES + i;
+                     WARN_ON(!test_and_clear_bit(reason,
+                                             &wl->queue_stop_reasons[h]));
+             }

Same thing here.  Actually, wouldn't it make more sense to have a
separate bitmask for the "full-stops"? In that way, at least it would be
very clear that the full-stop reasons (eg. flush, restart...) cannot be
used with single queues.

I think the WARN_ON separates this quite well already, I would prefer
to avoid the extra complexity. Again this can be converted to
WARN_ON_ONCE, since WARN_ON is frowned upon (and rightly so).

A WARN_ON separates it only at runtime.  We could miss it if the exact
combination of factors don't happen during testing.  If we would
separate them, we could also have a global all_queues_stop_reason
instead of going through all the queues and marking them. ;)

I guess it's a good point. But I think this should be in a separate
patch. This is not directly related to this one and would complicate
it as is.

Agreed.

--
Luca.