[PATCH] iwldvm: fix chain gain calibration when firmware return zero values

[PATCH] iwldvm: fix chain gain calibration when firmware return zero values

[Nikolay Martynov]

From: Nikolay Martynov <(none)>

It looks like sometimes firmware returns zero for chain noise and signal
during calibration period. This seems to be a known problem and current
implementation accounts for this by ignoring invalid data when all chains
return zero signal and noise.

The problem is that sometimes firmware returns zero for only one chain for
some (not all) beacons used for calibration. This leads to perfectly valid
chains be disabled and may cause invalid gain settings.
For example this is calibration data taken on laptop with Intel 6300 card
with all three antennas attached:

active_chains:			 3
chain_noise_a:			 312
chain_noise_b:			 297
chain_noise_c:			 0
chain_signal_a:			 549
chain_signal_b:			 513
chain_signal_c:			 0
beacon_count:			 16
disconn_array:			 0 0 1
delta_gain_code:		 4 0 0
radio_write:			 1
state:				 3

This patch changes statistics gathering to make sure that zero noise
results are ignored for valid rx chains. The rationale being that even if
anntenna is not connected we should be able to see non zero noise if rx
chain is present.

This patch fixes the problem of disabling working chains on hardware I
have (6300 and 5300). It also works fine in case one 3-chain hardware has
only two antennas attached.

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
---
 drivers/net/wireless/iwlwifi/dvm/calib.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
index 20e6aa9..59e73e2 100644
--- a/drivers/net/wireless/iwlwifi/dvm/calib.c
+++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
@@ -1026,6 +1026,18 @@ void iwl_chain_noise_calibration(struct iwl_priv *priv)
 
 	spin_unlock_bh(&priv->statistics.lock);
 
+	/* Sometimes firmware returns zero for chain noise and signal
+	 * even if chain is present and antenna is connected. This
+	 * often results in perfectly valid chains being disabled.
+	 * Ignore statistics if it contains zero noise for valid rx
+	 * chain: even with antenna disconnected we should hear some noise.
+	 */
+	if (((priv->nvm_data->valid_rx_ant & (1 << 0)) && chain_noise_a == 0) ||
+	    ((priv->nvm_data->valid_rx_ant & (2 << 0)) && chain_noise_b == 0) ||
+	    ((priv->nvm_data->valid_rx_ant & (3 << 0)) && chain_noise_c == 0)) {
+		return;
+	}
+
 	data->beacon_count++;
 
 	data->chain_noise_a = (chain_noise_a + data->chain_noise_a);
-- 
2.7.0

Re: [PATCH] iwldvm: fix chain gain calibration when firmware return zero values

[Emmanuel Grumbach]

Hi,

Note that Wey-Yi has stepped down from maintaining iwlwifi for a while now.

On Sat, Jan 23, 2016 at 7:35 AM, Nikolay Martynov <mar.kolya@gmail.com> wrote:
> From: Nikolay Martynov <(none)>
>
> It looks like sometimes firmware returns zero for chain noise and signal
> during calibration period. This seems to be a known problem and current
> implementation accounts for this by ignoring invalid data when all chains
> return zero signal and noise.
>
> The problem is that sometimes firmware returns zero for only one chain for
> some (not all) beacons used for calibration. This leads to perfectly valid
> chains be disabled and may cause invalid gain settings.
> For example this is calibration data taken on laptop with Intel 6300 card
> with all three antennas attached:
>
> active_chains:                   3
> chain_noise_a:                   312
> chain_noise_b:                   297
> chain_noise_c:                   0
> chain_signal_a:                  549
> chain_signal_b:                  513
> chain_signal_c:                  0
> beacon_count:                    16
> disconn_array:                   0 0 1
> delta_gain_code:                 4 0 0
> radio_write:                     1
> state:                           3
>
> This patch changes statistics gathering to make sure that zero noise
> results are ignored for valid rx chains. The rationale being that even if
> anntenna is not connected we should be able to see non zero noise if rx
> chain is present.
>
> This patch fixes the problem of disabling working chains on hardware I
> have (6300 and 5300). It also works fine in case one 3-chain hardware has
> only two antennas attached.

So how will that work if an antenna is disconnected? You always seem
to ignore the sampling that returned 0.

>
> Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
> ---
>  drivers/net/wireless/iwlwifi/dvm/calib.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
> index 20e6aa9..59e73e2 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/calib.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
> @@ -1026,6 +1026,18 @@ void iwl_chain_noise_calibration(struct iwl_priv *priv)
>
>         spin_unlock_bh(&priv->statistics.lock);
>
> +       /* Sometimes firmware returns zero for chain noise and signal
> +        * even if chain is present and antenna is connected. This
> +        * often results in perfectly valid chains being disabled.
> +        * Ignore statistics if it contains zero noise for valid rx
> +        * chain: even with antenna disconnected we should hear some noise.
> +        */
> +       if (((priv->nvm_data->valid_rx_ant & (1 << 0)) && chain_noise_a == 0) ||
> +           ((priv->nvm_data->valid_rx_ant & (2 << 0)) && chain_noise_b == 0) ||
> +           ((priv->nvm_data->valid_rx_ant & (3 << 0)) && chain_noise_c == 0)) {
> +               return;
> +       }

I guess this is a typo, you wanted BIT(1), BIT(2) and BIT(3), right?

> +
>         data->beacon_count++;
>
>         data->chain_noise_a = (chain_noise_a + data->chain_noise_a);
> --
> 2.7.0
>
> --
> 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] iwldvm: fix chain gain calibration when firmware return zero values

[Julian Calaby]

Hi Nikolay,

A couple of minor points:

On Sat, Jan 23, 2016 at 4:35 PM, Nikolay Martynov <mar.kolya@gmail.com> wrote:
> From: Nikolay Martynov <(none)>

You really need an email address here instead of "(none)" as this will
be used for the author field of the eventual commit. Check the "user"
section of your git configuration. (Try git config --list)

> It looks like sometimes firmware returns zero for chain noise and signal
> during calibration period. This seems to be a known problem and current
> implementation accounts for this by ignoring invalid data when all chains
> return zero signal and noise.
>
> The problem is that sometimes firmware returns zero for only one chain for
> some (not all) beacons used for calibration. This leads to perfectly valid
> chains be disabled and may cause invalid gain settings.
> For example this is calibration data taken on laptop with Intel 6300 card
> with all three antennas attached:
>
> active_chains:                   3
> chain_noise_a:                   312
> chain_noise_b:                   297
> chain_noise_c:                   0
> chain_signal_a:                  549
> chain_signal_b:                  513
> chain_signal_c:                  0
> beacon_count:                    16
> disconn_array:                   0 0 1
> delta_gain_code:                 4 0 0
> radio_write:                     1
> state:                           3
>
> This patch changes statistics gathering to make sure that zero noise
> results are ignored for valid rx chains. The rationale being that even if
> anntenna is not connected we should be able to see non zero noise if rx
> chain is present.
>
> This patch fixes the problem of disabling working chains on hardware I
> have (6300 and 5300). It also works fine in case one 3-chain hardware has
> only two antennas attached.
>
> Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
> ---
>  drivers/net/wireless/iwlwifi/dvm/calib.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
> index 20e6aa9..59e73e2 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/calib.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
> @@ -1026,6 +1026,18 @@ void iwl_chain_noise_calibration(struct iwl_priv *priv)
>
>         spin_unlock_bh(&priv->statistics.lock);
>
> +       /* Sometimes firmware returns zero for chain noise and signal
> +        * even if chain is present and antenna is connected. This
> +        * often results in perfectly valid chains being disabled.
> +        * Ignore statistics if it contains zero noise for valid rx
> +        * chain: even with antenna disconnected we should hear some noise.
> +        */
> +       if (((priv->nvm_data->valid_rx_ant & (1 << 0)) && chain_noise_a == 0) ||
> +           ((priv->nvm_data->valid_rx_ant & (2 << 0)) && chain_noise_b == 0) ||
> +           ((priv->nvm_data->valid_rx_ant & (3 << 0)) && chain_noise_c == 0)) {

To expand on Emmanuel's comment, 3 << 0 is equivalent to 3. If you're
trying to set specific bits then you need to use the BIT() macro,
otherwise just get rid of the "<< 0"s.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Nikolay Martynov]

