Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck <wim@iguana.be>]

Hi Lars-Peter,

> On 07/11/2011 04:19 PM, Wim Van Sebroeck wrote:
> > [...]
> > +
> > +
> > +static int watchdog_release(struct inode *inode, struct file *file)
> > +{
> > +	int err;
> > +
> > +	/* stop the watchdog */
> > +	err = wdd->ops->stop(wdd);
> Does it really make sense to allow stop() to fail? Will this ever happen, and
> if yes do we gain anything by sending a additional ping?

It buys user-space extra time to recover if necessary.

> > [...]
> > +
> > +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;
> > +	}
> > +
> > +	/* Unregister the miscdevice */
> > +	misc_deregister(&watchdog_miscdev);
> > +	wdd = NULL;
> > +	clear_bit(0, &watchdog_dev_busy);
> > +	return 0;
> > +}
> 
> What happens if the watchdog gets unregistered if the device is still opened?
> Even though if you'd check wdd for not being NULL in the file callbacks there
> is still a chance for races if the devices is unregistered at the same time as
> the callback is running. You'd either need a big lock to protect from having a
> file callback and unregister running concurrently or add ref-counting to the
> watchdog_device, the later best done by embedding a struct device and using the
> device driver model.

You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
So if the watchdog_unregister function is in the exit function of the module
then we are safe. But I think you have a point if that is not the case.
Solution would be to return an error when the watchdog_unregister_device routine
is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.

> > [...]
> >  
> > +struct watchdog_ops;
> > +struct watchdog_device;
> > +
> > +/* The watchdog-devices operations */
> > +struct watchdog_ops {
> > +	struct module *owner;
> > +	/* mandatory operations */
> > +	int (*start)(struct watchdog_device *);
> > +	int (*stop)(struct watchdog_device *);
> > +	/* optional operations */
> > +	int (*ping)(struct watchdog_device *);
> > +};
> > +
> > +/* The structure that defines a watchdog device */
> > +struct watchdog_device {
> > +	const struct watchdog_info *info;
> > +	const struct watchdog_ops *ops;
> > +	void *priv;
> 
> You should provide getter/setter methods for priv, so that when the watchdog
> API is changed to make use of the device driver model it becomes easier to get
> rid of the priv field and use dev_{get,set}_drvdata instead.

Added.

> > +	unsigned long status;
> > +/* Bit numbers for status flags */
> > +#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
> > +};
> 
> Kernel-doc style documentation for the structs would be nice.

Added.

Kind regards,
Wim.