| Message ID | 20191029174037.25381-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/5] watchdog: imx7ulp: Fix reboot hang | expand |
On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote: > The following hang is observed when a 'reboot' command is issued: > > # reboot > # Stopping network: OK > Stopping klogd: OK > Stopping syslogd: OK > umount: devtmpfs busy - remounted read-only > [ 8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null) > The system is going down NOW! > Sent SIGTERM to all processes > Sent SIGKILL to all processes > Requesting system reboot > [ 10.694753] reboot: Restarting system > [ 11.699008] Reboot failed -- System halted > > Fix this problem by adding a .restart ops member. > > Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support") > Signed-off-by: Fabio Estevam <festevam@gmail.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> However, just to be sure: This registers the watchdog based restart handler as restart handler of last resort. I assume this on purpose, I just want to make sure it is intentional since it is not explicitly mentioned in the commit message. Thanks, Guenter > --- > drivers/watchdog/imx7ulp_wdt.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c > index 5ce51026989a..ba5d535a6db2 100644 > --- a/drivers/watchdog/imx7ulp_wdt.c > +++ b/drivers/watchdog/imx7ulp_wdt.c > @@ -106,12 +106,28 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, > return 0; > } > > +static int imx7ulp_wdt_restart(struct watchdog_device *wdog, > + unsigned long action, void *data) > +{ > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > + > + imx7ulp_wdt_enable(wdt->base, true); > + imx7ulp_wdt_set_timeout(&wdt->wdd, 1); > + > + /* wait for wdog to fire */ > + while (true) > + ; > + > + return NOTIFY_DONE; > +} > + > static const struct watchdog_ops imx7ulp_wdt_ops = { > .owner = THIS_MODULE, > .start = imx7ulp_wdt_start, > .stop = imx7ulp_wdt_stop, > .ping = imx7ulp_wdt_ping, > .set_timeout = imx7ulp_wdt_set_timeout, > + .restart = imx7ulp_wdt_restart, > }; > > static const struct watchdog_info imx7ulp_wdt_info = {
Hi Guenter, On Sat, Nov 2, 2019 at 12:36 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote: > > The following hang is observed when a 'reboot' command is issued: > > > > # reboot > > # Stopping network: OK > > Stopping klogd: OK > > Stopping syslogd: OK > > umount: devtmpfs busy - remounted read-only > > [ 8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null) > > The system is going down NOW! > > Sent SIGTERM to all processes > > Sent SIGKILL to all processes > > Requesting system reboot > > [ 10.694753] reboot: Restarting system > > [ 11.699008] Reboot failed -- System halted > > > > Fix this problem by adding a .restart ops member. > > > > Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support") > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > However, just to be sure: This registers the watchdog based restart handler > as restart handler of last resort. I assume this on purpose, I just want > to make sure it is intentional since it is not explicitly mentioned in > the commit message. To be honest, I thought that registering the restart handler was mandatory. By the way, I have just noticed that even though this patch fixes the reboot on a imx7ulp evk board, it does not work on a imx7ulp com board. I will debug this, so please discard this patch for now. The other ones of this series should be fine to apply. Thanks
On 11/4/19 5:45 AM, Fabio Estevam wrote: > Hi Guenter, > > On Sat, Nov 2, 2019 at 12:36 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote: >>> The following hang is observed when a 'reboot' command is issued: >>> >>> # reboot >>> # Stopping network: OK >>> Stopping klogd: OK >>> Stopping syslogd: OK >>> umount: devtmpfs busy - remounted read-only >>> [ 8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null) >>> The system is going down NOW! >>> Sent SIGTERM to all processes >>> Sent SIGKILL to all processes >>> Requesting system reboot >>> [ 10.694753] reboot: Restarting system >>> [ 11.699008] Reboot failed -- System halted >>> >>> Fix this problem by adding a .restart ops member. >>> >>> Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support") >>> Signed-off-by: Fabio Estevam <festevam@gmail.com> >> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> >> However, just to be sure: This registers the watchdog based restart handler >> as restart handler of last resort. I assume this on purpose, I just want >> to make sure it is intentional since it is not explicitly mentioned in >> the commit message. > > To be honest, I thought that registering the restart handler was mandatory. > Maybe I should have said "there is no call to watchdog_set_restart_priority()". > By the way, I have just noticed that even though this patch fixes the > reboot on a imx7ulp evk board, it does not work on a imx7ulp com > board. > > I will debug this, so please discard this patch for now. The other > ones of this series should be fine to apply. > Thanks for the update. Guenter
Hi Guenter, On Mon, Nov 4, 2019 at 11:06 AM Guenter Roeck <linux@roeck-us.net> wrote: > Maybe I should have said "there is no call to watchdog_set_restart_priority()". Would you like me to respin this series with a call to watchdog_set_restart_priority()? > > By the way, I have just noticed that even though this patch fixes the > > reboot on a imx7ulp evk board, it does not work on a imx7ulp com > > board. > > > > I will debug this, so please discard this patch for now. The other > > ones of this series should be fine to apply. > > > > Thanks for the update. I have just retested this on a imx7ulp-com and imx7ulp-evk boards and the "reboot" command is working fine on the two boards. Looks like the issue I had earlier was related to dtb. Thanks
On 11/7/19 9:46 AM, Fabio Estevam wrote: > Hi Guenter, > > On Mon, Nov 4, 2019 at 11:06 AM Guenter Roeck <linux@roeck-us.net> wrote: > >> Maybe I should have said "there is no call to watchdog_set_restart_priority()". > > Would you like me to respin this series with a call to > watchdog_set_restart_priority()? > Your call, really. As written, the restart handler serves as restart handler of last resort. If that is the intention we are fine. >>> By the way, I have just noticed that even though this patch fixes the >>> reboot on a imx7ulp evk board, it does not work on a imx7ulp com >>> board. >>> >>> I will debug this, so please discard this patch for now. The other >>> ones of this series should be fine to apply. >>> >> >> Thanks for the update. > > I have just retested this on a imx7ulp-com and imx7ulp-evk boards and > the "reboot" command is working fine on the two boards. > > Looks like the issue I had earlier was related to dtb. > Thanks for the update! Guenter
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c index 5ce51026989a..ba5d535a6db2 100644 --- a/drivers/watchdog/imx7ulp_wdt.c +++ b/drivers/watchdog/imx7ulp_wdt.c @@ -106,12 +106,28 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, return 0; } +static int imx7ulp_wdt_restart(struct watchdog_device *wdog, + unsigned long action, void *data) +{ + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); + + imx7ulp_wdt_enable(wdt->base, true); + imx7ulp_wdt_set_timeout(&wdt->wdd, 1); + + /* wait for wdog to fire */ + while (true) + ; + + return NOTIFY_DONE; +} + static const struct watchdog_ops imx7ulp_wdt_ops = { .owner = THIS_MODULE, .start = imx7ulp_wdt_start, .stop = imx7ulp_wdt_stop, .ping = imx7ulp_wdt_ping, .set_timeout = imx7ulp_wdt_set_timeout, + .restart = imx7ulp_wdt_restart, }; static const struct watchdog_info imx7ulp_wdt_info = {
The following hang is observed when a 'reboot' command is issued: # reboot # Stopping network: OK Stopping klogd: OK Stopping syslogd: OK umount: devtmpfs busy - remounted read-only [ 8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null) The system is going down NOW! Sent SIGTERM to all processes Sent SIGKILL to all processes Requesting system reboot [ 10.694753] reboot: Restarting system [ 11.699008] Reboot failed -- System halted Fix this problem by adding a .restart ops member. Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support") Signed-off-by: Fabio Estevam <festevam@gmail.com> --- drivers/watchdog/imx7ulp_wdt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)