[B.A.T.M.A.N.] [PATCH 1/5] batman-adv: Wrapper functions for sysfs storing

[B.A.T.M.A.N.] [PATCH 1/5] batman-adv: Wrapper functions for sysfs storing

[Linus Lüssing]

Sysfs configuration options that just took a boolean value
(enable(d)/disable(d)/0/1) and integer setting basically all had the same
structure.

To avoid even more copy and pasting in the future and to make introducing
new configuration parameters for batman-adv simpler, more generic
wrapper functions are being introduced with this commit. They can deal with
boolean and unsigned integer parameters, storing them in the specified
atomic_t variables.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bat_sysfs.c |  224 +++++++++++++++++++++++-----------------------------------
 1 files changed, 89 insertions(+), 135 deletions(-)

diff --git a/bat_sysfs.c b/bat_sysfs.c
index 3f551f3..60e3122 100644
--- a/bat_sysfs.c
+++ b/bat_sysfs.c
@@ -39,6 +39,81 @@ struct bat_attribute bat_attr_##_name = {	\
 	.store  = _store,			\
 };
 
+static int store_switch_attr(char *buff, size_t count,
+			     struct net_device *net_dev,
+			     char *attr_name, atomic_t *attr)
+{
+	int enabled = -1;
+
+	if (buff[count - 1] == '\n')
+		buff[count - 1] = '\0';
+
+	if ((strncmp(buff, "1", 2) == 0) ||
+	    (strncmp(buff, "enable", 7) == 0) ||
+	    (strncmp(buff, "enabled", 8) == 0))
+		enabled = 1;
+
+	if ((strncmp(buff, "0", 2) == 0) ||
+	    (strncmp(buff, "disable", 8) == 0) ||
+	    (strncmp(buff, "disabled", 9) == 0))
+		enabled = 0;
+
+	if (enabled < 0) {
+		bat_info(net_dev,
+			 "%s: Invalid parameter received: %s\n",
+			 attr_name, buff);
+		return -EINVAL;
+	}
+
+	if (atomic_read(attr) == enabled)
+		return count;
+
+	bat_info(net_dev, "%s: Changing from: %s to: %s\n", attr_name,
+		 atomic_read(attr) == 1 ? "enabled" : "disabled",
+		 enabled == 1 ? "enabled" : "disabled");
+
+	atomic_set(attr, (unsigned)enabled);
+	return count;
+}
+
+static int store_uint_attr(char *buff, size_t count,
+			   struct net_device *net_dev, char *attr_name,
+			   unsigned int min, unsigned int max, atomic_t *attr)
+{
+	unsigned long uint_val;
+	int ret;
+
+	ret = strict_strtoul(buff, 10, &uint_val);
+	if (ret) {
+		bat_info(net_dev,
+			 "%s: Invalid parameter received: %s\n",
+			 attr_name, buff);
+		return -EINVAL;
+	}
+
+	if (uint_val < min) {
+		bat_info(net_dev, "%s: Value is too small: %lu min: %u\n",
+			 attr_name, uint_val, min);
+		return -EINVAL;
+	}
+
+	if (uint_val > max) {
+		bat_info(net_dev, "%s: Value is too big: %lu max: %u\n",
+			 attr_name, uint_val, max);
+		return -EINVAL;
+	}
+
+	if (atomic_read(attr) == uint_val)
+		return count;
+
+	bat_info(net_dev, "%s: Changing from: %i to: %lu\n",
+		 attr_name, atomic_read(attr), uint_val);
+
+	atomic_set(attr, uint_val);
+	return count;
+}
+
+
 static ssize_t show_aggr_ogms(struct kobject *kobj, struct attribute *attr,
 			     char *buff)
 {
@@ -56,36 +131,9 @@ static ssize_t store_aggr_ogms(struct kobject *kobj, struct attribute *attr,
 	struct device *dev = to_dev(kobj->parent);
 	struct net_device *net_dev = to_net_dev(dev);
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	int aggr_tmp = -1;
-
-	if (((count == 2) && (buff[0] == '1')) ||
-	    (strncmp(buff, "enable", 6) == 0))
-		aggr_tmp = 1;
-
-	if (((count == 2) && (buff[0] == '0')) ||
-	    (strncmp(buff, "disable", 7) == 0))
-		aggr_tmp = 0;
-
-	if (aggr_tmp < 0) {
-		if (buff[count - 1] == '\n')
-			buff[count - 1] = '\0';
-
-		bat_info(net_dev,
-			 "Invalid parameter for 'aggregate OGM' setting"
-			 "received: %s\n", buff);
-		return -EINVAL;
-	}
-
-	if (atomic_read(&bat_priv->aggregation_enabled) == aggr_tmp)
-		return count;
-
-	bat_info(net_dev, "Changing aggregation from: %s to: %s\n",
-		 atomic_read(&bat_priv->aggregation_enabled) == 1 ?
-		 "enabled" : "disabled", aggr_tmp == 1 ? "enabled" :
-		 "disabled");
 
-	atomic_set(&bat_priv->aggregation_enabled, (unsigned)aggr_tmp);
-	return count;
+	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
+				 &bat_priv->aggregation_enabled);
 }
 
 static ssize_t show_bond(struct kobject *kobj, struct attribute *attr,
@@ -105,36 +153,9 @@ static ssize_t store_bond(struct kobject *kobj, struct attribute *attr,
 	struct device *dev = to_dev(kobj->parent);
 	struct net_device *net_dev = to_net_dev(dev);
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	int bonding_enabled_tmp = -1;
-
-	if (((count == 2) && (buff[0] == '1')) ||
-	    (strncmp(buff, "enable", 6) == 0))
-		bonding_enabled_tmp = 1;
 
-	if (((count == 2) && (buff[0] == '0')) ||
-	    (strncmp(buff, "disable", 7) == 0))
-		bonding_enabled_tmp = 0;
-
-	if (bonding_enabled_tmp < 0) {
-		if (buff[count - 1] == '\n')
-			buff[count - 1] = '\0';
-
-		bat_err(net_dev,
-			"Invalid parameter for 'bonding' setting received: "
-			"%s\n", buff);
-		return -EINVAL;
-	}
-
-	if (atomic_read(&bat_priv->bonding_enabled) == bonding_enabled_tmp)
-		return count;
-
-	bat_info(net_dev, "Changing bonding from: %s to: %s\n",
-		 atomic_read(&bat_priv->bonding_enabled) == 1 ?
-		 "enabled" : "disabled",
-		 bonding_enabled_tmp == 1 ? "enabled" : "disabled");
-
-	atomic_set(&bat_priv->bonding_enabled, (unsigned)bonding_enabled_tmp);
-	return count;
+	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
+				 &bat_priv->bonding_enabled);
 }
 
 static ssize_t show_frag(struct kobject *kobj, struct attribute *attr,
@@ -154,37 +175,14 @@ static ssize_t store_frag(struct kobject *kobj, struct attribute *attr,
 	struct device *dev = to_dev(kobj->parent);
 	struct net_device *net_dev = to_net_dev(dev);
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	int frag_enabled_tmp = -1;
-
-	if (((count == 2) && (buff[0] == '1')) ||
-	    (strncmp(buff, "enable", 6) == 0))
-		frag_enabled_tmp = 1;
-
-	if (((count == 2) && (buff[0] == '0')) ||
-	    (strncmp(buff, "disable", 7) == 0))
-		frag_enabled_tmp = 0;
-
-	if (frag_enabled_tmp < 0) {
-		if (buff[count - 1] == '\n')
-			buff[count - 1] = '\0';
-
-		bat_err(net_dev,
-			"Invalid parameter for 'fragmentation' setting on mesh"
-			"received: %s\n", buff);
-		return -EINVAL;
-	}
-
-	if (atomic_read(&bat_priv->frag_enabled) == frag_enabled_tmp)
-		return count;
+	int ret;
 
-	bat_info(net_dev, "Changing fragmentation from: %s to: %s\n",
-		 atomic_read(&bat_priv->frag_enabled) == 1 ?
-		 "enabled" : "disabled",
-		 frag_enabled_tmp == 1 ? "enabled" : "disabled");
+	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name,
+				&bat_priv->frag_enabled);
+	if (ret)
+		update_min_mtu(net_dev);
 
-	atomic_set(&bat_priv->frag_enabled, (unsigned)frag_enabled_tmp);
-	update_min_mtu(net_dev);
-	return count;
+	return ret;
 }
 
 static ssize_t show_vis_mode(struct kobject *kobj, struct attribute *attr,
@@ -300,31 +298,9 @@ static ssize_t store_orig_interval(struct kobject *kobj, struct attribute *attr,
 	struct device *dev = to_dev(kobj->parent);
 	struct net_device *net_dev = to_net_dev(dev);
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	unsigned long orig_interval_tmp;
-	int ret;
 
-	ret = strict_strtoul(buff, 10, &orig_interval_tmp);
-	if (ret) {
-		bat_info(net_dev, "Invalid parameter for 'orig_interval' "
-			 "setting received: %s\n", buff);
-		return -EINVAL;
-	}
-
-	if (orig_interval_tmp < JITTER * 2) {
-		bat_info(net_dev, "New originator interval too small: %li "
-			 "(min: %i)\n", orig_interval_tmp, JITTER * 2);
-		return -EINVAL;
-	}
-
-	if (atomic_read(&bat_priv->orig_interval) == orig_interval_tmp)
-		return count;
-
-	bat_info(net_dev, "Changing originator interval from: %i to: %li\n",
-		 atomic_read(&bat_priv->orig_interval),
-		 orig_interval_tmp);
-
-	atomic_set(&bat_priv->orig_interval, orig_interval_tmp);
-	return count;
+	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
+			       2 * JITTER, UINT_MAX, &bat_priv->orig_interval);
 }
 
 #ifdef CONFIG_BATMAN_ADV_DEBUG
