watchdog infrastructure

Hi Wim,

I noticed you have been working on sanitizing the watchdog driver in your http://linux-watchdog.bkbits.net/linux-2.6-watchdog-experimental
tree. What are your plans for this, i.e. do you see this as 2.7 only stuff or do you intend to merge the at least the infrastructure code so it can be used by future 2.6 drivers?

I'm asking because I have a new driver and I would prefer not to add yet another copy of the ioctl code, which I don't even know how to test properly.

If we can get your watchdog.c into a mergable state (in which it arguably isn't at the moment), I could use that to base my driver on, while the other drivers get converted during 2.7.

	Arnd <><

Hi Arnd,

Plan is to build a generic watchdog driver that has a frame-work for all watchdog drivers (or at least most of them). I think that it can be future 2.6 driver code. It's not a fundamental change :-).

Good idea. I'm first going to test iit on my different hardware (pcwd
+ i8xx + w83627hf). Once that's good enough we can start with the generic watchdog driver and some converted drivers.

Did you have a look allready at the different watchdog operations in include/linux/watchdog.h ?

Greetings,
Wim.

Yes, I already have a working driver, which does not use the experimental infrastructure code. I'm just not allowed to publish it until the hardware is available.

There are a couple of things I noticed about your new code:

- Is there any reason having an alloc_watchdogdev function in the common code? Simply statically allocating the structure in each device driver should be a lot simpler.
- Keeping watchdog_ops out of watchdog_device will simplify the lifetime rules. Just put them in the same structure, add an owner field and get rid of the *private field.
- watchdog_is_open_sem can just be an atomic_t, you never actually down() it.
- You need to get the module reference count before calling any watchdog operation, the best place for this is probably the open() fop.
- Maybe its easier to always register the misc devices when watchdog.ko is loaded, and then deny opening them when no actual watchdog driver is registered to it.
- Why do you need seperate operations for start and keepalive?
- the reboot notifier and the nowayout parameter are probably common enough to be put into the generic module.

	Arnd <><

Hello Arnd,

sorry for the delay, but it's pretty busy at my work at the moment.

Oops: I forgot apparently: the experimental code is indeed more for

2.7 . Plan is to go in 2 stages: have a generic watchdog for v2.6 which is just generic code that can be used by any single watchdog. This general code will contain the common things (basically the misc device + reboot_notifier). The second stage (and that's what's I actually created the experimetal directory) is to go to a general watchdog driver that supports sysfs and multiple watchdog-drivers (although only one driver will use /dev/watchdog). Because of the fact that you can use multiple watchdog-driver (even multiple cards of the same driver), you need to allocate it with alloc-watchdog. The experimental code will get the notifier in it as a second step and after that the sysfs support.
I'll putt the new 2.6 code in linux-2.6-watchdog-development (but I'll be on holiday first :-) ).

I'll have a look at that. The *private fiels is for the 2.7 code with multiple watchdogs.

Hmmm, that's indeed a bug then. Will have a look at it later on.

I don't see any reason why we should get the module reference count. Could you enlighten why this would be necessary? (This should only happen when you want to have a "nowayout").

Interesting idea. Although it's better now that we keep it like it is because else the daemon that poll's /dev/watchdog will will open something that's not there and think that everything is fine and that the system is monitored (which it isn't). A second problem you have is that this daemon absolutely doesn't know when it will need to reopen /dev/watchdog (to really start the hardware watchdog) after you've loaded a "working" watchdog device driver.

Some watchdog devices/cards have seperate functions for starting their internal counters and for keeping the watchdog alive. Thus you need 2 different operations. For other cards this function is the same and thus the start and keepalive code is the same.

The reboot notifier was the next step. That's indeed common. the nowayout parameter should stay together with the watchdog driver in my opinion (because if you would use more then 1 watchdog device, you might want to set this for each device independently).

Greetings,
Wim.

Huh? Just set ->owner in file_operations and be done with that.

Yes, that would work. However, I don't feel comfortable with setting fops->owner to anything else than THIS_MODULE. In particular, this causes problems when multiple watchdog drivers register with the watchdog base module.

The sequence I had in mind was:

chrdev_open()
try_module_get(fops->owner)
watchdog_open()

      try_module_get(wdops->owner)
      wdops->start()

vfs_write
watchdog_write()

      wdops->keepalive()

...

fput()
watchdog_release()

       wdops->stop()
       module_put(wdops->owner)

module_put(fops->owner)

This would practically do the same for the watchdog layer that we already do for inside vfs for the file_operations.

	Arnd <><

ok.

Sorry, but I don't understand that argument. Drivers that only support a single device can easily do static allocation. Drivers with potentially multiple watchdogs can just do the allocation themselves with no more complexity than calling alloc_watchdogdev. The advantage is that you don't need to deal with magic pointers if you just embed the struct watchdog_device in the drivers structure, like:

struct softdog_device {
	struct timer_list ticktock;
	struct watchdog_device dev;
};

#define to_softdog(wd) container_of((wd), struct softdog_device, dev)

static int softdog_keepalive(struct watchdog_device *wd_dev)

{
	struct softdog_private *softdog_info = to_softdog(wd_dev);
	/* ... */
}
static int softdog_init(void)
{
	struct softdog_device *sd;
	sd = kmalloc(sizeof(*sd), GFP_KERNEL);
	/* ... /*
	return register_watchdogdevice(&sd->dev);
}

ok.

If you want to allow unloading the device driver, you need to prevent it from being unloaded while another thread is using it. E.g. you might be in the middle of the keepalive function while the function code is released from memory.

What I meant is that watchdog_open() would simply return -ENODEV when no watchdog is registered, which looks to the application just like no it does when the misc device is not registered. You can even do request_module("watchdog") in watchdog_open() do autoload the low-level driver.
BTW: with the current approach, the daemon won't be able to open the device at all if you are using udev, so module autoload is not possible either.

Ok. While this could of course be hidden in the device driver by keeping a private state in order to reduce interface complexity, You're probably right that reducing implementation complexity gains more here.

ok.

	Arnd <><