[patch] carl9170: remove unneeded NULL check

[patch] carl9170: remove unneeded NULL check

[Dan Carpenter]

The "sta" variable is not checked for NULL consistently and it makes the
static checkers complain.  I asked Christian Lamparter about this and
it turns out the check is not needed.  "In fact, in order to set up a
ampdu session, the stack would call the driver's op_ampdu_action
callback which always needs a station."

I have removed the check.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 84377cf..23d33c3 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -1463,6 +1463,7 @@ void carl9170_op_tx(struct ieee80211_hw *hw,
 	struct ar9170 *ar = hw->priv;
 	struct ieee80211_tx_info *info;
 	struct ieee80211_sta *sta = control->sta;
+	struct carl9170_sta_info *stai;
 	bool run;
 
 	if (unlikely(!IS_STARTED(ar)))
@@ -1479,10 +1480,8 @@ void carl9170_op_tx(struct ieee80211_hw *hw,
 	 * all ressouces which are associated with the frame.
 	 */
 
-	if (sta) {
-		struct carl9170_sta_info *stai = (void *) sta->drv_priv;
-		atomic_inc(&stai->pending_frames);
-	}
+	stai = (void *) sta->drv_priv;
+	atomic_inc(&stai->pending_frames);
 
 	if (info->flags & IEEE80211_TX_CTL_AMPDU) {
 		run = carl9170_tx_ampdu_queue(ar, sta, skb);

Re: [patch] carl9170: remove unneeded NULL check

[Christian Lamparter]

On Sunday 02 December 2012 11:42:38 Dan Carpenter wrote:
> The "sta" variable is not checked for NULL consistently and it makes the
> static checkers complain.  I asked Christian Lamparter about this and
> it turns out the check is not needed.  "In fact, in order to set up a
> ampdu session, the stack would call the driver's op_ampdu_action
> callback which always needs a station."

that would be from the thread:
<http://www.spinics.net/lists/linux-wireless/msg94526.html>
 
> I have removed the check.
I think this will bug for multicast and injected frames.
 
It is not possible for the sta(tion) pointer to be NULL if
the frame has the IEEE80211_TX_CTL_AMPDU flag set. So the
sta == NULL check can be avoided when calling 
carl9170_tx_ampdu_queue. This is because mac80211 tracks
all aggregation sessions within the station struct.
Of course, this is something that the checker tool can't
possibly deduce, but it has a point and we can add a check
like this [see attached draft patch]:

What do you think [or more to the point: what does the
checker say?]

Regards,
	Chr
---
diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 84377cf..e09d293 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -1470,6 +1470,9 @@ void carl9170_op_tx(struct ieee80211_hw *hw,
 
 	info = IEEE80211_SKB_CB(skb);
 
+	if ((info->flags & IEEE80211_TX_CTL_AMPDU) && WARN_ON_ONCE(!sta))
+		goto err_free;
+
 	if (unlikely(carl9170_tx_prepare(ar, sta, skb)))
 		goto err_free;
 

Re: [patch] carl9170: remove unneeded NULL check

[Dan Carpenter]

On Sun, Dec 02, 2012 at 02:49:20PM +0100, Christian Lamparter wrote:
> On Sunday 02 December 2012 11:42:38 Dan Carpenter wrote:
> > The "sta" variable is not checked for NULL consistently and it makes the
> > static checkers complain.  I asked Christian Lamparter about this and
> > it turns out the check is not needed.  "In fact, in order to set up a
> > ampdu session, the stack would call the driver's op_ampdu_action
> > callback which always needs a station."
> 
> that would be from the thread:
> <http://www.spinics.net/lists/linux-wireless/msg94526.html>
>  
> > I have removed the check.
> I think this will bug for multicast and injected frames.
>  
> It is not possible for the sta(tion) pointer to be NULL if
> the frame has the IEEE80211_TX_CTL_AMPDU flag set. So the
> sta == NULL check can be avoided when calling 
> carl9170_tx_ampdu_queue. This is because mac80211 tracks
> all aggregation sessions within the station struct.
> Of course, this is something that the checker tool can't
> possibly deduce, but it has a point and we can add a check
> like this [see attached draft patch]:
> 
> What do you think [or more to the point: what does the
> checker say?]
> 

So we wouldn't apply my patch, we would apply that one instead?

I think that's great.  My static checker doesn't understand bit
flags yet so it would complain but it would be obvious to a human
reader.

Could you just resend that patch with a signed-off-by?

regards,
dan carpenter

Re: [patch] carl9170: remove unneeded NULL check

[Christian Lamparter]

On Sunday, December 02, 2012 05:51:53 PM Dan Carpenter wrote:
> On Sun, Dec 02, 2012 at 02:49:20PM +0100, Christian Lamparter wrote:
> > On Sunday 02 December 2012 11:42:38 Dan Carpenter wrote:
> > > The "sta" variable is not checked for NULL consistently and it makes the
> > > static checkers complain.  I asked Christian Lamparter about this and
> > > it turns out the check is not needed.  "In fact, in order to set up a
> > > ampdu session, the stack would call the driver's op_ampdu_action
> > > callback which always needs a station."
> > 
> > that would be from the thread:
> > <http://www.spinics.net/lists/linux-wireless/msg94526.html>
> >  
> > > I have removed the check.
> > I think this will bug for multicast and injected frames.
> >  
> > It is not possible for the sta(tion) pointer to be NULL if
> > the frame has the IEEE80211_TX_CTL_AMPDU flag set. So the
> > sta == NULL check can be avoided when calling 
> > carl9170_tx_ampdu_queue. This is because mac80211 tracks
> > all aggregation sessions within the station struct.
> > Of course, this is something that the checker tool can't
> > possibly deduce, but it has a point and we can add a check
> > like this [see attached draft patch]:
> > 
> > What do you think [or more to the point: what does the
> > checker say?]
> > 
> 
> So we wouldn't apply my patch, we would apply that one instead?
We could, but that's up for debate (no, I don't think we are done
just yet).

> I think that's great.  My static checker doesn't understand bit
> flags yet so it would complain but it would be obvious to a human
> reader.
then we might as well add a comment to carl9170_tx_ampdu_queue
and explain the situation [in a way that's obvious to a
human reader]. This way we can save the "if"... which is a small
win since carl9170_op_tx is sort of a hot-path.
 
> Could you just resend that patch with a signed-off-by?
Once we know what to do... yes :)
I have attached another patch. With this patch the checker
should be able to read the code without throwing any
warnings.

Regards,
	Chr
---
diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 84377cf..6c83328 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -1463,13 +1463,16 @@ void carl9170_op_tx(struct ieee80211_hw *hw,
 	struct ar9170 *ar = hw->priv;
 	struct ieee80211_tx_info *info;
 	struct ieee80211_sta *sta = control->sta;
-	bool run;
+	bool run, aggr;
 
 	if (unlikely(!IS_STARTED(ar)))
 		goto err_free;
 
 	info = IEEE80211_SKB_CB(skb);
 
+	aggr = !!(info->flags & IEEE80211_TX_CTL_AMPDU) &&
+		!WARN_ON_ONCE(!sta);
+
 	if (unlikely(carl9170_tx_prepare(ar, sta, skb)))
 		goto err_free;
 
@@ -1484,7 +1487,7 @@ void carl9170_op_tx(struct ieee80211_hw *hw,
 		atomic_inc(&stai->pending_frames);
 	}
 
-	if (info->flags & IEEE80211_TX_CTL_AMPDU) {
+	if (aggr) {
 		run = carl9170_tx_ampdu_queue(ar, sta, skb);
 		if (run)
 			carl9170_tx_ampdu(ar);

Re: [patch] carl9170: remove unneeded NULL check

[Dan Carpenter]

On Sun, Dec 02, 2012 at 09:24:53PM +0100, Christian Lamparter wrote:
> On Sunday, December 02, 2012 05:51:53 PM Dan Carpenter wrote:
> > On Sun, Dec 02, 2012 at 02:49:20PM +0100, Christian Lamparter wrote:
> > > On Sunday 02 December 2012 11:42:38 Dan Carpenter wrote:
> > > > The "sta" variable is not checked for NULL consistently and it makes the
> > > > static checkers complain.  I asked Christian Lamparter about this and
> > > > it turns out the check is not needed.  "In fact, in order to set up a
> > > > ampdu session, the stack would call the driver's op_ampdu_action
> > > > callback which always needs a station."
> > > 
> > > that would be from the thread:
> > > <http://www.spinics.net/lists/linux-wireless/msg94526.html>
> > >  
> > > > I have removed the check.
> > > I think this will bug for multicast and injected frames.
> > >  
> > > It is not possible for the sta(tion) pointer to be NULL if
> > > the frame has the IEEE80211_TX_CTL_AMPDU flag set. So the
> > > sta == NULL check can be avoided when calling 
> > > carl9170_tx_ampdu_queue. This is because mac80211 tracks
> > > all aggregation sessions within the station struct.
> > > Of course, this is something that the checker tool can't
> > > possibly deduce, but it has a point and we can add a check
> > > like this [see attached draft patch]:
> > > 
> > > What do you think [or more to the point: what does the
> > > checker say?]
> > > 
> > 
> > So we wouldn't apply my patch, we would apply that one instead?
> We could, but that's up for debate (no, I don't think we are done
> just yet).
> 
> > I think that's great.  My static checker doesn't understand bit
> > flags yet so it would complain but it would be obvious to a human
> > reader.
> then we might as well add a comment to carl9170_tx_ampdu_queue
> and explain the situation [in a way that's obvious to a
> human reader]. This way we can save the "if"... which is a small
> win since carl9170_op_tx is sort of a hot-path.
>  

Putting a comment there is fine.  Without the comment it's easy for
a human reader to get confused why the check is there.  So long as
humans can read the code, that's all that matters.

> > Could you just resend that patch with a signed-off-by?
> Once we know what to do... yes :)
> I have attached another patch. With this patch the checker
> should be able to read the code without throwing any
> warnings.

Heh.  You have a lot of faith in checker's ability to read code.  In
theory you are right, but it turns out that Smatch is ignoring the
stuff inside the WARN_ON_ONCE().  It's not supposed to do that in
this case; it should only do that if the WARN_ON_ONCE() is in a
statement by itself.  I'll take a look at this, but not tonight.

Anyway, do whatever you think is best.  I just misunderstood what
you said earlier about it not being possible to be NULL.  I
understand it better now I think.  Thanks.

regards,
dan carpenter