| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150605094830.1e928932f4f68da6045573c3@adeneo-embedded.us>
Date: Fri, 5 Jun 2015 09:48:30 -0700
From: Jean-Baptiste Theou <jtheou@...neo-embedded.us>
To: Guenter Roeck <linux@...ck-us.net>
CC: Wim Van Sebroeck <wim@...ana.be>, <linux-watchdog@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] gpio_wdt: change initcall level
Agreed. Thanks for the feedbacks. I am working an a new patch.
Best regards,
On Fri, 5 Jun 2015 01:09:20 -0700
Guenter Roeck <linux@...ck-us.net> wrote:
> On 06/04/2015 11:05 PM, Jean-Baptiste Theou wrote:
> > Hi Guenter,
> >
> > I based my work on the work done in mpc8xxx_wdt.c, which is in mainline.
> >
> Yes, you mentioned that before. However, that is a driver for a specific
> chip, not a generic driver like this one. That driver is only messy for
> one chip. Your driver. on the other side, changes the logic of a generic
> driver, and I am not even sure if it would work as implemented. One key
> logical change is that if the driver is built into the kernel, it will
> not report an error if watchdog_register_device fails. That may be
> acceptable for a dedicated driver, but I don't think it is acceptable
> to impose this onto a generic driver. Also, I suspect that if it ever
> happens that there is more than one such watchdog in the system, and
> the driver is built as module, it would fail all over the place and try
> to register each driver instance multiple times.
>
> Plus, you call both gpio_wdt_init_late and gpio_wdt_remove_late before
> the functions are declared, and I don't see any forward declarations.
> That suggests that you will see at least warnings if not errors
> if you compile the driver as module, which in turn doesn't make me feel
> very comfortable about the level of compile testing you have done,
> much less runtime testing.
>
> A more generic solution might be to improve watchdog_register_device
> and let it handle the situation where a driver registers early
> (for example by queuing the request to register the misc device
> if that is the problem).
>
> Guenter
>
> > The point of my patch is for a built-in scenario.
> > I have an external chip who controls the watchdog, and it need to have
> > it IN pin toggle within 1.6s, otherwise it trigger the watchdog.
> >
> > With a default gpio_wdt built-in module, module_init initcall level is
> > too late, and the board reboot (the watchdog cannot be disabled, I am
> > using "always-running" property of this module.)
> >
> > The point of my patch is to start the watchdog at arch_init call level,
> > and the "tweak" for late init is due to the fact that miscdev is not
> > ready at the level of initcall, as explained on the comment.
> >
> > If there is some part that aren't clear and if you have a better idea
> > on how to raise the level of initcall for this module, on a cleaner
> > way, I am all hears.
> >
> > Best regards,
> >
> > On Thu, 4 Jun 2015 21:37:03 -0700
> > Guenter Roeck <linux@...ck-us.net> wrote:
> >
> >> On 06/04/2015 12:21 PM, Jean-Baptiste Theou wrote:
> >>> gpio_wdt may need to start the GPIO toggle as soon as possible,
> >>> when the watchdog cannot be disabled. Raise the initcall to
> >>> arch_initcall.
> >>>
> >>> We need to split the initiation, because of miscdev, as done in
> >>> mpc8xxx_wdt.c
> >>>
> >>> Signed-off-by: Jean-Baptiste Theou <jtheou@...neo-embedded.us>
> >>> ---
> >>> drivers/watchdog/gpio_wdt.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
> >>> 1 file changed, 74 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> >>> index cbc313d..8ecfe7e 100644
> >>> --- a/drivers/watchdog/gpio_wdt.c
> >>> +++ b/drivers/watchdog/gpio_wdt.c
> >>> @@ -14,6 +14,7 @@
> >>> #include <linux/module.h>
> >>> #include <linux/notifier.h>
> >>> #include <linux/of_gpio.h>
> >>> +#include <linux/of_platform.h>
> >>> #include <linux/platform_device.h>
> >>> #include <linux/reboot.h>
> >>> #include <linux/watchdog.h>
> >>> @@ -223,10 +224,11 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>>
> >>> setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
> >>>
> >>> - ret = watchdog_register_device(&priv->wdd);
> >>> +#ifdef MODULE
> >>> + ret = gpio_wdt_init_late();
> >>> if (ret)
> >>> return ret;
> >>> -
> >>> +#endif
> >>> priv->notifier.notifier_call = gpio_wdt_notify_sys;
> >>> ret = register_reboot_notifier(&priv->notifier);
> >>> if (ret)
> >>> @@ -235,10 +237,13 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>> if (priv->always_running)
> >>> gpio_wdt_start_impl(priv);
> >>>
> >>> + platform_set_drvdata(pdev, priv);
> >>> return 0;
> >>>
> >>> error_unregister:
> >>> - watchdog_unregister_device(&priv->wdd);
> >>> +#ifdef MODULE
> >>> + ret = gpio_wdt_remove_late(&priv->wdd);
> >>> +#endif
> >>> return ret;
> >>> }
> >>>
> >>> @@ -267,7 +272,72 @@ static struct platform_driver gpio_wdt_driver = {
> >>> .probe = gpio_wdt_probe,
> >>> .remove = gpio_wdt_remove,
> >>> };
> >>> -module_platform_driver(gpio_wdt_driver);
> >>> +
> >>> +/*
> >>> + * We do wdt initialization in two steps: arch_initcall probes the wdt
> >>> + * very early to start pinging the watchdog (misc devices are not yet
> >>> + * available), and later module_init() just registers the misc device.
> >>> + */
> >>> +static int gpio_wdt_init_late(void)
> >>> +{
> >>> + struct platform_device *pdev;
> >>> + struct device_node *wdt_node;
> >>> + struct gpio_wdt_priv *priv;
> >>> + int ret;
> >>> +
> >>> + for_each_compatible_node(wdt_node, NULL, "linux,wdt-gpio") {
> >>> + pdev = of_find_device_by_node(wdt_node);
> >>> + priv = platform_get_drvdata(pdev);
> >>> + if (&priv->wdd) {
> >>> + ret = watchdog_register_device(&priv->wdd);
> >>> + if (ret)
> >>> + return ret;
> >>> + } else {
> >>> + dev_err(&pdev->dev, "Unable to register the watchdog\n");
> >>> + return -1;
> >>> + }
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +#ifndef MODULE
> >>> +module_init(gpio_wdt_init_late);
> >>> +#endif
> >>> +
> >>> +#ifdef MODULE
> >>> +int gpio_wdt_remove_late(void)
> >>> +{
> >>> + struct platform_device *pdev;
> >>> + struct device_node *wdt_node;
> >>> + struct gpio_wdt_priv *priv;
> >>> + int ret;
> >>> +
> >>> + for_each_compatible_node(wdt_node, NULL, "linux,wdt-gpio") {
> >>> + pdev = of_find_device_by_node(wdt_node);
> >>> + priv = platform_get_drvdata(pdev);
> >>> + if (&priv->wdd) {
> >>> + ret = watchdog_unregister_device(&priv->wdd);
> >>> + if (ret)
> >>> + return ret;
> >>> + } else {
> >>> + dev_err(&pdev->dev, "Unable to register the watchdog\n");
> >>> + return -1;
> >>> + }
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +#endif
> >>> +
> >>> +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);
> >>>
> >>> MODULE_AUTHOR("Alexander Shiyan <shc_work@...l.ru>");
> >>> MODULE_DESCRIPTION("GPIO Watchdog");
> >>>
> >>
> >> This looks really messy, you don't explain why you think you need
> >> gpio_wdt_remove_late, and I do wonder if there are compile warnings
> >> when this is compiled as module.
> >>
> >> If, in a given system, initialization can not wait until modules are loaded,
> >> maybe it makes more sense to build the driver into the kernel instead of
> >> introducing all this mess. If built into the kernel the latency should
> >> not be that bad that this is really needed.
> >>
> >> Guenter
> >>
> >
> >
>
--
Jean-Baptiste Theou <jtheou@...neo-embedded.us>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists