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

[Kalle Valo]

Maxim Altshul  writes:

> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
>
> - Change wlcore op accordingly.
>
> Signed-off-by: Maxim Altshul 

You didn't CC linux-wireless, adding it now. Others can find the full
discussion here:

http://lkml.kernel.org/g/20160804124314.7636-2-maxim.alts...@ti.com

-- 
Kalle Valo

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

[Kalle Valo]

Maxim Altshul  writes:

> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
>
> - Change wlcore op accordingly.
>
> Signed-off-by: Maxim Altshul 

You didn't CC linux-wireless, adding it now. Others can find the full
discussion here:

http://lkml.kernel.org/g/20160804124314.7636-2-maxim.alts...@ti.com

-- 
Kalle Valo

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

[Altshul, Maxim]

On Tue, Aug 09, 2016 at 10:58:30, Johannes Berg wrote:
> Subject: Re: [PATCH 1/2] mac80211/wlcore: Add ieee80211_hw variable to 
> get_expected_throughput
> > 
> > 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.
> 
> I don't think adding the check changes anything wrt. the mesh patch?
> 

I thought about changing the header of drv_get_expected_throughput to receive 
sta_info *sta so that I can get sta->uploaded from that.
I could use get container of >sta to get sta->uploaded, but I think it's 
better to get the pointer directly?

Max

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

[Altshul, Maxim]

On Tue, Aug 09, 2016 at 10:58:30, Johannes Berg wrote:
> Subject: Re: [PATCH 1/2] mac80211/wlcore: Add ieee80211_hw variable to 
> get_expected_throughput
> > 
> > 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.
> 
> I don't think adding the check changes anything wrt. the mesh patch?
> 

I thought about changing the header of drv_get_expected_throughput to receive 
sta_info *sta so that I can get sta->uploaded from that.
I could use get container of >sta to get sta->uploaded, but I think it's 
better to get the pointer directly?

Max

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

[Johannes Berg]

On Mon, 2016-08-08 at 10:42 +, Altshul, Maxim wrote:
> Yes, exactly! Should I send the patch then so that we protect
> get_expected_throughput?

Yes please.

> 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".

Looks ok, I just haven't applied it because net-next was closed until
today :)

> 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.

I don't think adding the check changes anything wrt. the mesh patch?

johannes

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

[Johannes Berg]

On Mon, 2016-08-08 at 10:42 +, Altshul, Maxim wrote:
> Yes, exactly! Should I send the patch then so that we protect
> get_expected_throughput?

Yes please.

> 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".

Looks ok, I just haven't applied it because net-next was closed until
today :)

> 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.

I don't think adding the check changes anything wrt. the mesh patch?

johannes

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:johan...@sipsolutions.net] 
Sent: Monday, August 08, 2016 9:11 AM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 +, 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

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:johan...@sipsolutions.net] 
Sent: Monday, August 08, 2016 9:11 AM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 +, 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

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

[Johannes Berg]

On Sun, 2016-08-07 at 13:42 +, 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

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

[Johannes Berg]

On Sun, 2016-08-07 at 13:42 +, 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

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

[Altshul, Maxim]

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:johan...@sipsolutions.net] 
Sent: Friday, August 05, 2016 6:34 PM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 +, 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

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

[Altshul, Maxim]

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:johan...@sipsolutions.net] 
Sent: Friday, August 05, 2016 6:34 PM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 +, 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

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

[Johannes Berg]

On Fri, 2016-08-05 at 13:25 +, 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

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

[Johannes Berg]

On Fri, 2016-08-05 at 13:25 +, 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

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

[Altshul, Maxim]

Hi,
1) Sorry about the change log, I will try to be clearer next time.

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).

4) Sorry, I just saw now that everything has a space before the line. I used 
checkpatch.pl and it was ok. I will be more careful next time.

BR,
Max

From: Johannes Berg [johan...@sipsolutions.net]
Sent: Friday, August 05, 2016 3:24 PM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 14:22 +0200, Johannes Berg wrote:
> On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> >
> > - Change wlcore op accordingly.
> >
> I'm applying this now, with a big BUT:
>
> 1) your changelog is crap - I've rewritten it to indicate what's
> going
> on
>
> 2) I think the change makes sense, but as far as fixing the bug is
> concerned it's actually completely stupid
>
> 3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
> call into the driver before the station is actually known to the
> driver.

4) please indent your code properly when submitting...

johannes

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

[Altshul, Maxim]

Hi,
1) Sorry about the change log, I will try to be clearer next time.

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).

4) Sorry, I just saw now that everything has a space before the line. I used 
checkpatch.pl and it was ok. I will be more careful next time.

BR,
Max

