[PATCH 2/2] staging: brcm80211: replace asserts close to Mac80211 interface
rvossen at broadcom.com
Tue Mar 29 05:03:36 PDT 2011
Hello Dan, Greg and John,
Greg and John: there is a request for you at the bottom of this email.
Dan, thanks for your elaborate answer on my questions/statements. I
appreciate the time you put into this. It cleared up quite some things
for me, so your typing was not in vain :-)
> It doesn't though... WARN_ON() just gives you the file and line number.
> Here is an example:
> That's why it's best to not combine conditions but to test each one on
> a separate line.
> If you
> really want to put a check there, then it would be more readable to just
> put a "WARN_ON(!wl);" instead of the "if (WARN_ON(!wl) return;".
> Adding the return is extra code added to handle conditions that can
> never be true. It's not even effective because obviously the code is so
> buggy at this point that there is no way to recover.
> Just calling WARN_ON() without returning would mean that we'd hit a NULL
> dereference and the kernel would oops. An oops doesn't take down the
> system. It would kill whichever program triggered the oops and
> afterward the driver would be dead. But the driver was going to be dead
> anyway because of the bug.
> BUG_ON() is the wrong idea as well because that means we can't close our
> documents and save everything before doing a reboot. (BUG_ON() should
> be used in places where continuing would result in filesystem
> There are basically three types conditions where people add WARN_ON()s.
> 1) Impossible conditions.
> 2) Things which are possible if people write buggy code.
> 3) Things which should be impossible but we're not sure.
> We shouldn't check for impossible things.
> It's good to check for buggy code, but it's better to focus on subtle
> things that might be missed by a code review.
> The CodingStyle doc is talking about if you have a kfree() or an unlock
> before the return. When I see a goto, I always assume that there is a
> unlock down there so I have to scroll down. The goto is misleading.
> I worry that you're focused way too much on the wrong things. The main
> thing this driver needs is to be more readable so it can get merged out
> from staging.
I (actually I should say: we) try to focus my attention on the most
important items: the tasks that would ultimately get us out of staging
(code cleanup and stability improvements).
Since its release, the brcm80211 source saw more than 400 patches, the
majority of them being code cleanup. Cleanup changes included:
- removing #ifdef'ed and unused code
- removed unused preprocessor constants
- reduced number of .c and .h files.
- reorganized directory structure to make clear what is softmac and
- replaced broadcom specific structs with Linux structs (eg, the ones in
- replaced typedefs by structs
On the stability front: both softmac and fullmac drivers are now, with a
new (upcoming) firmware release, stable. Users on several fora are
reporting positive experiences with them. The README and TODO files have
been updated accordingly (but are pending patches @ Greg).
The reason that I am investing time in the ASSERT/WARN_ON etc patches is
that Greg KH does not want ASSERT statement in our driver. It would be
great if I could simply replace all ASSERTs with WARN_ONs (inversing the
condition checked of course), so I can focus on improving code readability.
Can you tell me what kind of code readability improvements you suggest ?
We are now at a point where we are seeking specific feedback from the
community on what is needed to get brmc80211 out of staging.
Any feedback from you, John and/or Greg would certainly help us move in
the right direction.
More information about the devel