Re: [PATCH 4/7] o80211s: (mac80211s) basic mesh interface support

[Johannes Berg <johannes@sipsolutions.net>]

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


> @@ -521,6 +522,8 @@ struct ieee80211_if_init_conf {
>   *	config_interface() call, so copy the value somewhere if you need
>   *	it.
>   * @ssid_len: length of the @ssid field.
> + * @mesh_id: mesh ID of the mesh network we will be part of.
> + * @mesh_id_len: length of the @mesh_id field.

Why would the driver need this?

> +ieee80211s-objs = ieee80211s.o ieee80211s_stats.o ieee80211s_pathtable.o

I think I'd prefer shorter filenames. Maybe mesh networking should also
be made configurable in Kconfig?

> +static int ieee80211_if_set_mesh_cfg(struct wiphy *wiphy,
> +		struct net_device *dev, struct mesh_params *params)
> +{
> +	struct ieee80211_local *local = wiphy_priv(wiphy);
> +	struct ieee80211_if_sta *ifsta;
> +	struct ieee80211_sub_if_data *sdata = NULL;
> +	if (unlikely(local->reg_state != IEEE80211_DEV_REGISTERED))
> +		return -ENODEV;
> +
> +	if (!dev)
> +		return 0;
> +	
> +	if (!(dev->ieee80211_ptr && dev->ieee80211_ptr->wiphy->privid 
> +				== mac80211_wiphy_privid))
> +		return -EINVAL;

This cannot happen if you wrote the nl80211 counterpart correctly.
 
>  	switch (sdata->type) {
>  	case IEEE80211_IF_TYPE_STA:
> +	case IEEE80211_IF_TYPE_MESH:
>  	case IEEE80211_IF_TYPE_IBSS:
>  		add_sta_files(sdata);

There are mesh APs too, that is not supported yet I take it? How do you
plan to support this when you're essentially hard-coding MESH == STA
here and in many other places?

> +static void ieee80211_mesh(struct net_device *dev,
> +			   struct ieee80211_if_sta *ifsta)
> +{
> +	mod_timer(&ifsta->timer, jiffies + IEEE80211_MONITORING_INTERVAL);
> +}

?? That needs a better name, whatever it does.

> +++ b/net/mac80211/ieee80211s.c
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright (c) 2007 open80211s Ltd.
> + * Authors :   Luis Carlos Cobo <luisca@cozybit.com>
> + * 	       Javier Cardona <javier@cozybit.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "ieee80211s.h"
> +
> +int ieee80211s_start(void)
> +{
> +	o80211s_pathtable_init();
> +	return o80211s_create_procmesh();
> +}

procmesh?

The whole ieee80211s.c file is useless. _start() and _stop() can well be
split into the two calls at the callsites, and the header functions can
be part of util.c or so.

> + * Print one entry (line) of /proc/net/mesh

Why do we need /proc/net/mesh? Whatever interface it provides should be
in nl80211 and if you need it for testing, debugfs.

> +	if (rx->sdata->type == IEEE80211_IF_TYPE_MESH) {
> +		if (((rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA) &&
> +		     !((rx->fc & IEEE80211_FCTL_FROMDS) &&
> +		      (rx->fc & IEEE80211_FCTL_TODS)))

I'd rewrite that as ((rx->fc & (FROMDS|TODS)) == TODS) or whatever it is.
 
> +	if (sdata->type == IEEE80211_IF_TYPE_MESH) {
> +		int meshhdrlen = get_mesh_header_len(
> +				(struct ieee80211s_hdr*) skb->data);
> +		/* copy mesh header on cb to be use if forwarded */
> +		memcpy(skb->cb, skb->data + hdrlen, meshhdrlen);
> +		hdrlen += meshhdrlen;
> +	}

A note to which code uses it from cb please.
 
> +	if (tx->sdata->type == IEEE80211_IF_TYPE_MESH) {
> +		return TXRX_CONTINUE;}
> +

That looks crappy. Whats with the braces?

johannes

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