Great to see TOE support getting reviewed.

Some general stuff:
  * Use linux indentation style (Documentation/Codingstyle)
     "Tabs are 8 characters, and thus indentations are also 8
characters." You are using 4 spaces.    

  * Don't use // for comments

  * All patches must have Signed-off-by: line. This is a legal
requirement see Documentation/SubmittingPatches.

   * Why use name for binding, it is slow to lookup and can change.
     Either use if_index or pointer. If you used driver model/sysfs
     you wouldn't need to.

   * Please make the toe devices part of sysfs.
     Probably simplest to make toedev a kobject that is a child
     of the netdevice.

   * Can't you use sysfs instead of /proc? (see above)

   * Please change all EXPORT_SYMBOL to EXPORT_SYMBOL_GPL to explicitly
     impede binary drivers.

   * Please don't include 2.4 compatiablity in the 2.6 version. This
     is new code.

   * Consider making TOE a config option, and stubbing as appropriate
     many platforms may not want it.


diff -Naur linux-2.6.13-rc3/include/linux/toedev.h linux-2.6.13-
rc3.patched/include/linux/toedev.h --- linux-2.6.13-rc3/include/linux/
toedev.h        1969-12-31 16:00:00.000000000 -0800 +++ linux-2.6.13-
rc3.patched/include/linux/toedev.h      2005-07-26 12:23:53.796497048
-0700 @@ -0,0 +1,96 @@

...
+
+#ifndef _TOEDEV_H_
+#define _TOEDEV_H_
+
+#include <linux/list.h>
+#include <asm/atomic.h>
+
+#define TOENAMSIZ 16
See above comments about name

+/* belongs in linux/netdevice.h */
+#define NETIF_F_TCPIP_OFFLOAD (1 << 16)
If it belongs there, then put it there!

+/* Get the toedev associated with a net_device */
+#define TOEDEV(netdev) ((struct toedev *)(netdev)->ec_ptr)
Use inline instead to get type checking.

+/* TOE type ids */
+enum {
+    TOE_ID_CHELSIO_T1  = 1,
+    TOE_ID_CHELSIO_T1C,
+    TOE_ID_CHELSIO_T3,
+};
+
+struct toe_id {
+    unsigned int id;
+    unsigned long data;
+};
+
+#define END_OF_TOE_ID_TABLE { 0, 0UL }
Get rid of this macro. that's just obfuscation.

+
+struct net_device;
+struct neighbour;
+struct tom_info;
+struct proc_dir_entry;
+struct sock;
+struct sk_buff;

Include the necessary definitions.


+struct toedev {
+    char name[TOENAMSIZ];               /* TOE device name */
+    struct list_head toe_list;          /* for list linking */
+    int toe_index;                      /* unique TOE device index */
+    unsigned int ttid;                  /* TOE type id */
+    unsigned long flags;                /* device flags */
+    unsigned int mtu;                   /* max size of TX offloaded
data */ 
+    unsigned int nconn;                 /* max # of offloaded
connections */
+    struct net_device *lldev;      /* LL device associated with TOE
messages */ ...
diff -Naur linux-2.6.13-rc3/include/net/offload.h linux-2.6.13-
rc3.patched/include/net/offload.h --- linux-2.6.13-rc3/include/net/
offload.h       1969-12-31 16:00:00.000000000 -0800 +++ linux-2.6.13-
rc3.patched/include/net/offload.h       2005-07-26 12:23:53.796497048
-0700 @@ -0,0 +1,47 @@ +
+/* Returns true if sk is an offloaded IPv4 TCP socket. */
+#define IS_OFFLOADED(sk) (((sk)->sk_family == AF_INET && (sk)-
>sk_prot != &tcp_prot))

inline?

+struct toedev;
+struct sk_buff;
+struct sock;

More redundant fwd declarations.


+/* Per-skb backlog handler.  Run when a socket's backlog is processed.
*/ +struct blog_skb_cb {
+       void (*backlog_rcv)(struct sock *sk, struct sk_buff *skb);
+       struct toedev *dev;
+};
+
+#define BLOG_SKB_CB(skb) ((struct blog_skb_cb *)(skb)->cb)
+
+/* belongs in linux/tcp_diag.h */
+#define TCPDIAG_OFFLOAD 5
+
Please patch tcp_diag.h then.


diff -Naur linux-2.6.13-rc3/net/core/toedev.c linux-2.6.13-rc3.patched/
net/core/toedev.c --- linux-2.6.13-rc3/net/core/toedev.c
1969-12-31 16:00:00.000000000 -0800 +++ linux-2.6.13-rc3.patched/net/
core/toedev.c   2005-07-26 12:23:53.799496592 -0700 @@ -0,0 +1,432
@@ +
+/* 2.4 compatibility */
+#ifndef subsys_initcall
+#define subsys_initcall(fn) module_init(fn)
+
+static int boot_phase = 1;
+#else
+#define boot_phase 0
+#endif

No, just do 2.6
+
+/*
+ * Returns the entry in the TOE id table 'table' that has a given id,
or NULL
+ * if the id is not found.
+ */
+static const struct toe_id *id_find(unsigned int id,
+                                   const struct toe_id *table)
+{
+    const struct toe_id *p;
+
+    for (p = table; p->id; ++p)
+       if (p->id == id) return p;
        please split line


Could you use RCU?
+                       
+/*
+ * Register a TCP Offload Module (TOM).
+ */
+int register_tom(struct tom_info *t)
+{
+    down(&toedev_db_lock);
+    list_add(&t->list_node, &tom_list);
+    up(&toedev_db_lock);
+    return 0;
+}

Using a semaphore seems more than you need here.
Given the code path you could either assume RT
netlink semaphore or just use a spinlock.

... 
diff -Naur linux-2.6.13-rc3/net/ipv4/tcp_ipv4.c linux-2.6.13-
rc3.patched/net/ipv4/tcp_ipv4.c --- linux-2.6.13-rc3/net/ipv4/
tcp_ipv4.c      2005-07-26 10:16:10.000000000 -0700 +++ linux-2.6.13-
rc3.patched/net/ipv4/tcp_ipv4.c 2005-07-26 12:23:53.803495984
-0700 /* Caller must disable local BH processing. */
-static __inline__ void __tcp_inherit_port(struct sock *sk, struct sock
*child) +/* static */ __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 +352,7 @@
        }
 }
 
-static __inline__ void __tcp_v4_hash(struct sock *sk, const int
listen_possible) +/* static */ __inline__ void __tcp_v4_hash(struct
sock *sk, const int listen_possible) {

I don't understatnd why you made these no longer static?
-
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

Reply via email to