It looks like sometimes firmware returns zero for chain noise and signal
during calibration period. This seems to be a known problem and current
implementation accounts for this by ignoring invalid data when all chains
return zero signal and noise.

The problem is that sometimes firmware returns zero for only one chain for
some (not all) beacons used for calibration. This leads to perfectly valid
chains be disabled and may cause invalid gain settings.
For example this is calibration data taken on laptop with Intel 6300 card
with all three antennas attached:

active_chains:			 3
chain_noise_a:			 312
chain_noise_b:			 297
chain_noise_c:			 0
chain_signal_a:			 549
chain_signal_b:			 513
chain_signal_c:			 0
beacon_count:			 16
disconn_array:			 0 0 1
delta_gain_code:		 4 0 0
radio_write:			 1
state:				 3

This patch changes statistics gathering to make sure that zero noise
results are ignored for valid rx chains. The rationale being that even if
anntenna is not connected we should be able to see non zero noise if rx
chain is present.

This patch fixes the problem of disabling working chains on hardware I
have (6300 and 5300). With three connected antennas statistics in
chain_noise looks like this (6300):

active_chains:			 7
chain_noise_a:			 321
chain_noise_b:			 243
chain_noise_c:			 311
chain_signal_a:			 559
chain_signal_b:			 452
chain_signal_c:			 512
beacon_count:			 16
disconn_array:			 0 0 0
delta_gain_code:		 4 3 0
radio_write:			 1
state:				 3

It also works fine in case one 3-chain hardware has only two antennas
attached (tested in 5300):

active_chains:			 3
chain_noise_a:			 238
chain_noise_b:			 234
chain_noise_c:			 219
chain_signal_a:			 533
chain_signal_b:			 514
chain_signal_c:			 206
beacon_count:			 16
disconn_array:			 0 0 1
delta_gain_code:		 4 0 0
radio_write:			 1
state:				 3

Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
---
 drivers/net/wireless/iwlwifi/dvm/calib.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
index 20e6aa9..cdaa88d 100644
--- a/drivers/net/wireless/iwlwifi/dvm/calib.c
+++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
@@ -1026,6 +1026,18 @@ void iwl_chain_noise_calibration(struct iwl_priv *priv)
 
 	spin_unlock_bh(&priv->statistics.lock);
 
+	/* Sometimes firmware returns zero for chain noise and signal
+	 * even if chain is present and antenna is connected. This
+	 * often results in perfectly valid chains being disabled.
+	 * Ignore statistics if it contains zero noise for valid rx
+	 * chain: even with antenna disconnected we should hear some noise.
+	 */
+	if ((priv->nvm_data->valid_rx_ant & ANT_A && chain_noise_a == 0) ||
+	    (priv->nvm_data->valid_rx_ant & ANT_B && chain_noise_b == 0) ||
+	    (priv->nvm_data->valid_rx_ant & ANT_C && chain_noise_c == 0)) {
+		return;
+	}
+
 	data->beacon_count++;
 
 	data->chain_noise_a = (chain_noise_a + data->chain_noise_a);
-- 
2.7.0

Re: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Grumbach, Emmanuel]

On 01/26/2016 12:20 AM, Nikolay Martynov wrote:
> It looks like sometimes firmware returns zero for chain noise and signal
> during calibration period. This seems to be a known problem and current
> implementation accounts for this by ignoring invalid data when all chains
> return zero signal and noise.
>
> The problem is that sometimes firmware returns zero for only one chain for
> some (not all) beacons used for calibration. This leads to perfectly valid
> chains be disabled and may cause invalid gain settings.
> For example this is calibration data taken on laptop with Intel 6300 card
> with all three antennas attached:
>
> active_chains:			 3
> chain_noise_a:			 312
> chain_noise_b:			 297
> chain_noise_c:			 0
> chain_signal_a:			 549
> chain_signal_b:			 513
> chain_signal_c:			 0
> beacon_count:			 16
> disconn_array:			 0 0 1
> delta_gain_code:		 4 0 0
> radio_write:			 1
> state:				 3
>
> This patch changes statistics gathering to make sure that zero noise
> results are ignored for valid rx chains. The rationale being that even if
> anntenna is not connected we should be able to see non zero noise if rx
> chain is present.

But then the firmware will continue to send zero for signal and this
will impact lots
of flows like roaming. If the driver allows the firmware to use that
antenna, the
firmware may use this antenna for scanning and roaming will be broken.
This seems to be a bug in the firmware, but there isn't much I can do
about it.
Sorry, I have to NACK this patch.

> This patch fixes the problem of disabling working chains on hardware I
> have (6300 and 5300). With three connected antennas statistics in
> chain_noise looks like this (6300):
>
> active_chains:			 7
> chain_noise_a:			 321
> chain_noise_b:			 243
> chain_noise_c:			 311
> chain_signal_a:			 559
> chain_signal_b:			 452
> chain_signal_c:			 512
> beacon_count:			 16
> disconn_array:			 0 0 0
> delta_gain_code:		 4 3 0
> radio_write:			 1
> state:				 3
>
> It also works fine in case one 3-chain hardware has only two antennas
> attached (tested in 5300):
>
> active_chains:			 3
> chain_noise_a:			 238
> chain_noise_b:			 234
> chain_noise_c:			 219
> chain_signal_a:			 533
> chain_signal_b:			 514
> chain_signal_c:			 206
> beacon_count:			 16
> disconn_array:			 0 0 1
> delta_gain_code:		 4 0 0
> radio_write:			 1
> state:				 3
>
> Signed-off-by: Nikolay Martynov <mar.kolya@gmail.com>
> ---
>  drivers/net/wireless/iwlwifi/dvm/calib.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
> index 20e6aa9..cdaa88d 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/calib.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
> @@ -1026,6 +1026,18 @@ void iwl_chain_noise_calibration(struct iwl_priv *priv)
>  
>  	spin_unlock_bh(&priv->statistics.lock);
>  
> +	/* Sometimes firmware returns zero for chain noise and signal
> +	 * even if chain is present and antenna is connected. This
> +	 * often results in perfectly valid chains being disabled.
> +	 * Ignore statistics if it contains zero noise for valid rx
> +	 * chain: even with antenna disconnected we should hear some noise.
> +	 */
> +	if ((priv->nvm_data->valid_rx_ant & ANT_A && chain_noise_a == 0) ||
> +	    (priv->nvm_data->valid_rx_ant & ANT_B && chain_noise_b == 0) ||
> +	    (priv->nvm_data->valid_rx_ant & ANT_C && chain_noise_c == 0)) {
> +		return;
> +	}
> +
>  	data->beacon_count++;
>  
>  	data->chain_noise_a = (chain_noise_a + data->chain_noise_a);

