[RFT] ar9170: AP broadcast buffering

[RFT] ar9170: AP broadcast buffering

[Christian Lamparter]

This patch should fix "broadcast traffic before DTIM beacon",
But there is a downside: the broadcast (e.g arp) frames
might get delayed and won't make it in time.

let me know, if this patch does any good, or not...
---
diff --git a/drivers/net/wireless/ath/ar9170/ar9170.h b/drivers/net/wireless/ath/ar9170/ar9170.h
index b99a8c2..c3d1349 100644
--- a/drivers/net/wireless/ath/ar9170/ar9170.h
+++ b/drivers/net/wireless/ath/ar9170/ar9170.h
@@ -176,6 +176,7 @@ struct ar9170 {
 	/* beaconing */
 	struct sk_buff *beacon;
 	struct work_struct beacon_work;
+	struct work_struct cab_work;
 	bool enable_beacon;
 
 	/* cryptographic engine */
@@ -278,6 +279,7 @@ int ar9170_set_basic_rates(struct ar9170 *ar);
 int ar9170_set_hwretry_limit(struct ar9170 *ar, u32 max_retry);
 int ar9170_update_beacon(struct ar9170 *ar);
 void ar9170_new_beacon(struct work_struct *work);
+void ar9170_send_buffered(struct work_struct *work);
 int ar9170_upload_key(struct ar9170 *ar, u8 id, const u8 *mac, u8 ktype,
 		      u8 keyidx, u8 *keydata, int keylen);
 int ar9170_disable_key(struct ar9170 *ar, u8 id);
diff --git a/drivers/net/wireless/ath/ar9170/mac.c b/drivers/net/wireless/ath/ar9170/mac.c
index 857e861..805b4f4 100644
--- a/drivers/net/wireless/ath/ar9170/mac.c
+++ b/drivers/net/wireless/ath/ar9170/mac.c
@@ -463,7 +463,6 @@ void ar9170_new_beacon(struct work_struct *work)
 {
 	struct ar9170 *ar = container_of(work, struct ar9170,
 					 beacon_work);
-	struct sk_buff *skb;
 
 	if (unlikely(!IS_STARTED(ar)))
 		return ;
@@ -475,6 +474,24 @@ void ar9170_new_beacon(struct work_struct *work)
 
 	ar9170_update_beacon(ar);
 
+ out:
+	mutex_unlock(&ar->mutex);
+}
+
+void ar9170_send_buffered(struct work_struct *work)
+{
+	struct ar9170 *ar = container_of(work, struct ar9170,
+					 cab_work);
+	struct sk_buff *skb;
+
+	if (unlikely(!IS_STARTED(ar)))
+		return ;
+
+	mutex_lock(&ar->mutex);
+
+	if (!ar->vif)
+		goto out;
+
 	rcu_read_lock();
 	while ((skb = ieee80211_get_buffered_bc(ar->hw, ar->vif)))
 		ar9170_op_tx(ar->hw, skb);
diff --git a/drivers/net/wireless/ath/ar9170/main.c b/drivers/net/wireless/ath/ar9170/main.c
index 20f04ab..f027d74 100644
--- a/drivers/net/wireless/ath/ar9170/main.c
+++ b/drivers/net/wireless/ath/ar9170/main.c
@@ -660,6 +660,10 @@ void ar9170_handle_command_response(struct ar9170 *ar, void *buf, u32 len)
 		 * YY always 00
 		 * B1-B4 "should" be the number of send out beacons.
 		 */
+
+		if (ar->vif && ar->vif->type == NL80211_IFTYPE_AP)
+			ieee80211_queue_work(ar->hw, &ar->cab_work);
+
 		break;
 
 	case 0xc3:
@@ -1294,6 +1298,7 @@ static void ar9170_op_stop(struct ieee80211_hw *hw)
 	cancel_delayed_work_sync(&ar->led_work);
 #endif
 	cancel_work_sync(&ar->beacon_work);
+	cancel_work_sync(&ar->cab_work);
 
 	mutex_lock(&ar->mutex);
 
@@ -2549,6 +2554,7 @@ void *ar9170_alloc(size_t priv_size)
 	}
 	ar9170_rx_reset_rx_mpdu(ar);
 	INIT_WORK(&ar->beacon_work, ar9170_new_beacon);
+	INIT_WORK(&ar->cab_work, ar9170_send_buffered);
 	INIT_DELAYED_WORK(&ar->tx_janitor, ar9170_tx_janitor);
 	INIT_LIST_HEAD(&ar->tx_ampdu_list);
 

Re: [RFT] ar9170: AP broadcast buffering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3987 bytes --]

