Re: [PATCH 4/4] ath9k: Add new Atheros IEEE 802.11n driver [reference to URL]

[Johannes Berg <johannes@sipsolutions.net>]

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

On Mon, 2008-08-04 at 00:30 -0700, Luis R. Rodriguez wrote:
> This adds the new mac80211 11n ath9k Atheros driver. Only STA support
> is currently enabled and tested.

Couple of comments:

ath_softc is _huge_, and the spinlocks in it are at the worst possible
place. Placing them together with the data they protect should be
better.


There's talk about sysctls that do not exist.


+/* Until mac80211 includes these fields */

please just post a patch doing so.


You haven't explained ath_scan_end et al. yet.


+               /* For HT capable stations, we save tidno for later use.
+                * We also override seqno set by upper layer with the
one
+                * in tx aggregation state.
+                *
+                * First, the fragmentation stat is determined.
+                * If fragmentation is on, the sequence number is
+                * not overridden, since it has been
+                * incremented by the fragmentation routine.

do you really still need to override the seqno? Did I make a mistake? Or
is this just older code? Or is it necessary because the aggregation
callback wants the seqno? In that case I can preassign it there to the
right thing...


+        * Calculate duration.  This logically belongs in the 802.11
+        * layer but it lacks sufficient information to calculate it.

what information does it lack? It _does_ in fact calculate the duration.
Something wrong there with aggregation? Please realise that you can, in
fact, change the upper layer.


+#ifdef USE_LEGACY_HAL

eh?


I'd appreciate somebody else reviewing the driver from a stack
perspective too, I don't have all that much time right now.

johannes

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