Re: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Nikolay Martynov]

Hi

2016-01-26 3:28 GMT-05:00 Grumbach, Emmanuel <emmanuel.grumbach@intel.com>:
>
>
> On 01/26/2016 12:20 AM, Nikolay Martynov wrote:
>> It looks like sometimes firmware returns zero for chain noise and signal
>> during calibration period. This seems to be a known problem and current
>> implementation accounts for this by ignoring invalid data when all chains
>> return zero signal and noise.
>>
>> The problem is that sometimes firmware returns zero for only one chain for
>> some (not all) beacons used for calibration. This leads to perfectly valid
>> chains be disabled and may cause invalid gain settings.
>> For example this is calibration data taken on laptop with Intel 6300 card
>> with all three antennas attached:
>>
>> active_chains:                         3
>> chain_noise_a:                         312
>> chain_noise_b:                         297
>> chain_noise_c:                         0
>> chain_signal_a:                        549
>> chain_signal_b:                        513
>> chain_signal_c:                        0
>> beacon_count:                  16
>> disconn_array:                         0 0 1
>> delta_gain_code:               4 0 0
>> radio_write:                   1
>> state:                                 3
>>
>> This patch changes statistics gathering to make sure that zero noise
>> results are ignored for valid rx chains. The rationale being that even if
>> anntenna is not connected we should be able to see non zero noise if rx
>> chain is present.
>
> But then the firmware will continue to send zero for signal and this
> will impact lots
> of flows like roaming. If the driver allows the firmware to use that
> antenna, the
> firmware may use this antenna for scanning and roaming will be broken.
> This seems to be a bug in the firmware, but there isn't much I can do
> about it.
> Sorry, I have to NACK this patch.

  Could you please elaborate on how this patch would affect roaming or
other things. As far as I can see this patch doesn't change much
behavior apart from ignoring invalid values from firmware.
Disconnected antennas still get disabled (as before) connected
antennas still work (more often than before). So I'm not sure I can
see how this patch would change what firmware does at all. I really
hope you could find a moment and explain this.

Thanks!
Nikolay.

RE: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Grumbach, Emmanuel]

PiBIaQ0KPiANCj4gMjAxNi0wMS0yNiAzOjI4IEdNVC0wNTowMCBHcnVtYmFjaCwgRW1tYW51ZWwN
Cj4gPGVtbWFudWVsLmdydW1iYWNoQGludGVsLmNvbT46DQo+ID4NCj4gPg0KPiA+IE9uIDAxLzI2
LzIwMTYgMTI6MjAgQU0sIE5pa29sYXkgTWFydHlub3Ygd3JvdGU6DQo+ID4+IEl0IGxvb2tzIGxp
a2Ugc29tZXRpbWVzIGZpcm13YXJlIHJldHVybnMgemVybyBmb3IgY2hhaW4gbm9pc2UgYW5kDQo+
ID4+IHNpZ25hbCBkdXJpbmcgY2FsaWJyYXRpb24gcGVyaW9kLiBUaGlzIHNlZW1zIHRvIGJlIGEg
a25vd24gcHJvYmxlbQ0KPiA+PiBhbmQgY3VycmVudCBpbXBsZW1lbnRhdGlvbiBhY2NvdW50cyBm
b3IgdGhpcyBieSBpZ25vcmluZyBpbnZhbGlkIGRhdGENCj4gPj4gd2hlbiBhbGwgY2hhaW5zIHJl
dHVybiB6ZXJvIHNpZ25hbCBhbmQgbm9pc2UuDQo+ID4+DQo+ID4+IFRoZSBwcm9ibGVtIGlzIHRo
YXQgc29tZXRpbWVzIGZpcm13YXJlIHJldHVybnMgemVybyBmb3Igb25seSBvbmUNCj4gPj4gY2hh
aW4gZm9yIHNvbWUgKG5vdCBhbGwpIGJlYWNvbnMgdXNlZCBmb3IgY2FsaWJyYXRpb24uIFRoaXMg
bGVhZHMgdG8NCj4gPj4gcGVyZmVjdGx5IHZhbGlkIGNoYWlucyBiZSBkaXNhYmxlZCBhbmQgbWF5
IGNhdXNlIGludmFsaWQgZ2FpbiBzZXR0aW5ncy4NCj4gPj4gRm9yIGV4YW1wbGUgdGhpcyBpcyBj
YWxpYnJhdGlvbiBkYXRhIHRha2VuIG9uIGxhcHRvcCB3aXRoIEludGVsIDYzMDANCj4gPj4gY2Fy
ZCB3aXRoIGFsbCB0aHJlZSBhbnRlbm5hcyBhdHRhY2hlZDoNCj4gPj4NCj4gPj4gYWN0aXZlX2No
YWluczogICAgICAgICAgICAgICAgICAgICAgICAgMw0KPiA+PiBjaGFpbl9ub2lzZV9hOiAgICAg
ICAgICAgICAgICAgICAgICAgICAzMTINCj4gPj4gY2hhaW5fbm9pc2VfYjogICAgICAgICAgICAg
ICAgICAgICAgICAgMjk3DQo+ID4+IGNoYWluX25vaXNlX2M6ICAgICAgICAgICAgICAgICAgICAg
ICAgIDANCj4gPj4gY2hhaW5fc2lnbmFsX2E6ICAgICAgICAgICAgICAgICAgICAgICAgNTQ5DQo+
ID4+IGNoYWluX3NpZ25hbF9iOiAgICAgICAgICAgICAgICAgICAgICAgIDUxMw0KPiA+PiBjaGFp
bl9zaWduYWxfYzogICAgICAgICAgICAgICAgICAgICAgICAwDQo+ID4+IGJlYWNvbl9jb3VudDog
ICAgICAgICAgICAgICAgICAxNg0KPiA+PiBkaXNjb25uX2FycmF5OiAgICAgICAgICAgICAgICAg
ICAgICAgICAwIDAgMQ0KPiA+PiBkZWx0YV9nYWluX2NvZGU6ICAgICAgICAgICAgICAgNCAwIDAN
Cj4gPj4gcmFkaW9fd3JpdGU6ICAgICAgICAgICAgICAgICAgIDENCj4gPj4gc3RhdGU6ICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgMw0KPiA+Pg0KPiA+PiBUaGlzIHBhdGNoIGNoYW5n
ZXMgc3RhdGlzdGljcyBnYXRoZXJpbmcgdG8gbWFrZSBzdXJlIHRoYXQgemVybyBub2lzZQ0KPiA+
PiByZXN1bHRzIGFyZSBpZ25vcmVkIGZvciB2YWxpZCByeCBjaGFpbnMuIFRoZSByYXRpb25hbGUg
YmVpbmcgdGhhdA0KPiA+PiBldmVuIGlmIGFubnRlbm5hIGlzIG5vdCBjb25uZWN0ZWQgd2Ugc2hv
dWxkIGJlIGFibGUgdG8gc2VlIG5vbiB6ZXJvDQo+ID4+IG5vaXNlIGlmIHJ4IGNoYWluIGlzIHBy
ZXNlbnQuDQo+ID4NCj4gPiBCdXQgdGhlbiB0aGUgZmlybXdhcmUgd2lsbCBjb250aW51ZSB0byBz
ZW5kIHplcm8gZm9yIHNpZ25hbCBhbmQgdGhpcw0KPiA+IHdpbGwgaW1wYWN0IGxvdHMgb2YgZmxv
d3MgbGlrZSByb2FtaW5nLiBJZiB0aGUgZHJpdmVyIGFsbG93cyB0aGUNCj4gPiBmaXJtd2FyZSB0
byB1c2UgdGhhdCBhbnRlbm5hLCB0aGUgZmlybXdhcmUgbWF5IHVzZSB0aGlzIGFudGVubmEgZm9y
DQo+ID4gc2Nhbm5pbmcgYW5kIHJvYW1pbmcgd2lsbCBiZSBicm9rZW4uDQo+ID4gVGhpcyBzZWVt
cyB0byBiZSBhIGJ1ZyBpbiB0aGUgZmlybXdhcmUsIGJ1dCB0aGVyZSBpc24ndCBtdWNoIEkgY2Fu
IGRvDQo+ID4gYWJvdXQgaXQuDQo+ID4gU29ycnksIEkgaGF2ZSB0byBOQUNLIHRoaXMgcGF0Y2gu
DQo+IA0KPiAgIENvdWxkIHlvdSBwbGVhc2UgZWxhYm9yYXRlIG9uIGhvdyB0aGlzIHBhdGNoIHdv
dWxkIGFmZmVjdCByb2FtaW5nIG9yIG90aGVyDQo+IHRoaW5ncy4gQXMgZmFyIGFzIEkgY2FuIHNl
ZSB0aGlzIHBhdGNoIGRvZXNuJ3QgY2hhbmdlIG11Y2ggYmVoYXZpb3IgYXBhcnQNCj4gZnJvbSBp
Z25vcmluZyBpbnZhbGlkIHZhbHVlcyBmcm9tIGZpcm13YXJlLg0KPiBEaXNjb25uZWN0ZWQgYW50
ZW5uYXMgc3RpbGwgZ2V0IGRpc2FibGVkIChhcyBiZWZvcmUpIGNvbm5lY3RlZCBhbnRlbm5hcyBz
dGlsbA0KPiB3b3JrIChtb3JlIG9mdGVuIHRoYW4gYmVmb3JlKS4gU28gSSdtIG5vdCBzdXJlIEkg
Y2FuIHNlZSBob3cgdGhpcyBwYXRjaA0KPiB3b3VsZCBjaGFuZ2Ugd2hhdCBmaXJtd2FyZSBkb2Vz
IGF0IGFsbC4gSSByZWFsbHkgaG9wZSB5b3UgY291bGQgZmluZCBhDQo+IG1vbWVudCBhbmQgZXhw
bGFpbiB0aGlzLg0KPiANCg0KV2hhdCB5b3UgYXJlIHNheWluZyBoZXJlIGlzIHRoYXQgdGhlcmUg
aXMgYSBidWcgaW4gdGhlIGZpcm13YXJlIHdoaWNoIG1ha2VzIGl0IHJlcG9ydCB3cm9uZw0KdmFs
dWVzIGZvciBvbmUgb2YgdGhlIGFudGVubmFzLiBCdXQgd2hlbiB5b3Ugd2lsbCBoYXZlIHRoaXMg
YW50ZW5uYSBlbmFibGVkICh3aXRoIHlvdXIgcGF0Y2gpLA0KdGhlIGZpcm13YXJlIHdpbGwga2Vl
cCBzZW5kaW5nIGJhZCBzaWduYWwgLyBub2lzZSB2YWx1ZXMgZm9yIGl0LiBJZiB0aGUgZHJpdmVy
IGFsbG93cyB0aGUgZmlybXdhcmUNCnRvIHVzZSB0aGlzIGFudGVubmEgKGFmdGVyIHlvdXIgcGF0
Y2gpLCB0aGUgZmlybXdhcmUgd2lsbCBjaG9vc2UgdGhpcyBhbnRlbm5hIHRvIHJlY2VpdmUgYmVh
Y29ucw0Kb3IgdG8gc2Nhbi4gVGhlbiwgdGhlIGRyaXZlciB3aWxsIGxvb2sgYXQgdGhlIGJlYWNv
bnMnIHJzc2kgKHdoaWNoIHdpbGwgYmUgd3JvbmcpIGFuZCBpdCB3aWxsIHRoaW5rIHRoYXQNCmFu
IEFQIHdoaWNoIGlzIHZlcnkgY2xvc2UgaXMgaW4gZmFjdCBmYXIgYXdheS4NCg0K