Christian Lamparter wrote:
> This patch should fix "broadcast traffic before DTIM beacon",
> But there is a downside: the broadcast (e.g arp) frames
> might get delayed and won't make it in time.
> 
> let me know, if this patch does any good, or not...

Mmm, how is this supposed to work? I still haven't started hacking on
this as well, but I read a bit about DTIM. One thing I think to have
understood is that the DTIM interval is typically larger than the beacon
interval. There is even some interrupt on ar9170 indicating this event,
it's just not enabled yet.

Moreover, I do not yet understand how deferring multicast frames works
in this patch. How are they filtered out from normal frames and queued
until the beacon event? (But that misunderstanding might be due to the
fact that I haven't read the whole driver code yet.)

Jan

> ---
> diff --git a/drivers/net/wireless/ath/ar9170/ar9170.h b/drivers/net/wireless/ath/ar9170/ar9170.h
> index b99a8c2..c3d1349 100644
> --- a/drivers/net/wireless/ath/ar9170/ar9170.h
> +++ b/drivers/net/wireless/ath/ar9170/ar9170.h
> @@ -176,6 +176,7 @@ struct ar9170 {
>  	/* beaconing */
>  	struct sk_buff *beacon;
>  	struct work_struct beacon_work;
> +	struct work_struct cab_work;
>  	bool enable_beacon;
>  
>  	/* cryptographic engine */
> @@ -278,6 +279,7 @@ int ar9170_set_basic_rates(struct ar9170 *ar);
>  int ar9170_set_hwretry_limit(struct ar9170 *ar, u32 max_retry);
>  int ar9170_update_beacon(struct ar9170 *ar);
>  void ar9170_new_beacon(struct work_struct *work);
> +void ar9170_send_buffered(struct work_struct *work);
>  int ar9170_upload_key(struct ar9170 *ar, u8 id, const u8 *mac, u8 ktype,
>  		      u8 keyidx, u8 *keydata, int keylen);
>  int ar9170_disable_key(struct ar9170 *ar, u8 id);
> diff --git a/drivers/net/wireless/ath/ar9170/mac.c b/drivers/net/wireless/ath/ar9170/mac.c
> index 857e861..805b4f4 100644
> --- a/drivers/net/wireless/ath/ar9170/mac.c
> +++ b/drivers/net/wireless/ath/ar9170/mac.c
> @@ -463,7 +463,6 @@ void ar9170_new_beacon(struct work_struct *work)
>  {
>  	struct ar9170 *ar = container_of(work, struct ar9170,
>  					 beacon_work);
> -	struct sk_buff *skb;
>  
>  	if (unlikely(!IS_STARTED(ar)))
>  		return ;
> @@ -475,6 +474,24 @@ void ar9170_new_beacon(struct work_struct *work)
>  
>  	ar9170_update_beacon(ar);
>  
> + out:
> +	mutex_unlock(&ar->mutex);
> +}
> +
> +void ar9170_send_buffered(struct work_struct *work)
> +{
> +	struct ar9170 *ar = container_of(work, struct ar9170,
> +					 cab_work);
> +	struct sk_buff *skb;
> +
> +	if (unlikely(!IS_STARTED(ar)))
> +		return ;
> +
> +	mutex_lock(&ar->mutex);
> +
> +	if (!ar->vif)
> +		goto out;
> +
>  	rcu_read_lock();
>  	while ((skb = ieee80211_get_buffered_bc(ar->hw, ar->vif)))
>  		ar9170_op_tx(ar->hw, skb);
> diff --git a/drivers/net/wireless/ath/ar9170/main.c b/drivers/net/wireless/ath/ar9170/main.c
> index 20f04ab..f027d74 100644
> --- a/drivers/net/wireless/ath/ar9170/main.c
> +++ b/drivers/net/wireless/ath/ar9170/main.c
> @@ -660,6 +660,10 @@ void ar9170_handle_command_response(struct ar9170 *ar, void *buf, u32 len)
>  		 * YY always 00
>  		 * B1-B4 "should" be the number of send out beacons.
>  		 */
> +
> +		if (ar->vif && ar->vif->type == NL80211_IFTYPE_AP)
> +			ieee80211_queue_work(ar->hw, &ar->cab_work);
> +
>  		break;
>  
>  	case 0xc3:
> @@ -1294,6 +1298,7 @@ static void ar9170_op_stop(struct ieee80211_hw *hw)
>  	cancel_delayed_work_sync(&ar->led_work);
>  #endif
>  	cancel_work_sync(&ar->beacon_work);
> +	cancel_work_sync(&ar->cab_work);
>  
>  	mutex_lock(&ar->mutex);
>  
> @@ -2549,6 +2554,7 @@ void *ar9170_alloc(size_t priv_size)
>  	}
>  	ar9170_rx_reset_rx_mpdu(ar);
>  	INIT_WORK(&ar->beacon_work, ar9170_new_beacon);
> +	INIT_WORK(&ar->cab_work, ar9170_send_buffered);
>  	INIT_DELAYED_WORK(&ar->tx_janitor, ar9170_tx_janitor);
>  	INIT_LIST_HEAD(&ar->tx_ampdu_list);
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [RFT] ar9170: AP broadcast buffering

[Christian Lamparter]

On Sunday 06 December 2009 19:23:09 Jan Kiszka wrote:
> Christian Lamparter wrote:
> > This patch should fix "broadcast traffic before DTIM beacon",
> > But there is a downside: the broadcast (e.g arp) frames
> > might get delayed and won't make it in time.
> > 
> > let me know, if this patch does any good, or not...
> 
> Mmm, how is this supposed to work?
The driver sets the PRETBTT interval and the HW will trigger
the PRETBTT event. Which will tell the driver to update the beacon,
which _in turn_ triggers the BCN_CFG event and the buffered
BC frames are _released_ into the queues.

Now the problem is that under traffic the BC/MC frames might
be delayed in the tx_pending queues and don't make it in time.

> I still haven't started hacking on this as well, but I read a bit
> about DTIM.
> One thing I think to have understood is that the DTIM interval is typically
> larger than the beacon interval. There is even some interrupt on ar9170
> indicating this event, it's just not enabled yet.
Na, I _think_ that BIT is meant for the PS/station mode and
is used to say to the fw/driver to stay alert for incoming
broadcast/multicast data.

If this really is the case, then I'm afraid to say:
the bit is probably for little use in this situation. 
> Moreover, I do not yet understand how deferring multicast frames works
> in this patch. How are they filtered out from normal frames and queued
> until the beacon event? (But that misunderstanding might be due to the
> fact that I haven't read the whole driver code yet.)
ar9170 does not have a "Content After Beacon" queue like p54 or ath5k/ath9k.
But luckily the mac80211 stack provides the necessary queuing mechanism.
And all the driver needs to do: 
 - set IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING
 - grab the buffered BC/MC frames and push them to the device.

Note:

That's the current situation. But maybe you'll be in for a big surprise.
There is a special firmware & driver: "carl9170", which will add several
useful features:
 * accurate tx status feedback
 * _CAB_ queuing in firmware
  => this should be enough to finally enable NL80211_IFTYPE_AP / MESH.

 * multirate retry support (minstrel)
  => improve performance in suboptimal environments.

 * Real Hardware QoS support.
   (But unfortunately with a catch)

I hope it will be ready for Christmas ;-)

Regards,
	Chr

Re: [RFT] ar9170: AP broadcast buffering

[Kalle Valo]

Christian Lamparter <chunkeey@googlemail.com> writes:

> Now the problem is that under traffic the BC/MC frames might
> be delayed in the tx_pending queues and don't make it in time.

I don't know what time constraints you refer to here, but clients
should stay awake until they have received a multicast/broadcast
message with the moredata bit disabled. So, in theory, it doesn't
matter even if the frames are transmitted even 20 ms after the beacon.
(In practise some hardware might have timers for this, I think at
least wl1251 had one.)

Of course the downside is the increased power consumption. But it's
still better than to not receive the multicast/broadcast frames at all :)

-- 
Kalle Valo

Re: [RFT] ar9170: AP broadcast buffering

[Christian Lamparter]

On Monday 07 December 2009 21:08:32 Kalle Valo wrote:
> Christian Lamparter <chunkeey@googlemail.com> writes:
> 
> > Now the problem is that under traffic the BC/MC frames might
> > be delayed in the tx_pending queues and don't make it in time.
> 
> I don't know what time constraints you refer to here, but clients
> should stay awake until they have received a multicast/broadcast
> message with the moredata bit disabled. So, in theory, it doesn't
> matter even if the frames are transmitted even 20 ms after the beacon.
> (In practise some hardware might have timers for this, I think at
> least wl1251 had one.)

> Of course the downside is the increased power consumption. But it's
> still better than to not receive the multicast/broadcast frames at all :)
That's the thing: 802.11-2007 11.2.1.5 f) says:
"*Immediately* after every DTIM, the AP shall
  transmit all buffered broadcast/multicast MSDUs...".
