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

[Christoph Hellwig]

On Fri, Mar 07, 2008 at 08:28:44PM -0600, Murali Iyer wrote:
> ===================================================================
> --- /dev/null
> +++ linux-2.6.23.1/drivers/misc/triblade/axon.h
> @@ -0,0 +1,301 @@
> +/**
> + *  axon_h - axon driver data structs.

Just remove the (obsfucated) name of the file in these comments, those
bits just get easily out of date.

> +/* axon task queue */
> +static void  axon_process_workqueue( struct work_struct *ignored);
> +static DECLARE_WORK( axon_workqueue, axon_process_workqueue);
> +
> +/* wait queue for notifications */
> +static DECLARE_WAIT_QUEUE_HEAD(axon_notification_tqh);
> +
> +/* spinlock */
> +DEFINE_SPINLOCK( axon_taskqueue_spinlock);

static?

> +	spin_lock_irqsave( &axon_taskqueue_spinlock, flag);
> +	axon_put_tq( task);
> +	spin_unlock_irqrestore( &axon_taskqueue_spinlock, flag);

What protects there reader side of the queue?

> +static void  axon_process_workqueue( struct work_struct *ignored)
> +{
> +	int	       count;
> +
> +	/* read up to 16 items from queue */
> +	count = 16;
> +	while (count-- > 0) {
> +		/* get an item, and process it */
> +		struct AXON_TQ_TASK   task = { 0 };
> +		if  (axon_get_tq( &task)) {
> +			switch ( task.cmd) {
> +			case TQ_INIT:
> +			case TQ_DIAGS:
> +				break;
> +
> +			case TQ_IRQ:

This code structure seems odd to me.  Why doesn't every AXON_TQ_TASK
(which should get a lowercase name btw) embedd a work_struct so it
can be queued on the workqueue individually?  That way you can also
have one workqueue function pointer per type and don't have to have
the big switch statement here.

> +static int axon_open( struct inode *inode, struct file *filp)
> +{
> +	struct axon *axon_priv;
> +	int rc = 0;
> +
> +	if ( !inode ) {
> +		pr_debug( "ERROR -NOINODE\n");
> +		pr_debug( "ERROR -NOINODE\n");
> +		return -EAGAIN;
> +	}

this can't happen, no need for the check.

> +	/* enforce exclusive access */
> +	if(atomic_read(&axon_priv->open_count)){
> +		rc = -EBUSY;
> +		goto out;
> +	}
> +        atomic_inc(&axon_priv->open_count);

This is not actually atomic.  Better use test_and_set_bit with a bit
flag.

> +	/* initial checking */
> +	if (!axon_priv ) {
> +		pr_debug(": private data is uninitialied\n");
> +		return -EIO;
> +	}

No need for that check again.

> +	if ( count < 16) {
> +		return -EINVAL;
> +	}

Care to replace all these magic 16s witha constant?

> +			err = copy_to_user(buf, data, retval);
> +			if ( err < 0)

Any non-zero return from copy_{to,from}_user should be turned into
a -EFAULT syscall return.  Positive returns from copy_to_user mean
the copy partially succeed and mean the number of bytes sucessfully
written.

> +static ssize_t axon_write(struct file *filp, const char __user *buf,
> +			  size_t count, loff_t *ppos)
> +{
> +	struct axon *axon_priv = filp->private_data;
> +	struct d2d_message msg;
> +	static int msg_count = 0;
> +	msg.cmd     = D2D_MSG_DIAGS;
> +	msg.data[0] = msg_count++;
> +	msg.data[1] = 0x1234;
> +	msg.data[2] = 0x5678;
> +
> +	/* diags */
> +	axon_add_task( TQ_DIAGS, axon_priv, 0x10, 0x11,
> +		       "axon_write");
> +	axon_send_d2d_msg(axon_priv, &msg);

This is a quite strange write method tha at least needs lot of
documentation.

> +	/* retrieve private struct */
> +	axon_priv = (struct axon *) filp->private_data;
> +	if (axon_priv == 0) {
> +		pr_debug( "axon_ioctl:  bad private struct = %p\n", axon_priv);
> +		return -EFAULT;
> +	}

no check needed here.

> +	/* retrieve private struct */
> +	axon_priv = (struct axon *) filp->private_data;
> +	if (axon_priv == 0) {
> +		pr_debug( "axon_ioctl:  bad private struct = %p\n", axon_priv);
> +		return -EFAULT;
> +	}

Same here.

> +int axon_mmap( struct file *filp, struct vm_area_struct *vma)
> +{
> +	long	  length = vma->vm_end - vma->vm_start;
> +	int	  ret = -EIO;
> +	struct axon	 *axon_priv;
> +	unsigned int	   offset;
> +
> +	/* retrieve private struct */
> +	axon_priv = (struct axon *) filp->private_data;
> +	if (axon_priv == 0) {
> +		return -EIO;
> +	}

again.

> +	if ( axon_priv != NULL) {

again..

> +/*
> + * axon_get_num_devices - get number of axon devices
> + * Returns number of axon devices which are active
> + */
> +int axon_get_num_devices( void )

not a very good interface.  Please use an iterator over all available
devices instead, possibly using the drivers/base/ code.

> + * axon_get_remote_memory - get driver-to-driver memory area
> + * @dev_num - devicenumber
> + * @returns pointer to portion of remote D2D area, which belongs to apnet
> + */
> +void __iomem *axon_get_remote_memory(int dev_num)

As mentioned by Arnd earlier these kind of calls should get an object
passed to it, not an index.

> +	/* allocate table for device private structs */
> +	axon_device_table = kmalloc( axon_max_devices * sizeof(struct axon),
> +				     GFP_KERNEL);
> +	if ( !axon_device_table) {
> +		printk(KERN_ERR"Axon Driver could not allocate "
> +		    "driver table for %d drivers \n",
> +		    axon_max_devices);
> +		rc = -ENOMEM;
> +		goto  fail_malloc;
> +	}
> +	memset( axon_device_table, 0, axon_max_devices*sizeof(struct axon));

Uze kcalloc instead which does the memset internally.

> +	/* initialize work queue */
> +	axon_request_tq.read  = 0;
> +	axon_request_tq.write = 0;
> +	axon_request_tq.size  = TQ_TASK_QUEUE_SZ;
> +	axon_request_tq.status = 0;
> +	axon_request_tq.num_requests  = 0;
> +	axon_request_tq.num_responses = 0;
> +	axon_request_tq.num_errors    = 0;

static variables are set to 0 by default, so no need to initalize them
here.