Re: Please pull 'libertas' branch of wireless-2.6

[Marcelo Tosatti <marcelo@kvack.org>]

On Mon, Mar 05, 2007 at 02:08:25PM +0000, Christoph Hellwig wrote:
> On Sun, Mar 04, 2007 at 12:36:27PM -0300, Marcelo Tosatti wrote:
> > Hi Christoph,
> > 
> > On Sat, Mar 03, 2007 at 05:21:40AM +0000, Christoph Hellwig wrote:
> > > Umm, I can't remember the updated driver ever beeig posted for review.
> > 
> > http://lists.openwall.net/netdev/2007/02/10/25
> > 
> > > And to be honest I'd be surprised if it's in a good shape already.
> > 
> > Constructive comments are welcome... All comments made by Arnd (which
> > were many!) have been addressed.
> 
> Comments on that versions from a quick read over it:
> 
>  - the __le* annotation issue you mentioned in the mail is important.
>    We definitvely don't want any new drivers without endianess annotations,
>    because there are far too many endianess problems.  Even more so
>    in a case of a driver like this one that's only tested on LE hardware
>    and has LE device endianess.
>  - things like 11d.[ch] don't have business of beeing in a driver,
>    this should be somewhere in common code.

Most wireless drivers implement 11d internally. We certainly need to
unify the management code, but thats not a merge blocker.

>  - there shouldn't be a LICENSE file in individual driver directories,
>    especially if it's just plain old GPLv2.
>  - please get rid of setting -DFOO flags in the Makefile, just use
>    these directly as config symbols.
>  - there shouldn't be a README file in the driver directory, this
>    should be in Documentation/
>  - please don't use wlan_* foo types.  a) this should be structs, not
>    typedefs, and b) wlan is an utterly generic name for beeing inside
>    a driver.  Then again most things using this should be inside
>    generic code anyway.. (and yeah, all that is because the driver
>    copied braindead linux-wlan-ng code that probably needs a major
>    revision anyway)
>  - there seems to be lots of tabs vs spaces messups
>  - please get rid of the ENTER/LEAVE macros

These are useful for debugging. We have removed most of the useless ones 
already.

>  - there's an awful lot of headers without clear divided responsibilities,
>    there should be only a few ones left (internal interfaces and hw
>    interface basically)
>  - having lowercase names for lots of hw commands is a very bad idea
>    for readability
>  - please get rid of all your private ioctls and iwpriv stuff
>    (should I add !!!! here)

Rationale being? There are a bunch of device private knobs, and iwpriv
is the interface for such configuration. (????)

>  - scan.h has very strange almost docbook comments, please convert
>    them to real docbook comments and actually run things through
>    the tools to make sure it's right.
>  - scan.h has vi indentation comment helpers that are contrary to
>    linux coding style..
>  - the thread.h abstractions are really useless, opencoding them
>    would make the code a lot more readable.  And make people
>    notice it's actually wrong:
> 	o the return value from kthread_run needs to be checked
> 	o wlan_deactivate_thread is not needed at all
> 	o storing and checking the pid should go away
> 	o there is no need for an additional waitqueue, you can
> 	  just use wake_up_process for kernel threads.
>  - most of types.h should not be there but you should be using
>    the types from include/linux/*80211*
>  - version.h shouldn't exist
>  - the radiotap header changes should definitively not be in
>    a "add a new driver" diff

Other than this your comments are OK, will address them.

Thanks!