Currently, watchdog subsystem require the misc subsystem to
register a watchdog. This may not be the case in case of an
early registration of a watchdog, which can be required when
the watchdog cannot be disabled.
This patch use deferral mechanism to remove this requirement.
Signed-off-by: Jean-Baptiste Theou <[email protected]>
---
drivers/watchdog/watchdog_core.c | 92 ++++++++++++++++++++++++++++++++++++++--
include/linux/watchdog.h | 3 ++
2 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..edf6294 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -43,6 +43,41 @@
static DEFINE_IDA(watchdog_ida);
static struct class *watchdog_class;
+/*
+ * Deferred Registration infrastructure.
+ *
+ * Sometimes watchdog drivers need to be loaded as soon as possible,
+ * for example when it's impossible to disable it. To do so,
+ * raising the initcall level of the watchdog driver is a solution.
+ * But in such case, the miscdev is maybe not ready (subsys_initcall), and
+ * watchdog core need miscdev to be populate.
+ *
+ * The deferred registration infrastructure offer a way for the watchdog
+ * subsystem to register a watchdog properly, even before miscdev is ready
+ *
+ * This is based on driver probe deferral mechanism.
+ */
+
+static DEFINE_MUTEX(watchdog_deferred_registration_mutex);
+static LIST_HEAD(watchdog_deferred_registration_pending_list);
+static bool watchdog_deferred_registration_done;
+
+static int watchdog_deferred_registration_add(struct watchdog_device *wdd)
+{
+ dev_dbg(wdd->dev, "Added to deferred list\n");
+ list_add(&wdd->deferred_registration,
+ &watchdog_deferred_registration_pending_list);
+ return 0;
+}
+
+static void watchdog_deferred_registration_del(struct watchdog_device *wdd)
+{
+ if (!list_empty(&wdd->deferred_registration)) {
+ dev_dbg(wdd->dev, "Removed from deferred list\n");
+ list_del_init(&wdd->deferred_registration);
+ }
+}
+
static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
{
/*
@@ -99,7 +134,7 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
EXPORT_SYMBOL_GPL(watchdog_init_timeout);
/**
- * watchdog_register_device() - register a watchdog device
+ * __watchdog_register_device() - register a watchdog device
* @wdd: watchdog device
*
* Register a watchdog device with the kernel so that the
@@ -108,7 +143,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
* A zero is returned on success and a negative errno code for
* failure.
*/
-int watchdog_register_device(struct watchdog_device *wdd)
+int __watchdog_register_device(struct watchdog_device *wdd)
{
int ret, id, devno;
@@ -164,16 +199,35 @@ int watchdog_register_device(struct watchdog_device *wdd)
return 0;
}
+
+/*
+ * watchdog_register_device use either the deferred approach,
+ * or directly register the watchdog, depending on the current
+ * initcall level.
+ */
+
+int watchdog_register_device(struct watchdog_device *wdd)
+{
+ int ret;
+
+ mutex_lock(&watchdog_deferred_registration_mutex);
+ if (watchdog_deferred_registration_done)
+ ret = __watchdog_register_device(wdd);
+ else
+ ret = watchdog_deferred_registration_add(wdd);
+ mutex_unlock(&watchdog_deferred_registration_mutex);
+ return ret;
+}
EXPORT_SYMBOL_GPL(watchdog_register_device);
/**
- * watchdog_unregister_device() - unregister a watchdog device
+ * __watchdog_unregister_device() - unregister a watchdog device
* @wdd: watchdog device to unregister
*
* Unregister a watchdog device that was previously successfully
* registered with watchdog_register_device().
*/
-void watchdog_unregister_device(struct watchdog_device *wdd)
+void __watchdog_unregister_device(struct watchdog_device *wdd)
{
int ret;
int devno;
@@ -189,8 +243,37 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
ida_simple_remove(&watchdog_ida, wdd->id);
wdd->dev = NULL;
}
+
+void watchdog_unregister_device(struct watchdog_device *wdd)
+{
+ mutex_lock(&watchdog_deferred_registration_mutex);
+ if (watchdog_deferred_registration_done)
+ __watchdog_unregister_device(wdd);
+ else
+ watchdog_deferred_registration_del(wdd);
+ mutex_unlock(&watchdog_deferred_registration_mutex);
+}
+
EXPORT_SYMBOL_GPL(watchdog_unregister_device);
+/**
+ * watchdog_deferred_registration_initcall() - Register queued watchdog
+ */
+static int watchdog_deferred_registration_initcall(void)
+{
+ mutex_lock(&watchdog_deferred_registration_mutex);
+ watchdog_deferred_registration_done = true;
+ while (!list_empty(&watchdog_deferred_registration_pending_list)) {
+ struct watchdog_device *wdd = list_first_entry(&watchdog_deferred_registration_pending_list,
+ struct watchdog_device, deferred_registration);
+ list_del_init(&wdd->deferred_registration);
+ __watchdog_register_device(wdd);
+ }
+ mutex_unlock(&watchdog_deferred_registration_mutex);
+ return 0;
+}
+late_initcall(watchdog_deferred_registration_initcall);
+
static int __init watchdog_init(void)
{
int err;
@@ -222,5 +305,6 @@ module_exit(watchdog_exit);
MODULE_AUTHOR("Alan Cox <[email protected]>");
MODULE_AUTHOR("Wim Van Sebroeck <[email protected]>");
+MODULE_AUTHOR("Jean-Baptiste Theou <[email protected]>");
MODULE_DESCRIPTION("WatchDog Timer Driver Core");
MODULE_LICENSE("GPL");
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a746bf5..fcc190e 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -65,6 +65,8 @@ struct watchdog_ops {
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
* @status: Field that contains the devices internal status bits.
+ * @deferred_registration: entry in deferred_registration_list which is used to
+ * register early initialized watchdog.
*
* The watchdog_device structure contains all information about a
* watchdog timer device.
@@ -95,6 +97,7 @@ struct watchdog_device {
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
#define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
+ struct list_head deferred_registration;
};
#define WATCHDOG_NOWAYOUT IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
--
2.4.2
Add registration deferral mechanism capability information
Signed-off-by: Jean-Baptiste Theou <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..11c988b 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -36,6 +36,10 @@ The watchdog_unregister_device routine deregisters a registered watchdog timer
device. The parameter of this routine is the pointer to the registered
watchdog_device structure.
+The watchdog subsystem contain an registration deferral mechanism,
+which allow you to register an watchdog as early as you wish during
+the boot process, in the situation of a built-in module.
+
The watchdog device structure looks like this:
struct watchdog_device {
--
2.4.2
In some situation, mainly when it's not possible to disable a
watchdog, you may want the watchdog driver to be started as soon
as possible.
Adding GPIO_WATCHDOG_ARCH_INITCALL to raise initcall from
module_init to arch_initcall. This is only for a built-in
module.
This patch require watchdog registration deferral mechanism
Signed-off-by: Jean-Baptiste Theou <[email protected]>
---
drivers/watchdog/Kconfig | 14 ++++++++++++++
drivers/watchdog/gpio_wdt.c | 13 +++++++++++++
2 files changed, 27 insertions(+)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..a9655a9 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1,3 +1,4 @@
+
#
# Watchdog device configuration
#
@@ -114,6 +115,19 @@ config MENF21BMC_WATCHDOG
This driver can also be built as a module. If so the module
will be called menf21bmc_wdt.
+config GPIO_WATCHDOG_ARCH_INITCALL
+ bool "Register the watchdog as early as possible"
+ depends on GPIO_WATCHDOG
+ help
+ This option make sense only on a built-in situation.
+
+ In some situation, the default initcall level (module_init)
+ in not early enough on the boot process to avoid the watchdog
+ to be trigger.
+ If you say yes here, the initcall level would be raise to
+ arch_initcall.
+ "If in doubt, leave it out" - say N.
+
config WM831X_WATCHDOG
tristate "WM831x watchdog"
depends on MFD_WM831X
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index cbc313d..07a39cd 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -267,8 +267,21 @@ static struct platform_driver gpio_wdt_driver = {
.probe = gpio_wdt_probe,
.remove = gpio_wdt_remove,
};
+#ifndef GPIO_WATCHDOG_ARCH_INITCALL
module_platform_driver(gpio_wdt_driver);
+#else
+static int __init gpio_wdt_init(void)
+{
+ return platform_driver_register(&gpio_wdt_driver);
+}
+arch_initcall(gpio_wdt_init);
+static void __exit gpio_wdt_exit(void)
+{
+ platform_driver_unregister(&gpio_wdt_driver);
+}
+module_exit(gpio_wdt_exit);
+#endif
MODULE_AUTHOR("Alexander Shiyan <[email protected]>");
MODULE_DESCRIPTION("GPIO Watchdog");
MODULE_LICENSE("GPL");
--
2.4.2
On 06/05/2015 06:46 PM, Jean-Baptiste Theou wrote:
> Currently, watchdog subsystem require the misc subsystem to
> register a watchdog. This may not be the case in case of an
> early registration of a watchdog, which can be required when
> the watchdog cannot be disabled.
>
> This patch use deferral mechanism to remove this requirement.
... introduces a deferral ...
I think this is going into the right direction. Couple of comments below.
Guenter
>
> Signed-off-by: Jean-Baptiste Theou <[email protected]>
> ---
> drivers/watchdog/watchdog_core.c | 92 ++++++++++++++++++++++++++++++++++++++--
> include/linux/watchdog.h | 3 ++
> 2 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..edf6294 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,41 @@
> static DEFINE_IDA(watchdog_ida);
> static struct class *watchdog_class;
>
> +/*
> + * Deferred Registration infrastructure.
> + *
> + * Sometimes watchdog drivers need to be loaded as soon as possible,
> + * for example when it's impossible to disable it. To do so,
> + * raising the initcall level of the watchdog driver is a solution.
> + * But in such case, the miscdev is maybe not ready (subsys_initcall), and
> + * watchdog core need miscdev to be populate.
s/need/needs/
s/populate/populated/
watchdog_core needs miscdev to be populated for what ?
> + *
> + * The deferred registration infrastructure offer a way for the watchdog
> + * subsystem to register a watchdog properly, even before miscdev is ready
> + *
Add '.'.
> + * This is based on driver probe deferral mechanism.
I don't understand the last sentence, and I don't think it adds value.
> + */
> +
> +static DEFINE_MUTEX(watchdog_deferred_registration_mutex);
> +static LIST_HEAD(watchdog_deferred_registration_pending_list);
> +static bool watchdog_deferred_registration_done;
> +
> +static int watchdog_deferred_registration_add(struct watchdog_device *wdd)
> +{
> + dev_dbg(wdd->dev, "Added to deferred list\n");
Did you try to run this code with debugging enabled ?
Presumably not, because it should result in a NULL pointer access,
since wdd->dev is only set during registration.
This leads to the question why you have this debug message
in the first place.
> + list_add(&wdd->deferred_registration,
> + &watchdog_deferred_registration_pending_list);
list_add_tail(). We want to register watchdog devices in the original order,
not in reverse order. The first caller gets /dev/watchdog.
> + return 0;
> +}
> +
> +static void watchdog_deferred_registration_del(struct watchdog_device *wdd)
> +{
> + if (!list_empty(&wdd->deferred_registration)) {
I don't think this check is necessary, even if the member is uninitialized.
Problem here is that is watchdog_register was never called, the list members
will be NULL, list->next will be NULL, and not point to the list itself.
So you'd end up with trouble (try calling watchdog_unregister_device
before calling watchdog_register_device just for fun to see what happens).
Not really sure how to handle this. The safe approach may be to walk the list
and remove the entry (only) if it is found.
> + dev_dbg(wdd->dev, "Removed from deferred list\n");
... and another crash ;-)
> + list_del_init(&wdd->deferred_registration);
> + }
> +}
> +
> static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> {
> /*
> @@ -99,7 +134,7 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> /**
> - * watchdog_register_device() - register a watchdog device
> + * __watchdog_register_device() - register a watchdog device
> * @wdd: watchdog device
> *
> * Register a watchdog device with the kernel so that the
> @@ -108,7 +143,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
> * A zero is returned on success and a negative errno code for
> * failure.
> */
> -int watchdog_register_device(struct watchdog_device *wdd)
> +int __watchdog_register_device(struct watchdog_device *wdd)
Why do you want to make this global (but unexported) ? I think it should be static,
and the published API should remain the same.
> {
> int ret, id, devno;
>
> @@ -164,16 +199,35 @@ int watchdog_register_device(struct watchdog_device *wdd)
>
> return 0;
> }
> +
> +/*
> + * watchdog_register_device use either the deferred approach,
> + * or directly register the watchdog, depending on the current
> + * initcall level.
> + */
> +
Please move the API comment to here. The above comment doesn't really add
value unless it is part of the API documentation.
> +int watchdog_register_device(struct watchdog_device *wdd)
> +{
> + int ret;
> +
> + mutex_lock(&watchdog_deferred_registration_mutex);
> + if (watchdog_deferred_registration_done)
> + ret = __watchdog_register_device(wdd);
> + else
> + ret = watchdog_deferred_registration_add(wdd);
> + mutex_unlock(&watchdog_deferred_registration_mutex);
> + return ret;
> +}
> EXPORT_SYMBOL_GPL(watchdog_register_device);
>
> /**
> - * watchdog_unregister_device() - unregister a watchdog device
> + * __watchdog_unregister_device() - unregister a watchdog device
Same as above. This isn't the API.
> * @wdd: watchdog device to unregister
> *
> * Unregister a watchdog device that was previously successfully
> * registered with watchdog_register_device().
> */
> -void watchdog_unregister_device(struct watchdog_device *wdd)
> +void __watchdog_unregister_device(struct watchdog_device *wdd)
> {
> int ret;
> int devno;
> @@ -189,8 +243,37 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
> ida_simple_remove(&watchdog_ida, wdd->id);
> wdd->dev = NULL;
> }
> +
> +void watchdog_unregister_device(struct watchdog_device *wdd)
> +{
> + mutex_lock(&watchdog_deferred_registration_mutex);
> + if (watchdog_deferred_registration_done)
> + __watchdog_unregister_device(wdd);
> + else
> + watchdog_deferred_registration_del(wdd);
> + mutex_unlock(&watchdog_deferred_registration_mutex);
> +}
> +
> EXPORT_SYMBOL_GPL(watchdog_unregister_device);
>
> +/**
> + * watchdog_deferred_registration_initcall() - Register queued watchdog
> + */
> +static int watchdog_deferred_registration_initcall(void)
Mark with __init ?
> +{
> + mutex_lock(&watchdog_deferred_registration_mutex);
> + watchdog_deferred_registration_done = true;
> + while (!list_empty(&watchdog_deferred_registration_pending_list)) {
> + struct watchdog_device *wdd = list_first_entry(&watchdog_deferred_registration_pending_list,
> + struct watchdog_device, deferred_registration);
> + list_del_init(&wdd->deferred_registration);
> + __watchdog_register_device(wdd);
> + }
> + mutex_unlock(&watchdog_deferred_registration_mutex);
> + return 0;
> +}
> +late_initcall(watchdog_deferred_registration_initcall);
Too late. miscdev is initialized with subsys_initcall.
module_init maps to late_initcall as well, meaning many watchdog
devices would end up with deferred registration for no good reason.
I think it should be possible to just tag this onto watchdog_init,
and call watchdog_init with subsys_initcall_sync instead of
subsys_initcall.
> +
> static int __init watchdog_init(void)
> {
> int err;
> @@ -222,5 +305,6 @@ module_exit(watchdog_exit);
>
> MODULE_AUTHOR("Alan Cox <[email protected]>");
> MODULE_AUTHOR("Wim Van Sebroeck <[email protected]>");
> +MODULE_AUTHOR("Jean-Baptiste Theou <[email protected]>");
Not for a few lines of added code, please.
> MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> MODULE_LICENSE("GPL");
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index a746bf5..fcc190e 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -65,6 +65,8 @@ struct watchdog_ops {
> * @driver-data:Pointer to the drivers private data.
> * @lock: Lock for watchdog core internal use only.
> * @status: Field that contains the devices internal status bits.
> + * @deferred_registration: entry in deferred_registration_list which is used to
> + * register early initialized watchdog.
watchdogs.
> *
> * The watchdog_device structure contains all information about a
> * watchdog timer device.
> @@ -95,6 +97,7 @@ struct watchdog_device {
> #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
> #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
> #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
> + struct list_head deferred_registration;
> };
>
> #define WATCHDOG_NOWAYOUT IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
>
On 06/05/2015 06:46 PM, Jean-Baptiste Theou wrote:
> Add registration deferral mechanism capability information
>
> Signed-off-by: Jean-Baptiste Theou <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index a0438f3..11c988b 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -36,6 +36,10 @@ The watchdog_unregister_device routine deregisters a registered watchdog timer
> device. The parameter of this routine is the pointer to the registered
> watchdog_device structure.
>
> +The watchdog subsystem contain an registration deferral mechanism,
contains, or maybe better includes.
> +which allow you to register an watchdog as early as you wish during
allows
> +the boot process, in the situation of a built-in module.
Drop part after the , as irrelevant.
Please merge this patch into the first patch; no need to keep it separate.
Thanks,
Guenter
> +
> The watchdog device structure looks like this:
>
> struct watchdog_device {
>
On 06/05/2015 06:46 PM, Jean-Baptiste Theou wrote:
> In some situation, mainly when it's not possible to disable a
> watchdog, you may want the watchdog driver to be started as soon
> as possible.
>
> Adding GPIO_WATCHDOG_ARCH_INITCALL to raise initcall from
> module_init to arch_initcall. This is only for a built-in
> module.
>
> This patch require watchdog registration deferral mechanism
>
> Signed-off-by: Jean-Baptiste Theou <[email protected]>
> ---
> drivers/watchdog/Kconfig | 14 ++++++++++++++
> drivers/watchdog/gpio_wdt.c | 13 +++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..a9655a9 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1,3 +1,4 @@
> +
> #
> # Watchdog device configuration
> #
> @@ -114,6 +115,19 @@ config MENF21BMC_WATCHDOG
> This driver can also be built as a module. If so the module
> will be called menf21bmc_wdt.
>
> +config GPIO_WATCHDOG_ARCH_INITCALL
> + bool "Register the watchdog as early as possible"
> + depends on GPIO_WATCHDOG
depends on GPIO_WATCHDOG=y
> + help
> + This option make sense only on a built-in situation.
> +
and then you can drop this sentence.
> + In some situation, the default initcall level (module_init)
situations
> + in not early enough on the boot process to avoid the watchdog
in the boot process
> + to be trigger.
triggered.
> + If you say yes here, the initcall level would be raise to
will be raised to ...
> + arch_initcall.
> + "If in doubt, leave it out" - say N.
If in doubt, say N.
> +
> config WM831X_WATCHDOG
> tristate "WM831x watchdog"
> depends on MFD_WM831X
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index cbc313d..07a39cd 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -267,8 +267,21 @@ static struct platform_driver gpio_wdt_driver = {
> .probe = gpio_wdt_probe,
> .remove = gpio_wdt_remove,
> };
> +#ifndef GPIO_WATCHDOG_ARCH_INITCALL
> module_platform_driver(gpio_wdt_driver);
> +#else
> +static int __init gpio_wdt_init(void)
> +{
> + return platform_driver_register(&gpio_wdt_driver);
> +}
> +arch_initcall(gpio_wdt_init);
>
> +static void __exit gpio_wdt_exit(void)
> +{
> + platform_driver_unregister(&gpio_wdt_driver);
> +}
> +module_exit(gpio_wdt_exit);
> +#endif
If you change the dependency as suggested above, you don't need
the exit function at all, and you can simplify this code to
#ifdef GPIO_WATCHDOG_ARCH_INITCALL
arch_initcall(gpio_wdt_init);
#else
module_platform_driver(gpio_wdt_driver);
#endif
which would look much nicer.
Thanks,
Guenter
> MODULE_AUTHOR("Alexander Shiyan <[email protected]>");
> MODULE_DESCRIPTION("GPIO Watchdog");
> MODULE_LICENSE("GPL");
>
Thanks for the valuable feedbacks.
I am working on a v3.
JB
On Fri, 5 Jun 2015 21:24:52 -0700
Guenter Roeck <[email protected]> wrote:
> On 06/05/2015 06:46 PM, Jean-Baptiste Theou wrote:
> > Currently, watchdog subsystem require the misc subsystem to
> > register a watchdog. This may not be the case in case of an
> > early registration of a watchdog, which can be required when
> > the watchdog cannot be disabled.
> >
> > This patch use deferral mechanism to remove this requirement.
>
> ... introduces a deferral ...
>
> I think this is going into the right direction. Couple of comments below.
>
> Guenter
>
> >
> > Signed-off-by: Jean-Baptiste Theou <[email protected]>
> > ---
> > drivers/watchdog/watchdog_core.c | 92 ++++++++++++++++++++++++++++++++++++++--
> > include/linux/watchdog.h | 3 ++
> > 2 files changed, 91 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > index cec9b55..edf6294 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -43,6 +43,41 @@
> > static DEFINE_IDA(watchdog_ida);
> > static struct class *watchdog_class;
> >
> > +/*
> > + * Deferred Registration infrastructure.
> > + *
> > + * Sometimes watchdog drivers need to be loaded as soon as possible,
> > + * for example when it's impossible to disable it. To do so,
> > + * raising the initcall level of the watchdog driver is a solution.
> > + * But in such case, the miscdev is maybe not ready (subsys_initcall), and
> > + * watchdog core need miscdev to be populate.
>
> s/need/needs/
> s/populate/populated/
>
> watchdog_core needs miscdev to be populated for what ?
>
> > + *
> > + * The deferred registration infrastructure offer a way for the watchdog
> > + * subsystem to register a watchdog properly, even before miscdev is ready
> > + *
> Add '.'.
>
> > + * This is based on driver probe deferral mechanism.
>
> I don't understand the last sentence, and I don't think it adds value.
>
> > + */
> > +
> > +static DEFINE_MUTEX(watchdog_deferred_registration_mutex);
> > +static LIST_HEAD(watchdog_deferred_registration_pending_list);
> > +static bool watchdog_deferred_registration_done;
> > +
> > +static int watchdog_deferred_registration_add(struct watchdog_device *wdd)
> > +{
> > + dev_dbg(wdd->dev, "Added to deferred list\n");
>
> Did you try to run this code with debugging enabled ?
> Presumably not, because it should result in a NULL pointer access,
> since wdd->dev is only set during registration.
>
> This leads to the question why you have this debug message
> in the first place.
>
> > + list_add(&wdd->deferred_registration,
> > + &watchdog_deferred_registration_pending_list);
>
> list_add_tail(). We want to register watchdog devices in the original order,
> not in reverse order. The first caller gets /dev/watchdog.
>
> > + return 0;
> > +}
> > +
> > +static void watchdog_deferred_registration_del(struct watchdog_device *wdd)
> > +{
> > + if (!list_empty(&wdd->deferred_registration)) {
>
> I don't think this check is necessary, even if the member is uninitialized.
> Problem here is that is watchdog_register was never called, the list members
> will be NULL, list->next will be NULL, and not point to the list itself.
> So you'd end up with trouble (try calling watchdog_unregister_device
> before calling watchdog_register_device just for fun to see what happens).
>
> Not really sure how to handle this. The safe approach may be to walk the list
> and remove the entry (only) if it is found.
>
> > + dev_dbg(wdd->dev, "Removed from deferred list\n");
>
> ... and another crash ;-)
>
> > + list_del_init(&wdd->deferred_registration);
> > + }
> > +}
> > +
> > static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> > {
> > /*
> > @@ -99,7 +134,7 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
> > EXPORT_SYMBOL_GPL(watchdog_init_timeout);
> >
> > /**
> > - * watchdog_register_device() - register a watchdog device
> > + * __watchdog_register_device() - register a watchdog device
> > * @wdd: watchdog device
> > *
> > * Register a watchdog device with the kernel so that the
> > @@ -108,7 +143,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
> > * A zero is returned on success and a negative errno code for
> > * failure.
> > */
> > -int watchdog_register_device(struct watchdog_device *wdd)
> > +int __watchdog_register_device(struct watchdog_device *wdd)
>
> Why do you want to make this global (but unexported) ? I think it should be static,
> and the published API should remain the same.
>
> > {
> > int ret, id, devno;
> >
> > @@ -164,16 +199,35 @@ int watchdog_register_device(struct watchdog_device *wdd)
> >
> > return 0;
> > }
> > +
> > +/*
> > + * watchdog_register_device use either the deferred approach,
> > + * or directly register the watchdog, depending on the current
> > + * initcall level.
> > + */
> > +
> Please move the API comment to here. The above comment doesn't really add
> value unless it is part of the API documentation.
>
> > +int watchdog_register_device(struct watchdog_device *wdd)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&watchdog_deferred_registration_mutex);
> > + if (watchdog_deferred_registration_done)
> > + ret = __watchdog_register_device(wdd);
> > + else
> > + ret = watchdog_deferred_registration_add(wdd);
> > + mutex_unlock(&watchdog_deferred_registration_mutex);
> > + return ret;
> > +}
> > EXPORT_SYMBOL_GPL(watchdog_register_device);
> >
> > /**
> > - * watchdog_unregister_device() - unregister a watchdog device
> > + * __watchdog_unregister_device() - unregister a watchdog device
>
> Same as above. This isn't the API.
>
> > * @wdd: watchdog device to unregister
> > *
> > * Unregister a watchdog device that was previously successfully
> > * registered with watchdog_register_device().
> > */
> > -void watchdog_unregister_device(struct watchdog_device *wdd)
> > +void __watchdog_unregister_device(struct watchdog_device *wdd)
> > {
> > int ret;
> > int devno;
> > @@ -189,8 +243,37 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
> > ida_simple_remove(&watchdog_ida, wdd->id);
> > wdd->dev = NULL;
> > }
> > +
> > +void watchdog_unregister_device(struct watchdog_device *wdd)
> > +{
> > + mutex_lock(&watchdog_deferred_registration_mutex);
> > + if (watchdog_deferred_registration_done)
> > + __watchdog_unregister_device(wdd);
> > + else
> > + watchdog_deferred_registration_del(wdd);
> > + mutex_unlock(&watchdog_deferred_registration_mutex);
> > +}
> > +
> > EXPORT_SYMBOL_GPL(watchdog_unregister_device);
> >
> > +/**
> > + * watchdog_deferred_registration_initcall() - Register queued watchdog
> > + */
> > +static int watchdog_deferred_registration_initcall(void)
>
> Mark with __init ?
>
> > +{
> > + mutex_lock(&watchdog_deferred_registration_mutex);
> > + watchdog_deferred_registration_done = true;
> > + while (!list_empty(&watchdog_deferred_registration_pending_list)) {
> > + struct watchdog_device *wdd = list_first_entry(&watchdog_deferred_registration_pending_list,
> > + struct watchdog_device, deferred_registration);
> > + list_del_init(&wdd->deferred_registration);
> > + __watchdog_register_device(wdd);
> > + }
> > + mutex_unlock(&watchdog_deferred_registration_mutex);
> > + return 0;
> > +}
> > +late_initcall(watchdog_deferred_registration_initcall);
>
> Too late. miscdev is initialized with subsys_initcall.
> module_init maps to late_initcall as well, meaning many watchdog
> devices would end up with deferred registration for no good reason.
>
> I think it should be possible to just tag this onto watchdog_init,
> and call watchdog_init with subsys_initcall_sync instead of
> subsys_initcall.
>
> > +
> > static int __init watchdog_init(void)
> > {
> > int err;
> > @@ -222,5 +305,6 @@ module_exit(watchdog_exit);
> >
> > MODULE_AUTHOR("Alan Cox <[email protected]>");
> > MODULE_AUTHOR("Wim Van Sebroeck <[email protected]>");
> > +MODULE_AUTHOR("Jean-Baptiste Theou <[email protected]>");
>
> Not for a few lines of added code, please.
>
> > MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> > MODULE_LICENSE("GPL");
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index a746bf5..fcc190e 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -65,6 +65,8 @@ struct watchdog_ops {
> > * @driver-data:Pointer to the drivers private data.
> > * @lock: Lock for watchdog core internal use only.
> > * @status: Field that contains the devices internal status bits.
> > + * @deferred_registration: entry in deferred_registration_list which is used to
> > + * register early initialized watchdog.
>
> watchdogs.
>
> > *
> > * The watchdog_device structure contains all information about a
> > * watchdog timer device.
> > @@ -95,6 +97,7 @@ struct watchdog_device {
> > #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
> > #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
> > #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
> > + struct list_head deferred_registration;
> > };
> >
> > #define WATCHDOG_NOWAYOUT IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
> >
>
--
Jean-Baptiste Theou <[email protected]>