diff mbox series

[v4,3/3] watchdog: meson_gxbb_wdt: remove stop_on_reboot

Message ID 20210730041355.2810397-4-art@khadas.com (mailing list archive)
State New, archived
Headers show
Series watchdog: meson_gxbb_wdt: improve | expand

Commit Message

Art Nikpal July 30, 2021, 4:13 a.m. UTC
Remove watchdog_stop_on_reboot()

Meson platform still have some hardware drivers problems for some
configurations which can freeze device on shutdown/reboot stage and i
think better to have reboot warranty by default.

I feel that it is important to keep the watchdog running during the
reboot sequence, in the event that an abnormal driver freezes the reboot
process.

This is my personal opinion and I hope the driver authors will agree
with my proposal, or just ignore this commit if not.

https://lore.kernel.org/linux-watchdog/20210729072308.1908904-1-art@khadas.com/T/#t

Signed-off-by: Artem Lapkin <art@khadas.com>
---
 drivers/watchdog/meson_gxbb_wdt.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Guenter Roeck July 30, 2021, 4:58 a.m. UTC | #1
On Fri, Jul 30, 2021 at 12:13:55PM +0800, Artem Lapkin wrote:
> Remove watchdog_stop_on_reboot()
> 
> Meson platform still have some hardware drivers problems for some
> configurations which can freeze device on shutdown/reboot stage and i
> think better to have reboot warranty by default.
> 
> I feel that it is important to keep the watchdog running during the
> reboot sequence, in the event that an abnormal driver freezes the reboot
> process.
> 
> This is my personal opinion and I hope the driver authors will agree
> with my proposal, or just ignore this commit if not.
> 
> https://lore.kernel.org/linux-watchdog/20210729072308.1908904-1-art@khadas.com/T/#t
> 

A much better description would be something like

"The Meson platform still has some hardware drivers problems for some
 configurations which can freeze devices on shutdown/reboot.
 Remove watchdog_stop_on_reboot() to catch this situation and ensure
 that the reboot happens anyway.
 Users who still want to stop the watchdog on reboot can still do so
 using the watchdog.stop_on_reboot=1 module parameter.
 "

That leaves the personal opinion out of the picture and provides both
a rationale for the change and an alternative for people who want
to stop the watchdog on reboot anyway.

> Signed-off-by: Artem Lapkin <art@khadas.com>

As mentioned, I'd still like to get an opinion from the driver
author and/or some other users of this platform. However, I'll
accept the patch with the above description change if I don't get
additional feedback.

Thanks,
Guenter

> ---
>  drivers/watchdog/meson_gxbb_wdt.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> index 945f5e65db57..d3c9e2f6e63b 100644
> --- a/drivers/watchdog/meson_gxbb_wdt.c
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -198,7 +198,6 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>  
>  	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>  
> -	watchdog_stop_on_reboot(&data->wdt_dev);
>  	return devm_watchdog_register_device(dev, &data->wdt_dev);
>  }
>  
> -- 
> 2.25.1
>
Art Nikpal July 30, 2021, 6:52 a.m. UTC | #2
Thank you very much for the helpful and detailed comments

Artem

