watchdog infrastructure

watchdog infrastructure

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

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 <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: watchdog infrastructure

[Wim Van Sebroeck]

Hi Arnd,

> 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?

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 :-).

> 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.

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.

Re: watchdog infrastructure

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Montag, 5. Juli 2004 11:38, you wrote:
> Did you have a look allready at the different watchdog operations in 
> include/linux/watchdog.h ?

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 <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: watchdog infrastructure

[Wim Van Sebroeck]

Hello Arnd,

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

> > Did you have a look allready at the different watchdog operations in 
> > include/linux/watchdog.h ?
> 
> 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.

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 :-) ).

> - 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.

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

> - watchdog_is_open_sem can just be an atomic_t, you never
>   actually down() it.

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

> - You need to get the module reference count before calling any
>   watchdog operation, the best place for this is probably the
>   open() fop.

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").

> - 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.

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.

> - Why do you need seperate operations for start and keepalive?

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 and the nowayout parameter are probably
>   common enough to be put into the generic module.

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.

Re: watchdog infrastructure

[viro]

On Mon, Jul 12, 2004 at 10:19:39AM +0200, Wim Van Sebroeck wrote:
> > - You need to get the module reference count before calling any
> >   watchdog operation, the best place for this is probably the
> >   open() fop.

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

Re: watchdog infrastructure

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Montag, 12. Juli 2004 10:23, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Mon, Jul 12, 2004 at 10:19:39AM +0200, Wim Van Sebroeck wrote:
> > > - You need to get the module reference count before calling any
> > >   watchdog operation, the best place for this is probably the
> > >   open() fop.
> 
> 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 <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: watchdog infrastructure

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 4653 bytes --]

On Montag, 12. Juli 2004 10:19, Wim Van Sebroeck wrote:

> > - 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.
> 
> 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).
ok.

> 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.
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);
}

> > - 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.
> 
> I'll have a look at that. The *private fiels is for the 2.7 code
> with multiple watchdogs.
ok.

> 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").

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.
 
> > - 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.
> 
> 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.

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.

> > - Why do you need seperate operations for start and keepalive?
> 
> 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.

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.

> > - the reboot notifier and the nowayout parameter are probably
> >   common enough to be put into the generic module.
> 
> 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).

ok.

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]