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