@@ -344,31 +320,9 @@ static ssize_t store_log_level(struct kobject *kobj, struct attribute *attr,
 	struct device *dev = to_dev(kobj->parent);
 	struct net_device *net_dev = to_net_dev(dev);
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	unsigned long log_level_tmp;
-	int ret;
-
-	ret = strict_strtoul(buff, 10, &log_level_tmp);
-	if (ret) {
-		bat_info(net_dev, "Invalid parameter for 'log_level' "
-			 "setting received: %s\n", buff);
-		return -EINVAL;
-	}
-
-	if (log_level_tmp > 3) {
-		bat_info(net_dev, "New log level too big: %li "
-			 "(max: %i)\n", log_level_tmp, 3);
-		return -EINVAL;
-	}
-
-	if (atomic_read(&bat_priv->log_level) == log_level_tmp)
-		return count;
-
-	bat_info(net_dev, "Changing log level from: %i to: %li\n",
-		 atomic_read(&bat_priv->log_level),
-		 log_level_tmp);
 
-	atomic_set(&bat_priv->log_level, (unsigned)log_level_tmp);
-	return count;
+	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
+			       0, 3, &bat_priv->log_level);
 }
 #endif
 
-- 
1.7.1

[B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Linus Lüssing]

This makes exposing bat_priv's atomic_ts storing boolean or uint values
a one-liner in the future. It also further reduces the amount of
duplicate code in bat_sysfs.c.

Additionally, the sysfs entries "aggregate_ogms" and "fragmentation"
get renamed to "aggregation" and "frag" for consistency reasons, they
match the *_enabled parts in bat_priv now.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bat_sysfs.c |  186 ++++++++++++++++++++---------------------------------------
 1 files changed, 62 insertions(+), 124 deletions(-)

diff --git a/bat_sysfs.c b/bat_sysfs.c
index 60e3122..878cb8e 100644
--- a/bat_sysfs.c
+++ b/bat_sysfs.c
@@ -31,6 +31,7 @@
 
 #define to_dev(obj)     container_of(obj, struct device, kobj)
 
+/* Use this, if you have customized show and store functions */
 #define BAT_ATTR(_name, _mode, _show, _store)	\
 struct bat_attribute bat_attr_##_name = {	\
 	.attr = {.name = __stringify(_name),	\
@@ -39,6 +40,60 @@ struct bat_attribute bat_attr_##_name = {	\
 	.store  = _store,			\
 };
 
