wdt, gpio: move arch_initcall into subsys_initcall ?

DISCUSSION Heiko Schocher 3 participants 6 messages Nov 15, 2016 – Nov 17, 2016

Subsystems (2)

GPIO Watchdog device drivers

lore

Share & tools

lore

Heiko Schocher

author

2016-11-15 10:20 UTC #0 lore raw

Hello,

commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level" moves the gpio initialization of the mxc gpio driver
from the arch_initcall level into subsys_initcall level.

This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.

I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).

Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.

So there are 2 solutions possible:

- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?

But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?

- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)

But this may results in problems on boards, which needs an early trigger on an gpio wdt ...

Other ideas?

Thanks for all suggestions!

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Vladimir Zapolskiy

2016-11-15 11:10 UTC #1 lore raw

Hello Heiko,

Quoted 13 lines · Heiko Schocher

Hello,
commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level" moves the gpio initialization of the mxc gpio driver
from the arch_initcall level into subsys_initcall level.
This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.
I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).

Quoted 8 lines

Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.
So there are 2 solutions possible:
- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?

in my opinion this is overly heavy solution and it might be better to avoid it if possible.

I would rather prefer to reconsider GPIO_WATCHDOG_ARCH_INITCALL usage in the watchdog driver.

Moreover adding this proposed GPIO_MCX_ARCH_INITCALL to call the driver on arch level will result in deferring the GPIO driver.

Quoted 8 lines

But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?
- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)
But this may results in problems on boards, which needs an early trigger on an gpio wdt ...

The level of "earliness" can not be defined in absolute time value in any case, why decreasing the init level of the watchdog driver to subsys level can cause problems? For that there should exist some kind of a dependency on IC or PCB hardware level, can you name it please?

Also please note that more than a half of all GPIO drivers settle on subsys or later initcall level, this means that there is an expected GPIO watchdog driver deferral for all of them.

I propose to send two patches for review:

1. remove GPIO_WATCHDOG_ARCH_INITCALL option completely and decouple module_platform_driver() into arch_initcall() and module_exit() unconditionally.

2. change arch_initcall() in the watchdog driver to subsys_initcall(). This change removes probe deferrals on boot, when the driver is used with the most of the GPIO controllers.

Quoted 4 lines

Other ideas?
Thanks for all suggestions!

--
With best wishes,
Vladimir

Vladimir Zapolskiy

2016-11-15 11:32 UTC #2 lore raw

Quoted 49 lines · Vladimir Zapolskiy

Hello Heiko,
Quoted 13 lines · Heiko Schocher
Hello,
commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level" moves the gpio initialization of the mxc gpio driver
from the arch_initcall level into subsys_initcall level.
This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.
I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).
Quoted 8 lines
Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.
So there are 2 solutions possible:
- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?
in my opinion this is overly heavy solution and it might be better to avoid it if possible.
I would rather prefer to reconsider GPIO_WATCHDOG_ARCH_INITCALL usage in the watchdog driver.
Moreover adding this proposed GPIO_MCX_ARCH_INITCALL to call the driver on arch level will result in deferring the GPIO driver.
Quoted 8 lines
But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?
- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)
But this may results in problems on boards, which needs an early trigger on an gpio wdt ...
The level of "earliness" can not be defined in absolute time value in any case, why decreasing the init level of the watchdog driver to subsys level can cause problems? For that there should exist some kind of a dependency on IC or PCB hardware level, can you name it please?

Quoted 3 lines

Also please note that more than a half of all GPIO drivers settle on subsys or later initcall level, this means that there is an expected GPIO watchdog driver deferral for all of them.

Please find two more late notes though.

Quoted 9 lines

I propose to send two patches for review:
1. remove GPIO_WATCHDOG_ARCH_INITCALL option completely and decouple module_platform_driver() into arch_initcall() and module_exit() unconditionally.
2. change arch_initcall() in the watchdog driver to subsys_initcall(). This change removes probe deferrals on boot, when the driver is used with the most of the GPIO controllers.

Alternatively commit 5e53c8ed813d ("watchdog: gpio_wdt: Add option for early registration") can be reverted and then module_platform_driver() is decoupled into subsys_initcall() and module_exit() as its replacement.

And also please note that since quite many GPIO controller drivers live on initcall levels after subsys_initcall(), the solution won't let to avoid watchdog driver deferrals totally, this should be accepted.

Signature · 2 lines

With best wishes,
Vladimir

Guenter Roeck

maintainer

2016-11-15 13:46 UTC #3 lore raw

Quoted 62 lines · Vladimir Zapolskiy

