[PATCH v2] carl9170: Remove redundant protection check

[PATCH v2] carl9170: Remove redundant protection check

[Andreea-Cristina Bernat]

The carl9170_op_ampdu_action() function is used only by the mac80211
framework.
Since the mac80211 already takes care of checks and properly serializing
calls to the driver's function there is no need for the driver to do the same
thing.

Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
---
Changes in v2:
 - Change subject line from
   "carl9170: Replace rcu_dereference() with rcu_access_pointer()"
   to
   "carl9170: Remove redundant protection check"
 - Update the commit message according to the modifications
 - Delete the lines of interest at the suggestion and explanations of
   Christian Lamparter <chunkeey@googlemail.com>

 drivers/net/wireless/ath/carl9170/main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index 12018ff..6758b9a 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;
 
-		rcu_read_lock();
-		if (rcu_access_pointer(sta_info->agg[tid])) {
-			rcu_read_unlock();
-			return -EBUSY;
-		}
-
 		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
 				   GFP_ATOMIC);
 		if (!tid_info) {
-- 
1.9.1

Re: [PATCH v2] carl9170: Remove redundant protection check

[Christian Lamparter]

On Friday, August 22, 2014 10:14:31 PM Andreea-Cristina Bernat wrote:
> The carl9170_op_ampdu_action() function is used only by the mac80211
> framework. Since the mac80211 already takes care of checks and 
> properly serializing calls to the driver's function there is no
> need for the driver to do the same thing.
> 
> Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> ---
> Changes in v2:
>  - Change subject line from
>    "carl9170: Replace rcu_dereference() with rcu_access_pointer()"
>    to
>    "carl9170: Remove redundant protection check"
>  - Update the commit message according to the modifications
>  - Delete the lines of interest at the suggestion and explanations of
>    Christian Lamparter <chunkeey@googlemail.com>
> 
>  drivers/net/wireless/ath/carl9170/main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> index 12018ff..6758b9a 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
>  		if (!sta_info->ht_sta)
>  			return -EOPNOTSUPP;
>  
> -		rcu_read_lock();
> -		if (rcu_access_pointer(sta_info->agg[tid])) {
> -			rcu_read_unlock();
> -			return -EBUSY;
> -		}
> -
>  		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
>  				   GFP_ATOMIC);
>  		if (!tid_info) {
> 

sparse [0] hit a bug when testing the patch:

drivers/net/wireless/ath/carl9170/main.c:1440:17: 
  warning: context imbalance in 'carl9170_op_ampdu_action' - unexpected unlock

This warning is caused by the remaining, stray rcu_read protection
in the code below (Sorry! Guess RCU needed a bit more explanation
in the previous post. If you are looking for *pointers*, there are
excellent resources available in Documentation/RCU/ [1]).

I've attached a full patch (see below) with all changes so far and
tested if the device/driver still behaves ;-). This patch applies 
cleanly on top of wireless-testing.

@John,
can you please take it?

---
From: Andreea-Cristina Bernat <bernat.ada@gmail.com>

The carl9170_op_ampdu_action() function is used only by the mac80211
framework. Since the mac80211 already takes care of checks and 
properly serializing calls to the driver's function there is no
need for the driver to do the same thing.
 
Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
[chunkeey@googlemail.com: remove two stray rcu_read_unlock()]
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index f8ded84..ef5b6dc 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;
 
-		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
-			rcu_read_unlock();
-			return -EBUSY;
-		}
-
 		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
 				   GFP_ATOMIC);
-		if (!tid_info) {
-			rcu_read_unlock();
+		if (!tid_info)
 			return -ENOMEM;
-		}
 
 		tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn);
 		tid_info->state = CARL9170_TID_STATE_PROGRESS;
@@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list);
 		rcu_assign_pointer(sta_info->agg[tid], tid_info);
 		spin_unlock_bh(&ar->tx_ampdu_list_lock);
-		rcu_read_unlock();
 
 		ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
 		break;
---

Regards
Christian

[0] <http://kernelnewbies.org/Sparse>
[1] <https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt>

Re: [PATCH v2] carl9170: Remove redundant protection check

[Eric Dumazet]

On Fri, 2014-08-22 at 22:38 +0200, Christian Lamparter wrote:
> On Friday, August 22, 2014 10:14:31 PM Andreea-Cristina Bernat wrote:
> > The carl9170_op_ampdu_action() function is used only by the mac80211
> > framework. Since the mac80211 already takes care of checks and 
> > properly serializing calls to the driver's function there is no
> > need for the driver to do the same thing.
> > 
> > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> > ---
> > Changes in v2:
> >  - Change subject line from
> >    "carl9170: Replace rcu_dereference() with rcu_access_pointer()"
> >    to
> >    "carl9170: Remove redundant protection check"
> >  - Update the commit message according to the modifications
> >  - Delete the lines of interest at the suggestion and explanations of
> >    Christian Lamparter <chunkeey@googlemail.com>
> > 
> >  drivers/net/wireless/ath/carl9170/main.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> > index 12018ff..6758b9a 100644
> > --- a/drivers/net/wireless/ath/carl9170/main.c
> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > @@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
> >  		if (!sta_info->ht_sta)
> >  			return -EOPNOTSUPP;
> >  
> > -		rcu_read_lock();
> > -		if (rcu_access_pointer(sta_info->agg[tid])) {
> > -			rcu_read_unlock();
> > -			return -EBUSY;
> > -		}
> > -
> >  		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
> >  				   GFP_ATOMIC);
> >  		if (!tid_info) {
> > 
> 
> sparse [0] hit a bug when testing the patch:
> 
> drivers/net/wireless/ath/carl9170/main.c:1440:17: 
>   warning: context imbalance in 'carl9170_op_ampdu_action' - unexpected unlock
> 
> This warning is caused by the remaining, stray rcu_read protection
> in the code below (Sorry! Guess RCU needed a bit more explanation
> in the previous post. If you are looking for *pointers*, there are
> excellent resources available in Documentation/RCU/ [1]).
> 
> I've attached a full patch (see below) with all changes so far and
> tested if the device/driver still behaves ;-). This patch applies 
> cleanly on top of wireless-testing.
> 
> @John,
> can you please take it?
> 
> ---
> From: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> 
> The carl9170_op_ampdu_action() function is used only by the mac80211
> framework. Since the mac80211 already takes care of checks and 
> properly serializing calls to the driver's function there is no
> need for the driver to do the same thing.
>  
> Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> [chunkeey@googlemail.com: remove two stray rcu_read_unlock()]
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> index f8ded84..ef5b6dc 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
>  		if (!sta_info->ht_sta)
>  			return -EOPNOTSUPP;
>  
> -		rcu_read_lock();
> -		if (rcu_dereference(sta_info->agg[tid])) {
> -			rcu_read_unlock();
> -			return -EBUSY;
> -		}
> -
>  		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
>  				   GFP_ATOMIC);
> -		if (!tid_info) {
> -			rcu_read_unlock();
> +		if (!tid_info)
>  			return -ENOMEM;
> -		}
>  
>  		tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn);
>  		tid_info->state = CARL9170_TID_STATE_PROGRESS;
> @@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
>  		list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list);
>  		rcu_assign_pointer(sta_info->agg[tid], tid_info);
>  		spin_unlock_bh(&ar->tx_ampdu_list_lock);
> -		rcu_read_unlock();
>  
>  		ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
>  		break;
> ---
> 
> Regards
> Christian
> 
> [0] <http://kernelnewbies.org/Sparse>
> [1] <https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt>
> 


Sorry but this patch is not complete.

You need to somehow return -EBUSY if sta_info->agg[tid] is set.

The test has to be done inside the
spin_lock_bh(&ar->tx_ampdu_list_lock) /
spin_unlock_bh(&ar->tx_ampdu_list_lock); region.

You are correct the RCU bits were wrong in this context, but we still
have to avoid leaks.

Re: [PATCH v2] carl9170: Remove redundant protection check

[Christian Lamparter]

On Friday, August 22, 2014 02:08:52 PM Eric Dumazet wrote:
> On Fri, 2014-08-22 at 22:38 +0200, Christian Lamparter wrote:
> [...]
> > From: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> > 
> > The carl9170_op_ampdu_action() function is used only by the mac80211
> > framework. Since the mac80211 already takes care of checks and 
> > properly serializing calls to the driver's function there is no
> > need for the driver to do the same thing.
> >  
> > ---
> > @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
> >  		if (!sta_info->ht_sta)
> >  			return -EOPNOTSUPP;
> >  
> > -		rcu_read_lock();
> > -		if (rcu_dereference(sta_info->agg[tid])) {
> > -			rcu_read_unlock();
> > -			return -EBUSY;
> > -		}
> > -
> > [...]
> > ---
> 
> Sorry but this patch is not complete.
> 
> You need to somehow return -EBUSY if sta_info->agg[tid] is set.
The sta_info->agg[tid] check is not needed (for reference, see [0]).
(There is already a check in mac80211 which prevents the leak of
sta_info->agg[tid] [1]).

