[PATCH 2/2] staging: brcm80211: replace asserts close to Mac80211 interface

Roland Vossen rvossen at broadcom.com
Fri Mar 25 12:10:59 UTC 2011


Hello guys,

Greg, please discard this [PATCH2/2], I will send out a new patch.

Dan, thanks for the valuable feedback. As you noticed I have not reached 
the end of my learning curve yet ;-)

> The brcm80211 ASSERT() macro is compiled out unless you have
> BCMDBG defined.  There is no way to enable that through
> "make menuconfig".  So up to now basically everyone except the broadcom
> devs have run with all the ASSERT()s compiled out.  We could just delete
> them all and no one would notice.  I've obviously said before that I'd
> prefer to delete them where possible.

Agree, our goal is also to get rid of them completely. We will do that 
in a series of patches instead of posting one big patch, to facilitate 
reviewing and quicken the feedback cycle.

>>   static void wl_ops_stop(struct ieee80211_hw *hw)
>>   {
>> -#ifdef BRCMDBG
>>        struct wl_info *wl = hw->priv;
>
> Blank line after declarations.

I did not read this in the CodingStyle doc. checkpatch.pl did not warn 
me for that. I agree that it looks better. Will implement it.

>> +     if (likely(!WARN_ON(wl == NULL)))
>
> WARN_ON() includes it's own likely/unlikely so the hint isn't needed
> here.

Ah, I was not aware of that. Agree.

> Really this one is utterly pointless like I mentioned earlier.  We used
> to use "wl" but now that we don't use the "wl" variable any more in this
> function and we don't care if it's NULL or not.
>
> This function should just be:
> static void wl_ops_stop(struct ieee80211_hw *hw)
> {
>          ieee80211_stop_queues(hw);
> }

Agree.

> Or better yet, we can just delete the whole function.

Disagree. According to mac80211.h:

     * @stop: Called after last netdevice attached to the hardware
     ...
     *	Must be implemented and can sleep.

>
>> +     if (likely(hw != NULL))
>> +             wl = hw->priv;
>
> This isn't a fast path.  The likely() here is not needed.  It makes the
> code less readable for no reason.

Good suggestion. Agree.

>> +     if (unlikely(wl->pub->ieee_hw->priv != wl)) {
>> +             goto fail;
>> +     }
>
> The curly braces are not needed.  Checkpatch should warn about this, but
> it missed it.  It does catch it if you run checkpatch -f on the patched
> file...  I'm not sure what's the story with that.

Agree. Not sure either why checkpatch does not see it.

> Also why was the ASSERT even here in the first place?  We already know
> this is is true because we did the assignments ourselves.  What are we
> trying to check here?  Let's just delete this.

Agree.

>> -     if (phy_list[0] == 'n' || phy_list[0] == 'c') {
>> +     if (likely(!WARN_ON(phy_list[0] != 'n'&&  phy_list[0] != 'c'))) {
>
> This is sort of ugly even after we remove the duplicate likely().  Why
> don't you just replace the BUG_ON() with a WARN_ON(1);

Because the WARN_ON will give the user a hint of what condition failed 
(vs logging just the source file and line #). But agreed that the 
condition is more difficult to read, so I will take your suggestion.

> Just keep the error code from pci_enable_device() instead of overwriting
> it with -ENODEV.

Agree.

>> +     if (unlikely(WARN_ON(wl == NULL)))
>> +             return;
>
> This is never called with a NULL pointer so we could just delete it.  On
> the other hand, free() functions often accept NULL pointers so it might
> be better to use:
>
>          if (!wl)
>                  return;
>
> But probably it's better to just delete the check.  If people start
> randomly introducing calls to wl_free() and we start merging them
> without any review then we're already screwed and this WARN_ON()
> won't help us.

Well, that seems to be based on the assumption that reviews will catch 
all coding errors, thus run-time checking is not needed anymore. I 
prefer to build a bit more feedback in the code, experience has taught 
me that some run time checks on critical spots can save a lot of debug 
work.

>> -                     ASSERT(wl->resched == false);
>> +                     WARN_ON_ONCE(wl->resched == true);
>
> I assume this one triggers quite a bit on your devel systems?

It does not trigger a single time. What makes you think so ?

>> +                                     return -1;
>
> -1 is -EPERM.  -EINVAL would be more appropriate here.

Agree.

>> +     if (unlikely(WARN_ON(wlc == NULL || aci>= AC_COUNT)))
>> +             return;
>
> wlc isn't going to be NULL here.

Agree.

>
> [snip]
>
>> @@ -5173,27 +5185,30 @@ wlc_sendpkt_mac80211(struct wlc_info *wlc, struct sk_buff *sdu,
>>        struct scb *scb =&global_scb;
>>        struct ieee80211_hdr *d11_header = (struct ieee80211_hdr *)(sdu->data);
>>
>> -     ASSERT(sdu);
>> -
>> +     if (unlikely(WARN_ON(sdu == NULL)))
>> +             goto wlc_sendpkt_fail;
>
> Just return directly.

I thought it would be better coding practice if all functions have only 
one exit point. The CodingStyle doc 'Chapter 7: Centralized exiting of 
functions' seems to agree.

>> -     ASSERT(IS_ALIGNED((unsigned long)skb->data, 2));
>
> This breaks the compile.

I'll be damned. I have a little script that should build for both BCMDBG 
on and off, but obviously something is wrong with that script. I will 
take a look at it.

>> +     WARN_ON(p->next != NULL || p->prev != NULL);
>
> If you break it up, it's more readable and if you ever trigger the
> warning then you'll know which one is non-NULL.
>
>          WARN_ON(p->next);
>          WARN_ON(p->prev);

Agree. An argument could be that combining the statements gives less CPU 
overhead, but wlc_recvctl() is not called frequent enough to warrant that.

> There was a lot of code to review so I didn't review this entire patch.
> Obviously it will have to be redone to fix the compile and remove the
> likely/unlikely hints.  I'll take another look at it when version 2 is
> ready.

I will work on a new patch and send it out shortly.

As already said, thanks for the time and effort you put in this.

Roland Vossen.




More information about the devel mailing list