Re: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Nikolay Martynov]

2016-01-27 2:46 GMT-05:00 Grumbach, Emmanuel <emmanuel.grumbach@intel.com>:
>> Hi
>>
>> 2016-01-26 3:28 GMT-05:00 Grumbach, Emmanuel
>> <emmanuel.grumbach@intel.com>:
>> >
>> >
>> > On 01/26/2016 12:20 AM, Nikolay Martynov wrote:
>> >> It looks like sometimes firmware returns zero for chain noise and
>> >> signal during calibration period. This seems to be a known problem
>> >> and current implementation accounts for this by ignoring invalid data
>> >> when all chains return zero signal and noise.
>> >>
>> >> The problem is that sometimes firmware returns zero for only one
>> >> chain for some (not all) beacons used for calibration. This leads to
>> >> perfectly valid chains be disabled and may cause invalid gain settings.
>> >> For example this is calibration data taken on laptop with Intel 6300
>> >> card with all three antennas attached:
>> >>
>> >> active_chains:                         3
>> >> chain_noise_a:                         312
>> >> chain_noise_b:                         297
>> >> chain_noise_c:                         0
>> >> chain_signal_a:                        549
>> >> chain_signal_b:                        513
>> >> chain_signal_c:                        0
>> >> beacon_count:                  16
>> >> disconn_array:                         0 0 1
>> >> delta_gain_code:               4 0 0
>> >> radio_write:                   1
>> >> state:                                 3
>> >>
>> >> This patch changes statistics gathering to make sure that zero noise
>> >> results are ignored for valid rx chains. The rationale being that
>> >> even if anntenna is not connected we should be able to see non zero
>> >> noise if rx chain is present.
>> >
>> > But then the firmware will continue to send zero for signal and this
>> > will impact lots of flows like roaming. If the driver allows the
>> > firmware to use that antenna, the firmware may use this antenna for
>> > scanning and roaming will be broken.
>> > This seems to be a bug in the firmware, but there isn't much I can do
>> > about it.
>> > Sorry, I have to NACK this patch.
>>
>>   Could you please elaborate on how this patch would affect roaming or other
>> things. As far as I can see this patch doesn't change much behavior apart
>> from ignoring invalid values from firmware.
>> Disconnected antennas still get disabled (as before) connected antennas still
>> work (more often than before). So I'm not sure I can see how this patch
>> would change what firmware does at all. I really hope you could find a
>> moment and explain this.
>>
>
> What you are saying here is that there is a bug in the firmware which makes it report wrong
> values for one of the antennas. But when you will have this antenna enabled (with your patch),
> the firmware will keep sending bad signal / noise values for it. If the driver allows the firmware
> to use this antenna (after your patch), the firmware will choose this antenna to receive beacons
> or to scan. Then, the driver will look at the beacons' rssi (which will be wrong) and it will think that
> an AP which is very close is in fact far away.
>
No. That is not correct, I think. What I'm saying is that sometimes
(not always) firmware is sending 0 (exactly 0) for signal and noise
for some (or all) chains.
The case when all chains get 0 seem to be a known problem: it is
worked around in iwl_find_disconn_antenna. The case when only one
chain gets zero is not currently handled.
And just to clarify - all chains are affected by this problem, it's
not like one specific chain is broken in some way and gets zero. So
both of the cards I have may be running with 3 chains or with 2 chains
depending on how lucky I'm during initial scan.

