Hi Johannes,
I have been running the wireless-testing kernel from yesterday and I
realized that something with the link quality values from scanning
results is broken. I know that you sent some patches in that area, but I
didn't really follow them. So it breaks WEXT and as much as we all
dislike them, we still can't break application without warning. So can
this be fixed inside mac80211 or do we have to fix this inside the
drivers and maybe just the iwlagn driver I am using needs an update?
Regards
Marcel
On Wed, Feb 18, 2009 at 05:57:39AM +0100, Marcel Holtmann wrote:
> > Would setting qual->updated |= IW_QUAL_QUAL_INVALID; work for you? If
> > some app doesn't handle that, it's totally, completely broken.
>
> does wpa_supplicant handle this value correctly? I actually never
> checked that.
No, it doesn't use qual->updated at all. Then again, neither does it
rely on qual->qual either. The signal level (qual->level) is used first
and only if that is not available, qual->qual will be used in sorting
the results.
> However I think it is bad practice to abandon a value that has been
> previously set by mac80211 and remove it without any further warning. If
> a driver never set the value before it is a different story than
> mac80211 deciding to not set it any more from one kernel version to
> another one.
It would be interesting to know which applications depend only on
qual->qual values being there and cannot use qual->level. Anyway, I
would agree that it would be reasonable to fill in something in the
qual->qual field for now. It should be enough to do this in the wext
compat code (i.e., not changing anything in mac80211 and only touching
#ifdef CONFIG_WIRELESS_EXT blocks in net/wireless/scan.c). This would
provide somewhat useful value for applications that depend on a value
that was not really guaranteed to be there in the first place. I think I
would include the IW_QUAL_QUAL_INVALID flag anyway and just target the
broken apps with this workaround.
--
Jouni Malinen PGP id EFC895FA
Hi Jouni,
> > > Would setting qual->updated |= IW_QUAL_QUAL_INVALID; work for you? If
> > > some app doesn't handle that, it's totally, completely broken.
> >
> > does wpa_supplicant handle this value correctly? I actually never
> > checked that.
>
> No, it doesn't use qual->updated at all. Then again, neither does it
> rely on qual->qual either. The signal level (qual->level) is used first
> and only if that is not available, qual->qual will be used in sorting
> the results.
so it seems the missing value is affecting the details wpa_supplicant is
handing out via its D-Bus interface. I didn't check the code yet to see
what is actually happening or if I just happen to run an outdated
version.
Regards
Marcel
On Wed, 2009-02-18 at 07:18 -0500, Dan Williams wrote:
[snipped explanation]
> Anyone think all this stuff really, really sucks? Yes!!! So lets just
> have drivers/stack provide a few sane values that userspace really
> doesn't have to go through this shit to calculate...
>
> <rant ends>
>
> NM is probably fine here with qual == 0 because I doubt the GIWRANGE
> handler is returning a valid max_qual.qual > 0 anymore with Johannes'
> patch. Could be wrong though.
We're actually getting _two_ things wrong now and nobody ever noticed.
So much for "but someone might care about the values wext returns". But
there actually is a problem.
First, we're reporting max_qual.qual != 0 still, this should be changed.
wpa_supplicant is still reporting quality values, so the invalid flags
aren't ever set. This needs to change in mac80211 (fixing the immediate
regression) and wpa_supplicant.
HOWEVER. Since all our qual.qual values are 0 though, your code _should_
cope fine, because of this (from NM):
/* If the quality percent was 0 or doesn't exist, then try to use signal levels instead */
if ((percent < 1) && (level_percent >= 0))
percent = level_percent;
Now, why doesn't it?
Because we don't report proper max_qual _level_ values. Something
clearly nobody cared about because NM was using qual.qual and not
qual.level. Here's the relevant snippet from mac80211:
/* cfg80211 requires this, and enforces 0..100 */
if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
range->max_qual.level = 100;
else if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
range->max_qual.level = -110;
else
range->max_qual.level = 0;
Note the dBm branch -- clearly wrong. Did anyone care? Clearly not.
Therefore, here's what I'm going to do:
1) always set IW_QUAL_QUAL_INVALID, even in max_qual (fixes bug #1)
2) set max_qual.level to 0 for dBm instead of -110 (fixes bug #2)
patch below.
johannes
Subject: mac80211: fix wext max_qual report
mac80211 is reporting a max_qual.level of -110 for dBm while it
should be reporting 0. It is also reporting a valid max_qual.qual
although we no longer set qual.qual, which trips up NetworkManager
due to a wpa_supplicant bug. But we don't need to report that our
max_qual.qual value is valid.
Fix the max_qual.level to be 0 for dBm and 100 for non-dBm and
remove max_qual.qual. max_qual.noise = -110 also seems wrong,
so fix that while at it.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/wext.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
--- wireless-testing.orig/net/mac80211/wext.c 2009-02-18 14:24:56.000000000 +0100
+++ wireless-testing/net/mac80211/wext.c 2009-02-18 14:29:07.000000000 +0100
@@ -148,11 +148,19 @@ static u8 ieee80211_get_wstats_flags(str
{
u8 wstats_flags = 0;
- wstats_flags |= local->hw.flags & (IEEE80211_HW_SIGNAL_UNSPEC |
- IEEE80211_HW_SIGNAL_DBM) ?
- IW_QUAL_QUAL_UPDATED : IW_QUAL_QUAL_INVALID;
- wstats_flags |= local->hw.flags & IEEE80211_HW_NOISE_DBM ?
- IW_QUAL_NOISE_UPDATED : IW_QUAL_NOISE_INVALID;
+ wstats_flags |= IW_QUAL_QUAL_INVALID;
+
+ if (local->hw.flags & (IEEE80211_HW_SIGNAL_UNSPEC |
+ IEEE80211_HW_SIGNAL_DBM))
+ wstats_flags |= IW_QUAL_LEVEL_UPDATED;
+ else
+ wstats_flags |= IW_QUAL_LEVEL_INVALID;
+
+ if (local->hw.flags & IEEE80211_HW_NOISE_DBM)
+ wstats_flags |= IW_QUAL_NOISE_UPDATED;
+ else
+ wstats_flags |= IW_QUAL_NOISE_INVALID;
+
if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
wstats_flags |= IW_QUAL_DBM;
@@ -191,19 +199,11 @@ static int ieee80211_ioctl_giwrange(stru
if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
range->max_qual.level = 100;
else if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
- range->max_qual.level = -110;
- else
range->max_qual.level = 0;
- if (local->hw.flags & IEEE80211_HW_NOISE_DBM)
- range->max_qual.noise = -110;
- else
- range->max_qual.noise = 0;
-
- range->max_qual.qual = 100;
+ range->max_qual.noise = 0;
range->max_qual.updated = ieee80211_get_wstats_flags(local);
- range->avg_qual.qual = 50;
/* not always true but better than nothing */
range->avg_qual.level = range->max_qual.level / 2;
range->avg_qual.noise = range->max_qual.noise / 2;
Hi Dan,
> > > > > What exactly is broken by this? Wext never guaranteed that 'qual' values
> > > > > be present, and thus any application that breaks from not having 'qual'
> > > > > is broken anyway.
> > > >
> > > > that be it, but I would still consider this a regression. Since when do
> > > > we just start removing API details without any proper warning or grace
> > > > period?
> > >
> > > Why not? The API even contains whether or not the values are valid, and
> > > after discussing with many of the stakeholders (network manager,
> > > wpa_supplicant) we've decided that there's little use in the qual.qual
> > > value. Especially since you want to compare the 'quality' of the AP
> > > against the one you're associated to, so qual isn't really useful at all
> > > due to the various factors it can contain.
> > >
> > > So hey, if you want to scream "regression" then we can add a 'qual.qual'
> > > value back. It'll still be entirely pointless, and I'll still be against
> > > it, but I'm not going to risk anyone reverting this patch, it's way too
> > > useful. And if you're going to scream regression then please keep in
> > > mind that then you're going to scream about the wext limit again... I
> > > can't fix it up in the next couple of weeks anyway.
> >
> > remember that I am all for replacing WEXT and getting rid of it, but we
> > need to be careful with just removing parts of the API in the middle
> > without people noticing. At least I missed it. So you can argue that it
> > is my fault, but I think it is not really clear to everybody that this
> > value is missing now with future kernel versions.
> >
> > And that WEXT doesn't guarantee that qual.qual is present is not a
> > really good point since cfg80211 in the past did fill in this value.
>
> Would setting qual->updated |= IW_QUAL_QUAL_INVALID; work for you? If
> some app doesn't handle that, it's totally, completely broken.
does wpa_supplicant handle this value correctly? I actually never
checked that.
However I think it is bad practice to abandon a value that has been
previously set by mac80211 and remove it without any further warning. If
a driver never set the value before it is a different story than
mac80211 deciding to not set it any more from one kernel version to
another one.
Regards
Marcel
On Wed, Feb 18, 2009 at 09:06:31AM +0100, Marcel Holtmann wrote:
> so it seems the missing value is affecting the details wpa_supplicant is
> handing out via its D-Bus interface. I didn't check the code yet to see
> what is actually happening or if I just happen to run an outdated
> version.
Yes, it looks like the dbus interface will get all three wext values
(quality, noise, level) regardless of whether they were available from
kernel or not. This does not cause any issues for wpa_supplicant, but if
other programs (e.g., NM) are using the "quality" value in preference to
"level", they would be processing zeros for all BSSes. If I understood
correctly, NM does indeed prefer to use "quality", so this will likely
explain why you are seeing different behavior with the qual->qual value
removed (= 0 for wpa_supplicant).
I don't know how NM would react to the "quality" value being removed,
but it would sound reasonable to do that in wpa_supplicant (and same for
"noise") if the value was not available from the driver (based on the
WEXT flags or in case of nl80211, just based on the fact that these
values are never reported). I would expect this change to go in shortly
(assuming it does not trigger any critical issue with NM), but anyway,
this does not help with old versions.
--
Jouni Malinen PGP id EFC895FA
On Wed, 2009-02-18 at 07:48 -0500, Dan Williams wrote:
> > > - max_qual.level == 0 (ie, dBm values)
> >
> > That is an area where NM (= 0) and mac80211 (= -110) do not agree.
>
> Then mac80211 is not conforming to WEXT... unless it's setting
> IW_QUAL_DBM in the updated field, which it probably is.
Yeah, it is.
> Before we added IW_QUAL_DBM, the switch between dBm and RSSI was
> max_qual.level; if it was 0, level was in dBm, because no cards in use
> in Linux at that time could support a signal of more 0 dBm. Thus, if it
> was over 0, the value was in RSSI.
>
> Here's the relevant bit of wireless.h:
>
> /* Quality range (link, level, noise)
> * If the quality is absolute, it will be in the range [0 ; max_qual],
> * if the quality is dBm, it will be in the range [max_qual ; 0].
>
> That doc never got updated for IW_QUAL_DBM either.
Fun. But what did I expect. I withdraw my earlier patch then.
> NM doesn't really handle IW_QUAL_DBM (added in WE-19). Mainly because
> stuff worked without it, and it wasn't implemented in drivers until
> quite recently. NM should handle IW_QUAL_DBM.
Does all that mean that we cannot actually get this working right now
without adding back qual.qual? Or should we just remove IW_QUAL_DBM?
johannes
On Wed, 2009-02-18 at 14:33 +0200, Jouni Malinen wrote:
> On Wed, Feb 18, 2009 at 07:18:43AM -0500, Dan Williams wrote:
>
> > With WEXT, there are three ways to calculate pretty bars. They *all*
> > require max_qual values returned from the GIWRANGE handler, because
> > otherwise you have no f**king clue what the upper or lower bounds are.
>
> > QUAL.LEVEL in dBm
> > --------------
> >
> > Requires:
> > - max_qual.level == 0 (ie, dBm values)
>
> That is an area where NM (= 0) and mac80211 (= -110) do not agree.
Then mac80211 is not conforming to WEXT... unless it's setting
IW_QUAL_DBM in the updated field, which it probably is.
Before we added IW_QUAL_DBM, the switch between dBm and RSSI was
max_qual.level; if it was 0, level was in dBm, because no cards in use
in Linux at that time could support a signal of more 0 dBm. Thus, if it
was over 0, the value was in RSSI.
Here's the relevant bit of wireless.h:
/* Quality range (link, level, noise)
* If the quality is absolute, it will be in the range [0 ; max_qual],
* if the quality is dBm, it will be in the range [max_qual ; 0].
That doc never got updated for IW_QUAL_DBM either.
> > NM is probably fine here with qual == 0 because I doubt the GIWRANGE
> > handler is returning a valid max_qual.qual > 0 anymore with Johannes'
> > patch. Could be wrong though.
>
> Well, it is not fine, but not only for that reason.. max_qual.qual is
> still set to 100 and the IW_QUAL_QUAL_INVALID is not used for it.
> However, even if I set IW_QUAL_QUAL_INVALID and remove "quality" from
> wpa_supplicant dbus interface, I still get NM showing perfect 100%
> signal all the time regardless of how close to losing the connection the
> card really is..
>
> I gave up on trying to understand all the cases, but my assumption is
> that the remaining issue is in the disagreement on max_qua.level for the
> dBm case. However, I'm not sure whether fixing that would automatically
> resolve the issues with wext (it might be enough for the current nl80211
> version with wpa_supplicant from git head).
NM doesn't really handle IW_QUAL_DBM (added in WE-19). Mainly because
stuff worked without it, and it wasn't implemented in drivers until
quite recently. NM should handle IW_QUAL_DBM.
> > Ah right; the dbus interface shouldn't be appending "quality" to the
> > dict if the driver doesn't provide valid quality (ie, max_qual.updated
> > has the QUAL_INVALID bit set). Same thing for noise and level.
>
> The unknown values are not included anymore in wpa_supplicant 0.7.x.
Just pulled; it doesn't show up in master. Do you have a separate git
repo for 0.7.x?
Dan
Marcel,
> I have been running the wireless-testing kernel from yesterday and I
> realized that something with the link quality values from scanning
> results is broken.
What exactly is broken by this? Wext never guaranteed that 'qual' values
be present, and thus any application that breaks from not having 'qual'
is broken anyway.
johannes
On Wed, Feb 18, 2009 at 07:48:17AM -0500, Dan Williams wrote:
> Then mac80211 is not conforming to WEXT... unless it's setting
> IW_QUAL_DBM in the updated field, which it probably is.
It should have IW_QUAL_DBM set.
> Here's the relevant bit of wireless.h:
>
> /* Quality range (link, level, noise)
> * If the quality is absolute, it will be in the range [0 ; max_qual],
> * if the quality is dBm, it will be in the range [max_qual ; 0].
>
> That doc never got updated for IW_QUAL_DBM either.
Yeah, the max_qual=-110 does not seem to fit into that description.
> > The unknown values are not included anymore in wpa_supplicant 0.7.x.
>
> Just pulled; it doesn't show up in master. Do you have a separate git
> repo for 0.7.x?
0.7.x is in hostap.git and this change should be there:
http://w1.fi/gitweb/gitweb.cgi?p=hostap.git;a=commitdiff;h=7c2849d2a09c1ba3cbc2247f08baad2e929464c1
I don't remember when I pushed that out, but anyway, it is there now.
--
Jouni Malinen PGP id EFC895FA
On Tue, 2009-02-17 at 22:09 +0100, Marcel Holtmann wrote:
> Hi Johannes,
>
> > > > What exactly is broken by this? Wext never guaranteed that 'qual' values
> > > > be present, and thus any application that breaks from not having 'qual'
> > > > is broken anyway.
> > >
> > > that be it, but I would still consider this a regression. Since when do
> > > we just start removing API details without any proper warning or grace
> > > period?
> >
> > Why not? The API even contains whether or not the values are valid, and
> > after discussing with many of the stakeholders (network manager,
> > wpa_supplicant) we've decided that there's little use in the qual.qual
> > value. Especially since you want to compare the 'quality' of the AP
> > against the one you're associated to, so qual isn't really useful at all
> > due to the various factors it can contain.
> >
> > So hey, if you want to scream "regression" then we can add a 'qual.qual'
> > value back. It'll still be entirely pointless, and I'll still be against
> > it, but I'm not going to risk anyone reverting this patch, it's way too
> > useful. And if you're going to scream regression then please keep in
> > mind that then you're going to scream about the wext limit again... I
> > can't fix it up in the next couple of weeks anyway.
>
> remember that I am all for replacing WEXT and getting rid of it, but we
> need to be careful with just removing parts of the API in the middle
> without people noticing. At least I missed it. So you can argue that it
> is my fault, but I think it is not really clear to everybody that this
> value is missing now with future kernel versions.
>
> And that WEXT doesn't guarantee that qual.qual is present is not a
> really good point since cfg80211 in the past did fill in this value.
Would setting qual->updated |= IW_QUAL_QUAL_INVALID; work for you? If
some app doesn't handle that, it's totally, completely broken.
>From wireless.h:
#define IW_QUAL_QUAL_INVALID 0x10 /* Driver doesn't provide value */
I don't think the docs can get much clearer than that... There are
still some drivers that will set INVALID on qual because they simply
don't provide it.
Dan
On Wed, 2009-02-18 at 10:13 -0500, Dan Williams wrote:
> > /* cfg80211 requires this, and enforces 0..100 */
> > if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
> > range->max_qual.level = 100;
> > else if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
> > range->max_qual.level = -110;
> > else
> > range->max_qual.level = 0;
> >
> > Note the dBm branch -- clearly wrong. Did anyone care? Clearly not.
>
> Almost; I was partially wrong as well. The comments from wireless.h
> imply that max_qual.level in the dBm case can be < 0. Remember that
> these values are *s8*, and thus max_qual.level can be < 0 and greater
> than -127 at least.
Ok, I wasn't looking at the header file, only your explanation.
> This of course doesn't allow RSSI-based levels of > 127, but whatever.
> To get the definitive determination of RSSI vs. dBm for level, you use
> (updated & IW_QUAL_DBM).
>
> The understanding of max_qual.level == 0 means dBm was probably from
> conversations with Jean and that's apparently not borne out by the
> evidence from wireless.h, unless there's something else I'm not reading.
>
> NM doesn't handle IW_QUAL_DBM. It should.
>
> > Therefore, here's what I'm going to do:
> > 1) always set IW_QUAL_QUAL_INVALID, even in max_qual (fixes bug #1)
> > 2) set max_qual.level to 0 for dBm instead of -110 (fixes bug #2)
> >
> > patch below.
>
> I think leaving max_qual.level == -110 is OK in mac80211 actually. It's
> NM that's got the problem by not respecting IW_QUAL_DBM.
Yeah, but now we've got three or four bugs that together mean we broke
NM. We can't really wait for NM *and* wpa_supplicant to get their
fixes...
Should we just calculate a qual.qual value anyway?
johannes
On Wed, Feb 18, 2009 at 07:18:43AM -0500, Dan Williams wrote:
> With WEXT, there are three ways to calculate pretty bars. They *all*
> require max_qual values returned from the GIWRANGE handler, because
> otherwise you have no f**king clue what the upper or lower bounds are.
> QUAL.LEVEL in dBm
> --------------
>
> Requires:
> - max_qual.level == 0 (ie, dBm values)
That is an area where NM (= 0) and mac80211 (= -110) do not agree.
> NM is probably fine here with qual == 0 because I doubt the GIWRANGE
> handler is returning a valid max_qual.qual > 0 anymore with Johannes'
> patch. Could be wrong though.
Well, it is not fine, but not only for that reason.. max_qual.qual is
still set to 100 and the IW_QUAL_QUAL_INVALID is not used for it.
However, even if I set IW_QUAL_QUAL_INVALID and remove "quality" from
wpa_supplicant dbus interface, I still get NM showing perfect 100%
signal all the time regardless of how close to losing the connection the
card really is..
I gave up on trying to understand all the cases, but my assumption is
that the remaining issue is in the disagreement on max_qua.level for the
dBm case. However, I'm not sure whether fixing that would automatically
resolve the issues with wext (it might be enough for the current nl80211
version with wpa_supplicant from git head).
> Ah right; the dbus interface shouldn't be appending "quality" to the
> dict if the driver doesn't provide valid quality (ie, max_qual.updated
> has the QUAL_INVALID bit set). Same thing for noise and level.
The unknown values are not included anymore in wpa_supplicant 0.7.x.
--
Jouni Malinen PGP id EFC895FA
Due to various bugs in the software stack we end up having
to fill qual.qual; level should be used, but wpa_supplicant
doesn't properly ignore qual.qual, NM should use qual.level
regardless of that because qual.qual is 0 but doesn't handle
IW_QUAL_DBM right now.
So fill qual.qual with the qual.level value clamped to
-110..-40 dBm or just the regular 'unspecified' signal level.
This requires a mac80211 change to properly announce the
max_qual.qual and avg_qual.qual values.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/wext.c | 18 ++++++++++++++++--
net/wireless/scan.c | 15 ++++++++++++---
2 files changed, 28 insertions(+), 5 deletions(-)
--- wireless-testing.orig/net/mac80211/wext.c 2009-02-18 18:13:49.000000000 +0100
+++ wireless-testing/net/mac80211/wext.c 2009-02-18 18:15:08.000000000 +0100
@@ -200,10 +200,24 @@ static int ieee80211_ioctl_giwrange(stru
else
range->max_qual.noise = 0;
- range->max_qual.qual = 100;
range->max_qual.updated = ieee80211_get_wstats_flags(local);
- range->avg_qual.qual = 50;
+ if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM) {
+ /*
+ * cfg80211 assumes -110 to -40 dBm and clamps to that range
+ * for qual.qual, so tell userspace this is what we give it
+ * but take into account that we have to start from 0.
+ */
+ range->max_qual.qual = 70;
+ range->avg_qual.qual = 35;
+ } else {
+ /*
+ * cfg80211 just uses the level value for qual too, and it
+ * requires the level value to be 0 .. 100.
+ */
+ range->max_qual.qual = 100;
+ range->avg_qual.qual = 50;
+ }
/* not always true but better than nothing */
range->avg_qual.level = range->max_qual.level / 2;
range->avg_qual.noise = range->max_qual.noise / 2;
--- wireless-testing.orig/net/wireless/scan.c 2009-02-18 18:08:20.000000000 +0100
+++ wireless-testing/net/wireless/scan.c 2009-02-18 18:13:44.000000000 +0100
@@ -614,7 +614,7 @@ ieee80211_bss(struct iw_request_info *in
struct iw_event iwe;
u8 *buf, *cfg, *p;
u8 *ie = bss->pub.information_elements;
- int rem = bss->pub.len_information_elements, i;
+ int rem = bss->pub.len_information_elements, i, sig;
bool ismesh = false;
memset(&iwe, 0, sizeof(iwe));
@@ -643,14 +643,23 @@ ieee80211_bss(struct iw_request_info *in
iwe.cmd = IWEVQUAL;
iwe.u.qual.updated = IW_QUAL_LEVEL_UPDATED |
IW_QUAL_NOISE_INVALID |
- IW_QUAL_QUAL_INVALID;
+ IW_QUAL_QUAL_UPDATED;
switch (bss->pub.signal_type) {
case CFG80211_SIGNAL_TYPE_MBM:
- iwe.u.qual.level = bss->pub.signal / 100;
+ sig = bss->pub.signal / 100;
+ iwe.u.qual.level = sig;
iwe.u.qual.updated |= IW_QUAL_DBM;
+ if (sig < -110) /* rather bad */
+ sig = -110;
+ else if (sig > -40) /* perfect */
+ sig = -40;
+ /* will give a range of 0 .. 70 */
+ iwe.u.qual.qual = sig + 110;
break;
case CFG80211_SIGNAL_TYPE_UNSPEC:
iwe.u.qual.level = bss->pub.signal;
+ /* will give range 0 .. 100 */
+ iwe.u.qual.qual = bss->pub.signal;
break;
default:
/* not reached */
Hi Johannes,
> > I have been running the wireless-testing kernel from yesterday and I
> > realized that something with the link quality values from scanning
> > results is broken.
>
> What exactly is broken by this? Wext never guaranteed that 'qual' values
> be present, and thus any application that breaks from not having 'qual'
> is broken anyway.
that be it, but I would still consider this a regression. Since when do
we just start removing API details without any proper warning or grace
period?
Regards
Marcel
On Wed, 2009-02-18 at 18:27 +0100, Johannes Berg wrote:
> Due to various bugs in the software stack we end up having
> to fill qual.qual; level should be used, but wpa_supplicant
> doesn't properly ignore qual.qual, NM should use qual.level
> regardless of that because qual.qual is 0 but doesn't handle
> IW_QUAL_DBM right now.
>
> So fill qual.qual with the qual.level value clamped to
> -110..-40 dBm or just the regular 'unspecified' signal level.
> This requires a mac80211 change to properly announce the
> max_qual.qual and avg_qual.qual values.
Forgot to mention .. with a little work we can move the entire iwrange
handling into cfg80211 -- happiness.
johannes
On Wed, 2009-02-18 at 14:37 +0100, Johannes Berg wrote:
> On Wed, 2009-02-18 at 07:18 -0500, Dan Williams wrote:
>
> [snipped explanation]
>
> > Anyone think all this stuff really, really sucks? Yes!!! So lets just
> > have drivers/stack provide a few sane values that userspace really
> > doesn't have to go through this shit to calculate...
> >
> > <rant ends>
> >
> > NM is probably fine here with qual == 0 because I doubt the GIWRANGE
> > handler is returning a valid max_qual.qual > 0 anymore with Johannes'
> > patch. Could be wrong though.
>
> We're actually getting _two_ things wrong now and nobody ever noticed.
> So much for "but someone might care about the values wext returns". But
> there actually is a problem.
>
> First, we're reporting max_qual.qual != 0 still, this should be changed.
> wpa_supplicant is still reporting quality values, so the invalid flags
> aren't ever set. This needs to change in mac80211 (fixing the immediate
> regression) and wpa_supplicant.
>
> HOWEVER. Since all our qual.qual values are 0 though, your code _should_
> cope fine, because of this (from NM):
>
> /* If the quality percent was 0 or doesn't exist, then try to use signal levels instead */
> if ((percent < 1) && (level_percent >= 0))
> percent = level_percent;
>
> Now, why doesn't it?
>
> Because we don't report proper max_qual _level_ values. Something
> clearly nobody cared about because NM was using qual.qual and not
> qual.level. Here's the relevant snippet from mac80211:
>
> /* cfg80211 requires this, and enforces 0..100 */
> if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
> range->max_qual.level = 100;
> else if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
> range->max_qual.level = -110;
> else
> range->max_qual.level = 0;
>
> Note the dBm branch -- clearly wrong. Did anyone care? Clearly not.
Almost; I was partially wrong as well. The comments from wireless.h
imply that max_qual.level in the dBm case can be < 0. Remember that
these values are *s8*, and thus max_qual.level can be < 0 and greater
than -127 at least.
This of course doesn't allow RSSI-based levels of > 127, but whatever.
To get the definitive determination of RSSI vs. dBm for level, you use
(updated & IW_QUAL_DBM).
The understanding of max_qual.level == 0 means dBm was probably from
conversations with Jean and that's apparently not borne out by the
evidence from wireless.h, unless there's something else I'm not reading.
NM doesn't handle IW_QUAL_DBM. It should.
> Therefore, here's what I'm going to do:
> 1) always set IW_QUAL_QUAL_INVALID, even in max_qual (fixes bug #1)
> 2) set max_qual.level to 0 for dBm instead of -110 (fixes bug #2)
>
> patch below.
I think leaving max_qual.level == -110 is OK in mac80211 actually. It's
NM that's got the problem by not respecting IW_QUAL_DBM.
Dan
> johannes
>
> Subject: mac80211: fix wext max_qual report
>
> mac80211 is reporting a max_qual.level of -110 for dBm while it
> should be reporting 0. It is also reporting a valid max_qual.qual
> although we no longer set qual.qual, which trips up NetworkManager
> due to a wpa_supplicant bug. But we don't need to report that our
> max_qual.qual value is valid.
>
> Fix the max_qual.level to be 0 for dBm and 100 for non-dBm and
> remove max_qual.qual. max_qual.noise = -110 also seems wrong,
> so fix that while at it.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/wext.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> --- wireless-testing.orig/net/mac80211/wext.c 2009-02-18 14:24:56.000000000 +0100
> +++ wireless-testing/net/mac80211/wext.c 2009-02-18 14:29:07.000000000 +0100
> @@ -148,11 +148,19 @@ static u8 ieee80211_get_wstats_flags(str
> {
> u8 wstats_flags = 0;
>
> - wstats_flags |= local->hw.flags & (IEEE80211_HW_SIGNAL_UNSPEC |
> - IEEE80211_HW_SIGNAL_DBM) ?
> - IW_QUAL_QUAL_UPDATED : IW_QUAL_QUAL_INVALID;
> - wstats_flags |= local->hw.flags & IEEE80211_HW_NOISE_DBM ?
> - IW_QUAL_NOISE_UPDATED : IW_QUAL_NOISE_INVALID;
> + wstats_flags |= IW_QUAL_QUAL_INVALID;
> +
> + if (local->hw.flags & (IEEE80211_HW_SIGNAL_UNSPEC |
> + IEEE80211_HW_SIGNAL_DBM))
> + wstats_flags |= IW_QUAL_LEVEL_UPDATED;
> + else
> + wstats_flags |= IW_QUAL_LEVEL_INVALID;
> +
> + if (local->hw.flags & IEEE80211_HW_NOISE_DBM)
> + wstats_flags |= IW_QUAL_NOISE_UPDATED;
> + else
> + wstats_flags |= IW_QUAL_NOISE_INVALID;
> +
> if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
> wstats_flags |= IW_QUAL_DBM;
>
> @@ -191,19 +199,11 @@ static int ieee80211_ioctl_giwrange(stru
> if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
> range->max_qual.level = 100;
> else if (local->hw.flags & IEEE80211_HW_SIGNAL_DBM)
> - range->max_qual.level = -110;
> - else
> range->max_qual.level = 0;
>
> - if (local->hw.flags & IEEE80211_HW_NOISE_DBM)
> - range->max_qual.noise = -110;
> - else
> - range->max_qual.noise = 0;
> -
> - range->max_qual.qual = 100;
> + range->max_qual.noise = 0;
> range->max_qual.updated = ieee80211_get_wstats_flags(local);
>
> - range->avg_qual.qual = 50;
> /* not always true but better than nothing */
> range->avg_qual.level = range->max_qual.level / 2;
> range->avg_qual.noise = range->max_qual.noise / 2;
>
>
On Wed, 2009-02-18 at 07:48 -0500, Dan Williams wrote:
> On Wed, 2009-02-18 at 14:33 +0200, Jouni Malinen wrote:
> > On Wed, Feb 18, 2009 at 07:18:43AM -0500, Dan Williams wrote:
> >
> > > With WEXT, there are three ways to calculate pretty bars. They *all*
> > > require max_qual values returned from the GIWRANGE handler, because
> > > otherwise you have no f**king clue what the upper or lower bounds are.
> >
> > > QUAL.LEVEL in dBm
> > > --------------
> > >
> > > Requires:
> > > - max_qual.level == 0 (ie, dBm values)
> >
> > That is an area where NM (= 0) and mac80211 (= -110) do not agree.
>
> Then mac80211 is not conforming to WEXT... unless it's setting
> IW_QUAL_DBM in the updated field, which it probably is.
>
> Before we added IW_QUAL_DBM, the switch between dBm and RSSI was
> max_qual.level; if it was 0, level was in dBm, because no cards in use
> in Linux at that time could support a signal of more 0 dBm. Thus, if it
> was over 0, the value was in RSSI.
>
> Here's the relevant bit of wireless.h:
>
> /* Quality range (link, level, noise)
> * If the quality is absolute, it will be in the range [0 ; max_qual],
> * if the quality is dBm, it will be in the range [max_qual ; 0].
>
> That doc never got updated for IW_QUAL_DBM either.
And yeah, the NM method isn't entirely consistent with this comment in
wext.h either; however, it's pretty unclear how to actually figure out
from max_qual whether the driver is reporting absolute or dBm unless you
use IW_QUAL_DBM.
Dan
> > > NM is probably fine here with qual == 0 because I doubt the GIWRANGE
> > > handler is returning a valid max_qual.qual > 0 anymore with Johannes'
> > > patch. Could be wrong though.
> >
> > Well, it is not fine, but not only for that reason.. max_qual.qual is
> > still set to 100 and the IW_QUAL_QUAL_INVALID is not used for it.
> > However, even if I set IW_QUAL_QUAL_INVALID and remove "quality" from
> > wpa_supplicant dbus interface, I still get NM showing perfect 100%
> > signal all the time regardless of how close to losing the connection the
> > card really is..
> >
> > I gave up on trying to understand all the cases, but my assumption is
> > that the remaining issue is in the disagreement on max_qua.level for the
> > dBm case. However, I'm not sure whether fixing that would automatically
> > resolve the issues with wext (it might be enough for the current nl80211
> > version with wpa_supplicant from git head).
>
> NM doesn't really handle IW_QUAL_DBM (added in WE-19). Mainly because
> stuff worked without it, and it wasn't implemented in drivers until
> quite recently. NM should handle IW_QUAL_DBM.
>
> > > Ah right; the dbus interface shouldn't be appending "quality" to the
> > > dict if the driver doesn't provide valid quality (ie, max_qual.updated
> > > has the QUAL_INVALID bit set). Same thing for noise and level.
> >
> > The unknown values are not included anymore in wpa_supplicant 0.7.x.
>
> Just pulled; it doesn't show up in master. Do you have a separate git
> repo for 0.7.x?
>
> Dan
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-02-17 at 21:24 +0100, Marcel Holtmann wrote:
> > What exactly is broken by this? Wext never guaranteed that 'qual' values
> > be present, and thus any application that breaks from not having 'qual'
> > is broken anyway.
>
> that be it, but I would still consider this a regression. Since when do
> we just start removing API details without any proper warning or grace
> period?
Why not? The API even contains whether or not the values are valid, and
after discussing with many of the stakeholders (network manager,
wpa_supplicant) we've decided that there's little use in the qual.qual
value. Especially since you want to compare the 'quality' of the AP
against the one you're associated to, so qual isn't really useful at all
due to the various factors it can contain.
So hey, if you want to scream "regression" then we can add a 'qual.qual'
value back. It'll still be entirely pointless, and I'll still be against
it, but I'm not going to risk anyone reverting this patch, it's way too
useful. And if you're going to scream regression then please keep in
mind that then you're going to scream about the wext limit again... I
can't fix it up in the next couple of weeks anyway.
johannes
Hi Johannes,
> > > What exactly is broken by this? Wext never guaranteed that 'qual' values
> > > be present, and thus any application that breaks from not having 'qual'
> > > is broken anyway.
> >
> > that be it, but I would still consider this a regression. Since when do
> > we just start removing API details without any proper warning or grace
> > period?
>
> Why not? The API even contains whether or not the values are valid, and
> after discussing with many of the stakeholders (network manager,
> wpa_supplicant) we've decided that there's little use in the qual.qual
> value. Especially since you want to compare the 'quality' of the AP
> against the one you're associated to, so qual isn't really useful at all
> due to the various factors it can contain.
>
> So hey, if you want to scream "regression" then we can add a 'qual.qual'
> value back. It'll still be entirely pointless, and I'll still be against
> it, but I'm not going to risk anyone reverting this patch, it's way too
> useful. And if you're going to scream regression then please keep in
> mind that then you're going to scream about the wext limit again... I
> can't fix it up in the next couple of weeks anyway.
remember that I am all for replacing WEXT and getting rid of it, but we
need to be careful with just removing parts of the API in the middle
without people noticing. At least I missed it. So you can argue that it
is my fault, but I think it is not really clear to everybody that this
value is missing now with future kernel versions.
And that WEXT doesn't guarantee that qual.qual is present is not a
really good point since cfg80211 in the past did fill in this value.
So if you really wanna remove this value (and I agree with the argument
that it is not useful), then we should do this gracefully. Using the
feature-removal-schedule.txt document comes to my mind.
Regards
Marcel
On Wed, 2009-02-18 at 10:25 +0200, Jouni Malinen wrote:
> On Wed, Feb 18, 2009 at 09:06:31AM +0100, Marcel Holtmann wrote:
>
> > so it seems the missing value is affecting the details wpa_supplicant is
> > handing out via its D-Bus interface. I didn't check the code yet to see
> > what is actually happening or if I just happen to run an outdated
> > version.
>
> Yes, it looks like the dbus interface will get all three wext values
> (quality, noise, level) regardless of whether they were available from
> kernel or not. This does not cause any issues for wpa_supplicant, but if
> other programs (e.g., NM) are using the "quality" value in preference to
> "level", they would be processing zeros for all BSSes. If I understood
> correctly, NM does indeed prefer to use "quality", so this will likely
> explain why you are seeing different behavior with the qual->qual value
> removed (= 0 for wpa_supplicant).
>
> I don't know how NM would react to the "quality" value being removed,
> but it would sound reasonable to do that in wpa_supplicant (and same for
With WEXT, there are three ways to calculate pretty bars. They *all*
require max_qual values returned from the GIWRANGE handler, because
otherwise you have no f**king clue what the upper or lower bounds are.
QUAL.QUAL
--------------
Requires:
- max_qual.qual > 0
- !(max_qual.updated & IW_QUAL_QUAL_INVALID)
- !(qual.updated & IW_QUAL_QUAL_INVALID)
pct = 100 * (qual.qual / max_qual.qual)
QUAL.LEVEL in dBm
--------------
Requires:
- max_qual.level == 0 (ie, dBm values)
- !(max_qual.updated & IW_QUAL_LEVEL_INVALID)
- !(qual.updated & IW_QUAL_LEVEL_INVALID)
- (max_qual.noise > 0) && !(max_qual.updated & IW_QUAL_NOISE_INVALID)
OR
(qual.noise > 0) && !(qual.updated & IW_QUAL_NOISE_INVALID)
pct = (clamp driver values between -90 dBm and -20 dBm, then some voodoo)
QUAL.LEVEL in RSSI
--------------
Requires:
- max_qual.level != 0
- !(max_qual.updated & IW_QUAL_LEVEL_INVALID)
- !(qual.updated & IW_QUAL_LEVEL_INVALID)
pct = 100 * (qual.level / max_qual.level)
Anyone think all this stuff really, really sucks? Yes!!! So lets just
have drivers/stack provide a few sane values that userspace really
doesn't have to go through this shit to calculate...
<rant ends>
NM is probably fine here with qual == 0 because I doubt the GIWRANGE
handler is returning a valid max_qual.qual > 0 anymore with Johannes'
patch. Could be wrong though.
> "noise") if the value was not available from the driver (based on the
> WEXT flags or in case of nl80211, just based on the fact that these
> values are never reported). I would expect this change to go in shortly
> (assuming it does not trigger any critical issue with NM), but anyway,
> this does not help with old versions.
Ah right; the dbus interface shouldn't be appending "quality" to the
dict if the driver doesn't provide valid quality (ie, max_qual.updated
has the QUAL_INVALID bit set). Same thing for noise and level.
Dan