And I cannot rule out that some Devs took this quite literally. ;-)

Anyway, here is a case that I've witnessed:

During testing, I caught samples where MC/BC frames were delayed for
more up to 115ms. (b_interval = 102.4ms and dtim_period = 3)
and this is where another paragraph in 11.2.1.5 f) kicks in:

 "If the AP is unable to transmit all of the buffered broadcast/multicast MSDUs
  before the TBTT following the DTIM, the AP shall indicate that it will
  continue to deliver the broadcast/multicast MSDUs by setting the bit for AID 0
  (zero) in the bit map control field of the TIM element of every Beacon frame,
  until all buffered broadcast/multicast frames have been transmitted."

And if you look at the mac80211 code: ieee80211_beacon_add_tim, you can see why
this is a problem:

if (bss->dtim_count == 0 && !skb_queue_empty(&bss->ps_bc_buf))
	aid0 = 1;

The BC/MC bit is only set for DTIM beacons, but the MC/BC traffic _arrived_
116ms after the DTIM beacon had aired and worse: even after the normal beacon,
without the BC/MC flag...

Note:
 * Unfortunately it is not unusual for ar9170 to *forget* tx_status
   reports and then we have to wait for the _janitor_ work to _retire_
   old frames.

 * the beacon (delta) data is uploaded through the command interface
   and therefore the beacon is always right on time.

