zps/greenmesh — comment on commit 7bf7434c6dd053e39b5da25c2109c3ca7d329f25
File:  line 0
By mporsch:

Good work! But some things should be changed.

1)
+#include "time_sync.h"
Where is the included file? Do you really need this?

As a reminder:
If we add a new file, we must add it to the Makefile under mac80211-$(CONFIG_MAC80211_MESH).

2)
See ieee80211_mesh_sync_ops_get(u8 method):
+ ops = &time_sync_methods.ops;
You forgot the array index again here.

3)
+static DEFINE_MUTEX(time_sync_mutex);
as long as time_sync_methods is generated at compilation - and not changed at runtime - we won't need a mutex at all. Or do we?

4)
See ieee80211_mesh_sync_ops_get:
+ struct mesh_sync_ops *ops = NULL;
[...]
+ return ops;

In error cases this may provide a NULL pointer, which is ok.

See ieee80211_mesh_rx_bcn_presp:
+ ifmsh->sync_ops->rx_bcn_presp(sdata, stype, mgmt, len, rx_status);

Please do a safety-check of the pointer before the call. Otherwise this will cause a kernel panic in error cases.

You may think about the case, that a NULL pointer for a specific method call is a normal case. For example for *add_vendor_ie which neighbor offset sync won't do (because it has no additional IE). Or we just add an empty function.

5)
See struct ieee80211_mesh_sync_ops:
+ void (*tbtt)(struct sk_buff *skb, struct ieee80211_sub_if_data *sdata);

The parameter list seems odd to me. Where would you call this from? Also I think *adjust_tbtt is a more fitting name.

6)
+ * Marco Porsch
could you change the email address to marco.porsch@s2005.tu-chemnitz.de? If I eventually leave my university, this alias will remain valid.

7)
+ const u8 methods_count = sizeof(time_sync_methods)/sizeof(time_sync_method);
little coding style thing with spaces before and after /