| Message ID | 1400186304-1691-2-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +void watchdog_do_reboot(void) > +{ > + if (wdd_reboot_dev) > + wdd_reboot_dev->ops->reboot(wdd_reboot_dev); > +} > +EXPORT_SYMBOL(watchdog_do_reboot); Crashes and burns if you are unloading a watchdog just as you try to reboot. Yes its wildly unlikely but it's still conceptually wrong. > > + if (wdd->ops->reboot) > + wdd_reboot_dev = wdd; > + Two parallel registers from different bus types, parallel register/unregister ? This feels to me like a backward step. We've gone from various device bits leaking into the core code (where they can work all the time) to various core code leaking into the devices which is asking for init order problems and other races. Given we are talking about stuff of the order of 10-20 instructions I think duplication is not only the lesser evil it's also smaller, more reliable and easier to maintain. IMHO this is a solution looking for a problem. Alan
On Thu, May 15, 2014 at 09:50:20PM +0100, One Thousand Gnomes wrote: > > +void watchdog_do_reboot(void) > > +{ > > + if (wdd_reboot_dev) > > + wdd_reboot_dev->ops->reboot(wdd_reboot_dev); > > +} > > +EXPORT_SYMBOL(watchdog_do_reboot); > > Crashes and burns if you are unloading a watchdog just as you try to > reboot. Yes its wildly unlikely but it's still conceptually wrong. > Possibly, but how is it different to the code it replaces ? > > > > + if (wdd->ops->reboot) > > + wdd_reboot_dev = wdd; > > + > > Two parallel registers from different bus types, parallel > register/unregister ? > Sorry, you lost me. What different bus types ? > This feels to me like a backward step. We've gone from various device > bits leaking into the core code (where they can work all the time) to > various core code leaking into the devices which is asking for init order > problems and other races. > > Given we are talking about stuff of the order of 10-20 instructions I > think duplication is not only the lesser evil it's also smaller, more > reliable and easier to maintain. > > IMHO this is a solution looking for a problem. > Really ? To me it seems to be much cleaner than setting the pointer to arm_pm_restart directly from individual watchdog drivers. Also, and I was told that other HW may benefit from it as well. Do I understand it correctly that you prefer watchdog drivers to set arm_pm_restart directly ? Maybe you can explain a bit why you believe that to be a superior solution. In addition to that, while I could obviously add some locking around access to wdd_reboot_dev, existing code doesn't lock any changes to arm_pm_restart. I am somewhat at loss why setting or clearing arm_pm_restart is less of a problem that setting wdd_reboot_dev. Thanks, Guenter
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..24ce780 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,6 +43,15 @@ static DEFINE_IDA(watchdog_ida); static struct class *watchdog_class; +static struct watchdog_device *wdd_reboot_dev; + +void watchdog_do_reboot(void) +{ + if (wdd_reboot_dev) + wdd_reboot_dev->ops->reboot(wdd_reboot_dev); +} +EXPORT_SYMBOL(watchdog_do_reboot); + static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) { /* @@ -162,6 +171,9 @@ int watchdog_register_device(struct watchdog_device *wdd) return ret; } + if (wdd->ops->reboot) + wdd_reboot_dev = wdd; + return 0; } EXPORT_SYMBOL_GPL(watchdog_register_device); @@ -181,6 +193,9 @@ void watchdog_unregister_device(struct watchdog_device *wdd) if (wdd == NULL) return; + if (wdd == wdd_reboot_dev) + wdd_reboot_dev = NULL; + devno = wdd->cdev.dev; ret = watchdog_dev_unregister(wdd); if (ret) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 2a3038e..1e9da0a 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -23,6 +23,7 @@ struct watchdog_device; * @start: The routine for starting the watchdog device. * @stop: The routine for stopping the watchdog device. * @ping: The routine that sends a keepalive ping to the watchdog device. + * @reboot: The routine for rebooting the system * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. * @get_timeleft:The routine that get's the time that's left before a reset. @@ -42,6 +43,7 @@ struct watchdog_ops { int (*stop)(struct watchdog_device *); /* optional operations */ int (*ping)(struct watchdog_device *); + void (*reboot)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); @@ -142,4 +144,10 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *); +#ifdef CONFIG_WATCHDOG_CORE +extern void watchdog_do_reboot(void); +#else +static inline void watchdog_do_reboot(void) { } +#endif + #endif /* ifndef _LINUX_WATCHDOG_H */