It's just firmware that has a bug that sometimes returns zero for
chain 1, sometimes for chain 2, and sometimes for all of them.
So currently driver is already enabling chains for which we may get
zero later for rssi (presumably this is true) if it gets non zero
during scan for first 16 beacons.
Moreover, if it gets non-zero for 15 out of 16 beacons the chain is
not disabled but gain values are wrong because of this - and one chain
would be amplifying things more than it should - this is currently
happening to the best of my understanding.

So my patch filters out results that we know are bad to account for
this firmware bug.
With this patch all chains with antenna attached get signal and noise
reading - suggesting that firmware actually returns zero only some
times and after several retries we get reasonable statistics. It looks
like there are some 'transitioning' processes in firmware and if we
out-wait them we get good statistics.

I'm not sure I see how this patch makes anything more worse than they
currently already are.
Currently it is already (presumably) possible to get wrong rssi
reading because chain that may have been enabled during first scan may
get zeros later. All my patch does is to enable all equivalently good
(or bad) antennas, instead of two equivalently good (or bad) as
current code does.

Does this explanation make any sense? Is it flawed in some way?
If patch in it's current state seems too controversial would patch
that enables this check if some module parameter is set (and it is not
set my default) be more acceptable?

Thanks!

-- 
Martynov Nikolay.
Email: mar.kolya@gmail.com

RE: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Grumbach, Emmanuel]

