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

[Srinivasa Duvvuri <sduvvuri@google.com>]

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