Regards,	
	Chr

Re: [RFT] ar9170: AP broadcast buffering

[Kalle Valo]

Christian Lamparter <chunkeey@googlemail.com> writes:

> On Monday 07 December 2009 21:08:32 Kalle Valo wrote:
>
>> I don't know what time constraints you refer to here, but clients
>> should stay awake until they have received a multicast/broadcast
>> message with the moredata bit disabled. So, in theory, it doesn't
>> matter even if the frames are transmitted even 20 ms after the beacon.
>> (In practise some hardware might have timers for this, I think at
>> least wl1251 had one.)
>
>> Of course the downside is the increased power consumption. But it's
>> still better than to not receive the multicast/broadcast frames at
>> all :)
>
> That's the thing: 802.11-2007 11.2.1.5 f) says:
> "*Immediately* after every DTIM, the AP shall
>   transmit all buffered broadcast/multicast MSDUs...".
> And I cannot rule out that some Devs took this quite literally. ;-)

Well, "immediately" is relative. If my wife says to immeaditely do
something, it might take a week before I'll do it ;)

> Anyway, here is a case that I've witnessed:
>
> During testing, I caught samples where MC/BC frames were delayed for
> more up to 115ms. (b_interval = 102.4ms and dtim_period = 3)

Yeah, if the time is longer than the beacon interval then it won't
definitely work.

