diff mbox

mac80211: SMPS: Fix the overwrite of rx_chains in the interface loop.

Message ID 1388870653-3507-1-git-send-email-chaitanya.mgit@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Krishna Chaitanya Jan. 4, 2014, 9:24 p.m. UTC
The interface loop identifies the no of static and dynamic
smps chains, but overwrites their values across the VIF's
(assuming there are more than one, in my case its AP and STA
loopback), so the driver might not intimated about this update
of SMPS mode, is the value is same. 

So inform driver for every VIF.

For Ex: STA's SMPS state is changed, but in the loop AP appears
at the last, SMPS state of AP is unchanged hence not intimated
to the driver.

Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
---

 net/mac80211/chan.c |   34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

--
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

Comments

Emmanuel Grumbach Jan. 5, 2014, 6:42 a.m. UTC | #1
On Sat, Jan 4, 2014 at 11:24 PM, Chaitanya T K <chaitanya.mgit@gmail.com> wrote:
> The interface loop identifies the no of static and dynamic
> smps chains, but overwrites their values across the VIF's
> (assuming there are more than one, in my case its AP and STA
> loopback), so the driver might not intimated about this update
> of SMPS mode, is the value is same.
>
> So inform driver for every VIF.
>
> For Ex: STA's SMPS state is changed, but in the loop AP appears
> at the last, SMPS state of AP is unchanged hence not intimated
> to the driver.
>
> Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
> ---

But the dynamic / static chains number is a property of the channel
context. If you have several vifs on the same channel context, the
maximum number of dynamic / static chains will be assigned to the
channel context. In short, I think that mac80211 already does the job
of taking all the parameters into account and putting the right value
into the channel context?
I don't really see what bug you are trying to solve - could you please
tell us exactly what is your configuration?
What SMPS do you have for each interface and what dynamic / static
chains settings does mac80211 end up with?
--
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
Krishna Chaitanya Jan. 5, 2014, 7:21 a.m. UTC | #2
On Sun, Jan 5, 2014 at 12:12 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>
> On Sat, Jan 4, 2014 at 11:24 PM, Chaitanya T K <chaitanya.mgit@gmail.com> wrote:
> > The interface loop identifies the no of static and dynamic
> > smps chains, but overwrites their values across the VIF's
> > (assuming there are more than one, in my case its AP and STA
> > loopback), so the driver might not intimated about this update
> > of SMPS mode, is the value is same.
> >
> > So inform driver for every VIF.
> >
> > For Ex: STA's SMPS state is changed, but in the loop AP appears
> > at the last, SMPS state of AP is unchanged hence not intimated
> > to the driver.
> >
> > Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
> > ---
>
> But the dynamic / static chains number is a property of the channel
> context. If you have several vifs on the same channel context, the
> maximum number of dynamic / static chains will be assigned to the
> channel context. In short, I think that mac80211 already does the job
> of taking all the parameters into account and putting the right value
> into the channel context?
> I don't really see what bug you are trying to solve - could you please
> tell us exactly what is your configuration?
> What SMPS do you have for each interface and what dynamic / static
> chains settings does mac80211 end up with?

Agree this looks good in channel context angle.

Lets say we have two VIF's one AP and one STA and the driver doesn't
support channel context's so in that case the config will happen
through hw_config but the problem is hw_config is out side the loop
so the config will happen with the parameters of the last VIF, which
is incorrect.
--
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
Emmanuel Grumbach Jan. 5, 2014, 7:33 a.m. UTC | #3
Emmanuel Grumbach
egrumbach@gmail.com


On Sun, Jan 5, 2014 at 9:21 AM, Krishna Chaitanya
<chaitanya.mgit@gmail.com> wrote:
> On Sun, Jan 5, 2014 at 12:12 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>
>> On Sat, Jan 4, 2014 at 11:24 PM, Chaitanya T K <chaitanya.mgit@gmail.com> wrote:
>> > The interface loop identifies the no of static and dynamic
>> > smps chains, but overwrites their values across the VIF's
>> > (assuming there are more than one, in my case its AP and STA
>> > loopback), so the driver might not intimated about this update
>> > of SMPS mode, is the value is same.
>> >
>> > So inform driver for every VIF.
>> >
>> > For Ex: STA's SMPS state is changed, but in the loop AP appears
>> > at the last, SMPS state of AP is unchanged hence not intimated
>> > to the driver.
>> >
>> > Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
>> > ---
>>
>> But the dynamic / static chains number is a property of the channel
>> context. If you have several vifs on the same channel context, the
>> maximum number of dynamic / static chains will be assigned to the
>> channel context. In short, I think that mac80211 already does the job
>> of taking all the parameters into account and putting the right value
>> into the channel context?
>> I don't really see what bug you are trying to solve - could you please
>> tell us exactly what is your configuration?
>> What SMPS do you have for each interface and what dynamic / static
>> chains settings does mac80211 end up with?
>
> Agree this looks good in channel context angle.
>
> Lets say we have two VIF's one AP and one STA and the driver doesn't
> support channel context's so in that case the config will happen
> through hw_config but the problem is hw_config is out side the loop
> so the config will happen with the parameters of the last VIF, which
> is incorrect.

