Re: [patch] at76_usb wireless driver

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: 528 bytes --]

Quick comments on this driver.

 (1) I'd prefer if the huge list of USB IDs was in a header file that is
     included, quick cut'n'paste job :)
 (2) remove the LED stuff for ipaq, that belongs into an LED trigger and
     ipaq can define an LED driver for the LED so you can hook it up,
     also makes it generic so other people can use the LED trigger
     functionality
 (3) don't initialise static variables to 0
 (4) reorder code so less function prototypes are required

more to follow after tea.

johannes

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

Re: [patch] at76_usb wireless driver

[Guido Guenther]

Hi Johannes,
On Sat, Mar 03, 2007 at 04:18:36PM +0100, Johannes Berg wrote:
> Quick comments on this driver.
thanks a lot for reviewing the code! These issues (except for (4)) are
already adressed in current git, Pavel fixed some more.
Cheers,
 -- Guido

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

Re: [patch] at76_usb wireless driver

[Pavel Roskin]

Quoting Johannes Berg <johannes@sipsolutions.net>:

> Looks pretty good, a few more comments.

Thanks!

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

I agree.  The purpose is likely to avoid scheduling too many different tasks,
but I don't think it's worth the trouble.  In fact, it would be better if the
driver had smaller tasks that would all compete with other scheduled tasks.  It
would be fair to others.

> 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!)

Good catch.

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

I agree, they are not needed.  It should be verified that all parameters can be
set using iwconfig or iwpriv.  Maybe "debug" should stay.

> 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??)

The code is quite old and may predate monitor mode.  Reference to Kismet means
that the driver was meant to emulate an old unofficial patch to orinoco driver.

On the other hand, the same setting selects whether to enable prism header, so
that part should probably stay.

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

I see Guido has done it already.

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

Agreed.

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

Actually, there is some code duplication between the branches.  It would be
great to avoid it first.  I also see that the code overuses debug printing.  I
prefer to add it to fix a specific problem.  Keeping large portions of debug
code just in case is probably not a good idea, since they reduce readability of
the code.

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

I think the driver uses USB functions here.  I think it's OK for a USB driver. 
But the own dbg macro should probably be avoided.

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

Absolutely!  But since it's mixed with definitions, perhaps the whole code could
be merged into at76_usb.c.  It's not like any other driver will ever include
that header.  And adding 30k to 200k won't change much.

> 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

I think Lindent (from scripts directory in Linux sources) should be run on the
driver.  Better yet, "Linudent --ignore-newlines).

>  * use compare_ether_addr() instead of memcmp (look for ETH_ALEN, lots
>    of places)

OK

>  * all the PROC ===== stuff looks pretty strange to my eyes but hey
>    that's just me I guess :)

That must be style from a language where functions are called procedures.  Sure,
it must be removed.

>  * 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?

Good idea.

>  * struct reg_domain should be tab-indented.

Lindent should take care of it.

>  * don't use typedefs for structs

All that code is prism header implementation from linux-wlan-ng, but I
understand that it's a bad excuse.  Besides, one of the typedefs is not used.

>  * attribute packed on members of a struct is pretty weird

Another ugly legacy of linux-wlan-ng.

>  * 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?)

Yes, this needs to be done, but it requires some work.  The names are different.

>  * hex2str wants proper indentation
>  * get_hw_config has weird indentation

Lindent will fix it.

>  * wait_completion sounds far too generic (wait_for_completion in
>    completion.h!)

Yes, many function should be renamed.

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

Thanks!

I would also add:

* useless macros (NUM_EP, EP)
* macros for locking should be avoided
* istate should be accessed using atomic_set/atomic_read; locking is overkill

--
Regards,
Pavel Roskin

Re: [patch] at76_usb wireless driver

[Johannes Berg]

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

On Sun, 2007-03-04 at 02:09 -0500, Pavel Roskin wrote:

> > 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??)
> 
> The code is quite old and may predate monitor mode.  Reference to Kismet means
> that the driver was meant to emulate an old unofficial patch to orinoco driver.
> 
> On the other hand, the same setting selects whether to enable prism header, so
> that part should probably stay.

