[PATCH 9/10 v2] Generic Watchdog Timer Driver

[PATCH 9/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

watchdog: WatchDog Timer Driver Core - Part 9

Add support for extra ioctl calls by adding a
ioctl watchdog operation. This operation will be
called before we do our own handling of ioctl
commands. This way we can override the internal
ioctl command handling and we can also add
extra ioctl commands. The ioctl watchdog operation
should return the appropriate error codes or
-ENOIOCTLCMD if the ioctl command should be handled
through the internal ioctl handling of the framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

diff -urN linux-2.6.38-generic-part8/Documentation/watchdog/watchdog-kernel-api.txt linux-2.6.38-generic-part9/Documentation/watchdog/watchdog-kernel-api.txt
--- linux-2.6.38-generic-part8/Documentation/watchdog/watchdog-kernel-api.txt	2011-06-17 10:03:41.326632851 +0200
+++ linux-2.6.38-generic-part9/Documentation/watchdog/watchdog-kernel-api.txt	2011-06-17 10:14:55.514632632 +0200
@@ -74,6 +74,7 @@
 	int (*ping)(struct watchdog_device *);
 	int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -119,6 +120,10 @@
   was just used to re-program the watchdog timer device.
   (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
   watchdog's info structure).
+* ioctl: if this routine is present then it will be called first before we do
+  our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
+  if a command is not supported. The parameters that are passsed to the ioctl
+  call are: watchdog_device, cmd and arg.
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bit's that are defined are:
diff -urN linux-2.6.38-generic-part8/drivers/watchdog/core/watchdog_dev.c linux-2.6.38-generic-part9/drivers/watchdog/core/watchdog_dev.c
--- linux-2.6.38-generic-part8/drivers/watchdog/core/watchdog_dev.c	2011-06-17 10:03:41.326632851 +0200
+++ linux-2.6.38-generic-part9/drivers/watchdog/core/watchdog_dev.c	2011-06-17 10:14:55.522632632 +0200
@@ -209,6 +209,12 @@
 
 	trace("%p, %u, %li", file, cmd, arg);
 
+	if (wdd->ops->ioctl) {
+		err = wdd->ops->ioctl(wdd, cmd, arg);
+		if (err != -ENOIOCTLCMD)
+			return err;
+	}
+
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		return copy_to_user(argp, wdd->info,
diff -urN linux-2.6.38-generic-part8/include/linux/watchdog.h linux-2.6.38-generic-part9/include/linux/watchdog.h
--- linux-2.6.38-generic-part8/include/linux/watchdog.h	2011-06-17 12:17:52.893063162 +0200
+++ linux-2.6.38-generic-part9/include/linux/watchdog.h	2011-06-17 12:18:05.261063514 +0200
@@ -72,6 +72,7 @@
 	int (*ping)(struct watchdog_device *);
 	int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 /* The structure that defines a watchdog device */

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Saturday 18 June 2011 19:27:27 Wim Van Sebroeck wrote:
> 
> watchdog: WatchDog Timer Driver Core - Part 9
> 
> Add support for extra ioctl calls by adding a
> ioctl watchdog operation. This operation will be
> called before we do our own handling of ioctl
> commands. This way we can override the internal
> ioctl command handling and we can also add
> extra ioctl commands. The ioctl watchdog operation
> should return the appropriate error codes or
> -ENOIOCTLCMD if the ioctl command should be handled
> through the internal ioctl handling of the framework.
> 
> Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

Hmm, I'm not sure about this one. It does make the conversion
of existing drivers easier but doesn't encourage doing a good
job there.

How about instead providing a compatibility helper module that
provides helper functions like this:

int watchdog_compat_getstatus(struct watchdog_device *wdd)
{
	int __user *tmp = compat_alloc_userspace(sizeof (int));
	int ret;

	ret = wdd->ops->ioctl(wdd, WDIOC_GETSTATUS, tmp);
	if (ret)
		return ret;

	__get_user(ret, tmp);
	return ret;
}
EXPORT_SYMBOL_GPL(watchdog_compat_getstatus);

This would let you use the common ioctl implementation for all
watchdogs without the option of an individual driver overriding
it, but a driver could still provide an ioctl method that only
gets called by the watchdog_compat_* functions.

	Arnd

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Alan Cox]

> Hmm, I'm not sure about this one. It does make the conversion
> of existing drivers easier but doesn't encourage doing a good
> job there.

It's not ultimately about conversion. A lot of watchdogs have interfaces
that are not quite generic, extra requirements and custom interfaces. The
ioctl interface is needed for that, not for conversions.

Alan

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Sunday 19 June 2011 12:01:41 Alan Cox wrote:
> > Hmm, I'm not sure about this one. It does make the conversion
> > of existing drivers easier but doesn't encourage doing a good
> > job there.
> 
> It's not ultimately about conversion. A lot of watchdogs have interfaces
> that are not quite generic, extra requirements and custom interfaces. The
> ioctl interface is needed for that, not for conversions.

It's probably fine to have additional custom interfaces in some watchdogs
where needed, but do you think we also need to give drivers a way to
override the common ones?

If a driver has a different interpretation of a common command, I would
consider that a bug, not a driver specific extension. I would also
like to see the proper extensions easily visible, so we can create
new common interfaces in cases where multiple drivers have the same
requirement.

	Arnd

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Alan Cox]

> If a driver has a different interpretation of a common command, I would
> consider that a bug, not a driver specific extension. I would also

And in the real world not every device fits the framework pefectly. It's
not necessarily about interpretation but about flexibility of
implementation.

Alan

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Arnd,

> > watchdog: WatchDog Timer Driver Core - Part 9
> > 
> > Add support for extra ioctl calls by adding a
> > ioctl watchdog operation. This operation will be
> > called before we do our own handling of ioctl
> > commands. This way we can override the internal
> > ioctl command handling and we can also add
> > extra ioctl commands. The ioctl watchdog operation
> > should return the appropriate error codes or
> > -ENOIOCTLCMD if the ioctl command should be handled
> > through the internal ioctl handling of the framework.
> > 
> > Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
> 
> Hmm, I'm not sure about this one. It does make the conversion
> of existing drivers easier but doesn't encourage doing a good
> job there.

That's why we also have a maintainer and a mailing list to review
patches... I can understand your concerns, but prefer the flexibility
and simpleness here.

> How about instead providing a compatibility helper module that
> provides helper functions like this:
> 
> int watchdog_compat_getstatus(struct watchdog_device *wdd)
> {
> 	int __user *tmp = compat_alloc_userspace(sizeof (int));
> 	int ret;
> 
> 	ret = wdd->ops->ioctl(wdd, WDIOC_GETSTATUS, tmp);
> 	if (ret)
> 		return ret;
> 
> 	__get_user(ret, tmp);
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(watchdog_compat_getstatus);
> 
> This would let you use the common ioctl implementation for all
> watchdogs without the option of an individual driver overriding
> it, but a driver could still provide an ioctl method that only
> gets called by the watchdog_compat_* functions.

Hmmm, not really in favour of that, this will creat a lot more overhead.

Kind regards,
Wim.

Re: [PATCH 9/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Wednesday 22 June 2011 22:05:14 Wim Van Sebroeck wrote:
> That's why we also have a maintainer and a mailing list to review
> patches... I can understand your concerns, but prefer the flexibility
> and simpleness here.

Ok, fine with me then.

Acked-by: Arnd Bergmann <arnd@arndb.de>