Re: [RFC v2 1/2] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy

[Stephen Hemminger <shemminger@xxxxxxxxxx>]

On Wed, 4 Jul 2012 00:12:13 +0000
Arvid Brodin <Arvid.Brodin@xxxxxxxx> wrote:

> diff --git a/Documentation/networking/hsr/hsr_genl.c b/Documentation/networking/hsr/hsr_genl.c

1. I like documentation, I like examples, but examples in the Documentation
directory get stale and end up not working.

2. I am not a fan of the non-standard unaligned access optimization.
Stinks too much like the old BSD IFF_TRAILERS thing that still persists
to this day.

3. The code seems to go to a lot of locking effort to get atomic state
   update, if it used the bits in dev_state (ie netif_carrier etc) instead of
   operstate and if_flags I think it would be simpler and safer.  I.e

+static int is_admin_up(struct net_device *dev)
+{
+	return (dev->flags & IFF_UP);
+}
    is redundant with netif_running()

4. Don't use mixed case as in:
     +	HSR_Ver = 0;

5. The header create code has to handle error cases where:
     1. skb must be copied to add header
     2. no space left for header in skb

6. Don't comment out code and then submit it. I don't like reading and
   reviewing leftover debug stuff.

7. Any operations type structure should be declared 'const'. This is safer
   from a security point of view and keeps dirty and immutable variables in separate
   cache areas.

8. If possible use standard netdev macros for printing info messages:
     see netdev_info() etc.

9. Be careful about variable names: "seqlock" implies you are using 
    seq_lock() but you aren't doing that. It is just a spinlock.

10. above() seems like it could be done by signed math in one operation
    without conditional ?: operation.
   See timer_before/timer_after for example.

11. All global variables need to have one prefix.  You are using both hsr_
     and framreg_. Can you just use hsr_?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html