On Fri, Jul 30, 2021 at 12:59 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Jul 30, 2021 at 12:13:55PM +0800, Artem Lapkin wrote:
> > Remove watchdog_stop_on_reboot()
> >
> > Meson platform still have some hardware drivers problems for some
> > configurations which can freeze device on shutdown/reboot stage and i
> > think better to have reboot warranty by default.
> >
> > I feel that it is important to keep the watchdog running during the
> > reboot sequence, in the event that an abnormal driver freezes the reboot
> > process.
> >
> > This is my personal opinion and I hope the driver authors will agree
> > with my proposal, or just ignore this commit if not.
> >
> > https://lore.kernel.org/linux-watchdog/20210729072308.1908904-1-art@khadas.com/T/#t
> >
>
> A much better description would be something like
>
> "The Meson platform still has some hardware drivers problems for some
>  configurations which can freeze devices on shutdown/reboot.
>  Remove watchdog_stop_on_reboot() to catch this situation and ensure
>  that the reboot happens anyway.
>  Users who still want to stop the watchdog on reboot can still do so
>  using the watchdog.stop_on_reboot=1 module parameter.
>  "
>
> That leaves the personal opinion out of the picture and provides both
> a rationale for the change and an alternative for people who want
> to stop the watchdog on reboot anyway.
>
> > Signed-off-by: Artem Lapkin <art@khadas.com>
>
> As mentioned, I'd still like to get an opinion from the driver
> author and/or some other users of this platform. However, I'll
> accept the patch with the above description change if I don't get
> additional feedback.
>
> Thanks,
> Guenter
>
> > ---
> >  drivers/watchdog/meson_gxbb_wdt.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> > index 945f5e65db57..d3c9e2f6e63b 100644
> > --- a/drivers/watchdog/meson_gxbb_wdt.c
> > +++ b/drivers/watchdog/meson_gxbb_wdt.c
> > @@ -198,7 +198,6 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> >
> >       meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
> >
> > -     watchdog_stop_on_reboot(&data->wdt_dev);
> >       return devm_watchdog_register_device(dev, &data->wdt_dev);
> >  }
> >
> > --
> > 2.25.1
> >
Neil Armstrong July 30, 2021, 8:20 a.m. UTC | #3
Hi Guenter,

On 30/07/2021 06:58, Guenter Roeck wrote:
> On Fri, Jul 30, 2021 at 12:13:55PM +0800, Artem Lapkin wrote:
>> Remove watchdog_stop_on_reboot()
>>
>> Meson platform still have some hardware drivers problems for some
>> configurations which can freeze device on shutdown/reboot stage and i
>> think better to have reboot warranty by default.
>>
>> I feel that it is important to keep the watchdog running during the
>> reboot sequence, in the event that an abnormal driver freezes the reboot
>> process.
>>
>> This is my personal opinion and I hope the driver authors will agree
>> with my proposal, or just ignore this commit if not.
>>
>> https://lore.kernel.org/linux-watchdog/20210729072308.1908904-1-art@khadas.com/T/#t
>>
> 
> A much better description would be something like
> 
> "The Meson platform still has some hardware drivers problems for some
>  configurations which can freeze devices on shutdown/reboot.
>  Remove watchdog_stop_on_reboot() to catch this situation and ensure
>  that the reboot happens anyway.
>  Users who still want to stop the watchdog on reboot can still do so
>  using the watchdog.stop_on_reboot=1 module parameter.
>  "
> 
> That leaves the personal opinion out of the picture and provides both
> a rationale for the change and an alternative for people who want
> to stop the watchdog on reboot anyway.
> 
>> Signed-off-by: Artem Lapkin <art@khadas.com>
> 
> As mentioned, I'd still like to get an opinion from the driver
> author and/or some other users of this platform. However, I'll
> accept the patch with the above description change if I don't get
> additional feedback.

Sorry for the reply delay and thanks a lot for your review.

The rationale from Artem is OK for me.

Please add my Acked-by for the whole patchset.

Neil

> 
> Thanks,
> Guenter
> 
>> ---
>>  drivers/watchdog/meson_gxbb_wdt.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
>> index 945f5e65db57..d3c9e2f6e63b 100644
>> --- a/drivers/watchdog/meson_gxbb_wdt.c
>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>> @@ -198,7 +198,6 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>  
>>  	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>>  
>> -	watchdog_stop_on_reboot(&data->wdt_dev);
>>  	return devm_watchdog_register_device(dev, &data->wdt_dev);
>>  }
>>  
>> -- 
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
index 945f5e65db57..d3c9e2f6e63b 100644
--- a/drivers/watchdog/meson_gxbb_wdt.c
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -198,7 +198,6 @@  static int meson_gxbb_wdt_probe(struct platform_device *pdev)
 
 	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
 
-	watchdog_stop_on_reboot(&data->wdt_dev);
 	return devm_watchdog_register_device(dev, &data->wdt_dev);
 }