On Fri, 2013-08-02 at 18:19 -0700, Bob Smith wrote:
> This character device can give daemons an interface similar to
> the kernel's /sys and /proc interfaces. It is a nice way to
> give user space drivers real device nodes in /dev.
Trivial notes:
> diff --git a/drivers/char/proxy.c b/drivers/char/proxy.c
[]
+int proxy_init_module(void)
> +{
> + int i, err;
> + px_devices = kmalloc(numberofdevs * sizeof(struct px), GFP_KERNEL);
> + if (px_devices == NULL) {
> + /* no memory available */
> + if (debuglevel >= 1)
> + pr_err("%s: init fails: no memory.\n",
> + DEVNAME);
OOM messages aren't necessary.
dump_stack() is already done on OOM.
> + return 0;
> + }
> + memset(px_devices, 0, numberofdevs * sizeof(struct px));
kcalloc instead of kmalloc/memset
> +
> + /* init devices in this block */
> + for (i = 0; i < numberofdevs; i++) { /* for every minor device */
> + px_devices[i].minor = i; /* set minor number */
> + px_devices[i].ewbuf.buf = (char *) 0;
> + px_devices[i].webuf.buf = (char *) 0;
> + px_devices[i].ewbuf.widx = 0;
> + px_devices[i].webuf.widx = 0;
> + px_devices[i].ewbuf.ridx = 0;
> + px_devices[i].webuf.ridx = 0;
sets to zero after zeroed alloc are redundant.
> + err = cdev_add(&px_cdev, px_devicenumber, numberofdevs);
> + if (err < 0) {
> + if (debuglevel >= 1)
> + pr_err("%s: init fails. err=%d.\n",
> + DEVNAME, err);
> + return err;
> + }
It's either an err or a debug message.
Don't hide actual errors under debugging levels.
I suggest you only use pr_debug and/or have a macro like
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#define proxy_dbg(level, fmt, ...) \
do { \
if (level >= debuglevel) \
pr_debug(fmt, ##__VA_ARGS__); \
} while (0)
> + if (debuglevel >= 2)
> + pr_info("%s: Installed %d minor devices on major number %d.\n",
> + DEVNAME, numberofdevs, px_major);
Then this (and the rest) becomes
proxy_dbg(2, "Installed %d minor devices on major %d\n",
numberofdevs, px_major);
etc...
> +static int proxy_open(struct inode *inode, struct file *filp)
> +{
> + if (!dev->ewbuf.buf) { /* get east-to-west buffer */
> + dev->ewbuf.buf = kmalloc(buffersize, GFP_KERNEL);
> + if (!dev->ewbuf.buf) {
> + if (debuglevel >= 1)
> + pr_err("%s: No memory dev=%d.\n",
> + DEVNAME, mnr);
> + up(&dev->sem);
> + return -ENOMEM;
> + }
> + }
> + if (!dev->webuf.buf) { /* get west-to-east buffer */
> + dev->webuf.buf = kmalloc(buffersize, GFP_KERNEL);
> + if (!dev->webuf.buf) {
> + if (debuglevel >= 1)
> + pr_err("%s: No memory dev=%d.\n",
> + DEVNAME, mnr);
no OOMs.
> +static int proxy_release(struct inode *inode, struct file *filp)
> +{
> + struct px *dev = (struct px *) filp->private_data;
> +
> + if (debuglevel >= 3)
> + pr_info("%s release. Minor#=%d.\n", DEVNAME,
> + ((struct px *) filp->private_data)->minor);
> +
> + if (down_interruptible(&dev->sem)) /* prevent races on close */
> + return -ERESTARTSYS;
> +
> + dev->nopen = dev->nopen - 1;
> +
> + if (dev->east == filp) {
> + dev->east = (struct file *) 0; /* mark as not in use */
s/0/NULL; (no need to cast either)
> + dev->ewbuf.cidx = dev->ewbuf.widx; /* set close index */
> + } else if (dev->west == filp) {
> + dev->west = (struct file *) 0; /* mark as not in use */
etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
