On Fri, 26 Aug 2005 00:31:06 +0200, Pavel Machek wrote:
> > ieee-dev-alignment.patch
>
> +static inline void *ieee80211_dev_to_priv(struct net_device *dev)
> +{
> + return (char *)dev +
> + ((sizeof(struct net_device) + NETDEV_ALIGN_CONST)
> + & ~NETDEV_ALIGN_CONST) +
> + ((sizeof(struct ieee80211_device) + NETDEV_ALIGN_CONST)
> + & ~NETDEV_ALIGN_CONST);
> +}
>
> Should not this use container_of() and friends?
Private data of network device are allocated by alloc_netdev() together
with struct net_device by one kmalloc(). So the priv field in net_device
points to the end of net_device (or, more precisely, to the end of
net_device plus some padding bytes so the address of private data is
aligned to NETDEV_ALIGN).
We could only use container_of() if net_device was the last field in
struct ieee80211_device. But in that case we could not use
alloc_netdev() anyway - and we definitely want to use it.
> > configuration-struct.patch
>
> You probably do not want to use u64... it is going to be slow on
> 32-bit architectures.
OK. Hope that 32 configuration options will be enough :-)
> +static inline int ieee80211_encrypt_frame(
> + struct ieee80211_device *ieee,
> + struct sk_buff *skb,
> + int hdr_len)
>
> this looks pretty ugly to my eyes. Could not we keep it on two lines?
OK. It was copied from ieee80211_encrypt_fragment() in the same file, which
probably needs to be reformatted later as well.
> Looks nice.. why is ieee->seq_number += 0x10; increased by 0x10?
Sequence number is stored in bits 4 to 15 of the Sequence Control field
in 802.11 header. Lower 4 bits are used for fragment number.
> > separate-from-ethernet.patch
>
> Okay, here it starts to be "interesting". Patches up to here seem
> quite mergeable to me. This one will rename wifi from eth1 to wlan0,
> right?
>
> OTOH that should not be a big problem; wifi is not in mainline, yet.
Hostap uses wlan0 as well.
> +#define IEEE80211_SNAP_IS_BRIDGE_TUNNEL(snap) \
> + ((snap)->oui[0] == 0 && (snap)->oui[1] == 0 && (snap)->oui[2]
> == 0xf8)
>
> You really need some shorter prefix, and shorter names would be
> welcome, too. [Maybe these should be inline functions?
I agree. Don't know who started to use ieee80211_ prefixes but I think
there wasn't better prefix available (p80211 is the name of another -
although dead - project, wlan is confusing, etc.). We try to use as
short names as possible - do you have any idea how to shorten this one?
> +#define IEEE80211_GET_SADDR(hdr) \
> + (IEEE80211_FC_GET_FROMDS(hdr) ? \
> + (IEEE80211_FC_GET_TODS(hdr) ? (hdr)->addr4 :
> (hdr)->addr3) \
> + : (hdr)->addr2)
>
> ]
I'm not against turning these macros into inline functions. But I'm
afraid that will make ieee80211.h longer and yet more unreadable. What
do you think is better?
> > fix-packet_socket-arp.patch
>
> +#if defined(CONFIG_IEEE80211) || defined(CONFIG_IEEE80211_MODULE)
> [...]
> +#endif
>
> I believe CONFIG_FOO is set if CONFIG_FOO_MODULE is set; therefore we
> do not need as ugly ifdefs.
I'm not sure about this - when I tested it few months ago, it wasn't.
And it seems that in net/ipv4/arp.c it is used the same way.
> > fix-definitions-for-xmit.patch
>
> +u8 ieee80211_rate_values[IEEE80211_NUM_RATES] = {
> + 0x02, 0x04,
> + 0x0b, 0x16,
> + 0x2c, 0x42,
> + 0x0c, 0x12, 0x18, 0x24, 0x30, 0x48, 0x60, 0x6c
> +};
> +EXPORT_SYMBOL(ieee80211_rate_values);
>
> Could we get some comment here?
It is a static table for converting of internally used rate constants (enum
ieee80211_rate) to (and from) values used in 802.11 management frames.
It is exported as drivers for some devices with more advanced firmware
need to convert the values themselves.
> > networks.patch
>
> +/* Allocate the table and initialize into an empty state. */
> +int ieee80211_networks_init(struct ieee80211_device *ieee)
> [...]
>
> Normal calling convention is 0 on success, -ERRNO on error.
OK.
> [...]
> + else
> + n->newer = WNETIDX_MAX;
>
> I'd not put newline before else.
OK.
>
> + if (!time_after(network->last_scanned + DEFAULT_MAX_SCAN_AGE,
> jiffies)) {
> + IEEE80211_DEBUG_SCAN(
> + "Not showing network '%s ("
> + MAC_FMT ")' due to age (%lums).\n",
> + escape_essid(network->ssid,
> + network->ssid_len),
> + MAC_ARG(network->bssid),
> + (jiffies - network->last_scanned) / (HZ / 100));
> + return 1;
> + }
>
> Ouch. Private debug macros are ugly, and putting 4 words on line
> because you are religious about 80-column-rule seems like very bad
> idea.
This is from the original code, just indentation was changed. Probably
all of debugging messages have to be revisited at some time.
>
> +static inline int _iwe_stream_add_event(struct translate_scan *data,
> + struct iw_event *iwe, int len)
> [...]
>
> Now this is strange. _foo that is wrapper around foo. Looks too
> similar to __foo convention to my eyes. Is some better naming
> possible?
Maybe it will be rewritten to take Jean Tourrilhes' comments into
account. I will discuss it with Jirka Bohac next week.
Corrected patches will be put to kernel.org/pub/linux/kernel/people/jbenc/
later today.
Thanks for your comments,
--
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