+/* Use this, if you are going to turn a [name]_enabled in bat_priv on or off */
+#define BAT_ATTR_SWITCH(_name, _mode, _post_func)			\
+static ssize_t store_##_name(struct kobject *kobj, struct attribute *attr, \
+		      char *buff, size_t count)				\
+{									\
+	struct device *dev = to_dev(kobj->parent);			\
+	struct net_device *net_dev = to_net_dev(dev);			\
+	struct bat_priv *bat_priv = netdev_priv(net_dev);		\
+	void (*post_func)(struct net_device *) = _post_func;		\
+	int ret;							\
+	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name, \
+				 &bat_priv->_name##_enabled);		\
+	if (post_func && ret)						\
+		post_func(net_dev);					\
+	return ret;							\
+}									\
+static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr, \
+			    char *buff)					\
+{									\
+	struct device *dev = to_dev(kobj->parent);			\
+	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));	\
+	return sprintf(buff, "%s\n",					\
+		       atomic_read(&bat_priv->_name##_enabled) == 0 ?	\
+		       "disabled" : "enabled");				\
+}									\
+BAT_ATTR(_name, _mode, show_##_name, store_##_name)
+
+/* Use this, if you are going to set [name] in bat_priv to unsigned integer
+ * values only */
+#define BAT_ATTR_UINT(_name, _mode, _min, _max, _post_func)		\
+static ssize_t store_##_name(struct kobject *kobj, struct attribute *attr, \
+		      char *buff, size_t count)				\
+{									\
+	struct device *dev = to_dev(kobj->parent);			\
+	struct net_device *net_dev = to_net_dev(dev);			\
+	struct bat_priv *bat_priv = netdev_priv(net_dev);		\
+	void (*post_func)(struct net_device *) = _post_func;		\
+	int ret;							\
+	ret = store_uint_attr(buff, count, net_dev, (char *)attr->name,	\
+			       _min, _max, &bat_priv->_name);		\
+	if (post_func && ret)						\
+		post_func(net_dev);					\
+	return ret;							\
+}									\
+static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr, \
+			    char *buff)					\
+{									\
+	struct device *dev = to_dev(kobj->parent);			\
+	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));	\
+	return sprintf(buff, "%i\n", atomic_read(&bat_priv->_name));	\
+}									\
+BAT_ATTR(_name, _mode, show_##_name, store_##_name)
+
+
 static int store_switch_attr(char *buff, size_t count,
 			     struct net_device *net_dev,
 			     char *attr_name, atomic_t *attr)
@@ -114,77 +169,6 @@ static int store_uint_attr(char *buff, size_t count,
 }
 
 
-static ssize_t show_aggr_ogms(struct kobject *kobj, struct attribute *attr,
-			     char *buff)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
-	int aggr_status = atomic_read(&bat_priv->aggregation_enabled);
-
-	return sprintf(buff, "%s\n",
-		       aggr_status == 0 ? "disabled" : "enabled");
-}
-
-static ssize_t store_aggr_ogms(struct kobject *kobj, struct attribute *attr,
-			      char *buff, size_t count)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct net_device *net_dev = to_net_dev(dev);
-	struct bat_priv *bat_priv = netdev_priv(net_dev);
-
-	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
-				 &bat_priv->aggregation_enabled);
-}
-
-static ssize_t show_bond(struct kobject *kobj, struct attribute *attr,
-			     char *buff)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
-	int bond_status = atomic_read(&bat_priv->bonding_enabled);
-
-	return sprintf(buff, "%s\n",
-		       bond_status == 0 ? "disabled" : "enabled");
-}
-
-static ssize_t store_bond(struct kobject *kobj, struct attribute *attr,
-			  char *buff, size_t count)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct net_device *net_dev = to_net_dev(dev);
-	struct bat_priv *bat_priv = netdev_priv(net_dev);
-
-	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
-				 &bat_priv->bonding_enabled);
-}
-
-static ssize_t show_frag(struct kobject *kobj, struct attribute *attr,
-			     char *buff)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
-	int frag_status = atomic_read(&bat_priv->frag_enabled);
-
-	return sprintf(buff, "%s\n",
-		       frag_status == 0 ? "disabled" : "enabled");
-}
-
-static ssize_t store_frag(struct kobject *kobj, struct attribute *attr,
-			  char *buff, size_t count)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct net_device *net_dev = to_net_dev(dev);
-	struct bat_priv *bat_priv = netdev_priv(net_dev);
-	int ret;
-
-	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name,
-				&bat_priv->frag_enabled);
-	if (ret)
-		update_min_mtu(net_dev);
-
-	return ret;
-}
-
 static ssize_t show_vis_mode(struct kobject *kobj, struct attribute *attr,
 			     char *buff)
 {
@@ -282,66 +266,20 @@ static ssize_t store_gw_mode(struct kobject *kobj, struct attribute *attr,
 	return gw_mode_set(net_dev, buff, count);
 }
 
-static ssize_t show_orig_interval(struct kobject *kobj, struct attribute *attr,
-				 char *buff)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
-
-	return sprintf(buff, "%i\n",
-		       atomic_read(&bat_priv->orig_interval));
-}
-
-static ssize_t store_orig_interval(struct kobject *kobj, struct attribute *attr,
-				  char *buff, size_t count)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct net_device *net_dev = to_net_dev(dev);
-	struct bat_priv *bat_priv = netdev_priv(net_dev);
-
-	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
-			       2 * JITTER, UINT_MAX, &bat_priv->orig_interval);
-}
-
-#ifdef CONFIG_BATMAN_ADV_DEBUG
-static ssize_t show_log_level(struct kobject *kobj, struct attribute *attr,
-			     char *buff)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
-	int log_level = atomic_read(&bat_priv->log_level);
-
-	return sprintf(buff, "%d\n", log_level);
-}
-
-static ssize_t store_log_level(struct kobject *kobj, struct attribute *attr,
-			      char *buff, size_t count)
-{
-	struct device *dev = to_dev(kobj->parent);
-	struct net_device *net_dev = to_net_dev(dev);
-	struct bat_priv *bat_priv = netdev_priv(net_dev);
-
-	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
-			       0, 3, &bat_priv->log_level);
-}
-#endif
-
-static BAT_ATTR(aggregated_ogms, S_IRUGO | S_IWUSR,
-		show_aggr_ogms, store_aggr_ogms);
-static BAT_ATTR(bonding, S_IRUGO | S_IWUSR, show_bond, store_bond);
-static BAT_ATTR(fragmentation, S_IRUGO | S_IWUSR, show_frag, store_frag);
+BAT_ATTR_SWITCH(aggregation, S_IRUGO | S_IWUSR, NULL);
+BAT_ATTR_SWITCH(bonding, S_IRUGO | S_IWUSR, NULL);
+BAT_ATTR_SWITCH(frag, S_IRUGO | S_IWUSR, update_min_mtu);
 static BAT_ATTR(vis_mode, S_IRUGO | S_IWUSR, show_vis_mode, store_vis_mode);
 static BAT_ATTR(gw_mode, S_IRUGO | S_IWUSR, show_gw_mode, store_gw_mode);
-static BAT_ATTR(orig_interval, S_IRUGO | S_IWUSR,
-		show_orig_interval, store_orig_interval);
+BAT_ATTR_UINT(orig_interval, S_IRUGO | S_IWUSR, 2 * JITTER, INT_MAX, NULL);
 #ifdef CONFIG_BATMAN_ADV_DEBUG
-static BAT_ATTR(log_level, S_IRUGO | S_IWUSR, show_log_level, store_log_level);
+BAT_ATTR_UINT(log_level, S_IRUGO | S_IWUSR, 0, 3, NULL);
 #endif
 
 static struct bat_attribute *mesh_attrs[] = {
-	&bat_attr_aggregated_ogms,
+	&bat_attr_aggregation,
 	&bat_attr_bonding,
-	&bat_attr_fragmentation,
+	&bat_attr_frag,
 	&bat_attr_vis_mode,
 	&bat_attr_gw_mode,
 	&bat_attr_orig_interval,
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Linus Lüssing]

