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

2014-08-22 Thread 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] 
> > [1] 
> > 
> 
> 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] 
[1] 
[2] 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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] 
> [1] 
> 

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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 
> > 
> > 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] 
[1] 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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 
> > ---
> > 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 
> > 
> >  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 
> 
> 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 
> [chunk...@googlemail.com: remove two stray rcu_read_unlock()]
> Signed-off-by: Christian Lamparter 
> ---
> 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(_info->list, >tx_ampdu_list);
>   rcu_assign_pointer(sta_info->agg[tid], tid_info);
>   spin_unlock_bh(>tx_ampdu_list_lock);
> - rcu_read_unlock();
>  
>   ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
>   break;
> ---
> 
> Regards
> Christian
> 
> [0] 
> [1] 
> 


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(>tx_ampdu_list_lock) /
spin_unlock_bh(>tx_ampdu_list_lock); region.

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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 
> ---
> 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 
> 
>  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 

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 
[chunk...@googlemail.com: remove two stray rcu_read_unlock()]
Signed-off-by: Christian Lamparter 
---
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(_info->list, >tx_ampdu_list);
rcu_assign_pointer(sta_info->agg[tid], tid_info);
spin_unlock_bh(>tx_ampdu_list_lock);
-   rcu_read_unlock();
 
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
---

Regards
Christian

[0] 
[1] 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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.



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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@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 chunk...@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@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@gmail.com
[chunk...@googlemail.com: remove two stray rcu_read_unlock()]
Signed-off-by: Christian Lamparter chunk...@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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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@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 chunk...@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@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@gmail.com
 [chunk...@googlemail.com: remove two stray rcu_read_unlock()]
 Signed-off-by: Christian Lamparter chunk...@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.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-08-22 Thread 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@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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/