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
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
