[PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Stanislaw Gruszka]

Add BSS_CHANGE_QOS and bss_conf->qos_disabled flags to pass to drivers
when want to disable QoS (aka WMM, WME).

When AP do not provide QoS parameters, we should assume that it not
supporting QoS and we should not send QoS frames to it.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 include/net/mac80211.h |    4 ++++
 net/mac80211/mlme.c    |    6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 45d7d44..0315ffc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -144,6 +144,7 @@ struct ieee80211_low_level_stats {
  *	new beacon (beaconing modes)
  * @BSS_CHANGED_BEACON_ENABLED: Beaconing should be
  *	enabled/disabled (beaconing modes)
+ * @BSS_CHANGED_QOS: QoS should be enabled/disabled
  */
 enum ieee80211_bss_change {
 	BSS_CHANGED_ASSOC		= 1<<0,
@@ -156,6 +157,7 @@ enum ieee80211_bss_change {
 	BSS_CHANGED_BSSID		= 1<<7,
 	BSS_CHANGED_BEACON		= 1<<8,
 	BSS_CHANGED_BEACON_ENABLED	= 1<<9,
+	BSS_CHANGED_QOS			= 1<<10,
 };
 
 /**
@@ -183,6 +185,7 @@ enum ieee80211_bss_change {
  *	the current band.
  * @bssid: The BSSID for this BSS
  * @enable_beacon: whether beaconing should be enabled or not
+ * @qos_disabled: whether QoS (aka WMM) should be disabled or not
  * @ht_operation_mode: HT operation mode (like in &struct ieee80211_ht_info).
  *	This field is only valid when the channel type is one of the HT types.
  */
@@ -196,6 +199,7 @@ struct ieee80211_bss_conf {
 	bool use_short_preamble;
 	bool use_short_slot;
 	bool enable_beacon;
+	bool qos_disabled;
 	u8 dtim_period;
 	u16 beacon_int;
 	u16 assoc_capability;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index be5f723..8bd2b05 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1129,8 +1129,10 @@ static bool ieee80211_assoc_success(struct ieee80211_work *wk,
 	if (elems.wmm_param)
 		ieee80211_sta_wmm_params(local, ifmgd, elems.wmm_param,
 					 elems.wmm_param_len);
-	else
-		ieee80211_set_wmm_default(sdata);
+	else {
+		changed |= BSS_CHANGED_QOS;
+		bss_conf->qos_disabled = true;
+	}
 
 	local->oper_channel = wk->chan;
 
-- 
1.6.6

[PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/iwlwifi/iwl-core.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 112149e..f1729ca 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -2410,6 +2410,15 @@ void iwl_bss_info_changed(struct ieee80211_hw *hw,
 		priv->ibss_beacon = ieee80211_beacon_get(hw, vif);
 	}
 
+	if (changes & BSS_CHANGED_QOS) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
+		iwl_activate_qos(priv, 1);
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+
 	if (changes & BSS_CHANGED_BEACON_INT) {
 		priv->beacon_int = bss_conf->beacon_int;
 		/* TODO: in AP mode, do something to make this take effect */
-- 
1.6.6

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Luis R. Rodriguez]

On Tue, Mar 23, 2010 at 5:46 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-core.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
> index 112149e..f1729ca 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-core.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-core.c
> @@ -2410,6 +2410,15 @@ void iwl_bss_info_changed(struct ieee80211_hw *hw,
>                priv->ibss_beacon = ieee80211_beacon_get(hw, vif);
>        }
>
> +       if (changes & BSS_CHANGED_QOS) {
> +               unsigned long flags;
> +
> +               spin_lock_irqsave(&priv->lock, flags);
> +               priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
> +               iwl_activate_qos(priv, 1);
> +               spin_unlock_irqrestore(&priv->lock, flags);
> +       }
> +
>        if (changes & BSS_CHANGED_BEACON_INT) {
>                priv->beacon_int = bss_conf->beacon_int;
>                /* TODO: in AP mode, do something to make this take effect */

What happened without this BTW, did it not associate? What AP was it?
Did you get to test with other drivers by chance?

  Luis

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Johannes Berg]

On Tue, 2010-03-23 at 09:18 -0700, Luis R. Rodriguez wrote:

> >        if (changes & BSS_CHANGED_BEACON_INT) {
> >                priv->beacon_int = bss_conf->beacon_int;
> >                /* TODO: in AP mode, do something to make this take effect */
> 
> What happened without this BTW, did it not associate? What AP was it?
> Did you get to test with other drivers by chance?

The thing is that the microcode manages the powersaving, and generates
qos-nullfunc frames which were not understood by some APs.

I'll take another look at the patches asap, but right now I have to do
some planning.

johannes

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Stanislaw Gruszka]

On Tue, Mar 23, 2010 at 09:18:33AM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 23, 2010 at 5:46 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  drivers/net/wireless/iwlwifi/iwl-core.c |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
> > index 112149e..f1729ca 100644
> > --- a/drivers/net/wireless/iwlwifi/iwl-core.c
> > +++ b/drivers/net/wireless/iwlwifi/iwl-core.c
> > @@ -2410,6 +2410,15 @@ void iwl_bss_info_changed(struct ieee80211_hw *hw,
> >                priv->ibss_beacon = ieee80211_beacon_get(hw, vif);
> >        }
> >
> > +       if (changes & BSS_CHANGED_QOS) {
> > +               unsigned long flags;
> > +
> > +               spin_lock_irqsave(&priv->lock, flags);
> > +               priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
> > +               iwl_activate_qos(priv, 1);
> > +               spin_unlock_irqrestore(&priv->lock, flags);
> > +       }
> > +
> >        if (changes & BSS_CHANGED_BEACON_INT) {
> >                priv->beacon_int = bss_conf->beacon_int;
> >                /* TODO: in AP mode, do something to make this take effect */
> 
> What happened without this BTW, did it not associate?
Associate, but after a short time, when any QoS frame is sent,
AP disassociate with MIC error.

> What AP was it?
Sitecom 161.

> Did you get to test with other drivers by chance?
rt73usb work well with that AP without any patches. I have also
rt2800usb, but it not work with any  AP's I have :(

Stanislaw

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Johannes Berg]

On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>

This really could use "more" text as to why etc.

> +	if (changes & BSS_CHANGED_QOS) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +		priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
> +		iwl_activate_qos(priv, 1);
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +	}

This seems alright, but we really need to revisit that since there's a
lot of odd logic in iwlwifi that makes some sense based on this now but
could probably be simplified now. I'll close with Wey-Yi, she worked on
QoS here at some point I think.

johannes

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Guy, Wey-Yi]

Hi Stanislaw,
 
On Tue, 2010-03-23 at 10:21 -0700, Johannes Berg wrote:
> On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> 
> This really could use "more" text as to why etc.
> 
> > +	if (changes & BSS_CHANGED_QOS) {
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&priv->lock, flags);
> > +		priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
> > +		iwl_activate_qos(priv, 1);
> > +		spin_unlock_irqrestore(&priv->lock, flags);
> > +	}
> 
> This seems alright, but we really need to revisit that since there's a
> lot of odd logic in iwlwifi that makes some sense based on this now but
> could probably be simplified now. I'll close with Wey-Yi, she worked on
> QoS here at some point I think.
> 
If I understand correctly, the current implementation has problem, by
just calling iwl_activate_qos(), it did not have the AC setup correctly,
take a look at the iwl_reset_qos(), it is the only place initialize the
table, for STA mode, QoS is de-active by default. iwl_activate_qos()
only change the qos flag, but did not re-initialize the table to proper
value. 

Thanks
Wey

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iwlwifi: implement disabling/enabling QoS

[Guy, Wey-Yi]

Hi Stanislaw,

On Wed, 2010-03-24 at 09:44 -0700, Guy, Wey-Yi wrote:
> Hi Stanislaw,
>  
> On Tue, 2010-03-23 at 10:21 -0700, Johannes Berg wrote:
> > On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > 
> > This really could use "more" text as to why etc.
> > 
> > > +	if (changes & BSS_CHANGED_QOS) {
> > > +		unsigned long flags;
> > > +
> > > +		spin_lock_irqsave(&priv->lock, flags);
> > > +		priv->qos_data.qos_active = bss_conf->qos_disabled ? 0 : 1;
> > > +		iwl_activate_qos(priv, 1);
> > > +		spin_unlock_irqrestore(&priv->lock, flags);
> > > +	}
> > 
> > This seems alright, but we really need to revisit that since there's a
> > lot of odd logic in iwlwifi that makes some sense based on this now but
> > could probably be simplified now. I'll close with Wey-Yi, she worked on
> > QoS here at some point I think.
> > 
> If I understand correctly, the current implementation has problem, by
> just calling iwl_activate_qos(), it did not have the AC setup correctly,
> take a look at the iwl_reset_qos(), it is the only place initialize the
> table, for STA mode, QoS is de-active by default. iwl_activate_qos()
> only change the qos flag, but did not re-initialize the table to proper
> value. 
> 
> Thanks
> Wey
> 
look at it again, please ignore my previous email, mac80211 will send
config_tx which make the ac table correct, so just call
iwl_activate_qos() should be ok.

Thanks
Wey

> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Johannes Berg]

On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:

> + * @qos_disabled: whether QoS (aka WMM) should be disabled or not

Why "disabled" btw, and not enabled? Everything else is usually done in
a positive sense here, I'd say.

Also, the default would now be enabled, and I think this needs more work
for AP mode. You're implementing generic API, don't fall into the trap
of thinking it's only for iwlwifi's special case. So at least there it
should be enabled if the hw supports it and the change flag should be
set at least once.

Also, your code sends a weird signal to hw that doesn't support QoS, it
actually enables qos for it ...

johannes

Re: [PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Johannes Berg]

On Tue, 2010-03-23 at 10:19 -0700, Johannes Berg wrote:
> On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:
> 
> > + * @qos_disabled: whether QoS (aka WMM) should be disabled or not
> 
> Why "disabled" btw, and not enabled? Everything else is usually done in
> a positive sense here, I'd say.
> 
> Also, the default would now be enabled, and I think this needs more work
> for AP mode. You're implementing generic API, don't fall into the trap
> of thinking it's only for iwlwifi's special case. So at least there it
> should be enabled if the hw supports it and the change flag should be
> set at least once.
> 
> Also, your code sends a weird signal to hw that doesn't support QoS, it
> actually enables qos for it ...

Also, you probably should set the flag again and set qos_disabled = true
(or qos_enabled = false, which would imho be better) when
disassociating.

johannes

Re: [PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Stanislaw Gruszka]

On Tue, Mar 23, 2010 at 10:19:55AM -0700, Johannes Berg wrote:
> On Tue, 2010-03-23 at 13:46 +0100, Stanislaw Gruszka wrote:
> 
> > + * @qos_disabled: whether QoS (aka WMM) should be disabled or not
> 
> Why "disabled" btw, and not enabled? Everything else is usually done in
> a positive sense here, I'd say.

To emphasize we disable QoS as conf_tx enable it implicitly.

> Also, the default would now be enabled, and I think this needs more work
> for AP mode. You're implementing generic API, don't fall into the trap
> of thinking it's only for iwlwifi's special case.

I did not think this is iwlwifi only case, but only for iwlwifi I have
bug reports as far.

> So at least there it
> should be enabled if the hw supports it and the change flag should be
> set at least once.

It must be enabled conf_tx is called anyway, so I do not think that is
needed. Ok, flag that device can handle bss_info_changed(BSS_CHANGEDS_QOS)
but that all is overcomplicated for me.

> Also, your code sends a weird signal to hw that doesn't support QoS, it
> actually enables qos for it ...

Not really. It's only used to disable QoS.

Well, my first patch which add new disable_qos callback, was like that.
If device do not have QoS, driver do not implement callback - wasn't
that simple and clean design?

In ideal case we want to change conf_tx to configure all queues
or disable qos. But since this is exported  to userspace additional 
disable_qos callback is the best implementation for me.

Stanislaw

Re: [PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Johannes Berg]

On Wed, 2010-03-24 at 10:37 +0100, Stanislaw Gruszka wrote:

> > Why "disabled" btw, and not enabled? Everything else is usually done in
> > a positive sense here, I'd say.
> 
> To emphasize we disable QoS as conf_tx enable it implicitly.

Which I actually think should be changed in iwlwifi. No other driver
cares, and this has always been a bit of a mess there (in iwlwifi).

> In ideal case we want to change conf_tx to configure all queues
> or disable qos. But since this is exported  to userspace additional 
> disable_qos callback is the best implementation for me.

Yes, that would be ideal, but the second best thing, and one that we can
do, would be to decouple configuring QoS and enabling/disabling QoS
completely, not do a jumbled-up implementation like you're suggesting?

johannes

Re: [PATCH 1/2] mac80211: add interface for disabling/enabling QoS

[Stanislaw Gruszka]

On Wed, Mar 24, 2010 at 09:13:53AM -0700, Johannes Berg wrote:
> On Wed, 2010-03-24 at 10:37 +0100, Stanislaw Gruszka wrote:
> 
> > > Why "disabled" btw, and not enabled? Everything else is usually done in
> > > a positive sense here, I'd say.
> > 
> > To emphasize we disable QoS as conf_tx enable it implicitly.
> 
> Which I actually think should be changed in iwlwifi. No other driver
> cares, and this has always been a bit of a mess there (in iwlwifi).

Hmm, for example in mwl8k have mwl8k_cmd_set_wmm_mode(1)in conf_tx()
as well. I think this is hardware/firmware requirement.

> > In ideal case we want to change conf_tx to configure all queues
> > or disable qos. But since this is exported  to userspace additional 
> > disable_qos callback is the best implementation for me.
> 
> Yes, that would be ideal, but the second best thing, and one that we can
> do, would be to decouple configuring QoS and enabling/disabling QoS
> completely, not do a jumbled-up implementation like you're suggesting?

Since conf_tx enable QoS, and must do it, we need only interface to 
disable QoS. Decoupling just give us only more code, but since you
require that, I will do it. If this must be called at initialization
ieee80211_ops->config is better for that, I will use new
ieee80211_conf->flags.

Stanislaw