From: Johannes Berg [johan...@sipsolutions.net]
Sent: Friday, August 05, 2016 3:24 PM
To: Altshul, Maxim; linux-kernel@vger.kernel.org
Cc: john.stu...@linaro.org; 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 14:22 +0200, Johannes Berg wrote:
> On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> >
> > - Change wlcore op accordingly.
> >
> I'm applying this now, with a big BUT:
>
> 1) your changelog is crap - I've rewritten it to indicate what's
> going
> on
>
> 2) I think the change makes sense, but as far as fixing the bug is
> concerned it's actually completely stupid
>
> 3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
> call into the driver before the station is actually known to the
> driver.

4) please indent your code properly when submitting...

johannes

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

[Johannes Berg]

On Fri, 2016-08-05 at 14:22 +0200, Johannes Berg wrote:
> On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> > 
> > - Change wlcore op accordingly.
> > 
> I'm applying this now, with a big BUT:
> 
> 1) your changelog is crap - I've rewritten it to indicate what's
> going
> on
> 
> 2) I think the change makes sense, but as far as fixing the bug is
> concerned it's actually completely stupid
> 
> 3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
> call into the driver before the station is actually known to the
> driver.

4) please indent your code properly when submitting...

johannes

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

[Johannes Berg]

On Fri, 2016-08-05 at 14:22 +0200, Johannes Berg wrote:
> On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> > 
> > - Change wlcore op accordingly.
> > 
> I'm applying this now, with a big BUT:
> 
> 1) your changelog is crap - I've rewritten it to indicate what's
> going
> on
> 
> 2) I think the change makes sense, but as far as fixing the bug is
> concerned it's actually completely stupid
> 
> 3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
> call into the driver before the station is actually known to the
> driver.

4) please indent your code properly when submitting...

johannes

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

[Johannes Berg]

On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
> 
> - Change wlcore op accordingly.
> 
I'm applying this now, with a big BUT:

1) your changelog is crap - I've rewritten it to indicate what's going
on

2) I think the change makes sense, but as far as fixing the bug is
concerned it's actually completely stupid

3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
call into the driver before the station is actually known to the
driver.

johannes

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

[Johannes Berg]

On Thu, 2016-08-04 at 15:43 +0300, Maxim Altshul wrote:
> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
> 
> - Change wlcore op accordingly.
> 
I'm applying this now, with a big BUT:

1) your changelog is crap - I've rewritten it to indicate what's going
on

2) I think the change makes sense, but as far as fixing the bug is
concerned it's actually completely stupid

3) I expect you to submit a proper bugfix for mac80211 ASAP, to not
call into the driver before the station is actually known to the
driver.

johannes

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

[John Stultz]

On Thu, Aug 4, 2016 at 10:40 PM, Johannes Berg
 wrote:
> On Thu, 2016-08-04 at 14:31 -0700, John Stultz wrote:
>> On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul 
>> wrote:
>> > - The variable is added to allow the driver an easy access
>> > to it's own hw->priv when the op is invoked.
>> >
>> > - Change wlcore op accordingly.
>> >
>> > Signed-off-by: Maxim Altshul 
>>
>> These two patches solve the regression I was seeing with pre-v4.8-rc1
>> kernels on HiKey.
>>
>
> Interesting, what was the regression? Was the station pointer there not
> valid?

Yea, a null pointer deference:
https://lkml.org/lkml/2016/7/27/712

thanks
-john

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

[John Stultz]

On Thu, Aug 4, 2016 at 10:40 PM, Johannes Berg
 wrote:
> On Thu, 2016-08-04 at 14:31 -0700, John Stultz wrote:
>> On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul 
>> wrote:
>> > - The variable is added to allow the driver an easy access
>> > to it's own hw->priv when the op is invoked.
>> >
>> > - Change wlcore op accordingly.
>> >
>> > Signed-off-by: Maxim Altshul 
>>
>> These two patches solve the regression I was seeing with pre-v4.8-rc1
>> kernels on HiKey.
>>
>
> Interesting, what was the regression? Was the station pointer there not
> valid?

Yea, a null pointer deference:
https://lkml.org/lkml/2016/7/27/712

thanks
-john

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

[Johannes Berg]

On Thu, 2016-08-04 at 14:31 -0700, John Stultz wrote:
> On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul 
> wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> > 
> > - Change wlcore op accordingly.
> > 
> > Signed-off-by: Maxim Altshul 
> 
> These two patches solve the regression I was seeing with pre-v4.8-rc1
> kernels on HiKey.
> 

Interesting, what was the regression? Was the station pointer there not
valid?

johannes

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

[Johannes Berg]

On Thu, 2016-08-04 at 14:31 -0700, John Stultz wrote:
> On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul 
> wrote:
> > - The variable is added to allow the driver an easy access
> > to it's own hw->priv when the op is invoked.
> > 
> > - Change wlcore op accordingly.
> > 
> > Signed-off-by: Maxim Altshul 
> 
> These two patches solve the regression I was seeing with pre-v4.8-rc1
> kernels on HiKey.
> 