Quoted 49 lines · Vladimir Zapolskiy
Hello Heiko,
Quoted 13 lines · Heiko Schocher
Hello,
commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level" moves the gpio initialization of the mxc gpio driver
from the arch_initcall level into subsys_initcall level.
This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.
I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).
Quoted 8 lines
Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.
So there are 2 solutions possible:
- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?
in my opinion this is overly heavy solution and it might be better to avoid it if possible.
I would rather prefer to reconsider GPIO_WATCHDOG_ARCH_INITCALL usage in the watchdog driver.
Moreover adding this proposed GPIO_MCX_ARCH_INITCALL to call the driver on arch level will result in deferring the GPIO driver.
Quoted 8 lines
But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?
- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)
But this may results in problems on boards, which needs an early trigger on an gpio wdt ...
The level of "earliness" can not be defined in absolute time value in any case, why decreasing the init level of the watchdog driver to subsys level can cause problems? For that there should exist some kind of a dependency on IC or PCB hardware level, can you name it please?
Quoted 3 lines
Also please note that more than a half of all GPIO drivers settle on subsys or later initcall level, this means that there is an expected GPIO watchdog driver deferral for all of them.
Please find two more late notes though.
Quoted 6 lines
I propose to send two patches for review:
1. remove GPIO_WATCHDOG_ARCH_INITCALL option completely and decouple module_platform_driver() into arch_initcall() and module_exit() unconditionally.

Quoted 8 lines

Quoted 3 lines
2. change arch_initcall() in the watchdog driver to subsys_initcall(). This change removes probe deferrals on boot, when the driver is used with the most of the GPIO controllers.
Alternatively commit 5e53c8ed813d ("watchdog: gpio_wdt: Add option for early registration") can be reverted and then module_platform_driver() is decoupled into subsys_initcall() and module_exit() as its replacement.

Sure, only the reason for that was that there are situations where subsys_initcall() was too late. Also, when using arch_initcall() only, we get deferrals again, which is apparently hated by many and a reason for all those "avoid probe deferrals" patches.

Quoted 4 lines

And also please note that since quite many GPIO controller drivers live on initcall levels after subsys_initcall(), the solution won't let to avoid watchdog driver deferrals totally, this should be accepted.

... except for others it isn't, and we are back to square one.

GPIO_WATCHDOG_ARCH_INITCALL was intended to be only used in situations where needed. Why is it used here in the first place if that is not the case ?

Guenter

Heiko Schocher

author

2016-11-17 07:08 UTC #4 lore raw

Hello Guenter, Vladimir,

Sorry for the late response, but I was "on the road" ...

Am 15.11.2016 um 14:46 schrieb Guenter Roeck:

Quoted 42 lines · Vladimir Zapolskiy

Quoted 41 lines · Vladimir Zapolskiy
Quoted 40 lines
Hello Heiko,
Quoted 13 lines · Heiko Schocher
Hello,
commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level" moves the gpio initialization of the mxc gpio driver
from the arch_initcall level into subsys_initcall level.
This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.
I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).
Quoted 8 lines
Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.
So there are 2 solutions possible:
- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?
in my opinion this is overly heavy solution and it might be better to avoid it if possible.
I would rather prefer to reconsider GPIO_WATCHDOG_ARCH_INITCALL usage in the watchdog driver.
Moreover adding this proposed GPIO_MCX_ARCH_INITCALL to call the driver on arch level will result in deferring the GPIO driver.
Quoted 6 lines
But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?
- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
But this may results in problems on boards, which needs an early trigger on an gpio wdt ...
The level of "earliness" can not be defined in absolute time value in any case, why decreasing the init level of the watchdog driver to subsys level can cause problems? For that there should exist some kind of a dependency on IC or PCB hardware level, can you name it please?

On the current problem, there is no dependency on PCB, but I know of watchdogs triggered through a gpio pin, which must triggered < 1 second and subsys_initcall is too late for this. I think, this was the reason for introducing the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option ...

Quoted 16 lines

Quoted 16 lines
Quoted 3 lines
Also please note that more than a half of all GPIO drivers settle on subsys or later initcall level, this means that there is an expected GPIO watchdog driver deferral for all of them.
Please find two more late notes though.
Quoted 9 lines
I propose to send two patches for review:
1. remove GPIO_WATCHDOG_ARCH_INITCALL option completely and decouple module_platform_driver() into arch_initcall() and module_exit() unconditionally.
2. change arch_initcall() in the watchdog driver to subsys_initcall(). This change removes probe deferrals on boot, when the driver is used with the most of the GPIO controllers.

Quoted 8 lines

Quoted 4 lines
Alternatively commit 5e53c8ed813d ("watchdog: gpio_wdt: Add option for early registration") can be reverted and then module_platform_driver() is decoupled into subsys_initcall() and module_exit() as its replacement.
Sure, only the reason for that was that there are situations where subsys_initcall() was too late. Also, when using arch_initcall() only, we get deferrals again, which is apparently hated by many and a reason for all those "avoid probe deferrals" patches.

