Re: [PATCH 1/1] gpio_wdt: change initcall level

[Jean-Baptiste Theou]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>    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@xxxxxxx>");
> >>>    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@xxxxxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/