Re: [PATCH] Altix system controller communication driver

[Christoph Hellwig <hch@infradead.org>]

> +config SGI_SNSC
> +	bool "SGI Altix system controller communication support"

this won't compile for non-ia64, so you need a depency here.

> +#include "snsc.h"
> +#include <linux/interrupt.h>
> +#include <linux/sched.h>
> +#include <linux/device.h>
> +#include <linux/poll.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <asm/sn/sn_sal.h>
> +#include <asm/sn/nodepda.h>

please include your private header after system headers.  But I don't
thing you need snsc.h at all, just move it's contents into snsc.c

> +
> +
> +#define SYSCTL_BASENAME	"snsc"
> +
> +#define SCDRV_BUFSZ	2048
> +
> +#ifdef SCDRV_DEBUG
> +#define DPRINTF(x...)	printk(x)
> +#else
> +#define DPRINTF(x...)	do {} while(0)
> +#endif

please use the existing pr_debug, or even dev_dbg where you have a struct
device.

> +static int scdrv_open(struct inode *, struct file *);
> +static int scdrv_release(struct inode *, struct file *);
> +static ssize_t scdrv_read(struct file *, char __user *, size_t, loff_t *);
> +static ssize_t scdrv_write(struct file *, const char __user *,
> +			   size_t, loff_t *);
> +static unsigned int scdrv_poll(struct file *, struct poll_table_struct *);
> +static irqreturn_t scdrv_interrupt(int, void *, struct pt_regs *);
> +
> +static struct file_operations scdrv_fops = {
> +	.owner =	THIS_MODULE,
> +	.read =		scdrv_read,
> +	.write =	scdrv_write,
> +	.poll =		scdrv_poll,
> +	.open =		scdrv_open,
> +	.release =	scdrv_release,
> +};

the driver would become more readable by having struct file_operations
after the actual function bodies, thus eliminating the need for additional
prototypes.

> +/*
> + * scdrv_wait
> + *
> + * Call this function to wait on one of the queues associated with an
> + * open subchannel.  Avoid races by entering this function with a held
> + * lock that protects the wait queue; don't release the lock until after
> + * we've added ourselves to the queue.
> + */
> +static inline int
> +scdrv_wait(wait_queue_head_t *waitq_head, spinlock_t *waitq_lock,
> +	   unsigned long flags, unsigned long timeout)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	int ret;
> +
> +	add_wait_queue(waitq_head, &wait);
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	spin_unlock_irqrestore(waitq_lock, flags);
> +
> +	if (timeout) {
> +		ret = schedule_timeout(timeout);
> +	} else {
> +		schedule();
> +	}

You're always calling this with a timeout set.  Also I think the code
would be much more readable by opencoding this in the two callers.  That'd
also give you a chance to switch to prepare_wait & co.

> +static int
> +scdrv_open(struct inode *inode, struct file *file)
> +{
> +	struct sysctl_data_s *scd;
> +	struct subch_data_s *sd;
> +	int rv;
> +
> +	/* look up device info for this device file */
> +	scd = container_of(inode->i_cdev, struct sysctl_data_s, scd_cdev);
> +
> +	if (!scd) {
> +		printk("%s: no such device\n", __FUNCTION__);
> +		return -ENODEV;
> +	}

Can't ever be NULL.

> +		len = min((int) count, len);
> +		if (copy_to_user(buf, sd->sd_rb, len))
> +			return -EFAULT;

Don't you have sd->sd_rbs held here?

> +static inline void
> +scdrv_lock_all(struct subch_data_s *sd, unsigned long *flags)
> +{
> +	spin_lock_irqsave(&sd->sd_rlock, *flags);
> +	spin_lock(&sd->sd_wlock);
> +}
> +
> +static inline void
> +scdrv_unlock_all(struct subch_data_s *sd, unsigned long flags)
> +{
> +	spin_unlock(&sd->sd_wlock);
> +	spin_unlock_irqrestore(&sd->sd_rlock, flags);
> +}

It would probably be more readable without these wrappers.