No driver initializes chan->max_antenna_gain to something sensible, and
the only place where it is being used right now is inside ath9k. This
leads to ath9k potentially using less tx power than it can use.
Rather than going through every single driver, this patch initializes
chan->orig_mag in wiphy_register(), ignoring whatever value the driver
left in there. If a driver for some reason wishes to limit it independent
from regulatory rulesets, it can do so internally.
Signed-off-by: Felix Fietkau <[email protected]>
Cc: [email protected]
---
net/wireless/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 443d4d7..3f72530 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -526,8 +526,7 @@ int wiphy_register(struct wiphy *wiphy)
for (i = 0; i < sband->n_channels; i++) {
sband->channels[i].orig_flags =
sband->channels[i].flags;
- sband->channels[i].orig_mag =
- sband->channels[i].max_antenna_gain;
+ sband->channels[i].orig_mag = INT_MAX;
sband->channels[i].orig_mpwr =
sband->channels[i].max_power;
sband->channels[i].band = band;
--
1.7.9.6 (Apple Git-31.1)
On Sat, Oct 06, 2012 at 02:40:54PM +0200, Felix Fietkau wrote:
> A few places touch chan->max_power based on updated tx power rules, but
> forget to do the same to chan->max_reg_power.
>
> Signed-off-by: Felix Fietkau <[email protected]>
> Cc: [email protected]
> ---
> net/wireless/reg.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 3b8cbbc..bcc7d7e 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -908,7 +908,7 @@ static void handle_channel(struct wiphy *wiphy,
> map_regdom_flags(reg_rule->flags) | bw_flags;
> chan->max_antenna_gain = chan->orig_mag =
> (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> - chan->max_power = chan->orig_mpwr =
> + chan->max_reg_power = chan->max_power = chan->orig_mpwr =
> (int) MBM_TO_DBM(power_rule->max_eirp);
> return;
> }
> @@ -1331,7 +1331,8 @@ static void handle_channel_custom(struct wiphy *wiphy,
>
> chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
> chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> - chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
> + chan->max_reg_power = chan->max_power =
> + (int) MBM_TO_DBM(power_rule->max_eirp);
This looks good to me, good catch! The commit log could use some love,
given that this is a stable patch it is worthy to describe the
consequences of not applying this patch. Can you describe what you
observed that makes this a critical patch? The only piece of code
that uses max_reg_power in cfg80211, mac80211 or drivers is on
net/wireless/reg.c and drivers/net/wireless/mwifiex/cfg80211.c.
In either case the issue the code you are patching is for code
that deals with drivers that have a custom regulatory domain
in which orig_mpwr would have been initialized to a non-zero value
upon registration. In such cases we could only potenially run into
an issue on this piece of code on handle_channel():
chan->max_reg_power = (int) MBM_TO_DBM(power_rule->max_eirp);
if (chan->orig_mpwr) {
/*
* Devices that have their own custom regulatory domain
* but also use WIPHY_FLAG_STRICT_REGULATORY will follow the
* passed country IE power settings.
*/
if (initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE &&
wiphy->flags & WIPHY_FLAG_CUSTOM_REGULATORY &&
wiphy->flags & WIPHY_FLAG_STRICT_REGULATORY)
chan->max_power = chan->max_reg_power;
else
chan->max_power = min(chan->orig_mpwr,
chan->max_reg_power);
} else
chan->max_power = chan->max_reg_power;
The issue would happen if orig_mpwr is non zero (custom) and
then max_reg_power would not have been initialized. This runs
when you change a regulatory domain on a card with a custom
regulatory domain and this would be an issue if max_reg_power
would not be initialized. This however does not happen due to
the first line above.
So I agree with this patch but do not see the requirement for
it to go in as a stable fix to older stable kernels.
Luis
On 2012-10-09 3:00 AM, Luis R. Rodriguez wrote:
> On Mon, Oct 08, 2012 at 10:52:50PM +0200, Felix Fietkau wrote:
>> On 2012-10-08 8:37 PM, Luis R. Rodriguez wrote:
>> > On Sat, Oct 06, 2012 at 02:40:53PM +0200, Felix Fietkau wrote:
>> >> No driver initializes chan->max_antenna_gain to something sensible, and
>> >> the only place where it is being used right now is inside ath9k. This
>> >> leads to ath9k potentially using less tx power than it can use.
>> >>
>> >> Rather than going through every single driver, this patch initializes
>> >> chan->orig_mag in wiphy_register(), ignoring whatever value the driver
>> >> left in there. If a driver for some reason wishes to limit it independent
>> >> from regulatory rulesets, it can do so internally.
>> >>
>> >> Signed-off-by: Felix Fietkau <[email protected]>
>> >> Cc: [email protected]
>> >> ---
>> >> net/wireless/core.c | 3 +--
>> >> 1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/net/wireless/core.c b/net/wireless/core.c
>> >> index 443d4d7..3f72530 100644
>> >> --- a/net/wireless/core.c
>> >> +++ b/net/wireless/core.c
>> >> @@ -526,8 +526,7 @@ int wiphy_register(struct wiphy *wiphy)
>> >> for (i = 0; i < sband->n_channels; i++) {
>> >> sband->channels[i].orig_flags =
>> >> sband->channels[i].flags;
>> >> - sband->channels[i].orig_mag =
>> >> - sband->channels[i].max_antenna_gain;
>> >> + sband->channels[i].orig_mag = INT_MAX;
>> >
>> > The commit log for a stable patch should describe the impact of not applying
>> > it. In this case perhaps not being able to associate to APs due to unnecessary
>> > power limitations -- but again that itself would only be true if some other
>> > code used it in a way that it should not. I rather fix that if such cases
>> > exist.
>>
>> I did mention that not applying this patch can in some circumstances
>> lead to using lower tx power. I think it's obvious that this impairs
>> performance/range.
>
> It may be obvious to you not to the applier of the stable patch. You can
> help by making it a bit more obvious to the reader what practical issues
> users would run into without this patch.
OK, I'll add this bit to the patch description.
>> > Now the purpose of orig_mag is to allow drivers to spit out to kernel and
>> > eventually userspace if we want it what it knows about the value of the
>> > embedded antenna gain. The regulatory code for antenna gain is useful for us in
>> > that it is the maximum allowed antenna gain values.
>>
>> If that was the purpose, then the implementation completely fails at
>> treating it as such.
>
> Indeed, we never really had a way to deal with this or a generic
> interface or way for devices to expose this information to the OS.
> Around March I had looked into ways in which we might be able
> to create a standarized mechanism to enable any device to expose
> its antenna gain to an OS. I only had poked a few folks but it
> seems like if we wanted to we may be able to try to propose exposing
> antenna gain via EFI. This would enable antenna gain to be exposed
> on ARM, x86 givne that both archs will end up using it.
>
> I never followed up but we should see if we can get this but I'll
> use this as a prelude to work on this.
Well, the regulatory related variable seem like a pretty bad place to
store the embedded antenna gain. Some drivers even share the
ieee80211_channel array between multiple instances...
>> Remember, that 'mag' stands for *maximum* antenna
>> gain, not effective antenna gain or something. It is indeed treated as a
>> regulatory maximum,
>
> The antenna gain nomenclature came from the HAL but that's just baggage. The
> antenna gain stuff remained a puzzle for a central regulatory code base due to
> the fact that as far as I am concerned only ath9k was exposing the antenna gain
> value from EEPROM and using it.
No, ath9k was (and still is) not filling anything EEPROM related into
chan->max_antenna_gain. And that's a good thing, because putting the
EEPROM value in there would have made the whole thing even more wrong
and confused than it already is.
> Other drivers would just use it as a max
> regulatory value, but as you can imagine code typically always just keeps this
> as a static value for all regulatory domains even though technically it is not,
> this includes the HAL. A lot of this is due to the fact that you are not
> supposed to be mucking with the antenna gain unless you are making APs and
> if you are then that would need to be adjusted on the EEPROM anyway and
> manufacturers have access to that. So in the end you're right but in
> perspective its baggage and in the long term we should strive to generalize
> an interface to expose the value to the OS if possible. In the meantime
> I suspect your future work may help here get ahead of any other OS by
> allowing us to treat dynamic antenna gains within the core of the kernel.
> That is, perhaps we don't have a way to query the gain from an existing
> card but at the very least the OS can enable us to report to the kernel
> if a system integrator / engineer / researcher did modify the antenna gain
> somehow to help adjust regulatory appropriately. So even if the old values
> are not known at least the new values would be (with the CONFIG_ONUS option).
It's hard for me to figure out what you're trying to say here. What
other drivers are you talking about? If you mean other Atheros drivers,
they use the EEPROM value in pretty much the same way as I described. If
you're talking about other mac80211 drivers, they do not use anything
related to chan->max_antenna_gain. ath9k is the only driver that makes
any use of this at the moment.
I do think it's a good idea to expose more things from the system, e.g.
what the effective antenna gain is. But for that it's important to stop
overloading variables or abusing variables intended for a completely
different purpose, because that's a big part of what made the current
regulatory code so screwed up and confusing. Just look at how tx power
is handled - it's a total mess.
>> and since only driver code uses the
>> regulatory-capped maximum antenna gain, it does not make sense to allow
>> the driver to set anything here initially.
>
> Well its up to us to do what we want with it. I do wish we had a generic
> interface to export antenna gains but we don't yet. Until then we can
> only rely on the driver to do this.
>
>> > This code itself hasn't been widely used throughout code but as you point out
>> > ath9k does use it given that we can populate the antenna gain with what we read
>> > from EEPROM and that in fact should *not* be reducing the max power from ath9k
>> > but instead *helping* it in case it is smaller than the max allowed.
>>
>> I think you're confusing what the EEPROM says vs. what orig_mag really
>> means. orig_mag means maximum allowable antenna gain at max. tx power
>> that is still compliant.
>
> Sure, but in the case of ath9k its also the programmed value on the EEPROM
> and that is the antenna gain on the card.
Isn't that just repeating the same confusion that I was trying to clear
up? chan->orig_mag and the EEPROM antenna gain value refer to different
things! chan->orig_max is the maximum regulatory antenna gain, EEPROM
antenna gain is the effective antenna gain of the system. I repeat,
different things!
>> The eeprom value contains the antenna gain of
>> the current system. These are two completely different things, and
>> storing anything from the EEPROM in chan->max_antenna_gain makes no
>> sense at all.
>
> Its subjective really, but again, this is all baggage and what we *do*
> should be to lay out the ground work to enable us to take this further.
> As it is ath9k *does* set these value from the EEPROM and hence why
> I considered it odd to set this a max value.
It's not subjective, and you're wrong - ath9k does *not* set this value
from EEPROM, nor should it! Did you even look at the code? In fact, *no*
driver initializes this to anything sane! That should say something...
The fact is that chan->max_antenna_gain is used by the driver, and the
way it is used actually makes some sense (surprise!). I just think you
had some wrong ideas of what it's for.
>> > In retrospect there are a few things we can do here to improve this and use
>> > this more efficiently. The orig_mag should be used by wiphy_register() as is.
>> > Then upon wiphy_register() it calls wiphy_regulatory_register() and that
>> > handles with anything regarding regulatory on the device. How about we change
>> > this so that within wiphy_regulatory_register() (or regulatory change,
>> > handle_channel()) we review if the device's antenna gain is over the regulatory
>> > domain max allowed antenna gain and if it is we kick back simply mute the
>> > device only allowing RX. If the device is changed to a regulatory domain that
>> > allows for the antenna gain registered for the device then we unmute it. This
>> > would in turn allow doing dynamic updates of the antenna gain as it seems you
>> > want to introduce API for later, we'd end up checking for the newly supplied
>> > antenna gain (and ensure that that value is higher than the embedded on on
>> > EEPROM).
>>
>> Sorry, but that makes no sense to me, you're solving a nonexistant
>> problem. The antenna gain value from EEPROM is already handled properly.
>
> For all drivers? If so how?
ath9k is the only driver that uses chan->max_antenna_gain.
>> If the code detects that EEPROM antenna gain is n dBi over the
>> regulatory antenna gain, it reduces tx power by n dBm. Muting the card
>> in that case sounds weird and completely unnecessary to me.
>
> Doesn't only ath9k do this?
Yes.
> In either case you are not disregarding the value set for custom
> regulatory domains here.
> How about just adding a check for now:
>
> - chan->max_antenna_gain = min(chan->orig_mag,
> - (int) MBI_TO_DBI(power_rule->max_antenna_gain));
> + if (chan->orig_mag)
> + chan->max_antenna_gain =
> + min(chan->orig_mag, (int) MBI_TO_DBI(power_rule->max_antenna_gain));
> + else
> + chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> +
>
> Then we can go ahead and clarify usage of these values.
No, because 0 is a valid value too, it does not indicate 'not present'.
I don't want to add fragile checks that will not do anything useful for
*any* current driver and are not useful for future changes either.
- Felix
On Tue, Oct 9, 2012 at 2:49 AM, Felix Fietkau <[email protected]> wrote:
> On 2012-10-09 3:00 AM, Luis R. Rodriguez wrote:
>> I never followed up but we should see if we can get this but I'll
>> use this as a prelude to work on this.
>
> Well, the regulatory related variable seem like a pretty bad place to
> store the embedded antenna gain. Some drivers even share the
> ieee80211_channel array between multiple instances...
True, specially no need to have it for each channel.
>> The antenna gain nomenclature came from the HAL but that's just baggage. The
>> antenna gain stuff remained a puzzle for a central regulatory code base due to
>> the fact that as far as I am concerned only ath9k was exposing the antenna gain
>> value from EEPROM and using it.
>
> No, ath9k was (and still is) not filling anything EEPROM related into
> chan->max_antenna_gain. And that's a good thing, because putting the
> EEPROM value in there would have made the whole thing even more wrong
> and confused than it already is.
Heh, OK. Then forget my ramblings as its eroded brain cache.
>> How about just adding a check for now:
>>
>> - chan->max_antenna_gain = min(chan->orig_mag,
>> - (int) MBI_TO_DBI(power_rule->max_antenna_gain));
>> + if (chan->orig_mag)
>> + chan->max_antenna_gain =
>> + min(chan->orig_mag, (int) MBI_TO_DBI(power_rule->max_antenna_gain));
>> + else
>> + chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
>> +
>>
>> Then we can go ahead and clarify usage of these values.
>
> No, because 0 is a valid value too, it does not indicate 'not present'.
> I don't want to add fragile checks that will not do anything useful for
> *any* current driver and are not useful for future changes either.
Alright, its obvious I'm rusty with antenna gain review here and
you've already thought out some kinks, this change is fine by me then
provided we also then proceed to clarify its usage more than what we
have.
Luis
On 2012-10-08 8:37 PM, Luis R. Rodriguez wrote:
> On Sat, Oct 06, 2012 at 02:40:53PM +0200, Felix Fietkau wrote:
>> No driver initializes chan->max_antenna_gain to something sensible, and
>> the only place where it is being used right now is inside ath9k. This
>> leads to ath9k potentially using less tx power than it can use.
>>
>> Rather than going through every single driver, this patch initializes
>> chan->orig_mag in wiphy_register(), ignoring whatever value the driver
>> left in there. If a driver for some reason wishes to limit it independent
>> from regulatory rulesets, it can do so internally.
>>
>> Signed-off-by: Felix Fietkau <[email protected]>
>> Cc: [email protected]
>> ---
>> net/wireless/core.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/wireless/core.c b/net/wireless/core.c
>> index 443d4d7..3f72530 100644
>> --- a/net/wireless/core.c
>> +++ b/net/wireless/core.c
>> @@ -526,8 +526,7 @@ int wiphy_register(struct wiphy *wiphy)
>> for (i = 0; i < sband->n_channels; i++) {
>> sband->channels[i].orig_flags =
>> sband->channels[i].flags;
>> - sband->channels[i].orig_mag =
>> - sband->channels[i].max_antenna_gain;
>> + sband->channels[i].orig_mag = INT_MAX;
>
> The commit log for a stable patch should describe the impact of not applying
> it. In this case perhaps not being able to associate to APs due to unnecessary
> power limitations -- but again that itself would only be true if some other
> code used it in a way that it should not. I rather fix that if such cases
> exist.
I did mention that not applying this patch can in some circumstances
lead to using lower tx power. I think it's obvious that this impairs
performance/range.
> Now the purpose of orig_mag is to allow drivers to spit out to kernel and
> eventually userspace if we want it what it knows about the value of the
> embedded antenna gain. The regulatory code for antenna gain is useful for us in
> that it is the maximum allowed antenna gain values.
If that was the purpose, then the implementation completely fails at
treating it as such. Remember, that 'mag' stands for *maximum* antenna
gain, not effective antenna gain or something. It is indeed treated as a
regulatory maximum, and since only driver code uses the
regulatory-capped maximum antenna gain, it does not make sense to allow
the driver to set anything here initially.
> This code itself hasn't been widely used throughout code but as you point out
> ath9k does use it given that we can populate the antenna gain with what we read
> from EEPROM and that in fact should *not* be reducing the max power from ath9k
> but instead *helping* it in case it is smaller than the max allowed.
I think you're confusing what the EEPROM says vs. what orig_mag really
means. orig_mag means maximum allowable antenna gain at max. tx power
that is still compliant. The eeprom value contains the antenna gain of
the current system. These are two completely different things, and
storing anything from the EEPROM in chan->max_antenna_gain makes no
sense at all.
> In retrospect there are a few things we can do here to improve this and use
> this more efficiently. The orig_mag should be used by wiphy_register() as is.
> Then upon wiphy_register() it calls wiphy_regulatory_register() and that
> handles with anything regarding regulatory on the device. How about we change
> this so that within wiphy_regulatory_register() (or regulatory change,
> handle_channel()) we review if the device's antenna gain is over the regulatory
> domain max allowed antenna gain and if it is we kick back simply mute the
> device only allowing RX. If the device is changed to a regulatory domain that
> allows for the antenna gain registered for the device then we unmute it. This
> would in turn allow doing dynamic updates of the antenna gain as it seems you
> want to introduce API for later, we'd end up checking for the newly supplied
> antenna gain (and ensure that that value is higher than the embedded on on
> EEPROM).
Sorry, but that makes no sense to me, you're solving a nonexistant
problem. The antenna gain value from EEPROM is already handled properly.
If the code detects that EEPROM antenna gain is n dBi over the
regulatory antenna gain, it reduces tx power by n dBm. Muting the card
in that case sounds weird and completely unnecessary to me.
- Felix
On Sat, Oct 06, 2012 at 02:40:53PM +0200, Felix Fietkau wrote:
> No driver initializes chan->max_antenna_gain to something sensible, and
> the only place where it is being used right now is inside ath9k. This
> leads to ath9k potentially using less tx power than it can use.
>
> Rather than going through every single driver, this patch initializes
> chan->orig_mag in wiphy_register(), ignoring whatever value the driver
> left in there. If a driver for some reason wishes to limit it independent
> from regulatory rulesets, it can do so internally.
>
> Signed-off-by: Felix Fietkau <[email protected]>
> Cc: [email protected]
> ---
> net/wireless/core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 443d4d7..3f72530 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -526,8 +526,7 @@ int wiphy_register(struct wiphy *wiphy)
> for (i = 0; i < sband->n_channels; i++) {
> sband->channels[i].orig_flags =
> sband->channels[i].flags;
> - sband->channels[i].orig_mag =
> - sband->channels[i].max_antenna_gain;
> + sband->channels[i].orig_mag = INT_MAX;
The commit log for a stable patch should describe the impact of not applying
it. In this case perhaps not being able to associate to APs due to unnecessary
power limitations -- but again that itself would only be true if some other
code used it in a way that it should not. I rather fix that if such cases
exist.
Now the purpose of orig_mag is to allow drivers to spit out to kernel and
eventually userspace if we want it what it knows about the value of the
embedded antenna gain. The regulatory code for antenna gain is useful for us in
that it is the maximum allowed antenna gain values.
This code itself hasn't been widely used throughout code but as you point out
ath9k does use it given that we can populate the antenna gain with what we read
from EEPROM and that in fact should *not* be reducing the max power from ath9k
but instead *helping* it in case it is smaller than the max allowed.
In retrospect there are a few things we can do here to improve this and use
this more efficiently. The orig_mag should be used by wiphy_register() as is.
Then upon wiphy_register() it calls wiphy_regulatory_register() and that
handles with anything regarding regulatory on the device. How about we change
this so that within wiphy_regulatory_register() (or regulatory change,
handle_channel()) we review if the device's antenna gain is over the regulatory
domain max allowed antenna gain and if it is we kick back simply mute the
device only allowing RX. If the device is changed to a regulatory domain that
allows for the antenna gain registered for the device then we unmute it. This
would in turn allow doing dynamic updates of the antenna gain as it seems you
want to introduce API for later, we'd end up checking for the newly supplied
antenna gain (and ensure that that value is higher than the embedded on on
EEPROM).
Luis
A few places touch chan->max_power based on updated tx power rules, but
forget to do the same to chan->max_reg_power.
Signed-off-by: Felix Fietkau <[email protected]>
Cc: [email protected]
---
net/wireless/reg.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3b8cbbc..bcc7d7e 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -908,7 +908,7 @@ static void handle_channel(struct wiphy *wiphy,
map_regdom_flags(reg_rule->flags) | bw_flags;
chan->max_antenna_gain = chan->orig_mag =
(int) MBI_TO_DBI(power_rule->max_antenna_gain);
- chan->max_power = chan->orig_mpwr =
+ chan->max_reg_power = chan->max_power = chan->orig_mpwr =
(int) MBM_TO_DBM(power_rule->max_eirp);
return;
}
@@ -1331,7 +1331,8 @@ static void handle_channel_custom(struct wiphy *wiphy,
chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
- chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
+ chan->max_reg_power = chan->max_power =
+ (int) MBM_TO_DBM(power_rule->max_eirp);
}
static void handle_band_custom(struct wiphy *wiphy, enum ieee80211_band band,
--
1.7.9.6 (Apple Git-31.1)
On Mon, Oct 08, 2012 at 10:52:50PM +0200, Felix Fietkau wrote:
> On 2012-10-08 8:37 PM, Luis R. Rodriguez wrote:
> > On Sat, Oct 06, 2012 at 02:40:53PM +0200, Felix Fietkau wrote:
> >> No driver initializes chan->max_antenna_gain to something sensible, and
> >> the only place where it is being used right now is inside ath9k. This
> >> leads to ath9k potentially using less tx power than it can use.
> >>
> >> Rather than going through every single driver, this patch initializes
> >> chan->orig_mag in wiphy_register(), ignoring whatever value the driver
> >> left in there. If a driver for some reason wishes to limit it independent
> >> from regulatory rulesets, it can do so internally.
> >>
> >> Signed-off-by: Felix Fietkau <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> net/wireless/core.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/net/wireless/core.c b/net/wireless/core.c
> >> index 443d4d7..3f72530 100644
> >> --- a/net/wireless/core.c
> >> +++ b/net/wireless/core.c
> >> @@ -526,8 +526,7 @@ int wiphy_register(struct wiphy *wiphy)
> >> for (i = 0; i < sband->n_channels; i++) {
> >> sband->channels[i].orig_flags =
> >> sband->channels[i].flags;
> >> - sband->channels[i].orig_mag =
> >> - sband->channels[i].max_antenna_gain;
> >> + sband->channels[i].orig_mag = INT_MAX;
> >
> > The commit log for a stable patch should describe the impact of not applying
> > it. In this case perhaps not being able to associate to APs due to unnecessary
> > power limitations -- but again that itself would only be true if some other
> > code used it in a way that it should not. I rather fix that if such cases
> > exist.
>
> I did mention that not applying this patch can in some circumstances
> lead to using lower tx power. I think it's obvious that this impairs
> performance/range.
It may be obvious to you not to the applier of the stable patch. You can
help by making it a bit more obvious to the reader what practical issues
users would run into without this patch.
> > Now the purpose of orig_mag is to allow drivers to spit out to kernel and
> > eventually userspace if we want it what it knows about the value of the
> > embedded antenna gain. The regulatory code for antenna gain is useful for us in
> > that it is the maximum allowed antenna gain values.
>
> If that was the purpose, then the implementation completely fails at
> treating it as such.
Indeed, we never really had a way to deal with this or a generic
interface or way for devices to expose this information to the OS.
Around March I had looked into ways in which we might be able
to create a standarized mechanism to enable any device to expose
its antenna gain to an OS. I only had poked a few folks but it
seems like if we wanted to we may be able to try to propose exposing
antenna gain via EFI. This would enable antenna gain to be exposed
on ARM, x86 givne that both archs will end up using it.
I never followed up but we should see if we can get this but I'll
use this as a prelude to work on this.
> Remember, that 'mag' stands for *maximum* antenna
> gain, not effective antenna gain or something. It is indeed treated as a
> regulatory maximum,
The antenna gain nomenclature came from the HAL but that's just baggage. The
antenna gain stuff remained a puzzle for a central regulatory code base due to
the fact that as far as I am concerned only ath9k was exposing the antenna gain
value from EEPROM and using it. Other drivers would just use it as a max
regulatory value, but as you can imagine code typically always just keeps this
as a static value for all regulatory domains even though technically it is not,
this includes the HAL. A lot of this is due to the fact that you are not
supposed to be mucking with the antenna gain unless you are making APs and
if you are then that would need to be adjusted on the EEPROM anyway and
manufacturers have access to that. So in the end you're right but in
perspective its baggage and in the long term we should strive to generalize
an interface to expose the value to the OS if possible. In the meantime
I suspect your future work may help here get ahead of any other OS by
allowing us to treat dynamic antenna gains within the core of the kernel.
That is, perhaps we don't have a way to query the gain from an existing
card but at the very least the OS can enable us to report to the kernel
if a system integrator / engineer / researcher did modify the antenna gain
somehow to help adjust regulatory appropriately. So even if the old values
are not known at least the new values would be (with the CONFIG_ONUS option).
> and since only driver code uses the
> regulatory-capped maximum antenna gain, it does not make sense to allow
> the driver to set anything here initially.
Well its up to us to do what we want with it. I do wish we had a generic
interface to export antenna gains but we don't yet. Until then we can
only rely on the driver to do this.
> > This code itself hasn't been widely used throughout code but as you point out
> > ath9k does use it given that we can populate the antenna gain with what we read
> > from EEPROM and that in fact should *not* be reducing the max power from ath9k
> > but instead *helping* it in case it is smaller than the max allowed.
>
> I think you're confusing what the EEPROM says vs. what orig_mag really
> means. orig_mag means maximum allowable antenna gain at max. tx power
> that is still compliant.
Sure, but in the case of ath9k its also the programmed value on the EEPROM
and that is the antenna gain on the card.
> The eeprom value contains the antenna gain of
> the current system. These are two completely different things, and
> storing anything from the EEPROM in chan->max_antenna_gain makes no
> sense at all.
Its subjective really, but again, this is all baggage and what we *do*
should be to lay out the ground work to enable us to take this further.
As it is ath9k *does* set these value from the EEPROM and hence why
I considered it odd to set this a max value.
> > In retrospect there are a few things we can do here to improve this and use
> > this more efficiently. The orig_mag should be used by wiphy_register() as is.
> > Then upon wiphy_register() it calls wiphy_regulatory_register() and that
> > handles with anything regarding regulatory on the device. How about we change
> > this so that within wiphy_regulatory_register() (or regulatory change,
> > handle_channel()) we review if the device's antenna gain is over the regulatory
> > domain max allowed antenna gain and if it is we kick back simply mute the
> > device only allowing RX. If the device is changed to a regulatory domain that
> > allows for the antenna gain registered for the device then we unmute it. This
> > would in turn allow doing dynamic updates of the antenna gain as it seems you
> > want to introduce API for later, we'd end up checking for the newly supplied
> > antenna gain (and ensure that that value is higher than the embedded on on
> > EEPROM).
>
> Sorry, but that makes no sense to me, you're solving a nonexistant
> problem. The antenna gain value from EEPROM is already handled properly.
For all drivers? If so how?
> If the code detects that EEPROM antenna gain is n dBi over the
> regulatory antenna gain, it reduces tx power by n dBm. Muting the card
> in that case sounds weird and completely unnecessary to me.
Doesn't only ath9k do this?
In either case you are not disregarding the value set for custom
regulatory domains here.
How about just adding a check for now:
- chan->max_antenna_gain = min(chan->orig_mag,
- (int) MBI_TO_DBI(power_rule->max_antenna_gain));
+ if (chan->orig_mag)
+ chan->max_antenna_gain =
+ min(chan->orig_mag, (int) MBI_TO_DBI(power_rule->max_antenna_gain));
+ else
+ chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
+
Then we can go ahead and clarify usage of these values.
Luis
On 2012-10-08 9:01 PM, Luis R. Rodriguez wrote:
> On Sat, Oct 06, 2012 at 02:40:54PM +0200, Felix Fietkau wrote:
>> A few places touch chan->max_power based on updated tx power rules, but
>> forget to do the same to chan->max_reg_power.
>>
>> Signed-off-by: Felix Fietkau <[email protected]>
>> Cc: [email protected]
>> ---
>> net/wireless/reg.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 3b8cbbc..bcc7d7e 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -908,7 +908,7 @@ static void handle_channel(struct wiphy *wiphy,
>> map_regdom_flags(reg_rule->flags) | bw_flags;
>> chan->max_antenna_gain = chan->orig_mag =
>> (int) MBI_TO_DBI(power_rule->max_antenna_gain);
>> - chan->max_power = chan->orig_mpwr =
>> + chan->max_reg_power = chan->max_power = chan->orig_mpwr =
>> (int) MBM_TO_DBM(power_rule->max_eirp);
>> return;
>> }
>> @@ -1331,7 +1331,8 @@ static void handle_channel_custom(struct wiphy *wiphy,
>>
>> chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
>> chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
>> - chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
>> + chan->max_reg_power = chan->max_power =
>> + (int) MBM_TO_DBM(power_rule->max_eirp);
>
> This looks good to me, good catch! The commit log could use some love,
> given that this is a stable patch it is worthy to describe the
> consequences of not applying this patch. Can you describe what you
> observed that makes this a critical patch? The only piece of code
> that uses max_reg_power in cfg80211, mac80211 or drivers is on
> net/wireless/reg.c and drivers/net/wireless/mwifiex/cfg80211.c.
> In either case the issue the code you are patching is for code
> that deals with drivers that have a custom regulatory domain
> in which orig_mpwr would have been initialized to a non-zero value
> upon registration. In such cases we could only potenially run into
> an issue on this piece of code on handle_channel():
>
> chan->max_reg_power = (int) MBM_TO_DBM(power_rule->max_eirp);
> if (chan->orig_mpwr) {
> /*
> * Devices that have their own custom regulatory domain
> * but also use WIPHY_FLAG_STRICT_REGULATORY will follow the
> * passed country IE power settings.
> */
> if (initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE &&
> wiphy->flags & WIPHY_FLAG_CUSTOM_REGULATORY &&
> wiphy->flags & WIPHY_FLAG_STRICT_REGULATORY)
> chan->max_power = chan->max_reg_power;
> else
> chan->max_power = min(chan->orig_mpwr,
> chan->max_reg_power);
> } else
> chan->max_power = chan->max_reg_power;
>
> The issue would happen if orig_mpwr is non zero (custom) and
> then max_reg_power would not have been initialized. This runs
> when you change a regulatory domain on a card with a custom
> regulatory domain and this would be an issue if max_reg_power
> would not be initialized. This however does not happen due to
> the first line above.
>
> So I agree with this patch but do not see the requirement for
> it to go in as a stable fix to older stable kernels.
OK, I'll extend the commit log and resend without the stable-Cc.
- Felix
On Mon, Oct 08, 2012 at 11:01:30PM +0200, Felix Fietkau wrote:
> On 2012-10-08 9:01 PM, Luis R. Rodriguez wrote:
> > On Sat, Oct 06, 2012 at 02:40:54PM +0200, Felix Fietkau wrote:
> >> A few places touch chan->max_power based on updated tx power rules, but
> >> forget to do the same to chan->max_reg_power.
> >>
> >> Signed-off-by: Felix Fietkau <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> net/wireless/reg.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> >> index 3b8cbbc..bcc7d7e 100644
> >> --- a/net/wireless/reg.c
> >> +++ b/net/wireless/reg.c
> >> @@ -908,7 +908,7 @@ static void handle_channel(struct wiphy *wiphy,
> >> map_regdom_flags(reg_rule->flags) | bw_flags;
> >> chan->max_antenna_gain = chan->orig_mag =
> >> (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> >> - chan->max_power = chan->orig_mpwr =
> >> + chan->max_reg_power = chan->max_power = chan->orig_mpwr =
> >> (int) MBM_TO_DBM(power_rule->max_eirp);
> >> return;
> >> }
> >> @@ -1331,7 +1331,8 @@ static void handle_channel_custom(struct wiphy *wiphy,
> >>
> >> chan->flags |= map_regdom_flags(reg_rule->flags) | bw_flags;
> >> chan->max_antenna_gain = (int) MBI_TO_DBI(power_rule->max_antenna_gain);
> >> - chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
> >> + chan->max_reg_power = chan->max_power =
> >> + (int) MBM_TO_DBM(power_rule->max_eirp);
> >
> > This looks good to me, good catch! The commit log could use some love,
> > given that this is a stable patch it is worthy to describe the
> > consequences of not applying this patch. Can you describe what you
> > observed that makes this a critical patch? The only piece of code
> > that uses max_reg_power in cfg80211, mac80211 or drivers is on
> > net/wireless/reg.c and drivers/net/wireless/mwifiex/cfg80211.c.
> > In either case the issue the code you are patching is for code
> > that deals with drivers that have a custom regulatory domain
> > in which orig_mpwr would have been initialized to a non-zero value
> > upon registration. In such cases we could only potenially run into
> > an issue on this piece of code on handle_channel():
> >
> > chan->max_reg_power = (int) MBM_TO_DBM(power_rule->max_eirp);
> > if (chan->orig_mpwr) {
> > /*
> > * Devices that have their own custom regulatory domain
> > * but also use WIPHY_FLAG_STRICT_REGULATORY will follow the
> > * passed country IE power settings.
> > */
> > if (initiator == NL80211_REGDOM_SET_BY_COUNTRY_IE &&
> > wiphy->flags & WIPHY_FLAG_CUSTOM_REGULATORY &&
> > wiphy->flags & WIPHY_FLAG_STRICT_REGULATORY)
> > chan->max_power = chan->max_reg_power;
> > else
> > chan->max_power = min(chan->orig_mpwr,
> > chan->max_reg_power);
> > } else
> > chan->max_power = chan->max_reg_power;
> >
> > The issue would happen if orig_mpwr is non zero (custom) and
> > then max_reg_power would not have been initialized. This runs
> > when you change a regulatory domain on a card with a custom
> > regulatory domain and this would be an issue if max_reg_power
> > would not be initialized. This however does not happen due to
> > the first line above.
> >
> > So I agree with this patch but do not see the requirement for
> > it to go in as a stable fix to older stable kernels.
> OK, I'll extend the commit log and resend without the stable-Cc.
Great in that case please feel free to add:
Acked-by: Luis R. Rodriguez <[email protected]>
Thanks!
Luis