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

[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