RE: [PATCH 1/2] mac80211/wlcore: Add ieee80211_hw variable to get_expected_throughput

[Altshul, Maxim]

Yes, exactly! Should I send the patch then so that we protect get_expected_throughput?

If so, then please have a look at my previous mesh patch (not yet applied) "[PATCH v3] mac80211: mesh: Add support for HW RC implementation".

If you approve the mesh patch, then I will send two patches:
1. Check for sta->uploaded in the drv wrapper (will change the drv wrapper header to get sta_info)
2. v4 for the mesh patch with respect to header change in patch 1.

Max

-----Original Message-----
From: Johannes Berg [mailto:johannes@xxxxxxxxxxxxxxxx] 
Sent: Monday, August 08, 2016 9:11 AM
To: Altshul, Maxim; linux-kernel@xxxxxxxxxxxxxxx
Cc: john.stultz@xxxxxxxxxx; Kalle Valo; Eliad Peller; Machani, Yaniv
Subject: Re: [PATCH 1/2] mac80211/wlcore: Add ieee80211_hw variable to get_expected_throughput

On Sun, 2016-08-07 at 13:42 +0000, Altshul, Maxim wrote:
> Hi Johaness,
> I have prepared a patch for the issue and it is waiting for me to send 
> it, but I feel that maybe I have not explained the previous issue well 
> enough or I did not understand your request fully.
> I would like to clarify about the previous patch (the one that you
> applied) again:
> 
> a. The bug occurred because I have added a member called wl to the 
> structure wl_sta, but it turned to be NULL when the function 
> drv_get_expected_throughput was called.

Right.

> b. This member was NULL because it was initialized in the wrong place 
> (sta_add instead of update_sta_state), and thus the regression has 
> failed.

Ah. So you *do* in fact implement the sta_state op (op_sta_state) instead of the sta_add op, which I thought you were using and which was causing the error. Perhaps sta_add came from being originally called through mac80211's sta_add op.

So in essence, in this particular case it ended up being just a driver bug because it was initializing the pointer in the wrong place, and I agree that the fix in mac80211 to pass the hw pointer like everywhere else makes perfect sense.

> c. Even so, wl_sta itself was not NULL at any point.

Right.

> d. This is why I have created two patches:
> First patch (the one that you have applied) made it easy for the 
> driver to access hw->priv (the problematic access to hw->priv was the 
> reason I added wl to wl_sta in the first place, which was a mistake).
> Second patch reverted the addition of wl member to wl_sta.

Right.
Â
> 2. From what I have seen, other ops that take ieee80211_sta as a 
> parameter do not check for sta->uploaded, which is why it feels a 
> little odd to do it in drv_get_expected_throughput and nowhere else.

I think most of them have a different protection; perhaps some are lacking it?

Â* set_tim: can only be called when the station is associated
Â* set_key: likewise, iirc, though perhaps userspace can mess up?
Â* update_tkip_key: must have a key and traffic
Â* sta_notify: powersave - must be associated
Â* sta_pre_rcu_remove: only pre removal etc.
Â* sta_rc_update: looks partially problematic through RX action frame,
         if a peer messes up and sends one ... oops
Â* TDLS ones look fine, I think

So I *think* that most are OK - RC update might be an issue.

get_expected_throughput is unique in that it can be called from userspace at any time after the station is added, and that happened in the case that John had (called immediately after ADD_STA notification,
afaict)

johannes