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:
signature.asc
Description: This is a digitally signed message part
