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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> [chunkeey@xxxxxxxxxxxxxx: remove two stray rcu_read_unlock()]
> Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> ---
> 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-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
