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

From: Altshul, Maxim
Date: Sun Aug 07 2016 - 09:43:24 EST


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.
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.
c. Even so, wl_sta itself was not NULL at any point.
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.

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.

Please tell me how to proceed. If you still think that a patch is needed, I will send it right away!

Max


-----Original Message-----
From: Johannes Berg [mailto:johannes@xxxxxxxxxxxxxxxx]
Sent: Friday, August 05, 2016 6:34 PM
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 Fri, 2016-08-05 at 13:25 +0000, Altshul, Maxim wrote:
> Hi,
> 1) Sorry about the change log, I will try to be clearer next time.

Just mention something about how the bug happens please, at least.

> 2+3) The issue is not that the station is not known, it's that
> wl_sta->wl was null.
> wl member is now completely removed from wl_sta (PATCH 2/2) and hw is
> sent directly from mac80211 to the driver (so it can get hw->priv).

Right, I understand that wl_sta->wl was NULL. But the driver must have some code to assign wl_sta->wl, right? And that would be called in add_sta or sta_state. Thus the reason for the crash would be that the station wasn't actually known to the driver yet.

Even if that wasn't quite the reason here, I think we need to take it into account and check sta->uploaded before calling the driver, so I'd like you to submit a patch for that.

johannes