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

[Roland Vossen]

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:
> http://marc.info/?l=user-mode-linux-devel&m=130036473931064&w=2
> That's why it's best to not combine conditions but to test each one on
> a separate line.

agree.

> 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;".

agree.

> 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
> corruption.)

agree.

> 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.

agree.

> 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.

ok.

> 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 
fullmac territory
- replaced broadcom specific structs with Linux structs (eg, the ones in 
ieee80211.h)
- 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.

Thanks, Roland.