Messages in this thread | | | Date | Sat, 31 Jul 2004 10:55:13 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH] Altix system controller communication driver |
| |
> +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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |