Re: [PATCH 1/2] wireless: move some utility functions from mac80211 to cfg80211

[Johannes Berg <johannes@sipsolutions.net>]

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

On Thu, 2009-05-21 at 10:18 +0800, Zhu Yi wrote:

> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -752,6 +752,32 @@ enum wiphy_params_flags {
>  };
>  
>  /**
> + * enum ieee80211_key_alg - key algorithm
> + * @ALG_WEP: WEP40 or WEP104
> + * @ALG_TKIP: TKIP
> + * @ALG_CCMP: CCMP (AES)
> + * @ALG_AES_CMAC: AES-128-CMAC
> + */
> +enum ieee80211_key_alg {
> +	ALG_WEP,
> +	ALG_TKIP,
> +	ALG_CCMP,
> +	ALG_AES_CMAC,
> +};
> +
> +/**
> + * enum ieee80211_key_len - key length
> + * @LEN_WEP40: WEP 5-byte long key
> + * @LEN_WEP104: WEP 13-byte long key
> + */
> +enum ieee80211_key_len {
> +	LEN_WEP40 = 5,
> +	LEN_WEP104 = 13,
> +	LEN_CCMP = 16,
> +	LEN_TKIP = 32,
> +};

This doesn't seem appropriate. ALG_* is purely mac80211 internals, and
shouldn't be in cfg80211's API since cfg80211 uses u32 values as cipher
suite specifiers. Why do you need these?

And if you want to move the key lengths they probably should be in
ieee80211.h, have a proper prefix and also be used within cfg80211
itself...

> +/* Default mapping in classifier to work with default queue setup. */
> +const int ieee802_1d_to_ac[8] = { 2, 3, 3, 2, 1, 1, 0, 0 };
> +EXPORT_SYMBOL(ieee802_1d_to_ac);

That seems a little odd, unless we want to specify how the default queue
setup should be ... which seems to make no sense. It probably will be
like this most of the time, but still, should this really be mandated by
cfg80211? There seems to be no reason to do that. I'd rather keep this
in every driver since the drivers may differ, and changing this for
mac80211 is unlikely.

> +int ieee80211_data_to_8023(struct sk_buff *skb, struct net_device *dev,
> +                          enum nl80211_iftype iftype)

Should probably pass in dev->dev_addr instead of dev for this and the
other direction? OTOH, I suppose it only makes sense to be used for a
netdev.


> 
> +u16 cfg80211_classify80211(struct sk_buff *skb)
> +{
> +       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
> +
> +       if (!ieee80211_is_data(hdr->frame_control)) {
> +               /* management frames go on AC_VO queue, but are sent
> +                * without QoS control fields */
> +               return 0;
> +       }
> +
> +       if (ieee80211_is_data_qos(hdr->frame_control))
> +               skb->priority = cfg80211_classify8021d(skb);
> +       else
> +               skb->priority = 0;
> +
> +       return ieee802_1d_to_ac[skb->priority];
> +}
> +EXPORT_SYMBOL(cfg80211_classify80211);

If you pass in suitable information for the ACM downgrade, it seems you
can move over that code too and remove one export. If your device
supports ACM you can always pass in 0 for the ACM bitfield, I think? It
seems a little weird to have this function and then write it in a way
mac80211 can't use it.

johannes

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