Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support

[Mohammed Shafi Shajakhan <mohammed@codeaurora.org>]

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
> <mohammed@codeaurora.org> 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
> >> <mohammed@qti.qualcomm.com> 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