| Message ID | 20210730041355.2810397-4-art@khadas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | watchdog: meson_gxbb_wdt: improve | expand |
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 >
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 > >
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 >>
hi Guenter Roeck why still not merged to upstream ? On Fri, Jul 30, 2021 at 12:14 PM Artem Lapkin <email2tema@gmail.com> 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 > > Signed-off-by: Artem Lapkin <art@khadas.com> > --- > 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 >
On 11/8/21 11:59 PM, Art Nikpal wrote: > hi Guenter Roeck > why still not merged to upstream ? > I had asked you to provide an updated description, without the "personal opinion" part which does not belong into a commit log. The other two patches wait for Wim to send them upstream. Guenter > On Fri, Jul 30, 2021 at 12:14 PM Artem Lapkin <email2tema@gmail.com> 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 >> >> Signed-off-by: Artem Lapkin <art@khadas.com> >> --- >> 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 >>
On Tue, Nov 9, 2021 at 11:30 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/8/21 11:59 PM, Art Nikpal wrote: > > hi Guenter Roeck > > why still not merged to upstream ? > > > > I had asked you to provide an updated description, without the "personal > opinion" part which does not belong into a commit log. The other two > patches wait for Wim to send them upstream. > ok i have send this patch with new description again > Guenter > > > > On Fri, Jul 30, 2021 at 12:14 PM Artem Lapkin <email2tema@gmail.com> 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 > >> > >> Signed-off-by: Artem Lapkin <art@khadas.com> > >> --- > >> 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 --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); }
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(-)