PiANCj4gMjAxNi0wMS0yNyAyOjQ2IEdNVC0wNTowMCBHcnVtYmFjaCwgRW1tYW51ZWwNCj4gPGVt
bWFudWVsLmdydW1iYWNoQGludGVsLmNvbT46DQo+ID4+IEhpDQo+ID4+DQo+ID4+IDIwMTYtMDEt
MjYgMzoyOCBHTVQtMDU6MDAgR3J1bWJhY2gsIEVtbWFudWVsDQo+ID4+IDxlbW1hbnVlbC5ncnVt
YmFjaEBpbnRlbC5jb20+Og0KPiA+PiA+DQo+ID4+ID4NCj4gPj4gPiBPbiAwMS8yNi8yMDE2IDEy
OjIwIEFNLCBOaWtvbGF5IE1hcnR5bm92IHdyb3RlOg0KPiA+PiA+PiBJdCBsb29rcyBsaWtlIHNv
bWV0aW1lcyBmaXJtd2FyZSByZXR1cm5zIHplcm8gZm9yIGNoYWluIG5vaXNlIGFuZA0KPiA+PiA+
PiBzaWduYWwgZHVyaW5nIGNhbGlicmF0aW9uIHBlcmlvZC4gVGhpcyBzZWVtcyB0byBiZSBhIGtu
b3duIHByb2JsZW0NCj4gPj4gPj4gYW5kIGN1cnJlbnQgaW1wbGVtZW50YXRpb24gYWNjb3VudHMg
Zm9yIHRoaXMgYnkgaWdub3JpbmcgaW52YWxpZA0KPiA+PiA+PiBkYXRhIHdoZW4gYWxsIGNoYWlu
cyByZXR1cm4gemVybyBzaWduYWwgYW5kIG5vaXNlLg0KPiA+PiA+Pg0KPiA+PiA+PiBUaGUgcHJv
YmxlbSBpcyB0aGF0IHNvbWV0aW1lcyBmaXJtd2FyZSByZXR1cm5zIHplcm8gZm9yIG9ubHkgb25l
DQo+ID4+ID4+IGNoYWluIGZvciBzb21lIChub3QgYWxsKSBiZWFjb25zIHVzZWQgZm9yIGNhbGli
cmF0aW9uLiBUaGlzIGxlYWRzDQo+ID4+ID4+IHRvIHBlcmZlY3RseSB2YWxpZCBjaGFpbnMgYmUg
ZGlzYWJsZWQgYW5kIG1heSBjYXVzZSBpbnZhbGlkIGdhaW4NCj4gc2V0dGluZ3MuDQo+ID4+ID4+
IEZvciBleGFtcGxlIHRoaXMgaXMgY2FsaWJyYXRpb24gZGF0YSB0YWtlbiBvbiBsYXB0b3Agd2l0
aCBJbnRlbA0KPiA+PiA+PiA2MzAwIGNhcmQgd2l0aCBhbGwgdGhyZWUgYW50ZW5uYXMgYXR0YWNo
ZWQ6DQo+ID4+ID4+DQo+ID4+ID4+IGFjdGl2ZV9jaGFpbnM6ICAgICAgICAgICAgICAgICAgICAg
ICAgIDMNCj4gPj4gPj4gY2hhaW5fbm9pc2VfYTogICAgICAgICAgICAgICAgICAgICAgICAgMzEy
DQo+ID4+ID4+IGNoYWluX25vaXNlX2I6ICAgICAgICAgICAgICAgICAgICAgICAgIDI5Nw0KPiA+
PiA+PiBjaGFpbl9ub2lzZV9jOiAgICAgICAgICAgICAgICAgICAgICAgICAwDQo+ID4+ID4+IGNo
YWluX3NpZ25hbF9hOiAgICAgICAgICAgICAgICAgICAgICAgIDU0OQ0KPiA+PiA+PiBjaGFpbl9z
aWduYWxfYjogICAgICAgICAgICAgICAgICAgICAgICA1MTMNCj4gPj4gPj4gY2hhaW5fc2lnbmFs
X2M6ICAgICAgICAgICAgICAgICAgICAgICAgMA0KPiA+PiA+PiBiZWFjb25fY291bnQ6ICAgICAg
ICAgICAgICAgICAgMTYNCj4gPj4gPj4gZGlzY29ubl9hcnJheTogICAgICAgICAgICAgICAgICAg
ICAgICAgMCAwIDENCj4gPj4gPj4gZGVsdGFfZ2Fpbl9jb2RlOiAgICAgICAgICAgICAgIDQgMCAw
DQo+ID4+ID4+IHJhZGlvX3dyaXRlOiAgICAgICAgICAgICAgICAgICAxDQo+ID4+ID4+IHN0YXRl
OiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIDMNCj4gPj4gPj4NCj4gPj4gPj4gVGhp
cyBwYXRjaCBjaGFuZ2VzIHN0YXRpc3RpY3MgZ2F0aGVyaW5nIHRvIG1ha2Ugc3VyZSB0aGF0IHpl
cm8NCj4gPj4gPj4gbm9pc2UgcmVzdWx0cyBhcmUgaWdub3JlZCBmb3IgdmFsaWQgcnggY2hhaW5z
LiBUaGUgcmF0aW9uYWxlIGJlaW5nDQo+ID4+ID4+IHRoYXQgZXZlbiBpZiBhbm50ZW5uYSBpcyBu
b3QgY29ubmVjdGVkIHdlIHNob3VsZCBiZSBhYmxlIHRvIHNlZQ0KPiA+PiA+PiBub24gemVybyBu
b2lzZSBpZiByeCBjaGFpbiBpcyBwcmVzZW50Lg0KPiA+PiA+DQo+ID4+ID4gQnV0IHRoZW4gdGhl
IGZpcm13YXJlIHdpbGwgY29udGludWUgdG8gc2VuZCB6ZXJvIGZvciBzaWduYWwgYW5kDQo+ID4+
ID4gdGhpcyB3aWxsIGltcGFjdCBsb3RzIG9mIGZsb3dzIGxpa2Ugcm9hbWluZy4gSWYgdGhlIGRy
aXZlciBhbGxvd3MNCj4gPj4gPiB0aGUgZmlybXdhcmUgdG8gdXNlIHRoYXQgYW50ZW5uYSwgdGhl
IGZpcm13YXJlIG1heSB1c2UgdGhpcyBhbnRlbm5hDQo+ID4+ID4gZm9yIHNjYW5uaW5nIGFuZCBy
b2FtaW5nIHdpbGwgYmUgYnJva2VuLg0KPiA+PiA+IFRoaXMgc2VlbXMgdG8gYmUgYSBidWcgaW4g
dGhlIGZpcm13YXJlLCBidXQgdGhlcmUgaXNuJ3QgbXVjaCBJIGNhbg0KPiA+PiA+IGRvIGFib3V0
IGl0Lg0KPiA+PiA+IFNvcnJ5LCBJIGhhdmUgdG8gTkFDSyB0aGlzIHBhdGNoLg0KPiA+Pg0KPiA+
PiAgIENvdWxkIHlvdSBwbGVhc2UgZWxhYm9yYXRlIG9uIGhvdyB0aGlzIHBhdGNoIHdvdWxkIGFm
ZmVjdCByb2FtaW5nDQo+ID4+IG9yIG90aGVyIHRoaW5ncy4gQXMgZmFyIGFzIEkgY2FuIHNlZSB0
aGlzIHBhdGNoIGRvZXNuJ3QgY2hhbmdlIG11Y2gNCj4gPj4gYmVoYXZpb3IgYXBhcnQgZnJvbSBp
Z25vcmluZyBpbnZhbGlkIHZhbHVlcyBmcm9tIGZpcm13YXJlLg0KPiA+PiBEaXNjb25uZWN0ZWQg
YW50ZW5uYXMgc3RpbGwgZ2V0IGRpc2FibGVkIChhcyBiZWZvcmUpIGNvbm5lY3RlZA0KPiA+PiBh
bnRlbm5hcyBzdGlsbCB3b3JrIChtb3JlIG9mdGVuIHRoYW4gYmVmb3JlKS4gU28gSSdtIG5vdCBz
dXJlIEkgY2FuDQo+ID4+IHNlZSBob3cgdGhpcyBwYXRjaCB3b3VsZCBjaGFuZ2Ugd2hhdCBmaXJt
d2FyZSBkb2VzIGF0IGFsbC4gSSByZWFsbHkNCj4gPj4gaG9wZSB5b3UgY291bGQgZmluZCBhIG1v
bWVudCBhbmQgZXhwbGFpbiB0aGlzLg0KPiA+Pg0KPiA+DQo+ID4gV2hhdCB5b3UgYXJlIHNheWlu
ZyBoZXJlIGlzIHRoYXQgdGhlcmUgaXMgYSBidWcgaW4gdGhlIGZpcm13YXJlIHdoaWNoDQo+ID4g
bWFrZXMgaXQgcmVwb3J0IHdyb25nIHZhbHVlcyBmb3Igb25lIG9mIHRoZSBhbnRlbm5hcy4gQnV0
IHdoZW4geW91DQo+ID4gd2lsbCBoYXZlIHRoaXMgYW50ZW5uYSBlbmFibGVkICh3aXRoIHlvdXIg
cGF0Y2gpLCB0aGUgZmlybXdhcmUgd2lsbA0KPiA+IGtlZXAgc2VuZGluZyBiYWQgc2lnbmFsIC8g
bm9pc2UgdmFsdWVzIGZvciBpdC4gSWYgdGhlIGRyaXZlciBhbGxvd3MNCj4gPiB0aGUgZmlybXdh
cmUgdG8gdXNlIHRoaXMgYW50ZW5uYSAoYWZ0ZXIgeW91ciBwYXRjaCksIHRoZSBmaXJtd2FyZSB3
aWxsDQo+ID4gY2hvb3NlIHRoaXMgYW50ZW5uYSB0byByZWNlaXZlIGJlYWNvbnMgb3IgdG8gc2Nh
bi4gVGhlbiwgdGhlIGRyaXZlciB3aWxsDQo+IGxvb2sgYXQgdGhlIGJlYWNvbnMnIHJzc2kgKHdo
aWNoIHdpbGwgYmUgd3JvbmcpIGFuZCBpdCB3aWxsIHRoaW5rIHRoYXQgYW4gQVANCj4gd2hpY2gg
aXMgdmVyeSBjbG9zZSBpcyBpbiBmYWN0IGZhciBhd2F5Lg0KPiA+DQo+IE5vLiBUaGF0IGlzIG5v
dCBjb3JyZWN0LCBJIHRoaW5rLiBXaGF0IEknbSBzYXlpbmcgaXMgdGhhdCBzb21ldGltZXMgKG5v
dA0KPiBhbHdheXMpIGZpcm13YXJlIGlzIHNlbmRpbmcgMCAoZXhhY3RseSAwKSBmb3Igc2lnbmFs
IGFuZCBub2lzZSBmb3Igc29tZSAob3IgYWxsKQ0KPiBjaGFpbnMuDQo+IFRoZSBjYXNlIHdoZW4g
YWxsIGNoYWlucyBnZXQgMCBzZWVtIHRvIGJlIGEga25vd24gcHJvYmxlbTogaXQgaXMgd29ya2Vk
DQo+IGFyb3VuZCBpbiBpd2xfZmluZF9kaXNjb25uX2FudGVubmEuIFRoZSBjYXNlIHdoZW4gb25s
eSBvbmUgY2hhaW4gZ2V0cw0KPiB6ZXJvIGlzIG5vdCBjdXJyZW50bHkgaGFuZGxlZC4NCj4gQW5k
IGp1c3QgdG8gY2xhcmlmeSAtIGFsbCBjaGFpbnMgYXJlIGFmZmVjdGVkIGJ5IHRoaXMgcHJvYmxl
bSwgaXQncyBub3QgbGlrZSBvbmUNCj4gc3BlY2lmaWMgY2hhaW4gaXMgYnJva2VuIGluIHNvbWUg
d2F5IGFuZCBnZXRzIHplcm8uIFNvIGJvdGggb2YgdGhlIGNhcmRzIEkNCj4gaGF2ZSBtYXkgYmUg
cnVubmluZyB3aXRoIDMgY2hhaW5zIG9yIHdpdGggMiBjaGFpbnMgZGVwZW5kaW5nIG9uIGhvdyBs
dWNreQ0KPiBJJ20gZHVyaW5nIGluaXRpYWwgc2Nhbi4NCj4gDQo+IEl0J3MganVzdCBmaXJtd2Fy
ZSB0aGF0IGhhcyBhIGJ1ZyB0aGF0IHNvbWV0aW1lcyByZXR1cm5zIHplcm8gZm9yIGNoYWluIDEs
DQo+IHNvbWV0aW1lcyBmb3IgY2hhaW4gMiwgYW5kIHNvbWV0aW1lcyBmb3IgYWxsIG9mIHRoZW0u
DQo+IFNvIGN1cnJlbnRseSBkcml2ZXIgaXMgYWxyZWFkeSBlbmFibGluZyBjaGFpbnMgZm9yIHdo
aWNoIHdlIG1heSBnZXQgemVybyBsYXRlcg0KPiBmb3IgcnNzaSAocHJlc3VtYWJseSB0aGlzIGlz
IHRydWUpIGlmIGl0IGdldHMgbm9uIHplcm8gZHVyaW5nIHNjYW4gZm9yIGZpcnN0IDE2DQo+IGJl
YWNvbnMuDQo+IE1vcmVvdmVyLCBpZiBpdCBnZXRzIG5vbi16ZXJvIGZvciAxNSBvdXQgb2YgMTYg
YmVhY29ucyB0aGUgY2hhaW4gaXMgbm90DQo+IGRpc2FibGVkIGJ1dCBnYWluIHZhbHVlcyBhcmUg
d3JvbmcgYmVjYXVzZSBvZiB0aGlzIC0gYW5kIG9uZSBjaGFpbiB3b3VsZCBiZQ0KPiBhbXBsaWZ5
aW5nIHRoaW5ncyBtb3JlIHRoYW4gaXQgc2hvdWxkIC0gdGhpcyBpcyBjdXJyZW50bHkgaGFwcGVu
aW5nIHRvIHRoZSBiZXN0DQo+IG9mIG15IHVuZGVyc3RhbmRpbmcuDQo+IA0KPiBTbyBteSBwYXRj
aCBmaWx0ZXJzIG91dCByZXN1bHRzIHRoYXQgd2Uga25vdyBhcmUgYmFkIHRvIGFjY291bnQgZm9y
IHRoaXMNCj4gZmlybXdhcmUgYnVnLg0KPiBXaXRoIHRoaXMgcGF0Y2ggYWxsIGNoYWlucyB3aXRo
IGFudGVubmEgYXR0YWNoZWQgZ2V0IHNpZ25hbCBhbmQgbm9pc2UgcmVhZGluZyAtDQo+IHN1Z2dl
c3RpbmcgdGhhdCBmaXJtd2FyZSBhY3R1YWxseSByZXR1cm5zIHplcm8gb25seSBzb21lIHRpbWVz
IGFuZCBhZnRlcg0KPiBzZXZlcmFsIHJldHJpZXMgd2UgZ2V0IHJlYXNvbmFibGUgc3RhdGlzdGlj
cy4gSXQgbG9va3MgbGlrZSB0aGVyZSBhcmUgc29tZQ0KPiAndHJhbnNpdGlvbmluZycgcHJvY2Vz
c2VzIGluIGZpcm13YXJlIGFuZCBpZiB3ZSBvdXQtd2FpdCB0aGVtIHdlIGdldCBnb29kDQo+IHN0
YXRpc3RpY3MuDQo+IA0KPiBJJ20gbm90IHN1cmUgSSBzZWUgaG93IHRoaXMgcGF0Y2ggbWFrZXMg
YW55dGhpbmcgbW9yZSB3b3JzZSB0aGFuIHRoZXkNCj4gY3VycmVudGx5IGFscmVhZHkgYXJlLg0K
PiBDdXJyZW50bHkgaXQgaXMgYWxyZWFkeSAocHJlc3VtYWJseSkgcG9zc2libGUgdG8gZ2V0IHdy
b25nIHJzc2kgcmVhZGluZw0KPiBiZWNhdXNlIGNoYWluIHRoYXQgbWF5IGhhdmUgYmVlbiBlbmFi
bGVkIGR1cmluZyBmaXJzdCBzY2FuIG1heSBnZXQgemVyb3MNCj4gbGF0ZXIuIEFsbCBteSBwYXRj
aCBkb2VzIGlzIHRvIGVuYWJsZSBhbGwgZXF1aXZhbGVudGx5IGdvb2QgKG9yIGJhZCkgYW50ZW5u
YXMsDQo+IGluc3RlYWQgb2YgdHdvIGVxdWl2YWxlbnRseSBnb29kIChvciBiYWQpIGFzIGN1cnJl
bnQgY29kZSBkb2VzLg0KPiANCj4gRG9lcyB0aGlzIGV4cGxhbmF0aW9uIG1ha2UgYW55IHNlbnNl
PyBJcyBpdCBmbGF3ZWQgaW4gc29tZSB3YXk/DQo+IElmIHBhdGNoIGluIGl0J3MgY3VycmVudCBz
dGF0ZSBzZWVtcyB0b28gY29udHJvdmVyc2lhbCB3b3VsZCBwYXRjaCB0aGF0IGVuYWJsZXMNCj4g
dGhpcyBjaGVjayBpZiBzb21lIG1vZHVsZSBwYXJhbWV0ZXIgaXMgc2V0IChhbmQgaXQgaXMgbm90
IHNldCBteSBkZWZhdWx0KSBiZQ0KPiBtb3JlIGFjY2VwdGFibGU/DQo+IA0KDQpBcmUgeW91IHN1
cmUgdGhhdCB0aGUgYW50ZW5uYSB0aGF0IHJlcG9ydGVkIDAgInJlY292ZXJzIiBhbmQgcmVwb3J0
cyBnb29kIHZhbHVlcyBsYXRlcj8NCg==

