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

[Bryan O'Sullivan]

Ramachandra K wrote:

> +void vnic_connected(struct vnic *vnic, struct netpath *netpath)
> +{
> +	VNIC_FUNCTION("vnic_connected()\n");
> +	if (netpath->second_bias)
> +		vnic_npevent_queue_evt(netpath, VNIC_SECNP_CONNECTED);
> +	else
> +		vnic_npevent_queue_evt(netpath, VNIC_PRINP_CONNECTED);
> +
> +	vnic_connected_stats(vnic);
> +}

This driver looks rather like it's duplicating at least some of the 
functionality provided by the bonding driver.  Could you explain the 
differences between the two approaches, please?

> +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;
> +		}
> +
> +		vnic_stop_xmit_stats(vnic);
> +	}
> +}

Why is there an asymmetry between some of the routines, which will 
operate on either netpath depending on the bias setting, and others like 
this, which only operate on the current netpath and silently do nothing 
otherwise?  This is at least confusing.

> +static struct vnic * vnic_handle_npevent(struct vnic *vnic,
> +					 enum vnic_npevent_type npevt_type)
> +{
> +	struct netpath	*netpath;
> +
> +	VNIC_INFO("%s: processing %s, netpath=%s, carrier=%d\n",
> +		  vnic->config->name, vnic_npevent_str[npevt_type],
> +		  netpath_to_string(vnic, vnic->current_path),
> +		  vnic->carrier);
> +
> +	switch (npevt_type) {
> +	case VNIC_PRINP_CONNECTED:

I still don't understand this business of queueing events in the normal 
net driver entry points, then redispatching them in this big switch 
statement.  Could you at least add a comment to the code that indicates 
why you're doing this?  This structure makes the driver harder to follow 
without providing any obvious benefits.

	<b