Skip to contentSkip to conversation

carl9170: Remove redundant protection check

PATCH In review Andreea-Cristina Bernat 3 participants 6 messages Aug 22, 2014 – Aug 23, 2014

Subsystems (3)
Networking Network drivers CARL9170 Linux community wireless driver
lore
Share & tools
lore

Andreea-Cristina Bernat

author
#0 lore raw

PATCH v2 1/1 carl9170: Remove redundant protection check

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 <[email protected]>

---
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 <[email protected]>

 drivers/net/wireless/ath/carl9170/main.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +0 -6
index 12018ff..6758b9a 100644
@@ -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) {
Signature · 1 line
1.9.1
maintainer
#1 lore raw
Quoted 27 lines · 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 <[email protected]>

---
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 <[email protected]>

 drivers/net/wireless/ath/carl9170/main.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index 12018ff..6758b9a 100644
@@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;

drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, · line 1435

Quoted 10 lines
drivers/net/wireless/ath/carl9170/main.c +0 -6
 
-		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 <[email protected]>

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 <[email protected]>
[[email protected]: remove two stray rcu_read_unlock()]
Signed-off-by Christian Lamparter <[email protected]>

---

drivers/net/wireless/ath/carl9170/main.c +1 -10
index f8ded84..ef5b6dc 100644
@@ -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>

#2 lore raw
Quoted 102 lines · Christian Lamparter
Quoted 27 lines · 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 <[email protected]>

---
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 <[email protected]>

 drivers/net/wireless/ath/carl9170/main.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index 12018ff..6758b9a 100644
@@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;

drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, · line 1435

Quoted 10 lines
drivers/net/wireless/ath/carl9170/main.c +0 -6
 
-		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 <[email protected]>

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 <[email protected]>
[[email protected]: remove two stray rcu_read_unlock()]
Signed-off-by Christian Lamparter <[email protected]>

---

drivers/net/wireless/ath/carl9170/main.c +1 -11
index f8ded84..ef5b6dc 100644
@@ -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;
---
Quoted 6 lines

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.

maintainer
#3 lore raw
Quoted 25 lines · Eric Dumazet

On Fri, 2014-08-22 at 22:38 +0200, Christian Lamparter wrote:
[...]

Quoted 10 lines

From: Andreea-Cristina Bernat <[email protected]>

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.

---

drivers/net/wireless/ath/carl9170/main.c +0 -0
@@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)

drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,

Quoted 10 lines
drivers/net/wireless/ath/carl9170/main.c +0 -7
 			return -EOPNOTSUPP;
 
-		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
-			rcu_read_unlock();
-			return -EBUSY;
-		}
-
[...]
---

Sorry but this patch is not complete.

Quoted 1 line

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>

#4 lore raw
Quoted 10 lines · Christian Lamparter

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.

maintainer
#5 lore raw
Quoted 6 lines · Eric Dumazet
Quoted 4 lines · Christian Lamparter

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

Quoted 8 lines
Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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!

Quoted 7 lines

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>