[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