Re: [PATCH] Altix system controller communication driver

From: Christoph Hellwig (hch_at_infradead.org)
Date: 07/31/04

  • Next message: Eric Anholt: "Re: drm - first steps towards 64-bit correctness.." 
    Date:	Sat, 31 Jul 2004 10:55:13 +0100
To: Greg Howard <ghoward@sgi.com>

    > +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/

  • Next message: Eric Anholt: "Re: drm - first steps towards 64-bit correctness.."

    Relevant Pages