Especially for this commit, I'm not quite sure, if the usage of
the macros is ok and appropritate or just a too extensive use / abuse
of them. So comments highly appreciated :).

Cheers, Linus

On Sun, Oct 10, 2010 at 06:29:58AM +0200, Linus Lüssing wrote:
> This makes exposing bat_priv's atomic_ts storing boolean or uint values
> a one-liner in the future. It also further reduces the amount of
> duplicate code in bat_sysfs.c.
> 
> Additionally, the sysfs entries "aggregate_ogms" and "fragmentation"
> get renamed to "aggregation" and "frag" for consistency reasons, they
> match the *_enabled parts in bat_priv now.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  bat_sysfs.c |  186 ++++++++++++++++++++---------------------------------------
>  1 files changed, 62 insertions(+), 124 deletions(-)
> 
> diff --git a/bat_sysfs.c b/bat_sysfs.c
> index 60e3122..878cb8e 100644
> --- a/bat_sysfs.c
> +++ b/bat_sysfs.c
> @@ -31,6 +31,7 @@
>  
>  #define to_dev(obj)     container_of(obj, struct device, kobj)
>  
> +/* Use this, if you have customized show and store functions */
>  #define BAT_ATTR(_name, _mode, _show, _store)	\
>  struct bat_attribute bat_attr_##_name = {	\
>  	.attr = {.name = __stringify(_name),	\
> @@ -39,6 +40,60 @@ struct bat_attribute bat_attr_##_name = {	\
>  	.store  = _store,			\
>  };
>  
> +/* Use this, if you are going to turn a [name]_enabled in bat_priv on or off */
> +#define BAT_ATTR_SWITCH(_name, _mode, _post_func)			\
> +static ssize_t store_##_name(struct kobject *kobj, struct attribute *attr, \
> +		      char *buff, size_t count)				\
> +{									\
> +	struct device *dev = to_dev(kobj->parent);			\
> +	struct net_device *net_dev = to_net_dev(dev);			\
> +	struct bat_priv *bat_priv = netdev_priv(net_dev);		\
> +	void (*post_func)(struct net_device *) = _post_func;		\
> +	int ret;							\
> +	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name, \
> +				 &bat_priv->_name##_enabled);		\
> +	if (post_func && ret)						\
> +		post_func(net_dev);					\
> +	return ret;							\
> +}									\
> +static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr, \
> +			    char *buff)					\
> +{									\
> +	struct device *dev = to_dev(kobj->parent);			\
> +	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));	\
> +	return sprintf(buff, "%s\n",					\
> +		       atomic_read(&bat_priv->_name##_enabled) == 0 ?	\
> +		       "disabled" : "enabled");				\
> +}									\
> +BAT_ATTR(_name, _mode, show_##_name, store_##_name)
> +
> +/* Use this, if you are going to set [name] in bat_priv to unsigned integer
> + * values only */
> +#define BAT_ATTR_UINT(_name, _mode, _min, _max, _post_func)		\
> +static ssize_t store_##_name(struct kobject *kobj, struct attribute *attr, \
> +		      char *buff, size_t count)				\
> +{									\
> +	struct device *dev = to_dev(kobj->parent);			\
> +	struct net_device *net_dev = to_net_dev(dev);			\
> +	struct bat_priv *bat_priv = netdev_priv(net_dev);		\
> +	void (*post_func)(struct net_device *) = _post_func;		\
> +	int ret;							\
> +	ret = store_uint_attr(buff, count, net_dev, (char *)attr->name,	\
> +			       _min, _max, &bat_priv->_name);		\
> +	if (post_func && ret)						\
> +		post_func(net_dev);					\
> +	return ret;							\
> +}									\
> +static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr, \
> +			    char *buff)					\
> +{									\
> +	struct device *dev = to_dev(kobj->parent);			\
> +	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));	\
> +	return sprintf(buff, "%i\n", atomic_read(&bat_priv->_name));	\
> +}									\
> +BAT_ATTR(_name, _mode, show_##_name, store_##_name)
> +
> +
>  static int store_switch_attr(char *buff, size_t count,
>  			     struct net_device *net_dev,
>  			     char *attr_name, atomic_t *attr)
> @@ -114,77 +169,6 @@ static int store_uint_attr(char *buff, size_t count,
>  }
>  
>  
> -static ssize_t show_aggr_ogms(struct kobject *kobj, struct attribute *attr,
> -			     char *buff)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
> -	int aggr_status = atomic_read(&bat_priv->aggregation_enabled);
> -
> -	return sprintf(buff, "%s\n",
> -		       aggr_status == 0 ? "disabled" : "enabled");
> -}
> -
> -static ssize_t store_aggr_ogms(struct kobject *kobj, struct attribute *attr,
> -			      char *buff, size_t count)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct net_device *net_dev = to_net_dev(dev);
> -	struct bat_priv *bat_priv = netdev_priv(net_dev);
> -
> -	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
> -				 &bat_priv->aggregation_enabled);
> -}
> -
> -static ssize_t show_bond(struct kobject *kobj, struct attribute *attr,
> -			     char *buff)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
> -	int bond_status = atomic_read(&bat_priv->bonding_enabled);
> -
> -	return sprintf(buff, "%s\n",
> -		       bond_status == 0 ? "disabled" : "enabled");
> -}
> -
> -static ssize_t store_bond(struct kobject *kobj, struct attribute *attr,
> -			  char *buff, size_t count)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct net_device *net_dev = to_net_dev(dev);
> -	struct bat_priv *bat_priv = netdev_priv(net_dev);
> -
> -	return store_switch_attr(buff, count, net_dev, (char *)attr->name,
> -				 &bat_priv->bonding_enabled);
> -}
> -
> -static ssize_t show_frag(struct kobject *kobj, struct attribute *attr,
> -			     char *buff)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
> -	int frag_status = atomic_read(&bat_priv->frag_enabled);
> -
> -	return sprintf(buff, "%s\n",
> -		       frag_status == 0 ? "disabled" : "enabled");
> -}
> -
> -static ssize_t store_frag(struct kobject *kobj, struct attribute *attr,
> -			  char *buff, size_t count)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct net_device *net_dev = to_net_dev(dev);
> -	struct bat_priv *bat_priv = netdev_priv(net_dev);
> -	int ret;
> -
> -	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name,
> -				&bat_priv->frag_enabled);
> -	if (ret)
> -		update_min_mtu(net_dev);
> -
> -	return ret;
> -}
> -
>  static ssize_t show_vis_mode(struct kobject *kobj, struct attribute *attr,
>  			     char *buff)
>  {
> @@ -282,66 +266,20 @@ static ssize_t store_gw_mode(struct kobject *kobj, struct attribute *attr,
>  	return gw_mode_set(net_dev, buff, count);
>  }
>  
> -static ssize_t show_orig_interval(struct kobject *kobj, struct attribute *attr,
> -				 char *buff)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
> -
> -	return sprintf(buff, "%i\n",
> -		       atomic_read(&bat_priv->orig_interval));
> -}
> -
> -static ssize_t store_orig_interval(struct kobject *kobj, struct attribute *attr,
> -				  char *buff, size_t count)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct net_device *net_dev = to_net_dev(dev);
> -	struct bat_priv *bat_priv = netdev_priv(net_dev);
> -
> -	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
> -			       2 * JITTER, UINT_MAX, &bat_priv->orig_interval);
> -}
> -
> -#ifdef CONFIG_BATMAN_ADV_DEBUG
> -static ssize_t show_log_level(struct kobject *kobj, struct attribute *attr,
> -			     char *buff)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
> -	int log_level = atomic_read(&bat_priv->log_level);
> -
> -	return sprintf(buff, "%d\n", log_level);
> -}
> -
> -static ssize_t store_log_level(struct kobject *kobj, struct attribute *attr,
> -			      char *buff, size_t count)
> -{
> -	struct device *dev = to_dev(kobj->parent);
> -	struct net_device *net_dev = to_net_dev(dev);
> -	struct bat_priv *bat_priv = netdev_priv(net_dev);
> -
> -	return store_uint_attr(buff, count, net_dev, (char *)attr->name,
> -			       0, 3, &bat_priv->log_level);
> -}
> -#endif
> -
> -static BAT_ATTR(aggregated_ogms, S_IRUGO | S_IWUSR,
> -		show_aggr_ogms, store_aggr_ogms);
> -static BAT_ATTR(bonding, S_IRUGO | S_IWUSR, show_bond, store_bond);
> -static BAT_ATTR(fragmentation, S_IRUGO | S_IWUSR, show_frag, store_frag);
> +BAT_ATTR_SWITCH(aggregation, S_IRUGO | S_IWUSR, NULL);
> +BAT_ATTR_SWITCH(bonding, S_IRUGO | S_IWUSR, NULL);
> +BAT_ATTR_SWITCH(frag, S_IRUGO | S_IWUSR, update_min_mtu);
>  static BAT_ATTR(vis_mode, S_IRUGO | S_IWUSR, show_vis_mode, store_vis_mode);
>  static BAT_ATTR(gw_mode, S_IRUGO | S_IWUSR, show_gw_mode, store_gw_mode);
> -static BAT_ATTR(orig_interval, S_IRUGO | S_IWUSR,
> -		show_orig_interval, store_orig_interval);
> +BAT_ATTR_UINT(orig_interval, S_IRUGO | S_IWUSR, 2 * JITTER, INT_MAX, NULL);
>  #ifdef CONFIG_BATMAN_ADV_DEBUG
> -static BAT_ATTR(log_level, S_IRUGO | S_IWUSR, show_log_level, store_log_level);
> +BAT_ATTR_UINT(log_level, S_IRUGO | S_IWUSR, 0, 3, NULL);
>  #endif
>  
>  static struct bat_attribute *mesh_attrs[] = {
> -	&bat_attr_aggregated_ogms,
> +	&bat_attr_aggregation,
>  	&bat_attr_bonding,
> -	&bat_attr_fragmentation,
> +	&bat_attr_frag,
>  	&bat_attr_vis_mode,
>  	&bat_attr_gw_mode,
>  	&bat_attr_orig_interval,
> -- 
> 1.7.1
> 

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Andrew Lunn]

