[PATCH 0/4] Enable uAPSD support in AP mode

[PATCH 0/4] Enable uAPSD support in AP mode

[Thirumalai]

In addition to legacy power save, this series of patch adds
support for uAPSD in AP mode.

When connected station goes into suspend state, the driver will
start queuing pkts in the separate queue (called uapsd queue)
until the station ask for it. 

The moment driver starting queuing, it will enable traffic indication
bit for the client connected to indicate data availability. 
Once station ask for the data (via Trigger frame), the driver will 
dequeue and send pkt one by one to the station.

Thirumalai (4):
  ath6kl: Add new WMI functions for uAPSD
  ath6kl: Introduce new queue for uAPSD.
  ath6kl: Add uAPSD support in rx path.
  ath6kl: Add UAPSD support in tx path.

 drivers/net/wireless/ath/ath6kl/cfg80211.c |    6 +
 drivers/net/wireless/ath/ath6kl/core.h     |    6 +-
 drivers/net/wireless/ath/ath6kl/main.c     |    8 +-
 drivers/net/wireless/ath/ath6kl/txrx.c     |  258 ++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath6kl/wmi.c      |   62 ++++++-
 drivers/net/wireless/ath/ath6kl/wmi.h      |   50 +++++-
 6 files changed, 339 insertions(+), 51 deletions(-)

Re: [PATCH 0/4] Enable uAPSD support in AP mode

[Johannes Berg]

On Mon, 2012-01-09 at 13:14 +0530, Thirumalai wrote:
> In addition to legacy power save, this series of patch adds
> support for uAPSD in AP mode.
> 
> When connected station goes into suspend state, the driver will
> start queuing pkts in the separate queue (called uapsd queue)
> until the station ask for it. 
> 
> The moment driver starting queuing, it will enable traffic indication
> bit for the client connected to indicate data availability. 
> Once station ask for the data (via Trigger frame), the driver will 
> dequeue and send pkt one by one to the station.

The way you describe this sounds incorrect, but I'll leave it to Kalle
to figure out what you're doing and whether it is correct or not ... if
only some ACs are trigger-enabled those don't influence the TIM bit.

johannes

Re: [PATCH 0/4] Enable uAPSD support in AP mode

[Kalle Valo]

On 01/09/2012 10:54 AM, Johannes Berg wrote:
> On Mon, 2012-01-09 at 13:14 +0530, Thirumalai wrote:
>> In addition to legacy power save, this series of patch adds
>> support for uAPSD in AP mode.
>>
>> When connected station goes into suspend state, the driver will
>> start queuing pkts in the separate queue (called uapsd queue)
>> until the station ask for it. 
>>
>> The moment driver starting queuing, it will enable traffic indication
>> bit for the client connected to indicate data availability. 
>> Once station ask for the data (via Trigger frame), the driver will 
>> dequeue and send pkt one by one to the station.
> 
> The way you describe this sounds incorrect, but I'll leave it to Kalle
> to figure out what you're doing and whether it is correct or not ... if
> only some ACs are trigger-enabled those don't influence the TIM bit.

Yeah, that does sound odd. Thirumalai, does the code really this way or
is it just a problem with the description? (I need to study the code in
detail still.)

Johannes, thanks for pointing this out.

Kalle

RE: [PATCH 0/4] Enable uAPSD support in AP mode

[Pachamuthu, Thirumalai]