Same applies if the driver doesn't support channel contexts. hw_config
will be called with the correct local->smps_mode based on the number
of channels required by each one of the vifs?
I don't see why only the last vif will be taken into account: the loop
computes the maximum number of antenna requires by *all* the vifs.
What am I missing here?
--
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
Emmanuel Grumbach Jan. 5, 2014, 7:37 a.m. UTC | #4
> On Sun, Jan 5, 2014 at 9:21 AM, Krishna Chaitanya
> <chaitanya.mgit@gmail.com> wrote:
>> On Sun, Jan 5, 2014 at 12:12 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>>
>>> On Sat, Jan 4, 2014 at 11:24 PM, Chaitanya T K <chaitanya.mgit@gmail.com> wrote:
>>> > The interface loop identifies the no of static and dynamic
>>> > smps chains, but overwrites their values across the VIF's
>>> > (assuming there are more than one, in my case its AP and STA
>>> > loopback), so the driver might not intimated about this update
>>> > of SMPS mode, is the value is same.
>>> >
>>> > So inform driver for every VIF.
>>> >
>>> > For Ex: STA's SMPS state is changed, but in the loop AP appears
>>> > at the last, SMPS state of AP is unchanged hence not intimated
>>> > to the driver.
>>> >
>>> > Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
>>> > ---
>>>
>>> But the dynamic / static chains number is a property of the channel
>>> context. If you have several vifs on the same channel context, the
>>> maximum number of dynamic / static chains will be assigned to the
>>> channel context. In short, I think that mac80211 already does the job
>>> of taking all the parameters into account and putting the right value
>>> into the channel context?
>>> I don't really see what bug you are trying to solve - could you please
>>> tell us exactly what is your configuration?
>>> What SMPS do you have for each interface and what dynamic / static
>>> chains settings does mac80211 end up with?
>>
>> Agree this looks good in channel context angle.
>>
>> Lets say we have two VIF's one AP and one STA and the driver doesn't
>> support channel context's so in that case the config will happen
>> through hw_config but the problem is hw_config is out side the loop
>> so the config will happen with the parameters of the last VIF, which
>> is incorrect.
>
> Same applies if the driver doesn't support channel contexts. hw_config
> will be called with the correct local->smps_mode based on the number
> of channels required by each one of the vifs?

s/channels/chains/

> I don't see why only the last vif will be taken into account: the loop
> computes the maximum number of antenna requires by *all* the vifs.
> What am I missing here?
--
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
Krishna Chaitanya Jan. 5, 2014, 7:43 a.m. UTC | #5
>> Same applies if the driver doesn't support channel contexts. hw_config
>> will be called with the correct local->smps_mode based on the number
>> of channels required by each one of the vifs?
>> I don't see why only the last vif will be taken into account: the loop
>> computes the maximum number of antenna requires by *all* the vifs.
>> What am I missing here?

Got it. Ours was a special loopback case where 2 VIF's talk to each other
(one AP and one STA), so we had a problem there.

Please ignore this patch. Thanks Emmanuel.
--
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
diff mbox

Patch

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f43613a..e153a7e 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -483,26 +483,26 @@  void ieee80211_recalc_smps_chanctx(struct ieee80211_local *local,
 
 		rx_chains_static = max(rx_chains_static, needed_static);
 		rx_chains_dynamic = max(rx_chains_dynamic, needed_dynamic);
-	}
-	rcu_read_unlock();
+		if (!local->use_chanctx) {
+				if (rx_chains_static > 1)
+					local->smps_mode = IEEE80211_SMPS_OFF;
+				else if (rx_chains_dynamic > 1)
+					local->smps_mode = IEEE80211_SMPS_DYNAMIC;
+				else
+					local->smps_mode = IEEE80211_SMPS_STATIC;
+						ieee80211_hw_config(local, 0);
+		}
 
-	if (!local->use_chanctx) {
-		if (rx_chains_static > 1)
-			local->smps_mode = IEEE80211_SMPS_OFF;
-		else if (rx_chains_dynamic > 1)
-			local->smps_mode = IEEE80211_SMPS_DYNAMIC;
-		else
-			local->smps_mode = IEEE80211_SMPS_STATIC;
-		ieee80211_hw_config(local, 0);
-	}
+		if (rx_chains_static == chanctx->conf.rx_chains_static &&
+		rx_chains_dynamic == chanctx->conf.rx_chains_dynamic)
+				return;
 
-	if (rx_chains_static == chanctx->conf.rx_chains_static &&
-	    rx_chains_dynamic == chanctx->conf.rx_chains_dynamic)
-		return;
+		chanctx->conf.rx_chains_static = rx_chains_static;
+		chanctx->conf.rx_chains_dynamic = rx_chains_dynamic;
+		drv_change_chanctx(local, chanctx, IEEE80211_CHANCTX_CHANGE_RX_CHAINS);
+	}
+	rcu_read_unlock();
 
-	chanctx->conf.rx_chains_static = rx_chains_static;
-	chanctx->conf.rx_chains_dynamic = rx_chains_dynamic;
-	drv_change_chanctx(local, chanctx, IEEE80211_CHANCTX_CHANGE_RX_CHAINS);
 }
 
 int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,