On Sun, Oct 10, 2010 at 06:39:31AM +0200, Linus L??ssing wrote:
> Especially for this commit, I'm not quite sure, if the usage of
> the macros is ok and appropritate or just a too extensive use / abuse
> of them. So comments highly appreciated :).

Just my personal preference. I suggest others comment as well.
 
> > +/* Use this, if you are going to turn a [name]_enabled in bat_priv on or off */
> > +#define BAT_ATTR_SWITCH(_name, _mode, _post_func)			\

I would probably break this up into 3 macros. 
(And problem rename to BOOL instead of SWITCH.)
BAT_ATTR_STORE_SWITCH()
BAT_ATTR_SHOW_SWITCH()
BAT_ATTR_SWITCH()

where BAT_ATTR_SWITCH() just invokes the other two. 

> > +static ssize_t store_##_name(struct kobject *kobj, struct attribute *attr, \
> > +		      char *buff, size_t count)				\
> > +{									\
> > +	struct device *dev = to_dev(kobj->parent);			\
> > +	struct net_device *net_dev = to_net_dev(dev);			\
> > +	struct bat_priv *bat_priv = netdev_priv(net_dev);		\
> > +	void (*post_func)(struct net_device *) = _post_func;		\
> > +	int ret;							\
> > +	ret = store_switch_attr(buff, count, net_dev, (char *)attr->name, \
> > +				 &bat_priv->_name##_enabled);		\
> > +	if (post_func && ret)						\
> > +		post_func(net_dev);					\
> > +	return ret;							\
> > +}									\

Maybe turn the above into an inline function and then have the macro
define a tiny wrapper function which just calls the inline function?

> > +static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr, \
> > +			    char *buff)					\
> > +{									\
> > +	struct device *dev = to_dev(kobj->parent);			\
> > +	struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));	\
> > +	return sprintf(buff, "%s\n",					\
> > +		       atomic_read(&bat_priv->_name##_enabled) == 0 ?	\
> > +		       "disabled" : "enabled");				\
> > +}

This is small enough that it is O.K. as a macro.

Then the same again for BAT_ATTR_UINT...

     Andrew

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 2187 bytes --]

Linus Lüssing wrote:
> +/* Use this, if you are going to turn a [name]_enabled in bat_priv on or
> off */ +#define BAT_ATTR_SWITCH(_name, _mode,
> _post_func)                      \ +static ssize_t store_##_name(struct
> kobject *kobj, struct attribute *attr, \ +                     char *buff,
> size_t count)                         \
> +{                                                                      \
> +       struct device *dev = to_dev(kobj->parent);                      \
> +       struct net_device *net_dev = to_net_dev(dev);                   \
> +       struct bat_priv *bat_priv = netdev_priv(net_dev);               \
> +       void (*post_func)(struct net_device *) = _post_func;            \
> +       int ret;                                                        \
> +       ret = store_switch_attr(buff, count, net_dev, (char *)attr->name,
> \ +                               
> &bat_priv->_name##_enabled);           \ +       if (post_func &&
> ret)                                           \
> +               post_func(net_dev);                                     \
> +       return ret;                                                     \
> +}                                                                      \
> +static ssize_t show_##_name(struct kobject *kobj, struct attribute *attr,
> \ +                           char
> *buff)                                 \
> +{                                                                      \
> +       struct device *dev = to_dev(kobj->parent);                      \
> +       struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));       \
> +       return sprintf(buff, "%s\n",                                    \
> +                      atomic_read(&bat_priv->_name##_enabled) == 0 ?   \
> +                      "disabled" : "enabled");                         \
> +}                                                                      \
> +BAT_ATTR(_name, _mode, show_##_name, store_##_name)

Please change the last line (and the similar line) to "static BAT_ATTR(...."

And yes, I like the idea to remove some redundancy.

Best regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Marek Lindner]

