[Cbe-oss-dev] [patch 04/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Arnd Bergmann]

On Saturday 08 March 2008, Murali Iyer wrote:
>        This patch provides a mechanism for modules such as APNET
>        to recieve interrupt notification.

It would be nice if this were closer integrated with the standard
machanisms we have in Linux for event notification. The three
most common mechanisms for this are

1. irq handlers
2. work queues
3. notifier lists.

Depending on how exactly you use these your notifiers, I'm sure
that one of these three would help you simplify the code.

Providing a virtual interrupt controller is probably the most
complicated way to do it, but may get your the best performance,
along with the benefits of seeing your events in /proc/interrutps
etc.

Work queues may be best if you want to process the events in a process
context anyway, but they add some latency.

Notifier lists are basically what you have reimplemented here, except
that they allow multiple listeners for each event.

> +extern int num_axons_found;

Again, there should not be an extern declaration for this in the
implementation file, and moreover, you should avoid this counter
entirely ;-)

> +/**
> + * get_priv
> + */
> +static struct axon *get_axon_priv( int msg_type, int dev_num, int *error)
> +{
> +	struct axon *axon_priv = NULL;
> +	*error = 0;
> +
> +	/* check for valid instance  & type */
> +	if (( dev_num < 0 ) || (dev_num >= num_axons_found)) {
> +		printk(KERN_ERR "axon: invalid dev_num %d\n", dev_num);
> +		*error = -1;
> +		return axon_priv;
> +	}
> +	if (( msg_type < 0 ) || (msg_type  >= AXON_NUM_CALLBACKS)) {
> +		printk(KERN_ERR "axon: invalid msg_type %d\n", msg_type);
> +		*error = -2;
> +		return axon_priv;
> +	}
> +
> +	/* make sure it is valid */
> +	axon_priv =  &(axon_device_table[dev_num]);
> +	if ( axon_priv == NULL) {
> +		printk(KERN_ERR "axon: invalid instance struct\n");
> +		*error = -3;
> +		return axon_priv;
> +	}
> +
> +	return axon_priv;
> +}

Please use the standard error handling here, it works as follows:

For every error condition that can happen, find the best match in
include/linux/errno.h.

Use ERR_PTR() to turn that into a pointer value that you can return
from a function.

In the caller, use IS_ERR() and PTR_ERR() to get the error back.
These will produce object code that is no less efficient what you
have, but is much more intuitive to read.

> +/**
> + * axon_register_notify_callback - register callback for msg type
> + * @type - type of callback ( enum message_type)
> + * @dev_num - device number
> + * @cb_data - data for callback
> + * @cb_func - function to call
> + *
> + */
> +int   axon_register_notify_callback( enum message_type	type,
> +				     int		dev_num,
> +				     void	       *cb_data,
> +				     axon_callback	cb_func)
> +{
> +	int msg_type = (int) type;
> +	struct axon *axon_priv;
> +	int error;
> +
> +	axon_priv = get_axon_priv( msg_type, dev_num, &error);
> +	if ( axon_priv == NULL) {
> +		printk(KERN_ERR "reg callback failed %d\n", error);
> +		return error;
> +	}

Don't check for this here, just make sure the callers don't get NULL
pointers to pass in here.

> +	/* check for existing callback*/
> +	if ( axon_priv->callbacks[msg_type].func != NULL) {
> +		printk(KERN_ERR "axon: callback of type %d exists\n", msg_type);
> +		return -4;
> +	}
> +
> +	/* apply */
> +	axon_priv->callbacks[msg_type].func    = cb_func;
> +	axon_priv->callbacks[msg_type].data    = cb_data;
> +	axon_priv->callbacks[msg_type].enabled = 1;
> +
> +	axon_send_remote_enable(axon_priv, msg_type);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL( axon_register_notify_callback);

I don't think you need the 'enabled' flag here, the presence of the
callback pointer should be sufficient. 
However, I can't find any locking around this, so it is unobvious that
the event can not be called while it is being updated.