carl9170: Replace rcu_dereference() with rcu_access_pointer()

PATCH In review Andreea-Cristina Bernat 2 participants 5 messages Aug 17, 2014 – Aug 21, 2014

Subsystems (3)

Networking Network drivers CARL9170 Linux community wireless driver

lore

Share & tools

lore

Andreea-Cristina Bernat

author

2014-08-17 10:48 UTC #0 lore raw

PATCH 1/1 carl9170: Replace rcu_dereference() with rcu_access_pointer()

The rcu_dereference() call is used directly in a condition. Since its return value is never dereferenced it is recommended to use "rcu_access_pointer()" instead of "rcu_dereference()". Therefore, this patch makes the replacement.

The following Coccinelle semantic patch was used:
@@
@@

(
if(
(<+...
- rcu_dereference
+ rcu_access_pointer
(...)
...+>)) {...}
|
while(
(<+...
- rcu_dereference
+ rcu_access_pointer
(...)
...+>)) {...}
)

Signed-off-by Andreea-Cristina Bernat <[email protected]>

---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/carl9170/main.c +1 -1

index f8ded84..12018ff 100644
@@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
+		if (rcu_access_pointer(sta_info->agg[tid])) {
 			rcu_read_unlock();
 			return -EBUSY;
 		}

Signature · 1 line

1.9.1

Christian Lamparter

maintainer

2014-08-18 19:29 UTC #1 lore raw

Quoted 15 lines · Andreea-Cristina Bernat

The rcu_dereference() call is used directly in a condition. Since its return value is never dereferenced it is recommended to use "rcu_access_pointer()" instead of "rcu_dereference()". Therefore, this patch makes the replacement.
[...]
Signed-off-by Andreea-Cristina Bernat <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index f8ded84..12018ff 100644

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

Quoted 8 lines

drivers/net/wireless/ath/carl9170/main.c +1 -1

@@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
+		if (rcu_access_pointer(sta_info->agg[tid])) {
 			rcu_read_unlock();
 			return -EBUSY;
 		}

There's more. The check does not do a whole lot. I think *it* [the check] and the rcu_read_[un]lock [and the return -EBUSY] can be removed completely from the IEEE80211_AMPDU_TX_START code-path in carl9170_op_ampdu_action.

It would be awesome, if you could you make a patch which removes this unneeded cosmic-ray-protection check :-) .

Thanks
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreea Bernat

author

2014-08-20 17:32 UTC #2 lore raw

Quoted 29 lines · Christian Lamparter

Quoted 15 lines · Andreea-Cristina Bernat
The rcu_dereference() call is used directly in a condition. Since its return value is never dereferenced it is recommended to use "rcu_access_pointer()" instead of "rcu_dereference()". Therefore, this patch makes the replacement.
[...]
Signed-off-by Andreea-Cristina Bernat <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index f8ded84..12018ff 100644
drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, · line 1437
Quoted 8 lines
drivers/net/wireless/ath/carl9170/main.c +1 -1
@@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
+		if (rcu_access_pointer(sta_info->agg[tid])) {
 			rcu_read_unlock();
 			return -EBUSY;
 		}
There's more. The check does not do a whole lot. I think *it* [the check] and the rcu_read_[un]lock [and the return -EBUSY] can be removed completely from the IEEE80211_AMPDU_TX_START code-path in carl9170_op_ampdu_action.

Quoted 2 lines

It would be awesome, if you could you make a patch which removes this unneeded cosmic-ray-protection check :-) .

Could you tell me why you think that those lines have to be removed? I would like to fully understand this before I remove them.

Thank you,
Andreea

Quoted 3 lines

Thanks
Christian

Christian Lamparter

maintainer

2014-08-20 20:20 UTC #3 lore raw

Quoted 30 lines · Andreea Bernat