On Sunday 10 October 2010 06:29:58 Linus Lüssing wrote:
> Additionally, the sysfs entries "aggregate_ogms" and "fragmentation"
> get renamed to "aggregation" and "frag" for consistency reasons, they
> match the *_enabled parts in bat_priv now.

I don't quite understand why you want to rename these names. I like the 
"aggregate_ogms" as it makes clear what is being aggregated. 

Please keep in mind that the kernel people won't be happy if we modify the 
user space API. Right now, it is less of an issue but once we leave the 
staging tree it is mostly unchangeable.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Linus Lüssing]

Hi Marek,

actually, there were two reasons for renaming that I guess. One
was out of "laziness" and I wanted to keep the number of
parameters of a BAT_ATTR_* macro as less as possible. Otherwise
I'd have to have another field, one for stating the name in sysfs
and one for the name in bat_priv.

I then was wondering why the names between sysfs and bat_priv
shouldn't be the same instead (or +_enabled for switch/bool
attributes). I could have renamed the part in bat_priv to
aggregate_ogms_enabled but found that a little long (the same for
fragmentation_enabled). Or the _enabled parts could be removed in
bat_priv, but making it less obvious, what kind of values they
might take.

So I thought that just aggregation might be a good trade-off. And
that anyone who might be confused by the names, not knowing what
they might be used for, should use batctl instead (and its manpage
for explanations).

Or would anybody prefer another solution (of the ones described
above)?

Cheers, Linus

On Sun, Oct 10, 2010 at 02:42:08PM +0200, Marek Lindner wrote:
> On Sunday 10 October 2010 06:29:58 Linus Lüssing wrote:
> > Additionally, the sysfs entries "aggregate_ogms" and "fragmentation"
> > get renamed to "aggregation" and "frag" for consistency reasons, they
> > match the *_enabled parts in bat_priv now.
> 
> I don't quite understand why you want to rename these names. I like the 
> "aggregate_ogms" as it makes clear what is being aggregated. 
> 
> Please keep in mind that the kernel people won't be happy if we modify the 
> user space API. Right now, it is less of an issue but once we leave the 
> staging tree it is mostly unchangeable.
> 
> Regards,
> Marek
> 

Re: [B.A.T.M.A.N.] [PATCH 2/5] batman-adv: Introduce generic BAT_ATTR_* macros

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 1389 bytes --]

Linus Lüssing wrote:
> Hi Marek,
> 
> actually, there were two reasons for renaming that I guess. One
> was out of "laziness" and I wanted to keep the number of
> parameters of a BAT_ATTR_* macro as less as possible. Otherwise
> I'd have to have another field, one for stating the name in sysfs
> and one for the name in bat_priv.
> 
> I then was wondering why the names between sysfs and bat_priv
> shouldn't be the same instead (or +_enabled for switch/bool
> attributes). I could have renamed the part in bat_priv to
> aggregate_ogms_enabled but found that a little long (the same for
> fragmentation_enabled). Or the _enabled parts could be removed in
> bat_priv, but making it less obvious, what kind of values they
> might take.
> 
> So I thought that just aggregation might be a good trade-off. And
> that anyone who might be confused by the names, not knowing what
> they might be used for, should use batctl instead (and its manpage
> for explanations).
> 
> Or would anybody prefer another solution (of the ones described
> above)?

Please don't change the sysfs names.

And document all your sysfs files for sysfs-class-net-mesh and
sysfs-class-net-batman-adv in linux-merge.git

Still missing stuff are:
 * fragmentation (@Andreas - your part)
 * gw_mode (can be documented later)
 * hop_penalty
 * num_bcasts
 * log_level

Thanks,
	Sven

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

[B.A.T.M.A.N.] [PATCH 3/5] batman-adv: Make hop_penalty configurable via sysfs

[Linus Lüssing]

When having a mixed topology of both very mobile and rather static
nodes, you are usually best advised to set the originator interval on
all nodes to a level best suited for the most mobile node.

However, if most of the nodes are rather static, this can create a lot
of undesired overhead as a trade-off then. If setting the interval too
low on the static nodes, a mobile node might be chosen as a router for
too long, not switching away from it fast enough because of its
mobility and the low frequency of ogms of static nodes.

Exposing the hop_penalty is especially useful for the stated scenario: A
static node can keep the default originator interval, a mobile node can
select a quicker one resulting in faster route updates towards this
mobile node. Additionally, such a mobile node could select a higher hop
penalty (or even set it to 255 to disable acting as a router for other
nodes) to make it less desirable, letting other nodes avoid selecting
this mobile node as a router.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bat_sysfs.c      |    2 ++
 send.c           |    7 ++++---
 soft-interface.c |    1 +
 types.h          |    1 +
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/bat_sysfs.c b/bat_sysfs.c
index 878cb8e..9fda75e 100644
--- a/bat_sysfs.c
+++ b/bat_sysfs.c
@@ -272,6 +272,7 @@ BAT_ATTR_SWITCH(frag, S_IRUGO | S_IWUSR, update_min_mtu);
 static BAT_ATTR(vis_mode, S_IRUGO | S_IWUSR, show_vis_mode, store_vis_mode);
 static BAT_ATTR(gw_mode, S_IRUGO | S_IWUSR, show_gw_mode, store_gw_mode);
 BAT_ATTR_UINT(orig_interval, S_IRUGO | S_IWUSR, 2 * JITTER, INT_MAX, NULL);
+BAT_ATTR_UINT(hop_penalty, S_IRUGO | S_IWUSR, 0, TQ_MAX_VALUE, NULL);
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 BAT_ATTR_UINT(log_level, S_IRUGO | S_IWUSR, 0, 3, NULL);
 #endif