Re: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Nikolay Martynov]

2016-01-28 2:43 GMT-05:00 Grumbach, Emmanuel <emmanuel.grumbach@intel.com>:
>>
>> 2016-01-27 2:46 GMT-05:00 Grumbach, Emmanuel
>> <emmanuel.grumbach@intel.com>:
>> >> Hi
>> >>
>> >> 2016-01-26 3:28 GMT-05:00 Grumbach, Emmanuel
>> >> <emmanuel.grumbach@intel.com>:
>> >> >
>> >> >
>> >> > On 01/26/2016 12:20 AM, Nikolay Martynov wrote:
>> >> >> It looks like sometimes firmware returns zero for chain noise and
>> >> >> signal during calibration period. This seems to be a known problem
>> >> >> and current implementation accounts for this by ignoring invalid
>> >> >> data when all chains return zero signal and noise.
>> >> >>
>> >> >> The problem is that sometimes firmware returns zero for only one
>> >> >> chain for some (not all) beacons used for calibration. This leads
>> >> >> to perfectly valid chains be disabled and may cause invalid gain
>> settings.
>> >> >> For example this is calibration data taken on laptop with Intel
>> >> >> 6300 card with all three antennas attached:
>> >> >>
>> >> >> active_chains:                         3
>> >> >> chain_noise_a:                         312
>> >> >> chain_noise_b:                         297
>> >> >> chain_noise_c:                         0
>> >> >> chain_signal_a:                        549
>> >> >> chain_signal_b:                        513
>> >> >> chain_signal_c:                        0
>> >> >> beacon_count:                  16
>> >> >> disconn_array:                         0 0 1
>> >> >> delta_gain_code:               4 0 0
>> >> >> radio_write:                   1
>> >> >> state:                                 3
>> >> >>
>> >> >> This patch changes statistics gathering to make sure that zero
>> >> >> noise results are ignored for valid rx chains. The rationale being
>> >> >> that even if anntenna is not connected we should be able to see
>> >> >> non zero noise if rx chain is present.
>> >> >
>> >> > But then the firmware will continue to send zero for signal and
>> >> > this will impact lots of flows like roaming. If the driver allows
>> >> > the firmware to use that antenna, the firmware may use this antenna
>> >> > for scanning and roaming will be broken.
>> >> > This seems to be a bug in the firmware, but there isn't much I can
>> >> > do about it.
>> >> > Sorry, I have to NACK this patch.
>> >>
>> >>   Could you please elaborate on how this patch would affect roaming
>> >> or other things. As far as I can see this patch doesn't change much
>> >> behavior apart from ignoring invalid values from firmware.
>> >> Disconnected antennas still get disabled (as before) connected
>> >> antennas still work (more often than before). So I'm not sure I can
>> >> see how this patch would change what firmware does at all. I really
>> >> hope you could find a moment and explain this.
>> >>
>> >
>> > What you are saying here is that there is a bug in the firmware which
>> > makes it report wrong values for one of the antennas. But when you
>> > will have this antenna enabled (with your patch), the firmware will
>> > keep sending bad signal / noise values for it. If the driver allows
>> > the firmware to use this antenna (after your patch), the firmware will
>> > choose this antenna to receive beacons or to scan. Then, the driver will
>> look at the beacons' rssi (which will be wrong) and it will think that an AP
>> which is very close is in fact far away.
>> >
>> No. That is not correct, I think. What I'm saying is that sometimes (not
>> always) firmware is sending 0 (exactly 0) for signal and noise for some (or all)
>> chains.
>> The case when all chains get 0 seem to be a known problem: it is worked
>> around in iwl_find_disconn_antenna. The case when only one chain gets
>> zero is not currently handled.
>> And just to clarify - all chains are affected by this problem, it's not like one
>> specific chain is broken in some way and gets zero. So both of the cards I
>> have may be running with 3 chains or with 2 chains depending on how lucky
>> I'm during initial scan.
>>
>> It's just firmware that has a bug that sometimes returns zero for chain 1,
>> sometimes for chain 2, and sometimes for all of them.
>> So currently driver is already enabling chains for which we may get zero later
>> for rssi (presumably this is true) if it gets non zero during scan for first 16
>> beacons.
>> Moreover, if it gets non-zero for 15 out of 16 beacons the chain is not
>> disabled but gain values are wrong because of this - and one chain would be
>> amplifying things more than it should - this is currently happening to the best
>> of my understanding.
>>
>> So my patch filters out results that we know are bad to account for this
>> firmware bug.
>> With this patch all chains with antenna attached get signal and noise reading -
>> suggesting that firmware actually returns zero only some times and after
>> several retries we get reasonable statistics. It looks like there are some
>> 'transitioning' processes in firmware and if we out-wait them we get good
>> statistics.
>>
>> I'm not sure I see how this patch makes anything more worse than they
>> currently already are.
>> Currently it is already (presumably) possible to get wrong rssi reading
>> because chain that may have been enabled during first scan may get zeros
>> later. All my patch does is to enable all equivalently good (or bad) antennas,
>> instead of two equivalently good (or bad) as current code does.
>>
>> Does this explanation make any sense? Is it flawed in some way?
>> If patch in it's current state seems too controversial would patch that enables
>> this check if some module parameter is set (and it is not set my default) be
>> more acceptable?
>>
>
> Are you sure that the antenna that reported 0 "recovers" and reports good values later?

