linux-kernel - Re: [PATCH] watchdog: meson_gxbb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0d5e53b2-873e-0ffa-32eb-87e96b51e263@baylibre.com>
Date:   Tue, 22 Jun 2021 14:43:45 +0200
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Art Nikpal <email2tema@...il.com>
Cc:     wim@...ux-watchdog.org, linux@...ck-us.net,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Christian Hewitt <christianshewitt@...il.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-watchdog@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@...ts.infradead.org>,
        linux-kernel@...r.kernel.org, Artem Lapkin <art@...das.com>,
        Nick Xie <nick@...das.com>, Gouwa Wang <gouwa@...das.com>
Subject: Re: [PATCH] watchdog: meson_gxbb_wdt: improve

Hi,

On 22/06/2021 13:53, Art Nikpal wrote:
>> Neil
>> Can you split the patch in 4 distinct changes ?
> 
> yes  no problem i can try to do it tomorrow !
> maybe somebody have other ideas, suggestion, comments ...

The changeset is clean, and overall I'm ok with the changes, but I'm pretty sure the wdt maintainers
will prefer separate changes in order to comment of each.
Neil

> 
> 
> On Tue, Jun 22, 2021 at 7:08 PM Neil Armstrong <narmstrong@...libre.com> wrote:
>>
>> Hi Art,
>>
>> On 22/06/2021 11:56, Artem Lapkin wrote:
>>> Improve meson_gxbb_wdt watchdog driver
>>> 1) added module param timeout and nowayout same as other modules
>>> 2) print watchdog driver start status
>>> 3) add watchdog_stop_on_unregister
>>> 4) remove watchdog_stop_on_reboot ( still can be activated by
>>> watchdog.stop_on_reboot=1 ) i think this driver configuration more useful
>>> becouse we can get reboot waranty for abnormal situations on shutdown stage
>>
>> Can you split the patch in 4 distinct changes ?
>>
>> Neil
>>
>>>
>>> Signed-off-by: Artem Lapkin <art@...das.com>
>>> ---
>>>  drivers/watchdog/meson_gxbb_wdt.c | 20 ++++++++++++++++++--
>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
>>> index 5a9ca10fbcfa..15c889932c13 100644
>>> --- a/drivers/watchdog/meson_gxbb_wdt.c
>>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>>> @@ -35,6 +35,17 @@ struct meson_gxbb_wdt {
>>>       struct clk *clk;
>>>  };
>>>
>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>> +static unsigned int timeout = DEFAULT_TIMEOUT;
>>> +
>>> +module_param(nowayout, bool, 0);
>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
>>> +                     __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>> +
>>> +module_param(timeout, uint, 0);
>>> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
>>> +                     __MODULE_STRING(DEFAULT_TIMEOUT) ")");
>>> +
>>>  static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
>>>  {
>>>       struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>>> @@ -174,7 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>>       data->wdt_dev.ops = &meson_gxbb_wdt_ops;
>>>       data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>>>       data->wdt_dev.min_timeout = 1;
>>> -     data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>>> +     data->wdt_dev.timeout = timeout;
>>>       watchdog_set_drvdata(&data->wdt_dev, data);
>>>
>>>       /* Setup with 1ms timebase */
>>> @@ -186,7 +197,12 @@ 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);
>>> +     watchdog_set_nowayout(&data->wdt_dev, nowayout);
>>> +     watchdog_stop_on_unregister(&data->wdt_dev);
>>> +
>>> +     dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
>>> +             data->wdt_dev.timeout, nowayout);
>>> +
>>>       return devm_watchdog_register_device(dev, &data->wdt_dev);
>>>  }
>>>
>>>
>>