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