In fact it does, but apparently that's not the problem (I've checked
with monitor device and after seeing zeros during calibration I see
packets and max rate coming in from AP).

I've done some more testing and it turns out that original problem of
one antenna being disabled is caused by power_save parameter being set
to true. I cannot reproduce the problem with that parameter being set
to false.

I guess firmware turns off some receiving antennas to conserve power
and reports that stat as zero.

I see there is some logic to prevent power saving from affecting this
calibration but it looks like it doesn't work properly when
power_save=true and unfortunately I could not immediately figure out
why.

Please disregard this patch because original problem is cause by power
saving, not by firmware issue (at least as far as I could reproduce).

Sorry for not testing it more thoroughly before sending the patch and
thanks for kindly reviewing it and providing comments!

-- 
Martynov Nikolay.
Email: mar.kolya@gmail.com

RE: [PATCH v2] iwldvm: fix chain gain calibration when firmware return zero values

[Grumbach, Emmanuel]

WyBzbmlwIF0NCg0KPiA+Pg0KPiA+DQo+ID4gQXJlIHlvdSBzdXJlIHRoYXQgdGhlIGFudGVubmEg
dGhhdCByZXBvcnRlZCAwICJyZWNvdmVycyIgYW5kIHJlcG9ydHMgZ29vZA0KPiB2YWx1ZXMgbGF0
ZXI/DQo+IA0KPiBJbiBmYWN0IGl0IGRvZXMsIGJ1dCBhcHBhcmVudGx5IHRoYXQncyBub3QgdGhl
IHByb2JsZW0gKEkndmUgY2hlY2tlZCB3aXRoDQo+IG1vbml0b3IgZGV2aWNlIGFuZCBhZnRlciBz
ZWVpbmcgemVyb3MgZHVyaW5nIGNhbGlicmF0aW9uIEkgc2VlIHBhY2tldHMgYW5kDQo+IG1heCBy
YXRlIGNvbWluZyBpbiBmcm9tIEFQKS4NCj4gDQo+IEkndmUgZG9uZSBzb21lIG1vcmUgdGVzdGlu
ZyBhbmQgaXQgdHVybnMgb3V0IHRoYXQgb3JpZ2luYWwgcHJvYmxlbSBvZiBvbmUNCj4gYW50ZW5u
YSBiZWluZyBkaXNhYmxlZCBpcyBjYXVzZWQgYnkgcG93ZXJfc2F2ZSBwYXJhbWV0ZXIgYmVpbmcg
c2V0IHRvDQo+IHRydWUuIEkgY2Fubm90IHJlcHJvZHVjZSB0aGUgcHJvYmxlbSB3aXRoIHRoYXQg
cGFyYW1ldGVyIGJlaW5nIHNldCB0byBmYWxzZS4NCj4gDQo+IEkgZ3Vlc3MgZmlybXdhcmUgdHVy
bnMgb2ZmIHNvbWUgcmVjZWl2aW5nIGFudGVubmFzIHRvIGNvbnNlcnZlIHBvd2VyIGFuZA0KPiBy
ZXBvcnRzIHRoYXQgc3RhdCBhcyB6ZXJvLg0KPiANCj4gSSBzZWUgdGhlcmUgaXMgc29tZSBsb2dp
YyB0byBwcmV2ZW50IHBvd2VyIHNhdmluZyBmcm9tIGFmZmVjdGluZyB0aGlzDQo+IGNhbGlicmF0
aW9uIGJ1dCBpdCBsb29rcyBsaWtlIGl0IGRvZXNuJ3Qgd29yayBwcm9wZXJseSB3aGVuIHBvd2Vy
X3NhdmU9dHJ1ZQ0KPiBhbmQgdW5mb3J0dW5hdGVseSBJIGNvdWxkIG5vdCBpbW1lZGlhdGVseSBm
aWd1cmUgb3V0IHdoeS4NCj4gDQo+IFBsZWFzZSBkaXNyZWdhcmQgdGhpcyBwYXRjaCBiZWNhdXNl
IG9yaWdpbmFsIHByb2JsZW0gaXMgY2F1c2UgYnkgcG93ZXINCj4gc2F2aW5nLCBub3QgYnkgZmly
bXdhcmUgaXNzdWUgKGF0IGxlYXN0IGFzIGZhciBhcyBJIGNvdWxkIHJlcHJvZHVjZSkuDQo+IA0K
PiBTb3JyeSBmb3Igbm90IHRlc3RpbmcgaXQgbW9yZSB0aG9yb3VnaGx5IGJlZm9yZSBzZW5kaW5n
IHRoZSBwYXRjaCBhbmQgdGhhbmtzDQo+IGZvciBraW5kbHkgcmV2aWV3aW5nIGl0IGFuZCBwcm92
aWRpbmcgY29tbWVudHMhDQo+IA0KDQpObyB3b3JyaWVzLCByZWFsbHkgOikNCg0K