Wouldn't you just always have prism headers in monitor mode? All cards I
know do that iirc. Or well, radiotap seems to be the format of fashion
these days. I don't particularly like either :)

> > static u8 snapsig/rfc1042sig/bc_addr/off_addr/hw_rates/channel_frequency
> > etc etc etc don't belong into a header file.
> 
> Absolutely!  But since it's mixed with definitions, perhaps the whole code could
> be merged into at76_usb.c.  It's not like any other driver will ever include
> that header.  And adding 30k to 200k won't change much.

Yeah, still, it's pretty weird to find that in a header file. It should
be pretty easy to move it too ;)

> > 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
> 
> I think Lindent (from scripts directory in Linux sources) should be run on the
> driver.  Better yet, "Linudent --ignore-newlines).

Not sure. I find that it messes up some things too. Maybe run Lindent,
diff the original vs. the result and take the hunks you like.

> >  * don't use typedefs for structs
> 
> All that code is prism header implementation from linux-wlan-ng, but I
> understand that it's a bad excuse.  Besides, one of the typedefs is not used.

Heh, I didn't really check if they were used.

> >  * 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?)
> 
> Yes, this needs to be done, but it requires some work.  The names are different.

Yeah, it shouldn't take long either, just remove the definitions that
are there and fix the compile errors :P

> * istate should be accessed using atomic_set/atomic_read; locking is overkill

atomic operations can be quite expensive too, but you probably know
better if you need it or not.

johannes

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

Re: [patch] at76_usb wireless driver

[Pavel Roskin]

On Mon, 2007-03-05 at 00:52 +0100, Johannes Berg wrote:
> Wouldn't you just always have prism headers in monitor mode? All cards I
> know do that iirc. Or well, radiotap seems to be the format of fashion
> these days. I don't particularly like either :)

Sounds good.  Radiotap is more flexible and better defined, but just
leaving prism headers for now would be fine.

> Not sure. I find that it messes up some things too. Maybe run Lindent,
> diff the original vs. the result and take the hunks you like.

Yes, something like that.

> > * istate should be accessed using atomic_set/atomic_read; locking is overkill
> 
> atomic operations can be quite expensive too, but you probably know
> better if you need it or not.

Actually, it looks like we don't need them either.  Atomic operations
are useful for counters or generally when data is read and written at
once.  Simply reading and writing integers is atomic by design of any
sane and supported CPU.

The istate locks were causing complains from the lock checkers, and my
attempt to replace them with atomic operations silenced those
complaints.  But It looks like we just need to drop those locks.  They
were put there in a hurry without due checking, they don't address any
problem, but they may create one.

-- 
Regards,
Pavel Roskin

Re: [patch] at76_usb wireless driver

[Johannes Berg]

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

And some more ;) These are mostly like "couldn't you do this and it'd
make changing the driver a bit less hazardous" :)

(We need to fix this in mac80211 too in various places)
You should probably use ARRAY_SIZE() in quite a few places, e.g.
at76c503_handler_def, getRegDomain and possibly more

I don't really remember, but wasn't the general concensus last time this
came up that the wext nickname is pretty much useless and doesn't really
need to be settable? I know, it's only like 5 lines of code :)


        /* do the restart after two seconds to catch
         * following ioctl's (from more params of iwconfig)
         * in _one_ restart */
        mod_timer(&dev->restart_timer, jiffies+2*HZ);

Isn't that why you should have a commit call in the first place? This
seems pointless. Or do you just do this because people expect "iwconfig
ethN essid mygreatAP" to actually do something right away (they really
shouldn't)?

"Firmware names - this must be in sync with boardtype definitions"

I guess you could use C99 array initialisers for the firmwares array to
make it safer.

#define DRIVER_AUTHOR ...

I think it's perfectly valid to have multiple MODULE_AUTHOR lines and
having a define for this (and DRIVER_DESC) seems pointless.

You can probably replace the semaphore by a mutex.

johannes

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