Re: [B.A.T.M.A.N.] [PATCHv2 05/10] batman-adv: Creating neighbor structures, updating LQs

[Marek Lindner <lindner_marek@yahoo.de>]

On Wednesday 15 December 2010 18:09:29 Linus Lüssing wrote:
> It will now be checked if a neighbor discovery packet from a new
> neighbor on a certain interface has been received. If so,
> structures in memory will be allocated for further seqno-tracking
> and RQ calculations, and TQ reception, which will be updated
> frequently from this commit on.

Checkpatch found 8 errors - please make all your patches "checkpatch clean".
I also noticed that the new ndp files ndp.c / ndp.h lack a proper licence 
header.


> @@ -1,6 +1,8 @@
>  #include "main.h"
>  #include "send.h"
>  #include "ndp.h"
> +#include "soft-interface.h"

This include is not necessary as far as I can tell.


> +int ndp_update_neighbor(uint8_t my_tq, uint32_t seqno,
> +			struct batman_if *batman_if, uint8_t *neigh_addr)
> +{
> +	struct bat_priv *bat_priv = netdev_priv(batman_if->soft_iface);
> +	struct neigh_node *neigh_node = NULL, *tmp_neigh_node = NULL;
> +	int ret = 1;
> +
> +	spin_lock_bh(&batman_if->neigh_list_lock);
> +	// old neighbor?
> +	list_for_each_entry(tmp_neigh_node, &batman_if->neigh_list, list) {
> +		if (!compare_orig(tmp_neigh_node->addr, neigh_addr))
> +			continue;
> +
> +		neigh_node = tmp_neigh_node;

A "break" could be added here.


> +	}
> +
> +	// new neighbor?
> +	if (!neigh_node) {
> +		neigh_node = ndp_create_neighbor(my_tq, seqno, neigh_addr,
> +						 bat_priv);
> +		if (!neigh_node)
> +			goto ret;
> +
> +		list_add_tail(&neigh_node->list, &batman_if->neigh_list);
> +	}
> +
> +	ndp_update_neighbor_lq(my_tq, seqno, neigh_node, bat_priv);
> +
> +	ret = 0;
> +
> +ret:
> +	spin_unlock_bh(&batman_if->neigh_list_lock);
> +	return ret;
> +}

Instead of holding a spinlock all the time to protect a single neigh_node 
pointer you should protect the NDP neighbor list with RCU locks and the 
pointers with refcounting.


> +	ret = ndp_update_neighbor(my_tq, ntohl(packet->seqno),
> +					batman_if, ethhdr->h_source);
> +	if (ret)
> +		return NET_RX_DROP;
> +
> +	kfree_skb(skb);
> +	return NET_RX_SUCCESS;

Why not simply returning NET_RX_DROP ?


> +	struct list_head neigh_list;
> +	spinlock_t neigh_list_lock;

We already have a neigh_list and a neigh_list_lock. Either the old one gets 
removed or we should pick another name to avoid confusion.


> +	TYPE_OF_WORD rq_real_bits[NUM_WORDS];

rq_real_bits is not a very good name. I know, the current OGM code uses the 
same bad name but I believe we can safely break with the tradition here. How 
about something more descriptive like "rq_ndp_window"  ?

Cheers,
Marek