* Added nowayout module parameter
* Added timeout module parameter
* Removed watchdog_stop_on_reboot
https://lore.kernel.org/linux-watchdog/[email protected]/T/#t
Artem Lapkin (3):
watchdog: meson_gxbb_wdt: add nowayout parameter
watchdog: meson_gxbb_wdt: add timeout parameter
watchdog: meson_gxbb_wdt: remove stop_on_reboot
drivers/watchdog/meson_gxbb_wdt.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
--
2.25.1
Add timeout module parameter
Signed-off-by: Artem Lapkin <[email protected]>
---
drivers/watchdog/meson_gxbb_wdt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
index 5aebc3a09652..945f5e65db57 100644
--- a/drivers/watchdog/meson_gxbb_wdt.c
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -34,6 +34,11 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+static unsigned int timeout;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
+ __MODULE_STRING(DEFAULT_TIMEOUT) ")");
+
struct meson_gxbb_wdt {
void __iomem *reg_base;
struct watchdog_device wdt_dev;
@@ -180,6 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
data->wdt_dev.min_timeout = 1;
data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+ watchdog_init_timeout(&data->wdt_dev, timeout, dev);
watchdog_set_nowayout(&data->wdt_dev, nowayout);
watchdog_set_drvdata(&data->wdt_dev, data);
--
2.25.1
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/[email protected]/T/#t
Signed-off-by: Artem Lapkin <[email protected]>
---
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 Fri, Jul 30, 2021 at 12:13:54PM +0800, Artem Lapkin wrote:
> Add timeout module parameter
>
> Signed-off-by: Artem Lapkin <[email protected]>
> ---
<Formletter>
Change log goes here. If it is missing, I won't know what changed.
That means I will have to dig out older patch versions to compare.
That costs time and would hold up both this patch as well as all other
patches which I still have to review.
For this reason, I will not review patches without change log.
</Formletter>
As before, the change log is small and recent enough that I remember,
so you are lucky.
Reviewed-by: Guenter Roeck <[email protected]>
Thanks,
Guenter
> drivers/watchdog/meson_gxbb_wdt.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> index 5aebc3a09652..945f5e65db57 100644
> --- a/drivers/watchdog/meson_gxbb_wdt.c
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -34,6 +34,11 @@ module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +static unsigned int timeout;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
> + __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> +
> struct meson_gxbb_wdt {
> void __iomem *reg_base;
> struct watchdog_device wdt_dev;
> @@ -180,6 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
> data->wdt_dev.min_timeout = 1;
> data->wdt_dev.timeout = DEFAULT_TIMEOUT;
> + watchdog_init_timeout(&data->wdt_dev, timeout, dev);
> watchdog_set_nowayout(&data->wdt_dev, nowayout);
> watchdog_set_drvdata(&data->wdt_dev, data);
>
> --
> 2.25.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/[email protected]/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 <[email protected]>
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 <[email protected]> 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/[email protected]/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 <[email protected]>
>
> 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/[email protected]/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 <[email protected]>
>
> 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 <[email protected]> wrote:
>
> Add timeout module parameter
>
> Signed-off-by: Artem Lapkin <[email protected]>
> ---
> drivers/watchdog/meson_gxbb_wdt.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> index 5aebc3a09652..945f5e65db57 100644
> --- a/drivers/watchdog/meson_gxbb_wdt.c
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -34,6 +34,11 @@ module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +static unsigned int timeout;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
> + __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> +
> struct meson_gxbb_wdt {
> void __iomem *reg_base;
> struct watchdog_device wdt_dev;
> @@ -180,6 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
> data->wdt_dev.min_timeout = 1;
> data->wdt_dev.timeout = DEFAULT_TIMEOUT;
> + watchdog_init_timeout(&data->wdt_dev, timeout, dev);
> watchdog_set_nowayout(&data->wdt_dev, nowayout);
> watchdog_set_drvdata(&data->wdt_dev, data);
>
> --
> 2.25.1
>
hi Guenter Roeck
why still not merged to upstream ?
On Fri, Jul 30, 2021 at 12:14 PM Artem Lapkin <[email protected]> wrote:
>
> * Added nowayout module parameter
> * Added timeout module parameter
> * Removed watchdog_stop_on_reboot
>
> https://lore.kernel.org/linux-watchdog/[email protected]/T/#t
>
> Artem Lapkin (3):
> watchdog: meson_gxbb_wdt: add nowayout parameter
> watchdog: meson_gxbb_wdt: add timeout parameter
> watchdog: meson_gxbb_wdt: remove stop_on_reboot
>
> drivers/watchdog/meson_gxbb_wdt.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> --
> 2.25.1
>
hi Guenter Roeck
why still not merged to upstream ?
On Fri, Jul 30, 2021 at 12:14 PM Artem Lapkin <[email protected]> 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/[email protected]/T/#t
>
> Signed-off-by: Artem Lapkin <[email protected]>
> ---
> 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 <[email protected]> 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/[email protected]/T/#t
>>
>> Signed-off-by: Artem Lapkin <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]> 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/[email protected]/T/#t
> >>
> >> Signed-off-by: Artem Lapkin <[email protected]>
> >> ---
> >> 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
> >>
>