2017-07-19 06:36:33

by Xinming Hu

[permalink] [raw]
Subject: [PATCH] mwifiex: add tdls uapsd support module parameter

From: Xinming Hu <[email protected]>

Add module parameter to control tdls uapsd support, which is
default disabled.

Signed-off-by: Xinming Hu <[email protected]>
Signed-off-by: Cathy Luo <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/init.c | 5 +++++
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 8 +++++---
3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 3ecb59f..2cc8e54 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -25,6 +25,10 @@
#include "wmm.h"
#include "11n.h"

+static bool tdls_uapsd;
+module_param(tdls_uapsd, bool, 0000);
+MODULE_PARM_DESC(tdls_uapsd, "tdls uapsd support enable:1, disable:0");
+
/*
* This function adds a BSS priority table to the table list.
*
@@ -154,6 +158,7 @@ int mwifiex_init_priv(struct mwifiex_private *priv)
priv->del_list_idx = 0;
priv->hs2_enabled = false;
priv->check_tdls_tx = false;
+ priv->tdls_uapsd_support = tdls_uapsd;
memcpy(priv->tos_to_tid_inv, tos_to_tid_inv, MAX_NUM_TID);

mwifiex_init_11h_params(priv);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index f8cf307..ef5eac72 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -660,6 +660,7 @@ struct mwifiex_private {
u8 check_tdls_tx;
struct timer_list auto_tdls_timer;
bool auto_tdls_timer_active;
+ u8 tdls_uapsd_support;
struct idr ack_status_frames;
/* spin lock for ack status */
spinlock_t ack_status_lock;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 534d94a..d5da565 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -1789,9 +1789,11 @@ static int mwifiex_cmd_chan_region_cfg(struct mwifiex_private *priv,
put_unaligned_le16(params->capability, pos);
config_len += sizeof(params->capability);

- qos_info = params->uapsd_queues | (params->max_sp << 5);
- wmm_qos_info = (struct mwifiex_ie_types_qos_info *)(pos +
- config_len);
+ if (priv->tdls_uapsd_support)
+ qos_info = params->uapsd_queues | (params->max_sp << 5);
+ else
+ qos_info = 0;
+ wmm_qos_info = (void *)(pos + config_len);
wmm_qos_info->header.type = cpu_to_le16(WLAN_EID_QOS_CAPA);
wmm_qos_info->header.len = cpu_to_le16(sizeof(qos_info));
wmm_qos_info->qos_info = qos_info;
--
1.9.1


2017-07-21 10:00:19

by Xinming Hu

[permalink] [raw]
Subject: Re: Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

SGksDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRG1pdHJ5IFRvcm9r
aG92IFttYWlsdG86ZHRvckBnb29nbGUuY29tXQ0KPiBTZW50OiAyMDE35bm0N+aciDIx5pelIDE6
MjkNCj4gVG86IEJyaWFuIE5vcnJpcw0KPiBDYzogWGlubWluZyBIdTsgWGlubWluZyBIdTsgR2Fu
YXBhdGhpIEJoYXQ7IExpbnV4IFdpcmVsZXNzOyBLYWxsZSBWYWxvOw0KPiByYWphdGphQGdvb2ds
ZS5jb207IFpoaXl1YW4gWWFuZzsgVGltIFNvbmc7IENhdGh5IEx1bw0KPiBTdWJqZWN0OiBbRVhU
XSBSZTogUmU6IFtQQVRDSF0gbXdpZmlleDogYWRkIHRkbHMgdWFwc2Qgc3VwcG9ydCBtb2R1bGUN
Cj4gcGFyYW1ldGVyDQo+IA0KPiBFeHRlcm5hbCBFbWFpbA0KPiANCj4gLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0K
PiBPbiBUaHUsIEp1bCAyMCwgMjAxNyBhdCAxMDoxNiBBTSwgQnJpYW4gTm9ycmlzIDxicmlhbm5v
cnJpc0BjaHJvbWl1bS5vcmc+DQo+IHdyb3RlOg0KPiA+IEhpLA0KPiA+DQo+ID4gT24gVGh1LCBK
dWwgMjAsIDIwMTcgYXQgMTA6NTQ6MTZBTSArMDAwMCwgWGlubWluZyBIdSB3cm90ZToNCj4gPj4g
SGkgQnJpYW4sDQo+ID4+DQo+ID4+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPj4g
PiBGcm9tOiBCcmlhbiBOb3JyaXMgW21haWx0bzpicmlhbm5vcnJpc0BjaHJvbWl1bS5vcmddDQo+
ID4+ID4gU2VudDogMjAxN+W5tDfmnIgyMOaXpSA0OjEwDQo+ID4+ID4gVG86IFhpbm1pbmcgSHUN
Cj4gPj4gPiBDYzogTGludXggV2lyZWxlc3M7IEthbGxlIFZhbG87IERtaXRyeSBUb3Jva2hvdjsN
Cj4gPj4gPiByYWphdGphQGdvb2dsZS5jb207IFpoaXl1YW4gWWFuZzsgVGltIFNvbmc7IENhdGh5
IEx1bzsgWGlubWluZyBIdQ0KPiA+PiA+IFN1YmplY3Q6IFtFWFRdIFJlOiBbUEFUQ0hdIG13aWZp
ZXg6IGFkZCB0ZGxzIHVhcHNkIHN1cHBvcnQgbW9kdWxlDQo+ID4+ID4gcGFyYW1ldGVyDQo+ID4+
ID4NCj4gPj4gPiBFeHRlcm5hbCBFbWFpbA0KPiA+PiA+DQo+ID4+ID4gLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+
PiA+IC0tLSBPbiBXZWQsIEp1bCAxOSwgMjAxNyBhdCAwNjozNjoyN0FNICswMDAwLCBYaW5taW5n
IEh1IHdyb3RlOg0KPiA+PiA+ID4gRnJvbTogWGlubWluZyBIdSA8aHV4bUBtYXJ2ZWxsLmNvbT4N
Cj4gPj4gPiA+DQo+ID4+ID4gPiBBZGQgbW9kdWxlIHBhcmFtZXRlciB0byBjb250cm9sIHRkbHMg
dWFwc2Qgc3VwcG9ydCwgd2hpY2ggaXMNCj4gPj4gPiA+IGRlZmF1bHQgZGlzYWJsZWQuDQo+ID4+
ID4NCj4gPj4gPiBXaHkgZGVmYXVsdCBkaXNhYmxlZCwgd2hlbiBpdCBsb29rcyBsaWtlIGl0IHVz
ZWQgdG8gYmUgb24gYnkgZGVmYXVsdD8NCj4gPj4NCj4gPj4gT2hvLCBJIHNob3VsZCBjb21tZW50
IG1vcmUgaW4gZGVzY3JpcHRpb24sIGl0IGlzIG5vdyBjb25mdXNpbmcuDQo+ID4+IFdlIGp1c3Qg
Zml4ZWQgYW4gdGRscyB1YXBzZCBpc3N1ZSBpbiBsYXRlc3QgZmlybXdhcmUsIHNvIHRyeSB0bw0K
PiA+PiBkaXNhYmxlIHRoaXMgZmVhdHVyZSBhcyBhIHdvcmthcm91bmQgdG8gdGhlIG9sZCBmaXJt
d2FyZS4gQXQgdGhlIHNhbWUNCj4gPj4gdGltZSwgaXQgaXMgb3B0aW9uYWwgdG8gZW5hYmxlIHRo
aXMgZmVhdHVyZSBpbiBzcGVjaWZpYyBjYXNlLg0KPiA+DQo+ID4gVGhhdCBoZWxwcyBhIGJpdCwg
dGhhbmtzLg0KPiA+DQo+ID4+IEkgd2lsbCBhZGQgbW9yZSBjb21tZW50cyBpbiBWMi4NCj4gPj4g
UGxlYXNlIGxldCB1cyBrbm93IGlmIHlvdSBoYXZlIG1vcmUgY29tbWVudHMuDQo+ID4NCj4gPiBJ
IHdvbid0IGluc2lzdCBvbiBjaGFuZ2VzLCBidXQgZm9yIHNvbWV0aGluZyBsaWtlIHRoaXMsIGl0
IHNlZW1zDQo+ID4gcmVhc29uYWJsZSAoaWYgeW91IGhhdmUgcmVhbGx5IGZpeGVkIHRoZSBpc3N1
ZSBpbiB0aGUgbGF0ZXN0IGZpcm13YXJlKQ0KPiA+IHRvIGp1c3QgcHJvdmlkZSB0aGUga25vYiB0
byBkaXNhYmxlIGFzIGEgYmFja3VwLCBub3QgYXMgYSBkZWZhdWx0LiBJZg0KPiA+IHNvbWVvbmUg
aXMgZ29pbmcgdG8gdXBkYXRlIHRoZWlyIGtlcm5lbCAodG8gaW5jbHVkZSB0aGlzIHBhdGNoKSwg
YnV0DQo+ID4gbm90IHVwZGF0ZSB0aGVpciBmaXJtd2FyZSwgdGhlbiB0aGV5IHByb2JhYmx5IHNo
b3VsZCBrbm93IGVub3VnaCB0byBiZQ0KPiA+IGFibGUgdG8gcHJvdmlkZSB0aGUgbW9kdWxlIHBh
cmFtZXRlciB0byBkaXNhYmxlLg0KPiA+DQo+ID4gT3IgYWx0ZXJuYXRpdmVseTogaG93IGlzIGFu
eW9uZSBzdXBwb3NlZCB0byBrbm93IHdoZXRoZXIgdGhlaXIgY3VycmVudA0KPiA+IGZpcm13YXJl
IGlzIGJyb2tlbiBvciBub3Q/IEFuZCBob3cgaXMgdGhpcyBmZWF0dXJlIGV2ZXIgZ29pbmcgdG8g
YmUNCj4gPiBkZWZhdWx0LWVuYWJsZWQgYWdhaW4/IE5ldyBjaGlwc2V0cyB3aXRoIG5ldyBmaXJt
d2FyZSBzaG91bGQgaG9wZWZ1bGx5DQo+ID4gbm90IGhhdmUgdGhlIHNhbWUgYnVncywgbm8/DQo+
IA0KPiBCZXR0ZXIgeWV0LCB0aGUgZHJpdmVyIGNvdWxkIGNoZWNrIHRoZSBmaXJtd2FyZSB2ZXJz
aW9uLCBhbmQgaWYgaXQgaXMga25vd24gYmFkDQo+IGRpc2FibGUgdGhlIGZlYXR1cmUgYXV0b21h
dGljYWxseS4gVGhlbiB0aGVyZSBpcyBubyBuZWVkIHRvIHByb3ZpZGUgdGhpcyBrbm9iLg0KPiAN
Cg0KVGhhbmtzIGZvciBhbGwgdGhlIGNvbW1lbnRzLiBJIGRlZWQgbWlzdXNlIHRoZSBtb2R1bGQg
cGFyYW1ldGVyLiANClRoZSB0ZGxzIHVwYXNkIGNhcGFiaWxpdHkgaGF2ZSBhbHJlYWR5IGJlZW4g
ZGlzYWJsZWQgZHVyaW5nIHRkbHMgc2V0dXAuDQpXaGlsZSBoZXJlIHdlIGFyZSB0cnlpbmcgdG8g
cmVseSBvbiB0ZGxzIHBlZXIgdWFwc2QgY2FwYWJpbGl0eSBpbiB0ZGxzIGNvbmZpZ3VyYXRpb24u
DQpTbyB3ZSBjYW4gc2ltcGx5IGRpc2FibGUgaXQgaW4gdGRscyBjb25maWcgdG8ga2VlcCBpdCBt
YXRjaCB3aXRoIG91ciBkZWZhdWx0IGNhcGFiaWxpdHkuDQoNClNvcnJ5IGZvciB0aGUgY29uZnVz
aW5nIGluIHRoaXMgcGF0Y2guDQpJIGhhdmUgc2VudCB0aGUgcmVwbGFjZSB2ZXJzaW9uIGZvciBy
ZXZpZXcuDQoNClJlZ2FyZHMsDQpTaW1vbg0KDQo+IFRoYW5rcy4NCj4gDQo+IC0tDQo+IERtaXRy
eQ0K

2017-07-20 17:28:40

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

On Thu, Jul 20, 2017 at 10:16 AM, Brian Norris <[email protected]> w=
rote:
> Hi,
>
> On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote:
>> Hi Brian,
>>
>> > -----Original Message-----
>> > From: Brian Norris [mailto:[email protected]]
>> > Sent: 2017=E5=B9=B47=E6=9C=8820=E6=97=A5 4:10
>> > To: Xinming Hu
>> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; [email protected]; Z=
hiyuan
>> > Yang; Tim Song; Cathy Luo; Xinming Hu
>> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module para=
meter
>> >
>> > External Email
>> >
>> > ----------------------------------------------------------------------
>> > On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote:
>> > > From: Xinming Hu <[email protected]>
>> > >
>> > > Add module parameter to control tdls uapsd support, which is default
>> > > disabled.
>> >
>> > Why default disabled, when it looks like it used to be on by default?
>>
>> Oho, I should comment more in description, it is now confusing.
>> We just fixed an tdls uapsd issue in latest firmware, so try to disable
>> this feature as a workaround to the old firmware. At the same time,
>> it is optional to enable this feature in specific case.
>
> That helps a bit, thanks.
>
>> I will add more comments in V2.
>> Please let us know if you have more comments.
>
> I won't insist on changes, but for something like this, it seems
> reasonable (if you have really fixed the issue in the latest firmware)
> to just provide the knob to disable as a backup, not as a default. If
> someone is going to update their kernel (to include this patch), but not
> update their firmware, then they probably should know enough to be able
> to provide the module parameter to disable.
>
> Or alternatively: how is anyone supposed to know whether their current
> firmware is broken or not? And how is this feature ever going to be
> default-enabled again? New chipsets with new firmware should hopefully
> not have the same bugs, no?

Better yet, the driver could check the firmware version, and if it is
known bad disable the feature automatically. Then there is no need to
provide this knob.

Thanks.

--=20
Dmitry

2017-07-19 20:09:38

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote:
> From: Xinming Hu <[email protected]>
>
> Add module parameter to control tdls uapsd support, which is
> default disabled.

Why default disabled, when it looks like it used to be on by default?

2017-07-20 17:16:42

by Brian Norris

[permalink] [raw]
Subject: Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

Hi,

On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote:
> Hi Brian,
>
> > -----Original Message-----
> > From: Brian Norris [mailto:[email protected]]
> > Sent: 2017年7月20日 4:10
> > To: Xinming Hu
> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; [email protected]; Zhiyuan
> > Yang; Tim Song; Cathy Luo; Xinming Hu
> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote:
> > > From: Xinming Hu <[email protected]>
> > >
> > > Add module parameter to control tdls uapsd support, which is default
> > > disabled.
> >
> > Why default disabled, when it looks like it used to be on by default?
>
> Oho, I should comment more in description, it is now confusing.
> We just fixed an tdls uapsd issue in latest firmware, so try to disable
> this feature as a workaround to the old firmware. At the same time,
> it is optional to enable this feature in specific case.

That helps a bit, thanks.

> I will add more comments in V2.
> Please let us know if you have more comments.

I won't insist on changes, but for something like this, it seems
reasonable (if you have really fixed the issue in the latest firmware)
to just provide the knob to disable as a backup, not as a default. If
someone is going to update their kernel (to include this patch), but not
update their firmware, then they probably should know enough to be able
to provide the module parameter to disable.

Or alternatively: how is anyone supposed to know whether their current
firmware is broken or not? And how is this feature ever going to be
default-enabled again? New chipsets with new firmware should hopefully
not have the same bugs, no?

Brian

2017-07-20 10:54:22

by Xinming Hu

[permalink] [raw]
Subject: Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

SGkgQnJpYW4sDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQnJpYW4g
Tm9ycmlzIFttYWlsdG86YnJpYW5ub3JyaXNAY2hyb21pdW0ub3JnXQ0KPiBTZW50OiAyMDE3xOo3
1MIyMMjVIDQ6MTANCj4gVG86IFhpbm1pbmcgSHUNCj4gQ2M6IExpbnV4IFdpcmVsZXNzOyBLYWxs
ZSBWYWxvOyBEbWl0cnkgVG9yb2tob3Y7IHJhamF0amFAZ29vZ2xlLmNvbTsgWmhpeXVhbg0KPiBZ
YW5nOyBUaW0gU29uZzsgQ2F0aHkgTHVvOyBYaW5taW5nIEh1DQo+IFN1YmplY3Q6IFtFWFRdIFJl
OiBbUEFUQ0hdIG13aWZpZXg6IGFkZCB0ZGxzIHVhcHNkIHN1cHBvcnQgbW9kdWxlIHBhcmFtZXRl
cg0KPiANCj4gRXh0ZXJuYWwgRW1haWwNCj4gDQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gT24gV2VkLCBK
dWwgMTksIDIwMTcgYXQgMDY6MzY6MjdBTSArMDAwMCwgWGlubWluZyBIdSB3cm90ZToNCj4gPiBG
cm9tOiBYaW5taW5nIEh1IDxodXhtQG1hcnZlbGwuY29tPg0KPiA+DQo+ID4gQWRkIG1vZHVsZSBw
YXJhbWV0ZXIgdG8gY29udHJvbCB0ZGxzIHVhcHNkIHN1cHBvcnQsIHdoaWNoIGlzIGRlZmF1bHQN
Cj4gPiBkaXNhYmxlZC4NCj4gDQo+IFdoeSBkZWZhdWx0IGRpc2FibGVkLCB3aGVuIGl0IGxvb2tz
IGxpa2UgaXQgdXNlZCB0byBiZSBvbiBieSBkZWZhdWx0Pw0KDQpPaG8sIEkgc2hvdWxkIGNvbW1l
bnQgbW9yZSBpbiBkZXNjcmlwdGlvbiwgaXQgaXMgbm93IGNvbmZ1c2luZy4NCldlIGp1c3QgZml4
ZWQgYW4gdGRscyB1YXBzZCBpc3N1ZSBpbiBsYXRlc3QgZmlybXdhcmUsIHNvIHRyeSB0byBkaXNh
YmxlDQp0aGlzIGZlYXR1cmUgYXMgYSB3b3JrYXJvdW5kIHRvIHRoZSBvbGQgZmlybXdhcmUuIEF0
IHRoZSBzYW1lIHRpbWUsIA0KaXQgaXMgb3B0aW9uYWwgdG8gZW5hYmxlIHRoaXMgZmVhdHVyZSBp
biBzcGVjaWZpYyBjYXNlLg0KDQpJIHdpbGwgYWRkIG1vcmUgY29tbWVudHMgaW4gVjIuDQpQbGVh
c2UgbGV0IHVzIGtub3cgaWYgeW91IGhhdmUgbW9yZSBjb21tZW50cy4NCg0KUmVnYXJkcywNClNp
bW9uDQo=