Re: [patch] carl9170: remove unneeded NULL check

[Dan Carpenter <dan.carpenter@oracle.com>]

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