Re: [PATCH 001/001] CHAR DRIVERS: a simple device to give daemons a /sys-like interface

[Joe Perches <joe@perches.com>]

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.