[PATCH] ath10k: handle cycle count wrap around

[PATCH] ath10k: handle cycle count wrap around

[Srinivasa Duvvuri]

When the cycle counts part of wmi_ch_info_ev_arg reach max value of
0xffffffff The HW/FW right shifts all the counters by 1 . The cycle
count will be reset to 0x7fffffff and starts counting from 0x7fffffff.
At the same time all the other counters will also have their values
right shifted by 1 (divided by 2). There is no way to handle this odd
wrap around. Detect and ignore the wrap around case.
This change has 2 parts.

 1) Ignore all the cycle counts , if the cycle count field wraps around.
 2) With HW frequency of 88MHz the cycle count wraps around every 24
seconds. the survey_last_cycle_count saved will become stale with in
24 seconds. Reset it to 0 at every scan start.

Signed-off-by: Srinivasa Duvvuri <sduvvuri@chromium.org>

diff --git a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
index 720b6eb..09616bb 100644
--- a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
+++ b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
@@ -1158,6 +1158,8 @@
  complete(&ar->scan.started);
  break;
  }
+ /* reset the last cycle count as the last cycle count is stale from
last scan. */
+ ar->survey_last_cycle_count = 0;
 }

 static void ath10k_wmi_event_scan_start_failed(struct ath10k *ar)
@@ -1716,21 +1718,36 @@
  goto exit;
  }

- if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE) {
+ if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE && ar->survey_last_cycle_count) {
  /* During scanning chan info is reported twice for each
- * visited channel. The reported cycle count is global
+ * visited channel. The reported cycle count is global
  * and per-channel cycle count must be calculated */

- cycle_count -= ar->survey_last_cycle_count;
- rx_clear_count -= ar->survey_last_rx_clear_count;
-
  survey = &ar->survey[idx];
- survey->channel_time = WMI_CHAN_INFO_MSEC(cycle_count);
- survey->channel_time_rx = WMI_CHAN_INFO_MSEC(rx_clear_count);
+
+ if (cycle_count  <  ar->survey_last_cycle_count) {
+ /*
+  * Ignore the wrapped ar^[ust_cycnd data. When the total cycle count
+  * wraps around, FW/HW will right shift all the counts
+  * (including total cycle counts) by 1.The cycle counts wrap around
+  * every 24 seconds. To handle this wrap around behavior,
+  * we need the value of each counter at the time of wrap around which
+  * FW does not provide. For now ignore the data.
+  */
+ survey->filled &= ~(SURVEY_INFO_CHANNEL_TIME |
+    SURVEY_INFO_CHANNEL_TIME_BUSY);
+ } else {
+ cycle_count -= ar->survey_last_cycle_count;
+ rx_clear_count -= ar->survey_last_rx_clear_count;
+
+ survey->channel_time = WMI_CHAN_INFO_MSEC(cycle_count);
+ /* the rx_clear_count count indicates the busy time */
+ survey->channel_time_busy = WMI_CHAN_INFO_MSEC(rx_clear_count);
+ survey->filled = SURVEY_INFO_CHANNEL_TIME |
+ SURVEY_INFO_CHANNEL_TIME_BUSY;
+ }
  survey->noise = noise_floor;
- survey->filled = SURVEY_INFO_CHANNEL_TIME |
- SURVEY_INFO_CHANNEL_TIME_RX |
- SURVEY_INFO_NOISE_DBM;
+ survey->filled |= SURVEY_INFO_NOISE_DBM;
  }

  ar->survey_last_rx_clear_count = rx_clear_count;

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle cycle count wrap around

[Michal Kazior]

On 14 May 2015 at 03:56, Srinivasa Duvvuri <sduvvuri@google.com> wrote:
> When the cycle counts part of wmi_ch_info_ev_arg reach max value of
> 0xffffffff The HW/FW right shifts all the counters by 1 . The cycle
> count will be reset to 0x7fffffff and starts counting from 0x7fffffff.
> At the same time all the other counters will also have their values
> right shifted by 1 (divided by 2). There is no way to handle this odd
> wrap around. Detect and ignore the wrap around case.
> This change has 2 parts.
>
>  1) Ignore all the cycle counts , if the cycle count field wraps around.
>  2) With HW frequency of 88MHz the cycle count wraps around every 24
> seconds. the survey_last_cycle_count saved will become stale with in
> 24 seconds. Reset it to 0 at every scan start.

