On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan <[email protected]> wrote: > Hi Michal, > > On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote: >> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan >> <[email protected]> wrote: >> [...] >> > +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. 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. 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. Michał _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
