Re: [ath9k-devel] [PATCH 1/4] Add new Atheros IEEE 802.11n ath9k driver

[Johannes Berg <johannes@sipsolutions.net>]

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


>  > +enum hal_freq_band {
>  > +	HAL_FREQ_BAND_5GHZ = 0,
>  > +	HAL_FREQ_BAND_2GHZ = 1,
>  > +};
>  > 
>  > You can remove those.
> 
> And use mac80211's band macros ?

I guess the question really is what you need them for, but yeah, if you
really need them you should be using the mac80211 stuff to avoid having
to translate the values into each other all the time.

>  > +struct ath_node *ath_node_get(struct ath_softc *sc, u8 *addr)
>  > 
>  > + * Setup driver-specific state for a newly associated node.  This routine
>  > + * really only applies if compression or XR are enabled, there is no code
>  > + * covering any other cases.
>  > 
>  > What sort of state do you need? Anything mac80211 could cover?
> 
> Aggregation information is maintained on a per-STA basis.
> All this node stuff will be cleaned up anyway.

Ok cool. As I just discussed with the Intel folks, we can give you a
private area in the sta_info struct that we pass down all the time so
you can look in there instead of implementing your own
hashtable/list/...

>  > What sort of node state do you really need? Why do you need PS state?
>  > Can we help you here with mac80211? Get some insight into a part of
>  > sta_info and have that passed to the driver to avoid having a list/hash
>  > table in the driver etc.
> 
> Can sta_info be accessed directly from the driver ?
> Don't we need a ieee80211_local ptr to retrieve sta_info ?
> And if so, is that ok ?
> Or will you be providing a private driver area embedded in sta_info ?

No, you cannot look at sta_info, but it shouldn't be hard to make part
of the structure public, embed a private area into it and pass that
down, like the "vif" pointer in a few places.

>  > +	error = ath_vap_attach(sc, 0, conf->vif, ic_opmode, ic_opmode, 0);
>  > +	if (error) {
>  > +		DPRINTF(sc, ATH_DEBUG_FATAL,
>  > +			"%s: Unable to attach vap, error: %d\n",
>  > +			__func__, error);
>  > +		goto bad;
>  > 
>  > Does that reject the call if ic_opmode is 0? Why is ic_opmode passed twice?
> 
> No it doesn't, for some reason IBSS has a value 0 for its opmode.
> But if opmode happens to be invalid, -EINVAL is returned.
> And yep, it shouldn't be passed twice.

Ok so of course I think you should use the mac80211 interface type
instead of opmode all the way through, and you should be checking the
interface types you support here because right now it looks like if I
try to bring up an AP or mesh interface I'll get IBSS?

johannes

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