Exactly. And I wonder, if this boards, who need this early trigger, work with current kernel ? (Or this boards use a gpio driver, which is not in subsys_initcall level ...)

Quoted 9 lines

Quoted 4 lines
And also please note that since quite many GPIO controller drivers live on initcall levels after subsys_initcall(), the solution won't let to avoid watchdog driver deferrals totally, this should be accepted.
... except for others it isn't, and we are back to square one.
GPIO_WATCHDOG_ARCH_INITCALL was intended to be only used in situations where needed. Why is it used here in the first place if that is not the case ?

Heh, good question ...

"/dev/watchdog" is in systemd hard-coded, see:
https://github.com/systemd/systemd/blob/dd8352659c9428b196706d04399eec106a8917ed/src/shared/watchdog.c#L86
http://0pointer.de/blog/projects/watchdog.html
https://www.freedesktop.org/software/systemd/man/systemd-system.conf.html

We have 2 WDT driver enabled on the board (GPIO WDT and the DA9063 WDT).

If both WDTs are in the subsys_initall level, first the da9063 wdt gets probed, and so "/dev/watchdog" points to the da9063 wdt, but we want to use the GPIO wdt as "/dev/watchdog" device.

Now, why not simple disabling the da9063 wdt ?

We could not disable the da9063 wdt functionallity, because we need for a clean reboot that the da9063_wdt_restart() is called in the restart sequence ... which is registered in the wdt driver:

static const struct watchdog_ops da9063_watchdog_ops = { [...]

         .restart = da9063_wdt_restart,
};

Is there a possbility to register this restart function may somewhere else? It seems, that this is not a WDT functionallity ...

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Vladimir Zapolskiy

2016-11-17 09:31 UTC #5 lore raw

Hello Heiko,

Quoted 57 lines · Heiko Schocher

Hello Guenter, Vladimir,
Sorry for the late response, but I was "on the road" ...
Am 15.11.2016 um 14:46 schrieb Guenter Roeck:
Quoted 43 lines · Vladimir Zapolskiy
Quoted 42 lines · Vladimir Zapolskiy
Quoted 41 lines
Hello Heiko,
Quoted 14 lines · Heiko Schocher
Hello,
commit e188cbf7564f: "gpio: mxc: shift gpio_mxc_init() to subsys_initcall level"
moves the gpio initialization of the mxc gpio driver from the arch_initcall level into subsys_initcall level.
This leads now on mxc boards, which use a gpio wdt driver and the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option enabled, to unwanted driver probe deferrals during kernel boot.
I see this currently on an imx6 based board (which has unfortunately 3 WDT: imx6 internal (disabled), gpio wdt and da9063 WDT ...).
Quoted 8 lines
Also a side effect from above commit is, that the da9063 WDT driver is now probed before the gpio WDT driver ... so /dev/watchdog now does not point to the gpio_wdt, instead it points to the da9063 WDT.
So there are 2 solutions possible:
- add a CONFIG_GPIO_MCX_ARCH_INITCALL option
in drivers/gpio/gpio-mxc.c like for the gpio_wdt.c driver?
in my opinion this is overly heavy solution and it might be better to avoid it if possible.
I would rather prefer to reconsider GPIO_WATCHDOG_ARCH_INITCALL usage in the watchdog driver.
Moreover adding this proposed GPIO_MCX_ARCH_INITCALL to call the driver on arch level will result in deferring the GPIO driver.
Quoted 6 lines
But how can we guarantee, that first the gpio driver and then the gpio_wdt driver gets probed?
- move the arch_initcall in gpio_wdt.c into a subsys_initcall (Tested this, and the probe dereferral messages are gone ...)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
But this may results in problems on boards, which needs an early trigger on an gpio wdt ...
The level of "earliness" can not be defined in absolute time value in any case, why decreasing the init level of the watchdog driver to subsys level can cause problems? For that there should exist some kind of a dependency on IC or PCB hardware level, can you name it please?

Quoted 3 lines

On the current problem, there is no dependency on PCB, but I know of watchdogs triggered through a gpio pin, which must triggered < 1 second and subsys_initcall is too late for this. I think, this was the reason

This is not a valid reason, nobody guarantees that arch_initcall() code is run in < 1 second time interval.

Quoted 2 lines

for introducing the CONFIG_GPIO_WATCHDOG_ARCH_INITCALL option ...

Putting GPIO driver on arch_initcall() level results in deferring of the GPIO driver. Because you desire to avoid probe deferring, you should not consider putting GPIO driver on arch_initcall() level.

--
With best wishes,
Vladimir