2011-11-04 15:04:35

by Janusz Dziedzic

Subject: mac80211: UAPSD - first release HW buffered frames next also check mac80211 buffered frames

Hello,

Seems currently we have implementation.

if(!driver_release_tids) {
...
} else {
...
}

Shouldn't we first release HW buffered frames and next check also SW
buffered frames?

Please check patch:
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 8eaa746..1303bfd 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1328,7 +1328,33 @@ ieee80211_sta_ps_deliver_response(struct sta_info *sta,
return;
}

- if (!driver_release_tids) {
+ /* First release driver buffered frames */
+ if (driver_release_tids){
+ /*
+ * We need to release a frame that is buffered somewhere in the
+ * driver ... it'll have to handle that.
+ * Note that, as per the comment above, it'll also have to see
+ * if there is more than just one frame on the specific TID that
+ * we're releasing from, and it needs to set the more-data bit
+ * accordingly if we tell it that there's no more data. If we do
+ * tell it there's more data, then of course the more-data bit
+ * needs to be set anyway.
+ */
+ drv_release_buffered_frames(local, sta, driver_release_tids,
+ n_frames, reason, more_data);
+
+ /*
+ * Note that we don't recalculate the TIM bit here as it would
+ * most likely have no effect at all unless the driver told us
+ * that the TID became empty before returning here from the
+ * release function.
+ * Either way, however, when the driver tells us that the TID
+ * became empty we'll do the TIM recalculation.
+ */
+ }
+
+ /* Now check SW buffered frames */
+ /* This if (found) seems to be not required in final patch */
+ if (found) {
struct sk_buff_head pending;
struct sk_buff *skb;
int num = 0;
@@ -1387,28 +1413,6 @@ ieee80211_sta_ps_deliver_response(struct sta_info *sta,
ieee80211_add_pending_skbs(local, &pending);

sta_info_recalc_tim(sta);
- } else {
- /*
- * We need to release a frame that is buffered somewhere in the
- * driver ... it'll have to handle that.
- * Note that, as per the comment above, it'll also have to see
- * if there is more than just one frame on the specific TID that
- * we're releasing from, and it needs to set the more-data bit
- * accordingly if we tell it that there's no more data. If we do
- * tell it there's more data, then of course the more-data bit
- * needs to be set anyway.
- */
- drv_release_buffered_frames(local, sta, driver_release_tids,
- n_frames, reason, more_data);
-

BR
Janusz

2011-11-04 15:07:10

by Johannes Berg

Subject: Re: mac80211: UAPSD - first release HW buffered frames next also check mac80211 buffered frames

On Fri, 2011-11-04 at 16:04 +0100, Janusz Dziedzic wrote:
> Hello,
>
> Seems currently we have implementation.
>
> if(!driver_release_tids) {
> ...
> } else {
> ...
> }
>
> Shouldn't we first release HW buffered frames and next check also SW
> buffered frames?

Only one of them can be true -- we don't release from driver & mac80211
queues at the same time. Technically we could, but the complexity would
be much higher than your simple patch for more-data handling etc.

johannes