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

[Grumbach, Emmanuel]

[ snip ]

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

No worries, really :)

N�r��yb�X��ǧv�^�)޺{.n�+{��*ޕ�,�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

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 :
>>
>> 2016-01-27 2:46 GMT-05:00 Grumbach, Emmanuel
>> :
>> >> Hi
>> >>
>> >> 2016-01-26 3:28 GMT-05:00 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.
>> >>
>> >>   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
>> 

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

[Grumbach, Emmanuel]

> 
> 2016-01-27 2:46 GMT-05:00 Grumbach, Emmanuel
> :
> >> Hi
> >>
> >> 2016-01-26 3:28 GMT-05:00 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.
> >>
> >>   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 

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 :
>> Hi
>>
>> 2016-01-26 3:28 GMT-05:00 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.
>>
>>   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 

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

[Grumbach, Emmanuel]

> Hi
> 
> 2016-01-26 3:28 GMT-05:00 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.
> 
>   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.

N�r��yb�X��ǧv�^�)޺{.n�+{��*ޕ�,�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

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 
> ---
>  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(>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);

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

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