Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

I've reviewed & tested this, it looks good and works as advertised.

But I'm afraid that I missed one item during my previous review,
this patch should also update:
Documentation/watchdog/watchdog-kernel-api.txt and the comment
above struct watchdog_device in include/linux/watchdog.h,
WRT the new members added to struct watchdog_device.

Besides that there are also some minor whitespace issues
noted by checkpatch / git am:

ERROR: space prohibited before that '++' (ctx:WxB)
#193: FILE: drivers/watchdog/watchdog_dev.c:395:
+       for (i = first; i < DOG_MAX; i ++) {
                                        ^
ERROR: trailing whitespace
#261: FILE: drivers/watchdog/watchdog_dev.c:450:
+}^I$

WARNING: please, no space before tabs
#265: FILE: drivers/watchdog/watchdog_dev.c:453:
+ *^Iwatchdog_dev_register ^I-^Iregister a watchdog$

/home/hans/projects/linux/.git/rebase-apply/patch:323: new blank line at EOF.

With these issues fixed, this patch is:
Acked-by: Hans de Goede <hdegoede@redhat.com>

(and/or Reviewed-by / Tested-by whichever you prefer :)

Regards,

Hans


On 03/21/2012 04:24 PM, Alan Cox wrote:
> From: Alan Cox<alan@linux.intel.com>
>
> We keep the old /dev/watchdog interface file for the first watchdog via
> miscdev. This is basically a cut and paste of the relevant interface code
> from the rtc driver layer tweaked for watchdog.
>
> Revised to fix problems noted by Hans de Goede
>
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   drivers/watchdog/watchdog_dev.c |  231 ++++++++++++++++++++++++++++++++-------
>   include/linux/watchdog.h        |    6 +
>   2 files changed, 193 insertions(+), 44 deletions(-)
>
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index c6e1b8d..40c557b 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -42,10 +42,12 @@
>   #include<linux/init.h>		/* For __init/__exit/... */
>   #include<linux/uaccess.h>	/* For copy_to_user/put_user/... */
>
> -/* make sure we only register one /dev/watchdog device */
> -static unsigned long watchdog_dev_busy;
> +static dev_t dog_devt;		/* Watchdog device range */
> +static unsigned long dog_mask;	/* Watchdogs that are in use */
>   /* the watchdog device behind /dev/watchdog */
> -static struct watchdog_device *wdd;
> +static struct watchdog_device *old_wdd;
> +
> +#define DOG_MAX		32	/* assign_id must be changed to boost this */
>
>   /*
>    *	watchdog_ping: ping the watchdog.
> @@ -136,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
>   static ssize_t watchdog_write(struct file *file, const char __user *data,
>   						size_t len, loff_t *ppos)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	size_t i;
>   	char c;
>
> @@ -175,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
>   static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   							unsigned long arg)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	void __user *argp = (void __user *)arg;
>   	int __user *p = argp;
>   	unsigned int val;
> @@ -251,7 +255,8 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>    *	it can only be opened once.
>    */
>
> -static int watchdog_open(struct inode *inode, struct file *file)
> +static int watchdog_do_open(struct watchdog_device *wdd,
> +				struct inode *inode, struct file *file)
>   {
>   	int err = -EBUSY;
>
> @@ -270,6 +275,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>   	if (err<  0)
>   		goto out_mod;
>
> +	file->private_data = wdd;
> +
>   	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
>   	return nonseekable_open(inode, file);
>
> @@ -281,7 +288,40 @@ out:
>   }
>
>   /*
> - *      watchdog_release: release the /dev/watchdog device.
> + *	watchdog_open: open the /dev/watchdog device.
> + *	@inode: inode of device
> + *	@file: file handle to device
> + *
> + *	When the /dev/watchdog device gets opened, we start the watchdog.
> + *	Watch out: the /dev/watchdog device is single open, so we make sure
> + *	it can only be opened once.
> + */
> +
> +static int watchdog_open(struct inode *inode, struct file *file)
> +{
> +	return watchdog_do_open(old_wdd, inode, file);
> +}
> +
> +/*
> + *	watchdog_mopen: open the newer watchdog devices.
> + *	@inode: inode of device
> + *	@file: file handle to device
> + *
> + *	When the watchdog device gets opened, we start the watchdog.
> + *	Watch out: the /dev/watchdog device is single open, so we make sure
> + *	it can only be opened once.
> + */
> +
> +static int watchdog_mopen(struct inode *inode, struct file *file)
> +{
> +	struct watchdog_device *wdd = container_of(inode->i_cdev,
> +		struct watchdog_device, cdev);
> +
> +	return watchdog_do_open(wdd, inode, file);
> +}
> +
> +/*
> + *      watchdog_release: release the watchdog device.
>    *      @inode: inode of device
>    *      @file: file handle to device
>    *
> @@ -292,6 +332,7 @@ out:
>
>   static int watchdog_release(struct inode *inode, struct file *file)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	int err = -EBUSY;
>
>   	/*
> @@ -326,69 +367,171 @@ static const struct file_operations watchdog_fops = {
>   	.release	= watchdog_release,
>   };
>
> +static const struct file_operations watchdog_multi_fops = {
> +	.owner		= THIS_MODULE,
> +	.write		= watchdog_write,
> +	.unlocked_ioctl	= watchdog_ioctl,
> +	.open		= watchdog_mopen,
> +	.release	= watchdog_release,
> +};
> +
>   static struct miscdevice watchdog_miscdev = {
>   	.minor		= WATCHDOG_MINOR,
>   	.name		= "watchdog",
>   	.fops		=&watchdog_fops,
>   };
>
> -/*
> - *	watchdog_dev_register:
> - *	@watchdog: watchdog device
> +/**
> + *	watchdog_assign_id	-	pick a free watchdog ident
> + *	@watchdog: the watchdog device
> + *	@first: number to scan from
>    *
> - *	Register a watchdog device as /dev/watchdog. /dev/watchdog
> - *	is actually a miscdevice and thus we set it up like that.
> + *	Assign a watchdog number to the device. For the moment we support
> + *	a starting offset so that a legacy watchdog can be worked around
>    */
> +static int watchdog_assign_id(struct watchdog_device *watchdog, int first)
> +{
> +	int i;
> +	for (i = first; i<  DOG_MAX; i ++) {
> +		if (test_and_set_bit(i,&dog_mask) == 0) {
> +			watchdog->id = i;
> +			return i;
> +		}
> +	}
> +	pr_err("no free watchdog slots.\n");
> +	return -EBUSY;
> +}
>
> -int watchdog_dev_register(struct watchdog_device *watchdog)
> +/**
> + *	watchdog_release_id	-	release a watchdg id
> + *	@watchdog: the watchdog device
> + *
> + *	Release the identifier associated with this watchdog. All cdev
> + *	resources must have been released first.
> + */
> +static void watchdog_release_id(struct watchdog_device *watchdog)
> +{
> +	clear_bit(watchdog->id,&dog_mask);
> +}
> +
> +/**
> + *	watchdog_add_device	-	add a watchdog device
> + *	@watchdog: the watchdog device
> + *
> + *	Add a watchdog device node to the system and set up all the
> + *	needed structures.
> + */
> +static int watchdog_add_device(struct watchdog_device *watchdog)
>   {
>   	int err;
>
> -	/* Only one device can register for /dev/watchdog */
> -	if (test_and_set_bit(0,&watchdog_dev_busy)) {
> -		pr_err("only one watchdog can use /dev/watchdog\n");
> -		return -EBUSY;
> -	}
> +	/* Fill in the data structures */
> +	watchdog->devt = MKDEV(MAJOR(dog_devt), watchdog->id);
> +	watchdog->cdev.owner = watchdog->ops->owner;
> +	cdev_init(&watchdog->cdev,&watchdog_multi_fops);
>
> -	wdd = watchdog;
> +	/* Add the device */
> +	err  = cdev_add(&watchdog->cdev, watchdog->devt, 1);
> +	if (err)
> +		pr_err("watchdog%d unable to add device %d:%d\n",
> +			watchdog->id,  MAJOR(dog_devt), watchdog->id);
> +	return err;
> +}
>
> -	err = misc_register(&watchdog_miscdev);
> -	if (err != 0) {
> -		pr_err("%s: cannot register miscdev on minor=%d (err=%d)\n",
> -		       watchdog->info->identity, WATCHDOG_MINOR, err);
> -		goto out;
> -	}
> +/**
> + *	watchdog_del_device	-	delete a watchdog device
> + *	@watchdog: the watchdog device
> + *
> + *	Delete a watchdog device cdev object
> + */
> +static void watchdog_del_device(struct watchdog_device *watchdog)
> +{
> +	cdev_del(&watchdog->cdev);
> +}	
>
> -	return 0;
> +/**
> + *	watchdog_dev_register 	-	register a watchdog
> + *	@watchdog: watchdog device
> + *
> + *	Register a watchdog device including handling the legacy
> + *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
> + *	thus we set it up like that.
> + */
> +int watchdog_dev_register(struct watchdog_device *watchdog)
> +{
> +	int err;
>
> -out:
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
> +	err = watchdog_assign_id(watchdog, 0);
> +	if (err<  0)
> +		return err;
> +	if (err == 0) {
> +		err = misc_register(&watchdog_miscdev);
> +		if (err == 0)
> +			old_wdd = watchdog;
> +		else {
> +			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +				watchdog->info->identity, WATCHDOG_MINOR, err);
> +			if (err == -EBUSY) {
> +				pr_err("%s: probably a legacy watchdog module is present.\n",
> +					watchdog->info->identity);
> +				watchdog_release_id(watchdog);
> +				err = watchdog_assign_id(watchdog, 1);
> +			}
> +			if (err<  0)
> +				return err;
> +		}
> +	}
> +	err = watchdog_add_device(watchdog);
> +	if (err<  0) {
> +		if (watchdog->id == 0) {
> +			misc_deregister(&watchdog_miscdev);
> +			old_wdd = NULL;
> +		}
> +		watchdog_release_id(watchdog);
> +	}
>   	return err;
>   }
>
> -/*
> - *	watchdog_dev_unregister:
> +/**
> + *	watchdog_dev_unregister		-	unregister a watchdog
>    *	@watchdog: watchdog device
>    *
> - *	Deregister the /dev/watchdog device.
> + *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
>    */
> -
>   int watchdog_dev_unregister(struct watchdog_device *watchdog)
>   {
> -	/* Check that a watchdog device was registered in the past */
> -	if (!test_bit(0,&watchdog_dev_busy) || !wdd)
> -		return -ENODEV;
> -
> -	/* We can only unregister the watchdog device that was registered */
> -	if (watchdog != wdd) {
> -		pr_err("%s: watchdog was not registered as /dev/watchdog\n",
> -		       watchdog->info->identity);
> -		return -ENODEV;
> +	watchdog_del_device(watchdog);
> +	if (watchdog->id == 0) {
> +		misc_deregister(&watchdog_miscdev);
> +		old_wdd = NULL;
>   	}
> -
> -	misc_deregister(&watchdog_miscdev);
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
> +	watchdog_release_id(watchdog);
>   	return 0;
>   }
> +
> +/**
> + *	watchdog_init	-	module init call
> + *
> + *	Allocate a range of chardev nodes to use for watchdog devices
> + */
> +int __init watchdog_init(void)
> +{
> +	int err = alloc_chrdev_region(&dog_devt, 0, DOG_MAX, "watchdog");
> +	if (err<  0)
> +		pr_err("watchdog: unable to allocate char dev region\n");
> +	return err;
> +}
> +
> +/**
> + *	watchdog_exit	-	module exit
> + *
> + *	Release the range of chardev nodes used for watchdog devices
> + */
> +void __exit watchdog_exit(void)
> +{
> +	unregister_chrdev_region(dog_devt, DOG_MAX);
> +}
> +
> +module_init(watchdog_init);
> +module_exit(watchdog_exit);
> +
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index de75167..0d5c3af 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -54,6 +54,8 @@ struct watchdog_info {
>   #ifdef __KERNEL__
>
>   #include<linux/bitops.h>
> +#include<linux/device.h>
> +#include<linux/cdev.h>
>
>   struct watchdog_ops;
>   struct watchdog_device;
> @@ -103,6 +105,10 @@ struct watchdog_ops {
>    * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
>    */
>   struct watchdog_device {
> +	struct cdev cdev;		/* Character device */
> +	dev_t devt;			/* Our device */
> +	int id;				/* Watchdog id */
> +
>   	const struct watchdog_info *info;
>   	const struct watchdog_ops *ops;
>   	unsigned int bootstatus;
>