Hi Michal,

thanks for the comments ..

On Thu, Mar 17, 2016 at 12:35:00PM +0100, Michal Kazior wrote:

On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
[off-list ref] wrote:

quoted

Hi Michal,

On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:

quoted

On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
[off-list ref] wrote:
[...]

quoted

+void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+                          struct ieee80211_sta *sta,
+                          struct station_info *sinfo)
+{
+       struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+       struct ath10k *ar = arsta->arvif->ar;
+
+       mutex_lock(&ar->conf_mutex);
+
+       if (ar->state != ATH10K_STATE_ON &&
+           ar->state != ATH10K_STATE_RESTARTED)
+               goto out;

Do you really need mutex and ar->state check in this function?

[shafi] By default peer stats will be disabled, we are enabling this by debugfs
(hw-restart) so i thought these checks are needed , please advise .. Do you say
they  will be never hit

Hmm.. I think mac80211 shouldn't call sta_statistics() before
sta_state() during recovery (it makes no sense). In practice I think
this isn't enforced in which case it's a mac80211 bug.

[shafi] sure i will check this. If the hardware is restarting there should
be no stations connected and station related info.

Anyway, this isn't much of a problem now. You only read out u64 from
`arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
no way for you to crash the system (it's not a list, spinlock or any
other complex data structure). Worst case userspace will get garbage
rx_duration value if it happens to get_station() while hw-restart is
ongoing.

[shafi] will check this ..

It'd be good to verify this is actually a problem and - assuming you
want to guarantee correct readouts at all times - to fix the problem
in mac80211.

[shafi] ok, sure


-shafi