S2FsbGUsDQoJSXQncyBqdXN0IGEgcHJvYmxlbSBvZiBkZXNjcmlwdGlvbiBvbmx5LiBXZSB3aWxs
IG5vdCBlbmFibGUgdGhlIFRJTSB3aGVuIHdlIGNvbmZpZ3VyZSBzb21lIG9mIHRoZSBhYyBhcyB0
cmlnZ2VyIGVuYWJsZWQuIEhvd2V2ZXIgd2Ugd2lsbCB1c2UgdGhlIFRJTSB3aGVuIHdlIGVuYWJs
ZWQgYWxsIHRoZSBBQydzLg0KLVRoaXJ1bWFsYWkNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0t
LS0NCkZyb206IFZhbG8sIEthbGxlIA0KU2VudDogTW9uZGF5LCBKYW51YXJ5IDA5LCAyMDEyIDU6
MTcgUE0NClRvOiBKb2hhbm5lcyBCZXJnDQpDYzogUGFjaGFtdXRodSwgVGhpcnVtYWxhaTsgbGlu
dXgtd2lyZWxlc3NAdmdlci5rZXJuZWwub3JnOyBhdGg2a2wtZGV2ZWwNClN1YmplY3Q6IFJlOiBb
UEFUQ0ggMC80XSBFbmFibGUgdUFQU0Qgc3VwcG9ydCBpbiBBUCBtb2RlDQoNCk9uIDAxLzA5LzIw
MTIgMTA6NTQgQU0sIEpvaGFubmVzIEJlcmcgd3JvdGU6DQo+IE9uIE1vbiwgMjAxMi0wMS0wOSBh
dCAxMzoxNCArMDUzMCwgVGhpcnVtYWxhaSB3cm90ZToNCj4+IEluIGFkZGl0aW9uIHRvIGxlZ2Fj
eSBwb3dlciBzYXZlLCB0aGlzIHNlcmllcyBvZiBwYXRjaCBhZGRzIHN1cHBvcnQgDQo+PiBmb3Ig
dUFQU0QgaW4gQVAgbW9kZS4NCj4+DQo+PiBXaGVuIGNvbm5lY3RlZCBzdGF0aW9uIGdvZXMgaW50
byBzdXNwZW5kIHN0YXRlLCB0aGUgZHJpdmVyIHdpbGwgc3RhcnQgDQo+PiBxdWV1aW5nIHBrdHMg
aW4gdGhlIHNlcGFyYXRlIHF1ZXVlIChjYWxsZWQgdWFwc2QgcXVldWUpIHVudGlsIHRoZSANCj4+
IHN0YXRpb24gYXNrIGZvciBpdC4NCj4+DQo+PiBUaGUgbW9tZW50IGRyaXZlciBzdGFydGluZyBx
dWV1aW5nLCBpdCB3aWxsIGVuYWJsZSB0cmFmZmljIGluZGljYXRpb24gDQo+PiBiaXQgZm9yIHRo
ZSBjbGllbnQgY29ubmVjdGVkIHRvIGluZGljYXRlIGRhdGEgYXZhaWxhYmlsaXR5Lg0KPj4gT25j
ZSBzdGF0aW9uIGFzayBmb3IgdGhlIGRhdGEgKHZpYSBUcmlnZ2VyIGZyYW1lKSwgdGhlIGRyaXZl
ciB3aWxsIA0KPj4gZGVxdWV1ZSBhbmQgc2VuZCBwa3Qgb25lIGJ5IG9uZSB0byB0aGUgc3RhdGlv
bi4NCj4gDQo+IFRoZSB3YXkgeW91IGRlc2NyaWJlIHRoaXMgc291bmRzIGluY29ycmVjdCwgYnV0
IEknbGwgbGVhdmUgaXQgdG8gS2FsbGUgDQo+IHRvIGZpZ3VyZSBvdXQgd2hhdCB5b3UncmUgZG9p
bmcgYW5kIHdoZXRoZXIgaXQgaXMgY29ycmVjdCBvciBub3QgLi4uIA0KPiBpZiBvbmx5IHNvbWUg
QUNzIGFyZSB0cmlnZ2VyLWVuYWJsZWQgdGhvc2UgZG9uJ3QgaW5mbHVlbmNlIHRoZSBUSU0gYml0
Lg0KDQpZZWFoLCB0aGF0IGRvZXMgc291bmQgb2RkLiBUaGlydW1hbGFpLCBkb2VzIHRoZSBjb2Rl
IHJlYWxseSB0aGlzIHdheSBvciBpcyBpdCBqdXN0IGEgcHJvYmxlbSB3aXRoIHRoZSBkZXNjcmlw
dGlvbj8gKEkgbmVlZCB0byBzdHVkeSB0aGUgY29kZSBpbiBkZXRhaWwgc3RpbGwuKQ0KDQpKb2hh
bm5lcywgdGhhbmtzIGZvciBwb2ludGluZyB0aGlzIG91dC4NCg0KS2FsbGUNCg==

Re: [PATCH 0/4] Enable uAPSD support in AP mode

[Kalle Valo]

On 01/09/2012 09:44 AM, Thirumalai wrote:
> In addition to legacy power save, this series of patch adds
> support for uAPSD in AP mode.
> 
> When connected station goes into suspend state, the driver will
> start queuing pkts in the separate queue (called uapsd queue)
> until the station ask for it. 
> 
> The moment driver starting queuing, it will enable traffic indication
> bit for the client connected to indicate data availability. 
> Once station ask for the data (via Trigger frame), the driver will 
> dequeue and send pkt one by one to the station.

I didn't do very thorough review but here are my first comments:

I see that you enable uapsd in patch 2 but add more functionality in
patches 3 and 4. That will break bisect as uapsd will be broken between
patches 2 and 4, no?

It's nice to have small patches, but it's more important to make sure
that bisect works. So you could just fold patches 2-4 into one. You
could even fold patch 1, as it doesn't give any benefit, and just have
one big patch adding the support. Or alternatively you need to enable
uapsd in the last patch so that it won't be used until all the code is
in place.

There are also some sparse warnings:

drivers/net/wireless/ath/ath6kl/wmi.h:232:21: warning: invalid
assignment: |=
drivers/net/wireless/ath/ath6kl/wmi.h:232:21:    left side has type
restricted __le16
drivers/net/wireless/ath/ath6kl/wmi.h:232:21:    right side has type int
drivers/net/wireless/ath/ath6kl/txrx.c:109:36: warning: incorrect type
in assignment (different base types)
drivers/net/wireless/ath/ath6kl/txrx.c:109:36:    expected unsigned
short [unsigned] [usertype] ether_type
drivers/net/wireless/ath/ath6kl/txrx.c:109:36:    got restricted __be16
[usertype] h_proto
drivers/net/wireless/ath/ath6kl/txrx.c:110:29: warning: cast to
restricted __be16
drivers/net/wireless/ath/ath6kl/txrx.c:110:29: warning: cast to
restricted __be16
drivers/net/wireless/ath/ath6kl/txrx.c:110:29: warning: cast to
restricted __be16
drivers/net/wireless/ath/ath6kl/txrx.c:110:29: warning: cast to
restricted __be16
drivers/net/wireless/ath/ath6kl/txrx.c:117:44: warning: incorrect type
in assignment (different base types)
drivers/net/wireless/ath/ath6kl/txrx.c:117:44:    expected unsigned
short [unsigned] [usertype] ether_type
drivers/net/wireless/ath/ath6kl/txrx.c:117:44:    got restricted __be16
[usertype] eth_type
drivers/net/wireless/ath/ath6kl/txrx.c:121:43: warning: restricted
__be16 degrades to integer
drivers/net/wireless/ath/ath6kl/txrx.c:1360:41: warning: restricted
__le16 degrades to integer

And please use your full name in the signed off by line.

Kalle