So what does it fix from a user perspective?


>
> Signed-off-by: Srinivasa Duvvuri <sduvvuri@chromium.org>
>
> diff --git a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
> b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
> index 720b6eb..09616bb 100644
> --- a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless-3.18/ath/ath10k/wmi.c

This doesn't look like an upstream kernel patch. Rebase it for
github.com/kvalo/ath, please.


> @@ -1158,6 +1158,8 @@
>   complete(&ar->scan.started);
>   break;
>   }
> + /* reset the last cycle count as the last cycle count is stale from
> last scan. */
> + ar->survey_last_cycle_count = 0;
>  }
>
>  static void ath10k_wmi_event_scan_start_failed(struct ath10k *ar)
> @@ -1716,21 +1718,36 @@
>   goto exit;
>   }
>
> - if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE) {
> + if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE && ar->survey_last_cycle_count) {

Channel info events come in pairs: without and with FLAG_COMPLETE
respectively. Thus it is pointless to reset
`ar->survey_last_cycle_count` and it's pointless to use it in this
condition.


>   /* During scanning chan info is reported twice for each
> - * visited channel. The reported cycle count is global
> + * visited channel. The reported cycle count is global
>   * and per-channel cycle count must be calculated */
>
> - cycle_count -= ar->survey_last_cycle_count;
> - rx_clear_count -= ar->survey_last_rx_clear_count;
> -
>   survey = &ar->survey[idx];
> - survey->channel_time = WMI_CHAN_INFO_MSEC(cycle_count);
> - survey->channel_time_rx = WMI_CHAN_INFO_MSEC(rx_clear_count);
> +
> + if (cycle_count  <  ar->survey_last_cycle_count) {
> + /*
> +  * Ignore the wrapped ar ust_cycnd data. When the total cycle count
> +  * wraps around, FW/HW will right shift all the counts
> +  * (including total cycle counts) by 1.The cycle counts wrap around
> +  * every 24 seconds. To handle this wrap around behavior,
> +  * we need the value of each counter at the time of wrap around which
> +  * FW does not provide. For now ignore the data.
> +  */

If you really think about it, this can be calculated.

Channel visit shouldn't be longer than 24s, not even 12s. It's mostly
20-100ms for hw_scan and up to 5s in offchannel (which is rare
anyway).

Hence you can detect the wrap around AND calculate the actual time difference.

Your patch seems like it's whitespace-damaged. Please use git send-email.

Also fix styling issues, please - you have typo in the comment, modify
a comment line needlessly, exceed 80 chars in line and use multiple
spaces between operator `<`.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle cycle count wrap around

[Srinivasa Duvvuri]

On Wed, May 13, 2015 at 10:25 PM, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 14 May 2015 at 03:56, Srinivasa Duvvuri <sduvvuri@google.com> wrote:
>> When the cycle counts part of wmi_ch_info_ev_arg reach max value of
>> 0xffffffff The HW/FW right shifts all the counters by 1 . The cycle
>> count will be reset to 0x7fffffff and starts counting from 0x7fffffff.
>> At the same time all the other counters will also have their values
>> right shifted by 1 (divided by 2). There is no way to handle this odd
>> wrap around. Detect and ignore the wrap around case.
>> This change has 2 parts.
>>
>>  1) Ignore all the cycle counts , if the cycle count field wraps around.
>>  2) With HW frequency of 88MHz the cycle count wraps around every 24
>> seconds. the survey_last_cycle_count saved will become stale with in
>> 24 seconds. Reset it to 0 at every scan start.
>
> So what does it fix from a user perspective?

User gets a really large/incorrect value at random times when he dumps
 the survey data
using iw dev wlan0 survey dump