@@ -283,6 +284,7 @@ static struct bat_attribute *mesh_attrs[] = {
 	&bat_attr_vis_mode,
 	&bat_attr_gw_mode,
 	&bat_attr_orig_interval,
+	&bat_attr_hop_penalty,
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 	&bat_attr_log_level,
 #endif
diff --git a/send.c b/send.c
index 180e18b..943437b 100644
--- a/send.c
+++ b/send.c
@@ -35,9 +35,10 @@
 static void send_outstanding_bcast_packet(struct work_struct *work);
 
 /* apply hop penalty for a normal link */
-static uint8_t hop_penalty(const uint8_t tq)
+static uint8_t hop_penalty(const uint8_t tq, struct bat_priv *bat_priv)
 {
-	return (tq * (TQ_MAX_VALUE - TQ_HOP_PENALTY)) / (TQ_MAX_VALUE);
+	int hop_penalty = atomic_read(&bat_priv->hop_penalty);
+	return (tq * (TQ_MAX_VALUE - hop_penalty)) / (TQ_MAX_VALUE);
 }
 
 /* when do we schedule our own packet to be sent */
@@ -339,7 +340,7 @@ void schedule_forward_packet(struct orig_node *orig_node,
 	}
 
 	/* apply hop penalty */
-	batman_packet->tq = hop_penalty(batman_packet->tq);
+	batman_packet->tq = hop_penalty(batman_packet->tq, bat_priv);
 
 	bat_dbg(DBG_BATMAN, bat_priv,
 		"Forwarding packet: tq_orig: %i, tq_avg: %i, "
diff --git a/soft-interface.c b/soft-interface.c
index 22fe548..d42cd02 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -592,6 +592,7 @@ struct net_device *softif_create(char *name)
 	atomic_set(&bat_priv->gw_mode, GW_MODE_OFF);
 	atomic_set(&bat_priv->gw_class, 0);
 	atomic_set(&bat_priv->orig_interval, 1000);
+	atomic_set(&bat_priv->hop_penalty, 30);
 	atomic_set(&bat_priv->log_level, 0);
 	atomic_set(&bat_priv->frag_enabled, 1);
 	atomic_set(&bat_priv->bcast_queue_left, BCAST_QUEUE_LEN);
diff --git a/types.h b/types.h
index dec2791..75e0b71 100644
--- a/types.h
+++ b/types.h
@@ -131,6 +131,7 @@ struct bat_priv {
 	atomic_t gw_mode;
 	atomic_t gw_class;
 	atomic_t orig_interval;
+	atomic_t hop_penalty;
 	atomic_t log_level;
 	atomic_t bcast_seqno;
 	atomic_t bcast_queue_left;
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH 3/5] batman-adv: Make hop_penalty configurable via sysfs

[Marek Lindner]

On Sunday 10 October 2010 06:29:59 Linus Lüssing wrote:
> When having a mixed topology of both very mobile and rather static
> nodes, you are usually best advised to set the originator interval on
> all nodes to a level best suited for the most mobile node.
> 
> However, if most of the nodes are rather static, this can create a lot
> of undesired overhead as a trade-off then. If setting the interval too
> low on the static nodes, a mobile node might be chosen as a router for
> too long, not switching away from it fast enough because of its
> mobility and the low frequency of ogms of static nodes.

Any good reason for all these "originator interval" explanations here ?


> @@ -592,6 +592,7 @@ struct net_device *softif_create(char *name)
>  	atomic_set(&bat_priv->gw_mode, GW_MODE_OFF);
>  	atomic_set(&bat_priv->gw_class, 0);
>  	atomic_set(&bat_priv->orig_interval, 1000);
> +	atomic_set(&bat_priv->hop_penalty, 30);

Either you initialize the value with TQ_HOP_PENALTY or you remove the define 
from the main.h file.

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 3/5] batman-adv: Make hop_penalty configurable via sysfs

[Linus Lüssing]

On Sun, Oct 10, 2010 at 02:49:51PM +0200, Marek Lindner wrote:
> Any good reason for all these "originator interval" explanations here ?
> 
> 
> > @@ -592,6 +592,7 @@ struct net_device *softif_create(char *name)
> >  	atomic_set(&bat_priv->gw_mode, GW_MODE_OFF);
> >  	atomic_set(&bat_priv->gw_class, 0);
> >  	atomic_set(&bat_priv->orig_interval, 1000);
> > +	atomic_set(&bat_priv->hop_penalty, 30);
> 
> Either you initialize the value with TQ_HOP_PENALTY or you remove the define 
> from the main.h file.
Argh, ehm, thanks... thought I had removed the define and set the
hop_penalty to 10... but...

eh, nevermind, will redo that :)

Cheers, Linus
> 
> Regards,
> Marek
> 

[B.A.T.M.A.N.] [PATCH 4/5] batman-adv: Make number of (re)broadcasts configurable via sysfs

[Linus Lüssing]

Depending on the scenario, people might want to adjust the number of
(re)broadcast of data packets - usually higher values in sparse or lower
values in dense networks.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bat_sysfs.c |    2 ++
 send.c      |    3 ++-
 types.h     |    1 +
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/bat_sysfs.c b/bat_sysfs.c
index 9fda75e..2f7f118 100644
--- a/bat_sysfs.c
+++ b/bat_sysfs.c
@@ -273,6 +273,7 @@ static BAT_ATTR(vis_mode, S_IRUGO | S_IWUSR, show_vis_mode, store_vis_mode);
 static BAT_ATTR(gw_mode, S_IRUGO | S_IWUSR, show_gw_mode, store_gw_mode);
 BAT_ATTR_UINT(orig_interval, S_IRUGO | S_IWUSR, 2 * JITTER, INT_MAX, NULL);
 BAT_ATTR_UINT(hop_penalty, S_IRUGO | S_IWUSR, 0, TQ_MAX_VALUE, NULL);
+BAT_ATTR_UINT(num_bcasts, S_IRUGO | S_IWUSR, 0, INT_MAX, NULL);
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 BAT_ATTR_UINT(log_level, S_IRUGO | S_IWUSR, 0, 3, NULL);
 #endif
