Re: [patch] at76_usb wireless driver

Re: [patch] at76_usb wireless driver

[Guido Guenther]

Hi,
I'd be glad if someone could review the at76_usb wireless driver, it
adds support for the at76c503, at76c505 and at76c505a wireless USB
adapters. Since it exceeds the lists size limit, please
    git-clone http://honk.sigxcpu.org/git/at76c503a.git/

The projects homepage is:
    http://at76c503a.berlios.de/
The above git tree is functionally equivalent to the CVS version at
berlios but removes support for older kernels, older wireless
extensions, C99 style comments, commented out code and shifts things
around a little for better readability. This is only done to ease
reviewing - there are no functional changes over the CVS archive.

The driver was tested (and is in pracitical use) on at least i386, amd64
and powerpc.

Please note that I'm not the author of the driver, the driver itself
lists these copyright holders (but none of them showed up on the
mailing list during the last couple of months):

Copyright (c) 2002 - 2003 Oliver Kurth
Copyright (c) 2004 Joerg Albert <joerg.albert@gmx.de>
Copyright (c) 2004 Nick Jones
Copyright (c) 2004 Balint Seeber <n0_5p4m_p13453@hotmail.com>

Pavel Roskin, Maxim Grechkin and me were committing to CVS recently.
Cheers,
 -- Guido

Re: [patch] at76_usb wireless driver

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

Pavel should know better and have told you that wireless got it's own
list ;)
Quoting fully to make linux-wireless aware. I guess we'll want to take
out netdev on replies to this.

johannes

On Sat, 2007-03-03 at 16:00 +0100, Guido Guenther wrote:
> Hi,
> I'd be glad if someone could review the at76_usb wireless driver, it
> adds support for the at76c503, at76c505 and at76c505a wireless USB
> adapters. Since it exceeds the lists size limit, please
>     git-clone http://honk.sigxcpu.org/git/at76c503a.git/
> 
> The projects homepage is:
>     http://at76c503a.berlios.de/
> The above git tree is functionally equivalent to the CVS version at
> berlios but removes support for older kernels, older wireless
> extensions, C99 style comments, commented out code and shifts things
> around a little for better readability. This is only done to ease
> reviewing - there are no functional changes over the CVS archive.
> 
> The driver was tested (and is in pracitical use) on at least i386, amd64
> and powerpc.
> 
> Please note that I'm not the author of the driver, the driver itself
> lists these copyright holders (but none of them showed up on the
> mailing list during the last couple of months):
> 
> Copyright (c) 2002 - 2003 Oliver Kurth
> Copyright (c) 2004 Joerg Albert <joerg.albert@gmx.de>
> Copyright (c) 2004 Nick Jones
> Copyright (c) 2004 Balint Seeber <n0_5p4m_p13453@hotmail.com>
> 
> Pavel Roskin, Maxim Grechkin and me were committing to CVS recently.
> Cheers,
>  -- Guido
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [patch] at76_usb wireless driver

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]

Looks pretty good, a few more comments.

KEVENT_* constants and functions are really really confusing when the
"kevent" subsystem is being discussed on netdev all the time. They're
also quite meaningless, please rename them to something like
AT76_DEVEVENT_* or whatever. While at that, the kevent() function really
could use splitting up into sub-functions, it's a pretty large mess.

The same about constant names goes for PM_* constants since linux/pm.h
defines a whole bunch of PM_* constants too (I initially thought you
were using those and was really confused what the driver does!)

Having a whole bunch of module parameters that only set initial settings
for things you later configure with iwconfig seems pretty useless but if
you really think that they're absolutely required I don't care too much.

PRIV_IOCTL_SET_MONITOR_MODE is wrong as far as I can tell, there's
"iwconfig ... mode monitor" which you should use instead. (Btw, why are
the priv ioctls spaced so strangely??)

There doesn't seem to be a need to include rtnetlink.h, what made you
think you'd need rtnl_lock()?
"putting this inside rtnl_lock() - rtnl_unlock() hangs modprobe" is
pretty obvious -- register_netdev does rtnl_lock()! And don't use
unregister_netdevice(), use unregister_netdev().

Your init_new_device() function can return an error but those errors are
not always checked, maybe a printk would be appropriate. Also,
generally, subfunctions are allowed to return errors directly, i.e.
instead of returning -1 return -ENODEV from there and just hand it up
from the caller.

at76c503_do_probe could use splitting into two functions, the two huge
parts of the if, if only to unindent the whole code a bit and get it to
adhere to 80 chars/line instead of 99.

"Use our own dbg macro" but mabye use dev_dbg?
Same for err() (which I didn't even know existed..) how about dev_err()?

static u8 snapsig/rfc1042sig/bc_addr/off_addr/hw_rates/channel_frequency
etc etc etc don't belong into a header file.



Some other (mostly style) issues:
 * kernel code prefers a space before the brace in
   "struct at76c503_command{" et al.
 * both your header and code files contain lots of trailing whitespace
 * kernel code prefers no space between function names and the opening
   parenthesis
 * use compare_ether_addr() instead of memcmp (look for ETH_ALEN, lots
   of places)
 * all the PROC ===== stuff looks pretty strange to my eyes but hey
   that's just me I guess :)
 * why does at76c503_get_fw_info take such a huge number of parameters?
   Couldn't you pass in a struct at76c503 * and have that filled?
 * struct reg_domain should be tab-indented.
 * don't use typedefs for structs
 * attribute packed on members of a struct is pretty weird
 * the various frame definitions like struct ieee802_11_beacon_data are
   useless, the same stuff is in struct ieee80211_mgmt (at least on
   wireless dev kernel, that might not be true on other kernels?)
 * hex2str wants proper indentation
 * get_hw_config has weird indentation
 * wait_completion sounds far too generic (wait_for_completion in
   completion.h!)



Hey, I need to go but that probably gives you a lot to review...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]