[openib-general] [PATCH 1/10] Driver Main files - netdev functions and corresponding state maintenance

2006-10-02 Thread Ramachandra K
Adds the driver main files. These files implement netdev
registration, netdev functions and state maintenance of the
virtual NIC corresponding to the various events associated
with the Virtual Ethernet IOC (VEx) connection.

Signed-off-by: Ramachandra K [EMAIL PROTECTED]
---

 drivers/infiniband/ulp/vnic/vnic_main.c | 1040 +++
 drivers/infiniband/ulp/vnic/vnic_main.h |  152 +
 2 files changed, 1192 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/vnic/vnic_main.c 
b/drivers/infiniband/ulp/vnic/vnic_main.c
new file mode 100644
index 000..b87e00b
--- /dev/null
+++ b/drivers/infiniband/ulp/vnic/vnic_main.c
@@ -0,0 +1,1040 @@
+/*
+ * Copyright (c) 2006 SilverStorm Technologies Inc.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ *  - Redistributions of source code must retain the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer.
+ *
+ *  - Redistributions in binary form must reproduce the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer in the documentation and/or other materials
+ *provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/errno.h
+#include linux/netdevice.h
+#include linux/etherdevice.h
+#include linux/skbuff.h
+#include linux/version.h
+#include linux/string.h
+#include linux/init.h
+#include linux/list.h
+#include linux/completion.h
+
+#include rdma/ib_cache.h
+
+#include vnic_util.h
+#include vnic_main.h
+#include vnic_netpath.h
+#include vnic_viport.h
+#include vnic_ib.h
+
+#define MODULEVERSION 0.1
+#define MODULEDETAILS Virtual NIC driver version  MODULEVERSION
+
+MODULE_AUTHOR(SilverStorm Technologies Inc.);
+MODULE_DESCRIPTION(MODULEDETAILS);
+MODULE_LICENSE(Dual BSD/GPL);
+MODULE_SUPPORTED_DEVICE(SilverStorm Ethernet Virtual I/O Controller);
+
+u32 vnic_debug = 0x0;
+
+module_param(vnic_debug, uint, 0444);
+
+LIST_HEAD(vnic_list);
+
+const char driver[] = vnic;
+
+void vnic_connected(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_connected()\n);
+   vnic_npevent_queue_evt(netpath, VNICNP_CONNECTED);
+#ifdef CONFIG_INFINIBAND_VNIC_STATS
+   if (vnic-statistics.conn_time == 0) {
+   vnic-statistics.conn_time =
+   get_cycles() - vnic-statistics.start_time;
+   }
+   if (vnic-statistics.disconn_ref != 0) {
+   vnic-statistics.disconn_time +=
+   get_cycles() - vnic-statistics.disconn_ref;
+   vnic-statistics.disconn_num++;
+   vnic-statistics.disconn_ref = 0;
+   }
+#endif /* CONFIG_INFINIBAND_VNIC_STATS */
+}
+
+void vnic_disconnected(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_disconnected()\n);
+   vnic_npevent_queue_evt(netpath, VNICNP_DISCONNECTED);
+}
+
+void vnic_link_up(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_link_up()\n);
+   vnic_npevent_queue_evt(netpath, VNICNP_LINKUP);
+}
+
+void vnic_link_down(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_link_down()\n);
+   vnic_npevent_queue_evt(netpath, VNICNP_LINKDOWN);
+}
+
+void vnic_stop_xmit(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_stop_xmit()\n);
+   if (netpath == vnic-current_path) {
+   if (vnic-xmit_started) {
+   netif_stop_queue(vnic-netdevice);
+   vnic-xmit_started = 0;
+   }
+#ifdef CONFIG_INFINIBAND_VNIC_STATS
+   if (vnic-statistics.xmit_ref == 0) {
+   vnic-statistics.xmit_ref = get_cycles();
+   }
+#endif /* CONFIG_INFINIBAND_VNIC_STATS */
+   }
+   return;
+}
+
+void vnic_restart_xmit(struct vnic *vnic, struct netpath *netpath)
+{
+   VNIC_FUNCTION(vnic_restart_xmit()\n);
+   if (netpath == vnic-current_path) {
+   if (!vnic-xmit_started) {
+   

Re: [openib-general] [PATCH 1/10] Driver Main files - netdev functions and corresponding state maintenance

2006-10-02 Thread Roland Dreier
  +#ifdef CONFIG_INFINIBAND_VNIC_STATS
  +extern cycles_t recv_ref;
  +#endif  /* CONFIG_INFINIBAND_VNIC_STATS */

put this declaration in a header file somewhere, not inside a function
in a .c file.

Also is it really worth having CONFIG_INFINIBAND_VNIC_STATS?  Who
would use it?  Or would anyone turn it off?  All the #ifdefs make the
code much harder to read so I think you need to figure out a better
way to make it conditional if you really want it to be configurable.

  +/* TBD */

  +/* TBD */

Umm...

  +static BOOLEAN vnic_npevent_register(struct vnic *vnic, struct netpath 
  *netpath)

What do you gain from having this shouting BOOLEAN type?

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general



Re: [openib-general] [PATCH 1/10] Driver Main files - netdev functions and corresponding state maintenance

2006-10-02 Thread Bryan O'Sullivan
Ramachandra K wrote:

 +#include linux/kernel.h

Not needed.

 +#include linux/version.h

Not needed.

 +#ifdef CONFIG_INFINIBAND_VNIC_STATS
 + if (vnic-statistics.conn_time == 0) {
 + vnic-statistics.conn_time =
 + get_cycles() - vnic-statistics.start_time;
 + }
 + if (vnic-statistics.disconn_ref != 0) {
 + vnic-statistics.disconn_time +=
 + get_cycles() - vnic-statistics.disconn_ref;
 + vnic-statistics.disconn_num++;
 + vnic-statistics.disconn_ref = 0;
 + }
 +#endif   /* CONFIG_INFINIBAND_VNIC_STATS */

Why does none of your stats code use locks?

 +static int vnic_open(struct net_device *device)
 +{
 + struct vnic *vnic;
 + int ret = 0;
 + struct netpath *np;
 +
 + VNIC_FUNCTION(vnic_open()\n);
 + vnic = (struct vnic *)device-priv;
 + np = vnic-current_path;
 +
 + if (vnic-state != VNIC_REGISTERED) {
 + ret = -ENODEV;
 + }
 +
 + vnic-open++;
 + vnic_npevent_queue_evt(vnic-primary_path, VNIC_NP_SETLINK);
 + vnic-xmit_started = 1;
 + netif_start_queue(vnic-netdevice);
 +
 + return ret;
 +}

If you're returning an error value, you shouldn't be finishing the open 
call as if nothing happened.


 +static int vnic_hard_start_xmit(struct sk_buff *skb, struct net_device 
 *device)
 +{

 + dev_kfree_skb(skb);
 + return 0;   /* TBD: what should I return? */
 +}

Any non-zero value means try again.

 +static void vnic_tx_timeout(struct net_device *device)
 
 + return;

Not needed.

 +static int vnic_do_ioctl(struct net_device *device, struct ifreq *ifr, int 
 cmd)
 +{
 + struct vnic *vnic;
 + int ret = 0;
 +
 + VNIC_FUNCTION(vnic_do_ioctl()\n);
 + vnic = (struct vnic *)device-priv;
 +
 + /* TBD */
 +
 + return ret;
 +}

If you don't do anything, don't implement this.  And especially don't 
return success no matter what you're passed.

 +static int vnic_set_config(struct net_device *device, struct ifmap *map)
 +{
 + struct vnic *vnic;
 + int ret = 0;
 +
 + VNIC_FUNCTION(vnic_set_config()\n);
 + vnic = (struct vnic *)device-priv;
 +
 + /* TBD */
 +
 + return ret;
 +}

Likewise.

 +static BOOLEAN vnic_npevent_register(struct vnic *vnic, struct netpath 
 *netpath)

There's no BOOLEAN type in the kernel; please don't add one.

 + if (register_netdev(vnic-netdevice) != 0) {
 + VNIC_ERROR(failed registering netdev\n);
 + return FALSE;
 + }

Propagate the error value instead.

 + vnic-state = VNIC_REGISTERED;
 + vnic-carrier = 2;  /* special value to force 
 netif_carrier_(on|off) */
 + return TRUE;
 +}

And return 0 on success.

 + BOOLEAN delay = TRUE;

No BOOLEANs, please.

 + if (!vnic-carrier) {
 + switch (netpath-timer_state) {
 + case NETPATH_TS_IDLE:
 + netpath-timer_state =
 + NETPATH_TS_ACTIVE;
 + if (vnic-state == VNIC_UNINITIALIZED)
 + netpath_timer(netpath,

This is a very deep nesting of conditionals.  Please restructure into 
something more compreshensible.

A general comment: I don't understand why you've moved a bunch of code 
with well-defined entry points into this big ugly single-function state 
machine.  It means you have a whole lot of trivial wrapper code that 
serves no purpose, and decreases the readability of the driver 
significantly.

b

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general