Quoted 29 lines · Christian Lamparter
Quoted 15 lines · Andreea-Cristina Bernat
The rcu_dereference() call is used directly in a condition. Since its return value is never dereferenced it is recommended to use "rcu_access_pointer()" instead of "rcu_dereference()". Therefore, this patch makes the replacement.
[...]
Signed-off-by Andreea-Cristina Bernat <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index f8ded84..12018ff 100644
drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, · line 1437
Quoted 8 lines
drivers/net/wireless/ath/carl9170/main.c +1 -1
@@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
+		if (rcu_access_pointer(sta_info->agg[tid])) {
 			rcu_read_unlock();
 			return -EBUSY;
 		}
There's more. The check does not do a whole lot. I think *it* [the check] and the rcu_read_[un]lock [and the return -EBUSY] can be removed completely from the IEEE80211_AMPDU_TX_START code-path in carl9170_op_ampdu_action.

Quoted 4 lines

Quoted 2 lines
It would be awesome, if you could you make a patch which removes this unneeded cosmic-ray-protection check :-) .
Could you tell me why you think that those lines have to be removed?

The carl9170_op_ampdu_action callback is used exclusively by the mac80211 framework to notify the driver about setup and tear down of TX and RX aggregation sessions. Hence, mac80211 takes great care of performing sanity checks and properly serializing calls to the driver's ampdu_action callback.

Specifically mac80211 already prevents the START of an TX aggregation session, if the aggregation session is already active [0]. Therefore the driver doesn't need to perform a similar check as well. This is why:
- the expression (rcu_dereference(sta_info->agg[tid])) never evaluates to true -> the -EBUSY exit path is "dead code"

And without the rcu_dereference(...) the rcu_read protection is not needed either. So it can be removed for this case as well.

Quoted 1 line

I would like to fully understand this before I remove them.

Let me know if the explanation above answers sufficient :). If not, I need some *pointers* to what needs further explanation.

Regards
Christian

[0] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>

Andreea Bernat

author

2014-08-21 20:10 UTC #4 lore raw

Quoted 47 lines · Christian Lamparter

Quoted 30 lines · Andreea Bernat
Quoted 29 lines · Christian Lamparter
Quoted 15 lines · Andreea-Cristina Bernat
The rcu_dereference() call is used directly in a condition. Since its return value is never dereferenced it is recommended to use "rcu_access_pointer()" instead of "rcu_dereference()". Therefore, this patch makes the replacement.
[...]
Signed-off-by Andreea-Cristina Bernat <[email protected]>
---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/main.c +0 -0
index f8ded84..12018ff 100644
drivers/net/wireless/ath/carl9170/main.c static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, · line 1437
Quoted 8 lines
drivers/net/wireless/ath/carl9170/main.c +1 -1
@@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 			return -EOPNOTSUPP;
 
 		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
+		if (rcu_access_pointer(sta_info->agg[tid])) {
 			rcu_read_unlock();
 			return -EBUSY;
 		}
There's more. The check does not do a whole lot. I think *it* [the check] and the rcu_read_[un]lock [and the return -EBUSY] can be removed completely from the IEEE80211_AMPDU_TX_START code-path in carl9170_op_ampdu_action.
Quoted 4 lines
Quoted 2 lines
It would be awesome, if you could you make a patch which removes this unneeded cosmic-ray-protection check :-) .
Could you tell me why you think that those lines have to be removed?
The carl9170_op_ampdu_action callback is used exclusively by the mac80211 framework to notify the driver about setup and tear down of TX and RX aggregation sessions. Hence, mac80211 takes great care of performing sanity checks and properly serializing calls to the driver's ampdu_action callback.
Specifically mac80211 already prevents the START of an TX aggregation session, if the aggregation session is already active [0]. Therefore the driver doesn't need to perform a similar check as well. This is why:
- the expression (rcu_dereference(sta_info->agg[tid])) never evaluates to true -> the -EBUSY exit path is "dead code"

Quoted 7 lines

And without the rcu_dereference(...) the rcu_read protection is not needed either. So it can be removed for this case as well.
Quoted 1 line
I would like to fully understand this before I remove them.
Let me know if the explanation above answers sufficient :). If not, I need some *pointers* to what needs further explanation.

Your explanation is clear. I will send a version two of the patch with those lines removed.

Thank you,
Andreea

Quoted 6 lines

Regards
Christian
[0] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>