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
