Re: [RFC PATCH net-next 0/6] net: add GeoNetworking protocol

[Simon Dietz <simon.dietz@plantwatch.de>]

> I'm probably doing a deep dive too early, but ...
Thanks for your time and the fast response.

> Nothing new has been added to /proc for a long time. Please consider
> a different interface. I've not yet looked to see what is there, but
> networking now pretty much only uses netlink.

/proc has been used during the development of the gn module for passing
gps data from the user space to the kernel. This behaviour has been
changed to ioctl. Procfs has only been kept for debugging reasons. It
can safely be removed and will be in the next patch version.

> New IOCTL code is also very likely to be rejected. The functionality
> should go through netlink.

I'll take a look into netlink and rewrite the new IOCTL code.

> Generally, inline functions in a .c file are rejected. It is better
> to let the compiler decide. The exception would be if you have a
> benchmark which shows inline actually helps.

Noted. No inline functions (besides proven by benchmarks).

> This seems like debug. At minimum, it should be _dbg(), but maybe it
> should be removed altogether.

I thought to have changed all pr_info to pr_debug, this one slipped
through.

> When does this wrap around? Maybe add it as a comment.

Noted.

> Commented out code is not something we want in the kernel.

Indeed all BUG calls should have been removed with one of the
subsequent patches together with most of the commented out code. I'll
take another look to ensure all comment out code is removed.

> Maybe one of your later patches fixes this. We might want to consider
> squashing them, so the review is done on the final clean code.

That's good advice. I'll adhere to it.

> netdev uses reverse christmas tree, longest lines first, shortest
> last. It should apply to all functions.

Fixed in patch 6/6. And partially included in squashing subsequent
patches so they fully adhere to the coding style from the start.

> Not the sort of thing you normally see in the kernel.
> I've not looked at the code enough to see the big picture, but
> generally, the kernel routing table is static, and fed from a user
> space daemon. Should all this code be in user space?

In GeoNetworking routing decisions can be and are in non broadcast
situations based on the distance to the other vehicle/host. To be more
precise routing is based on if the reciever is in the same area as the
sender. If the sender needs to forward a packet to a reciever outside
its own range, it is forward to the intermediary closest to the
reciever.

That's kind of the core idea/feature of GeoNetworking. I don't see the
possibility to move this part into the userspace. But this does not
mean that there is no option at all.

>> static void debug_loc_te(void) ...
> debugfs? a netlink dump operation?

I'd like to keep this debug function for now. I agree that it should be
removed/changed in the final patch before an actual merge to net-next.

Simon