Regards
Christian

[0] <https://lkml.org/lkml/2014/8/20/725>
[1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>

Re: [PATCH v2] carl9170: Remove redundant protection check

[Eric Dumazet]

On Fri, 2014-08-22 at 23:53 +0200, Christian Lamparter wrote:

> The sta_info->agg[tid] check is not needed (for reference, see [0]).
> (There is already a check in mac80211 which prevents the leak of
> sta_info->agg[tid] [1]).
> 
> Regards
> Christian
> 
> [0] <https://lkml.org/lkml/2014/8/20/725>
> [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>
> 

Hmpfff... this code is quite confusing. RCU is used both in tricky way
(carl9170_ampdu_gc() is an example) and a talisman (the part you remove)

Why is rcu_assign_pointer(sta_info->agg[tid], tid_info);
done inside the spinlock protected region, I don't know.

If this code relies on external protection, a comment would help its
comprehension for sure.

For example, you could add a 
BUG_ON(rcu_access_pointer(sta_info->agg[tid]));
so that we are sure requirements are not changed in the callers one day.

Re: [PATCH v2] carl9170: Remove redundant protection check

[Christian Lamparter]

On Friday, August 22, 2014 04:23:19 PM Eric Dumazet wrote:
> On Fri, 2014-08-22 at 23:53 +0200, Christian Lamparter wrote:
> 
> > The sta_info->agg[tid] check is not needed (for reference, see [0]).
> > (There is already a check in mac80211 which prevents the leak of
> > sta_info->agg[tid] [1]).
> > 
> > Regards
> > Christian
> > 
> > [0] <https://lkml.org/lkml/2014/8/20/725>
> > [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>
> > 
> 
> Hmpfff... this code is quite confusing. 
That's true. Furthermore, parts of the logic are also embedded in
the mac80211-stack and above. So, it's very hard to see the whole
big picture, just by looking at the driver code.

> RCU is used both in tricky way (carl9170_ampdu_gc() is an example)
> and a talisman (the part you remove)
I know that game ;-). But fair enough: if you have concerns about
the complexity of the code in question: I'm willing to help you
and explain the quirks in detail if necessary. I think this is a
valuable addition, since "external consultants" are hard to come
by.

> Why is rcu_assign_pointer(sta_info->agg[tid], tid_info);
> done inside the spinlock protected region, I don't know.
The pointer in sta_info->agg[tid] is used exclusively by 
the tx.c code... It is queried only if an outgoing frame
has the IEEE80211_TX_CTL_AMPDU flag set. 

But for this flag to be set, the aggregation session has
to be operational. This requires two calls to ampdu_action [0].
(first with: IEEE80211_AMPDU_TX_START and later with:
IEEE80211_AMPDU_TX_OPERATIONAL).

=> If you want to make a patch to move this rcu_assign_pointer(...)
after the spin_unlock_bh() - Then: Yes, GO FOR IT!
 
> If this code relies on external protection, a comment would help its
> comprehension for sure.
> 
> For example, you could add a 
> BUG_ON(rcu_access_pointer(sta_info->agg[tid]));
> so that we are sure requirements are not changed
> in the callers one day.
Maybe, but then: Is a "specific driver" the right place for this?
Other drivers may also depend on ampdu_action not changing.
As for the logic: The AMPDU handshake itself is part of the 802.11
spec. If you are interested you can get 802.11-2012 [1] and look 
into Section 9.21 "Block Acknowledgment". It contains a message
sequence chart and details about the setup and tear down procedures
for aggregation session [which is at the heart of the ampdu_action
callback issue].

Note: mac80211 has a "software simulator" mac80211_hwsim [2].
It can be (and is) used to test most of the mac80211 functionality.
So what do you think?

Regards
Christian 

[0] <https://www.kernel.org/doc/htmldocs/80211/aggregation.html>
[1] <http://standards.ieee.org/findstds/standard/802.11-2012.html>
[2] <http://wireless.kernel.org/en/users/Drivers/mac80211_hwsim>