>
>
>>
>> Signed-off-by: Srinivasa Duvvuri <sduvvuri@chromium.org>
>>
>> diff --git a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>> b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>> index 720b6eb..09616bb 100644
>> --- a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>
> This doesn't look like an upstream kernel patch. Rebase it for
> github.com/kvalo/ath, please.
>
>
>> @@ -1158,6 +1158,8 @@
>>   complete(&ar->scan.started);
>>   break;
>>   }
>> + /* reset the last cycle count as the last cycle count is stale from
>> last scan. */
>> + ar->survey_last_cycle_count = 0;
>>  }
>>
>>  static void ath10k_wmi_event_scan_start_failed(struct ath10k *ar)
>> @@ -1716,21 +1718,36 @@
>>   goto exit;
>>   }
>>
>> - if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE) {
>> + if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE && ar->survey_last_cycle_count) {
>
> Channel info events come in pairs: without and with FLAG_COMPLETE
> respectively. Thus it is pointless to reset
> `ar->survey_last_cycle_count` and it's pointless to use it in this
> condition.

You are right, this check seems to be not useful, but it addresses one
case where between 2 scans
FW generates 2 events back to back with WMI_CHAN_INFO_FLAG_COMPLETE flag set.
 here is how the events are generated from FW with 2 scans issued  .

 scan req 1 to FW.
 scan started  event from FW.
 scan bss_channel  event from FW .
 chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag set.
 scan foreign channel event from FW
 chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag cleared.
 scan bss_channel  event from FW .
 chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag set.
  ......
  ......
 scan foreign channel event from FW
 chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag cleared.
 scan bss_channel  event from FW .
 chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag set.
 scan event completed.


  scan req 2 to FW
  scan started  event from FW.
  scan bss_channel  event from FW .
  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
WMI_CHAN_INFO_FLAG_COMPLETE flag set.
  ............
  ............


From the events above there is one chan info event from FW with the
complete flag at the end of first scan
and  one chan info event  from FW at the beginning of second scan. The
logic in ath10k computes the
difference between the 2 events . Depending upon the time at which the
second scan is issued , the HW cycle
count could have rolled over several times. the check above ignores
the first chan info event.

Alternatively we can fix the FW to not generate first bss chan event
and the corresponding chan info event.
I guess the idea of having a bss chan event at the beginning and end
of scan followed by the chan
info event is to provide the survey  data on bss channel between the
scans. Since these counters wrap
around every 24 seconds,  the difference is not going to be reliable any way.

>
>
>>   /* During scanning chan info is reported twice for each
>> - * visited channel. The reported cycle count is global
>> + * visited channel. The reported cycle count is global
>>   * and per-channel cycle count must be calculated */
>>
>> - cycle_count -= ar->survey_last_cycle_count;
>> - rx_clear_count -= ar->survey_last_rx_clear_count;
>> -
>>   survey = &ar->survey[idx];
>> - survey->channel_time = WMI_CHAN_INFO_MSEC(cycle_count);
>> - survey->channel_time_rx = WMI_CHAN_INFO_MSEC(rx_clear_count);
>> +
>> + if (cycle_count  <  ar->survey_last_cycle_count) {
>> + /*
>> +  * Ignore the wrapped ar ust_cycnd data. When the total cycle count
>> +  * wraps around, FW/HW will right shift all the counts
>> +  * (including total cycle counts) by 1.The cycle counts wrap around
>> +  * every 24 seconds. To handle this wrap around behavior,
>> +  * we need the value of each counter at the time of wrap around which
>> +  * FW does not provide. For now ignore the data.
>> +  */
>
> If you really think about it, this can be calculated.
>
> Channel visit shouldn't be longer than 24s, not even 12s. It's mostly
> 20-100ms for hw_scan and up to 5s in offchannel (which is rare
> anyway).
>
> Hence you can detect the wrap around AND calculate the actual time difference.

These FW counters increment continuously and also across channel
changes and between scans.
So wrap around can happen  any time.

>
> Your patch seems like it's whitespace-damaged. Please use git send-email.
>
> Also fix styling issues, please - you have typo in the comment, modify
> a comment line needlessly, exceed 80 chars in line and use multiple
> spaces between operator `<`.

Will fix them and send out another patch.
>
>
> Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle cycle count wrap around

[Michal Kazior]

On 14 May 2015 at 20:40, Srinivasa Duvvuri <sduvvuri@google.com> wrote:
> On Wed, May 13, 2015 at 10:25 PM, Michal Kazior <michal.kazior@tieto.com> wrote:
>> On 14 May 2015 at 03:56, Srinivasa Duvvuri <sduvvuri@google.com> wrote:
>>> When the cycle counts part of wmi_ch_info_ev_arg reach max value of
>>> 0xffffffff The HW/FW right shifts all the counters by 1 . The cycle
>>> count will be reset to 0x7fffffff and starts counting from 0x7fffffff.
>>> At the same time all the other counters will also have their values
>>> right shifted by 1 (divided by 2). There is no way to handle this odd
>>> wrap around. Detect and ignore the wrap around case.
>>> This change has 2 parts.
>>>
>>>  1) Ignore all the cycle counts , if the cycle count field wraps around.
>>>  2) With HW frequency of 88MHz the cycle count wraps around every 24
>>> seconds. the survey_last_cycle_count saved will become stale with in
>>> 24 seconds. Reset it to 0 at every scan start.
>>
>> So what does it fix from a user perspective?
>
> User gets a really large/incorrect value at random times when he dumps
>  the survey data
> using iw dev wlan0 survey dump