@@ -285,6 +286,7 @@ static struct bat_attribute *mesh_attrs[] = {
 	&bat_attr_gw_mode,
 	&bat_attr_orig_interval,
 	&bat_attr_hop_penalty,
+	&bat_attr_num_bcasts,
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 	&bat_attr_log_level,
 #endif
diff --git a/send.c b/send.c
index 943437b..2f18a0d 100644
--- a/send.c
+++ b/send.c
@@ -455,6 +455,7 @@ static void send_outstanding_bcast_packet(struct work_struct *work)
 	struct sk_buff *skb1;
 	struct net_device *soft_iface = forw_packet->if_incoming->soft_iface;
 	struct bat_priv *bat_priv = netdev_priv(soft_iface);
+	int num_bcasts = atomic_read(&bat_priv->num_bcasts);
 
 	spin_lock_irqsave(&bat_priv->forw_bcast_list_lock, flags);
 	hlist_del(&forw_packet->list);
@@ -479,7 +480,7 @@ static void send_outstanding_bcast_packet(struct work_struct *work)
 	forw_packet->num_packets++;
 
 	/* if we still have some more bcasts to send */
-	if (forw_packet->num_packets < 3) {
+	if (forw_packet->num_packets < num_bcasts) {
 		_add_bcast_packet_to_list(bat_priv, forw_packet,
 					  ((5 * HZ) / 1000));
 		return;
diff --git a/types.h b/types.h
index 75e0b71..0063ad7 100644
--- a/types.h
+++ b/types.h
@@ -132,6 +132,7 @@ struct bat_priv {
 	atomic_t gw_class;
 	atomic_t orig_interval;
 	atomic_t hop_penalty;
+	atomic_t num_bcasts;
 	atomic_t log_level;
 	atomic_t bcast_seqno;
 	atomic_t bcast_queue_left;
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH 4/5] batman-adv: Make number of (re)broadcasts configurable via sysfs

[Marek Lindner]

On Sunday 10 October 2010 06:30:00 Linus Lüssing wrote:
> Depending on the scenario, people might want to adjust the number of
> (re)broadcast of data packets - usually higher values in sparse or lower
> values in dense networks.

Is there a good reason to change this value at runtime or would a define be 
enough ? I just get scared when we add too many options that almost nobody 
cares about. 

In other words: What is the use case ? Can we handle it by some kind of auto-
detection ?

Regards,
Marek

Re: [B.A.T.M.A.N.] [PATCH 4/5] batman-adv: Make number of (re)broadcasts configurable via sysfs

[Linus Lüssing]

On Sun, Oct 10, 2010 at 02:53:51PM +0200, Marek Lindner wrote:
> On Sunday 10 October 2010 06:30:00 Linus Lüssing wrote:
> > Depending on the scenario, people might want to adjust the number of
> > (re)broadcast of data packets - usually higher values in sparse or lower
> > values in dense networks.
> 
> Is there a good reason to change this value at runtime or would a define be 
> enough ? I just get scared when we add too many options that almost nobody 
> cares about.
Well, as said before, in some cases the default value of 3 might
be too much, in other cases too less, depending on the topology.
However, you might know the kind of topology you have in advance
and would like to tweak that without having to know how to for
example rebuild a complete image and reflashing it on all
devices. You might want to test different values for your topology
for tweaking, without having to reflash / reload the kernel module
on all devices.

I also don't think too many configuration options in sysfs are
that much of a problem. There are also a dozen options for
tweaking IP in /proc/sys/net which nearly no one uses. But if
someone might need it, (s)he'll be very pleased to not having to
recompile and restart the whole kernel for that (or a kernel module
in our case).

The average user who might be scared of too many configuration
options will be using batctl anyway, won't (s)he? So I'd rather
plead to add tweaking options to the sysfs, but omitting them in
batctl. Or what do you think?
> 
> In other words: What is the use case ? Can we handle it by some kind of auto-
> detection ?
Sure we could, but then we'd need some more information about the
neighbourhood, i.e. with something like a seperate neighbor
discovery protocol giving 2-hop-neighbourhood information.
Otherwise a node would probably usually send the maximum amount of
rebroadcasts as there might be a node in the far distance with a
very low tq-value. But that'd require much more effort to
implement then these couple of lines :).

Thanks for reviewing

Cheers, Linus
> 
> Regards,
> Marek
> 

[B.A.T.M.A.N.] [PATCH 5/5] batman-adv: Fix resizing of broadcast seqno buffers on if deletion

[Linus Lüssing]

Not only the entries of the deleted interface got erased, but also all
ones with a lower if_num. This commit fixes this issue by setting the
destination appropriately.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 originator.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/originator.c b/originator.c
index a7b74f0..6868a2a 100644
--- a/originator.c
+++ b/originator.c
@@ -465,7 +465,7 @@ static int orig_node_del_if(struct orig_node *orig_node,
 	memcpy(data_ptr, orig_node->bcast_own, del_if_num * chunk_size);
 
 	/* copy second part */
-	memcpy(data_ptr,
+	memcpy(data_ptr + del_if_num * chunk_size,
 	       orig_node->bcast_own + ((del_if_num + 1) * chunk_size),
 	       (max_if_num - del_if_num) * chunk_size);
 
@@ -485,7 +485,7 @@ free_bcast_own:
 	memcpy(data_ptr, orig_node->bcast_own_sum,
 	       del_if_num * sizeof(uint8_t));
 
-	memcpy(data_ptr,
+	memcpy(data_ptr + del_if_num * sizeof(uint8_t),
 	       orig_node->bcast_own_sum + ((del_if_num + 1) * sizeof(uint8_t)),
 	       (max_if_num - del_if_num) * sizeof(uint8_t));
 
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH 5/5] batman-adv: Fix resizing of broadcast seqno buffers on if deletion

[Marek Lindner]

On Sunday 10 October 2010 06:30:01 Linus Lüssing wrote:
> Not only the entries of the deleted interface got erased, but also all
> ones with a lower if_num. This commit fixes this issue by setting the
> destination appropriately.

Applied in revision 1824.

Thanks,
Marek

Re: [B.A.T.M.A.N.] [PATCH 1/5] batman-adv: Wrapper functions for sysfs storing

[Andrew Lunn]

On Sun, Oct 10, 2010 at 06:29:57AM +0200, Linus L??ssing wrote:
> Sysfs configuration options that just took a boolean value
> (enable(d)/disable(d)/0/1) and integer setting basically all had the same
> structure.
> 
> To avoid even more copy and pasting in the future and to make introducing
> new configuration parameters for batman-adv simpler, more generic
> wrapper functions are being introduced with this commit. They can deal with
> boolean and unsigned integer parameters, storing them in the specified
> atomic_t variables.
> 
> Signed-off-by: Linus L??ssing <linus.luessing@web.de>
> ---
>  bat_sysfs.c |  224 +++++++++++++++++++++++-----------------------------------
>  1 files changed, 89 insertions(+), 135 deletions(-)
> 
> diff --git a/bat_sysfs.c b/bat_sysfs.c
> index 3f551f3..60e3122 100644
> --- a/bat_sysfs.c
> +++ b/bat_sysfs.c
> @@ -39,6 +39,81 @@ struct bat_attribute bat_attr_##_name = {	\
>  	.store  = _store,			\
>  };
>  
> +static int store_switch_attr(char *buff, size_t count,
> +			     struct net_device *net_dev,
> +			     char *attr_name, atomic_t *attr)
> +{

Hi Linus

Maybe the name store_bool_attr() or store_enabled_attr() would be
clearer what it does?

	Andrew