[1/5] watchdog: imx7ulp: Fix reboot hang
diff mbox series

Message ID 20191029174037.25381-1-festevam@gmail.com
State Accepted
Headers show
Series
  • [1/5] watchdog: imx7ulp: Fix reboot hang
Related show

Commit Message

Fabio Estevam Oct. 29, 2019, 5:40 p.m. UTC
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(+)

Comments

Guenter Roeck Nov. 2, 2019, 3:36 p.m. UTC | #1
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 = {
Fabio Estevam Nov. 4, 2019, 1:45 p.m. UTC | #2
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
Guenter Roeck Nov. 4, 2019, 2:06 p.m. UTC | #3
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
Fabio Estevam Nov. 7, 2019, 5:46 p.m. UTC | #4
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
Guenter Roeck Nov. 8, 2019, 2:39 p.m. UTC | #5
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

Patch
diff mbox series

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 = {