This should be put in the commit log so anyone reading it will
understand what this patch fixes from the user point of view.


>>> Signed-off-by: Srinivasa Duvvuri <sduvvuri@chromium.org>
>>>
>>> diff --git a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>>> b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>>> index 720b6eb..09616bb 100644
>>> --- a/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>>> +++ b/drivers/net/wireless-3.18/ath/ath10k/wmi.c
>>
>> This doesn't look like an upstream kernel patch. Rebase it for
>> github.com/kvalo/ath, please.
>>
>>
>>> @@ -1158,6 +1158,8 @@
>>>   complete(&ar->scan.started);
>>>   break;
>>>   }
>>> + /* reset the last cycle count as the last cycle count is stale from
>>> last scan. */
>>> + ar->survey_last_cycle_count = 0;
>>>  }
>>>
>>>  static void ath10k_wmi_event_scan_start_failed(struct ath10k *ar)
>>> @@ -1716,21 +1718,36 @@
>>>   goto exit;
>>>   }
>>>
>>> - if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE) {
>>> + if (cmd_flags & WMI_CHAN_INFO_FLAG_COMPLETE && ar->survey_last_cycle_count) {
>>
>> Channel info events come in pairs: without and with FLAG_COMPLETE
>> respectively. Thus it is pointless to reset
>> `ar->survey_last_cycle_count` and it's pointless to use it in this
>> condition.
>
> You are right, this check seems to be not useful, but it addresses one
> case where between 2 scans
> FW generates 2 events back to back with WMI_CHAN_INFO_FLAG_COMPLETE flag set.
>  here is how the events are generated from FW with 2 scans issued  .
>
>  scan req 1 to FW.
>  scan started  event from FW.
>  scan bss_channel  event from FW .
>  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag set.
>  scan foreign channel event from FW
>  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag cleared.
>  scan bss_channel  event from FW .
>  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag set.
>   ......
>   ......
>  scan foreign channel event from FW
>  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag cleared.
>  scan bss_channel  event from FW .
>  chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag set.
>  scan event completed.
>
>
>   scan req 2 to FW
>   scan started  event from FW.
>   scan bss_channel  event from FW .
>   chan info event ( WMI_CHAN_INFO_EVENTID ) from FW with
> WMI_CHAN_INFO_FLAG_COMPLETE flag set.
>   ............
>   ............
>
>
> From the events above there is one chan info event from FW with the
> complete flag at the end of first scan
> and  one chan info event  from FW at the beginning of second scan. The
> logic in ath10k computes the
> difference between the 2 events . Depending upon the time at which the
> second scan is issued , the HW cycle
> count could have rolled over several times. the check above ignores
> the first chan info event.
>
> Alternatively we can fix the FW to not generate first bss chan event
> and the corresponding chan info event.
> I guess the idea of having a bss chan event at the beginning and end
> of scan followed by the chan
> info event is to provide the survey  data on bss channel between the
> scans. Since these counters wrap
> around every 24 seconds,  the difference is not going to be reliable any way.

Oh. I've never noticed that. I guess that's because I've mostly tested
survey with `iw scan` which scans all channels including BSS channel.
Otherwise this problem doesn't manifest because BSS-related chan-info
pair is overwritten by subsequent foreign-related chan-info.


>>>   /* During scanning chan info is reported twice for each
>>> - * visited channel. The reported cycle count is global
>>> + * visited channel. The reported cycle count is global
>>>   * and per-channel cycle count must be calculated */
>>>
>>> - cycle_count -= ar->survey_last_cycle_count;
>>> - rx_clear_count -= ar->survey_last_rx_clear_count;
>>> -
>>>   survey = &ar->survey[idx];
>>> - survey->channel_time = WMI_CHAN_INFO_MSEC(cycle_count);
>>> - survey->channel_time_rx = WMI_CHAN_INFO_MSEC(rx_clear_count);
>>> +
>>> + if (cycle_count  <  ar->survey_last_cycle_count) {
>>> + /*
>>> +  * Ignore the wrapped ar ust_cycnd data. When the total cycle count
>>> +  * wraps around, FW/HW will right shift all the counts
>>> +  * (including total cycle counts) by 1.The cycle counts wrap around
>>> +  * every 24 seconds. To handle this wrap around behavior,
>>> +  * we need the value of each counter at the time of wrap around which
>>> +  * FW does not provide. For now ignore the data.
>>> +  */
>>
>> If you really think about it, this can be calculated.
>>
>> Channel visit shouldn't be longer than 24s, not even 12s. It's mostly
>> 20-100ms for hw_scan and up to 5s in offchannel (which is rare
>> anyway).
>>
>> Hence you can detect the wrap around AND calculate the actual time difference.
>
> These FW counters increment continuously and also across channel
> changes and between scans.
> So wrap around can happen  any time.

For foreign-channel visits this can be easily calculated since they
are never longer than the wraparound time.

BSS-channel visits (or between-scans if you will) should be just
discarded though as you suggested. I'm not fond of re-using
ar->survey_last_cycle_count though because it always stores a value
and 0 is just one of them. Adding a special meaning to 0 looks ugly to
me. I would suggest a dedicated var which tracks whether chan_info
event without _FLAG_COMPLETE came in. Something along the logic of:

 event_chan_info():
  ...
  if (flags & COMPLETE) {
     if (ar->ch_info_can_process)
        update(ar->survey);

     ar->ch_info_can_process = false;
  } else {
     ar->ch_info_can_process = true;
  }

This cleanly enforces the expectation that was assumed before, i.e.
that ch_info comes in pairs always.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle cycle count wrap around

[Srinivasa Duvvuri]

Please find my responses inline.



>
> For foreign-channel visits this can be easily calculated since they
> are never longer than the wraparound time.

As explained before, the counter values keep increasing across all
channel changes
and they never reset to 0. if they had been reset to zero on every
foreign channel change
then we do not need to handle wrap around as you suggested.
Since they are not reset, we need to handle wrap around .
>
> BSS-channel visits (or between-scans if you will) should be just
> discarded though as you suggested. I'm not fond of re-using
> ar->survey_last_cycle_count though because it always stores a value
> and 0 is just one of them. Adding a special meaning to 0 looks ugly to
> me. I would suggest a dedicated var which tracks whether chan_info
> event without _FLAG_COMPLETE came in. Something along the logic of:
>
>  event_chan_info():
>   ...
>   if (flags & COMPLETE) {
>      if (ar->ch_info_can_process)
>         update(ar->survey);
>
>      ar->ch_info_can_process = false;
>   } else {
>      ar->ch_info_can_process = true;
>   }
>
> This cleanly enforces the expectation that was assumed before, i.e.
> that ch_info comes in pairs always.

agreed this is much cleaner. I will make the change as you suggested.

>
>
> Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k