Interesting, what was the regression? Was the station pointer there not
valid?

johannes

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

[John Stultz]

On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul  wrote:
> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
>
> - Change wlcore op accordingly.
>
> Signed-off-by: Maxim Altshul 

These two patches solve the regression I was seeing with pre-v4.8-rc1
kernels on HiKey.

Tested-by: John Stultz 

thanks
-john

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

[John Stultz]

On Thu, Aug 4, 2016 at 5:43 AM, Maxim Altshul  wrote:
> - The variable is added to allow the driver an easy access
> to it's own hw->priv when the op is invoked.
>
> - Change wlcore op accordingly.
>
> Signed-off-by: Maxim Altshul 

These two patches solve the regression I was seeing with pre-v4.8-rc1
kernels on HiKey.

Tested-by: John Stultz 

thanks
-john

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

[Maxim Altshul]

- The variable is added to allow the driver an easy access
to it's own hw->priv when the op is invoked.

- Change wlcore op accordingly.

Signed-off-by: Maxim Altshul 
---
 drivers/net/wireless/ti/wlcore/main.c | 5 +++--
 include/net/mac80211.h| 3 ++-
 net/mac80211/driver-ops.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c 
b/drivers/net/wireless/ti/wlcore/main.c
index 4573614..1ec3545 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -5803,10 +5803,11 @@ out:
mutex_unlock(>mutex);
 }
 
-static u32 wlcore_op_get_expected_throughput(struct ieee80211_sta *sta)
+static u32 wlcore_op_get_expected_throughput(struct ieee80211_hw *hw,
+   struct ieee80211_sta *sta)
 {
struct wl1271_station *wl_sta = (struct wl1271_station *)sta->drv_priv;
-   struct wl1271 *wl = wl_sta->wl;
+   struct wl1271 *wl = hw->priv;
u8 hlid = wl_sta->hlid;
 
/* return in units of Kbps */
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c328f35..464eb97 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3484,7 +3484,8 @@ struct ieee80211_ops {
 
int (*join_ibss)(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
void (*leave_ibss)(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
-   u32 (*get_expected_throughput)(struct ieee80211_sta *sta);
+   u32 (*get_expected_throughput)(struct ieee80211_hw *hw,
+   struct ieee80211_sta *sta);
int (*get_txpower)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
   int *dbm);
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 154ce4b..1f75195 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1081,7 +1081,7 @@ static inline u32 drv_get_expected_throughput(struct 
ieee80211_local *local,
 
trace_drv_get_expected_throughput(sta);
if (local->ops->get_expected_throughput)
-   ret = local->ops->get_expected_throughput(sta);
+   ret = local->ops->get_expected_throughput(>hw, sta);
trace_drv_return_u32(local, ret);
 
return ret;
-- 
2.9.0

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

[Maxim Altshul]

- The variable is added to allow the driver an easy access
to it's own hw->priv when the op is invoked.

- Change wlcore op accordingly.

Signed-off-by: Maxim Altshul 
---
 drivers/net/wireless/ti/wlcore/main.c | 5 +++--
 include/net/mac80211.h| 3 ++-
 net/mac80211/driver-ops.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c 
b/drivers/net/wireless/ti/wlcore/main.c
index 4573614..1ec3545 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -5803,10 +5803,11 @@ out:
mutex_unlock(>mutex);
 }
 
-static u32 wlcore_op_get_expected_throughput(struct ieee80211_sta *sta)
+static u32 wlcore_op_get_expected_throughput(struct ieee80211_hw *hw,
+   struct ieee80211_sta *sta)
 {
struct wl1271_station *wl_sta = (struct wl1271_station *)sta->drv_priv;
-   struct wl1271 *wl = wl_sta->wl;
+   struct wl1271 *wl = hw->priv;
u8 hlid = wl_sta->hlid;
 
/* return in units of Kbps */
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c328f35..464eb97 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3484,7 +3484,8 @@ struct ieee80211_ops {
 
int (*join_ibss)(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
void (*leave_ibss)(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
-   u32 (*get_expected_throughput)(struct ieee80211_sta *sta);
+   u32 (*get_expected_throughput)(struct ieee80211_hw *hw,
+   struct ieee80211_sta *sta);
int (*get_txpower)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
   int *dbm);
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 154ce4b..1f75195 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1081,7 +1081,7 @@ static inline u32 drv_get_expected_throughput(struct 
ieee80211_local *local,
 
trace_drv_get_expected_throughput(sta);
if (local->ops->get_expected_throughput)
-   ret = local->ops->get_expected_throughput(sta);
+   ret = local->ops->get_expected_throughput(>hw, sta);
trace_drv_return_u32(local, ret);
 
return ret;
-- 
2.9.0