The networking gurus can comment on the internals of your patch better than
I can. Just a few style notes though:
> +#ifdef CONFIG_TCP_OFFLOAD
> +#define NETIF_F_TCPIP_OFFLOAD 65536 /* Can offload TCP/IP */
> +#endif
No need to protect this inside CONFIG_* option
> +/* TOE API */
> +#ifdef CONFIG_TCP_OFFLOAD
> +#define TCPDIAG_OFFLOAD 5
> +#endif
Ditto
> +#ifdef CONFIG_TCP_OFFLOAD
> +struct toe_funcs;
> +#endif
Ditto
> +#ifdef CONFIG_TCP_OFFLOAD
> +#include <linux/toedev.h>
> +#endif
Include linux/toedev.h unconditionally. Have it handle the !CONFIG_TCP_OFFLOAD
case itself by declaring noop macros for things like toe_neigh_update().
This way you can remove a lot of the #ifdef's you've sprinkled all over the
.c files
> +#define boot_phase 0
Some explaination here? It looks like something left over from development.
> +#ifndef __raise_softirq_irqoff
> +#define __raise_softirq_irqoff(nr) __cpu_raise_softirq(smp_processor_id(),
> nr)
> +#endif
What is this needed for?
> +static int toedev_init(void);
This forward declaration seems to be only needed for the "boot_phase" thing
above, so if that goes this can go as well.
> +/*
> + * Allocate a unique index for a TOE device. We keep the index within 30
> bits
Maybe look at lib/idr.c to handle this?
> + struct toedev *dev = kmalloc(sizeof(struct toedev), GFP_KERNEL);
> +
> + if (dev) {
> + memset(dev, 0, sizeof(struct toedev));
Minor nitpick (that some might disagree with)... I usually prefer:
struct toedev *dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> +int toe_receive_skb(struct toedev *dev, struct sk_buff **skb, int n)
> +{
> + int i;
"n" and "i" should probably be "unsigned int"
> +#ifdef CONFIG_TCP_OFFLOAD
> + tcp_listen_offload(sk);
> +#endif
Another example of something that could be an empty macro in a .h file for
the !CONFIG_TCP_OFFLOAD case.
> +#ifndef CONFIG_TCP_OFFLOAD
> +static
> +#endif
Don't do this... just make it non-static unconditionally. It's not worth
the ugliness. Same applies to other places.
> +#ifndef CONFIG_TCP_OFFLOAD
> +static
> +#endif
> +__inline__ void __tcp_inherit_port(struct sock *sk, struct sock *child)
> {
> struct tcp_bind_hashbucket *head =
> &tcp_bhash[tcp_bhashfn(inet_sk(child)->num)];
> @@ -351,7 +357,10 @@
> }
> }
Things that are inline and are now going to be shared really need to just
remain "static inline" and move to a header file probably
> +#ifdef CONFIG_TCP_OFFLOAD
> + if (tcp_connect_offload(sk))
> + return 0;
> +#endif
Just another example of the kind of #ifdef that doesn't belong in the .c
files. If the !CONFIG_TCP_OFFLOAD case just had
#define tcp_connect_offload(sk) (0)
then you can skip the #ifdef
> +#ifndef CONFIG_TCP_OFFLOAD
> LIMIT_NETDEBUG(printk(KERN_DEBUG "TCP: drop open "
> "request from %u.%u."
> "%u.%u/%u\n",
> NIPQUAD(saddr),
> ntohs(skb->h.th->source)));
> +#else
> + NETDEBUG(if (net_ratelimit()) \
> + printk(KERN_DEBUG "TCP: drop open "
> + "request from %u.%u."
> + "%u.%u/%u\n", \
> + NIPQUAD(saddr),
> + ntohs(skb->h.th->source)));
> +#endif
Huh? What about TOE requires changes to printk ratelimiting?
-Mitch
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