-- 
Kalle Valo

Re: [RFT] ar9170: AP broadcast buffering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

Christian Lamparter wrote:
> On Sunday 06 December 2009 19:23:09 Jan Kiszka wrote:
>> Christian Lamparter wrote:
>>> This patch should fix "broadcast traffic before DTIM beacon",
>>> But there is a downside: the broadcast (e.g arp) frames
>>> might get delayed and won't make it in time.
>>>
>>> let me know, if this patch does any good, or not...
>> Mmm, how is this supposed to work?
> The driver sets the PRETBTT interval and the HW will trigger
> the PRETBTT event. Which will tell the driver to update the beacon,
> which _in turn_ triggers the BCN_CFG event and the buffered
> BC frames are _released_ into the queues.

When is that PRETBTT signaled, on every beacon or only on DTIM beacons?
If on the former, I do not see yet where the code filters for the latter.

> 
> Now the problem is that under traffic the BC/MC frames might
> be delayed in the tx_pending queues and don't make it in time.

I don't think it's the right long-term solution to let such
time-critical jobs be done by the host as long as the controller is
capable of handling it more accurately.

> 
>> I still haven't started hacking on this as well, but I read a bit
>> about DTIM.
>> One thing I think to have understood is that the DTIM interval is typically
>> larger than the beacon interval. There is even some interrupt on ar9170
>> indicating this event, it's just not enabled yet.
> Na, I _think_ that BIT is meant for the PS/station mode and
> is used to say to the fw/driver to stay alert for incoming
> broadcast/multicast data.

Likely. But maybe it also signals the that this period has started so
that the controller can send out the corresponding frames. Otherwise it
had to process every beacon interrupt to filter for DTIM.

> 
> If this really is the case, then I'm afraid to say:
> the bit is probably for little use in this situation. 
>> Moreover, I do not yet understand how deferring multicast frames works
>> in this patch. How are they filtered out from normal frames and queued
>> until the beacon event? (But that misunderstanding might be due to the
>> fact that I haven't read the whole driver code yet.)
> ar9170 does not have a "Content After Beacon" queue like p54 or ath5k/ath9k.
> But luckily the mac80211 stack provides the necessary queuing mechanism.
> And all the driver needs to do: 
>  - set IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING
>  - grab the buffered BC/MC frames and push them to the device.
> 

Ah, I see.

> Note:
> 
> That's the current situation. But maybe you'll be in for a big surprise.
> There is a special firmware & driver: "carl9170", which will add several
> useful features:
>  * accurate tx status feedback
>  * _CAB_ queuing in firmware
>   => this should be enough to finally enable NL80211_IFTYPE_AP / MESH.
> 
>  * multirate retry support (minstrel)
>   => improve performance in suboptimal environments.
> 
>  * Real Hardware QoS support.
>    (But unfortunately with a catch)
> 
> I hope it will be ready for Christmas ;-)

Sounds interesting. Is it derived from ar9170-fw or written from
scratch? In any case, I'm looking forward to those improvements.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]