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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
