Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:57406 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752650AbcCQMa0 (ORCPT ); Thu, 17 Mar 2016 08:30:26 -0400 Date: Thu, 17 Mar 2016 18:00:08 +0530 From: Mohammed Shafi Shajakhan To: Michal Kazior Cc: Mohammed Shafi Shajakhan , "ath10k@lists.infradead.org" , Kalle Valo , "kvalo@codeaurora.org" , linux-wireless Subject: Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support Message-ID: <20160317122956.GA30285@atheros-ThinkPad-T61> (sfid-20160317_133031_068624_292F1567) References: <1458211740-6875-1-git-send-email-mohammed@qca.qualcomm.com> <20160317112022.GA27512@atheros-ThinkPad-T61> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-wireless-owner@vger.kernel.org List-ID: 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 > 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 > >> 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. [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