| 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
| ||
|
Message-ID: <AGAAwgADDgCvfda5u2TY64qq.3.1619161944242.Hmail.wangqing@vivo.com>
Date: Fri, 23 Apr 2021 15:12:24 +0800 (GMT+08:00)
From: 王擎 <wangqing@...o.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>,
Rob Herring <robh+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re:Re: [PATCH V7 1/2] watchdog: mtk: support pre-timeout when the bark irq is available
>On 4/22/21 8:48 PM, 王擎 wrote:
>>
>>> On 4/22/21 7:53 PM, Wang Qing wrote:
>>>> Use the bark interrupt as the pretimeout notifier if available.
>>>>
>>>> When the watchdog timer expires in dual mode, an interrupt will be
>>>> triggered first, then the timing restarts. The reset signal will be
>>>> initiated when the timer expires again.
>>>>
>>>> The pretimeout notification shall occur at timeout-sec/2.
>>>>
>>>> V2:
>>>> - panic() by default if WATCHDOG_PRETIMEOUT_GOV is not enabled.
>>>>
>>>> V3:
>>>> - Modify the pretimeout behavior, manually reset after the pretimeout
>>>> - is processed and wait until timeout.
>>>>
>>>> V4:
>>>> - Remove pretimeout related processing.
>>>> - Add dual mode control separately.
>>>>
>>>> V5:
>>>> - Fix some formatting and printing problems.
>>>>
>>>> V6:
>>>> - Realize pretimeout processing through dualmode.
>>>>
>>>> V7:
>>>> - Add set_pretimeout().
>>>>
>>>> Signed-off-by: Wang Qing <wangqing@...o.com>
>>>> ---
>>>> drivers/watchdog/mtk_wdt.c | 76 +++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 71 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>> index 97ca993..ab3ac5d
>>>> --- a/drivers/watchdog/mtk_wdt.c
>>>> +++ b/drivers/watchdog/mtk_wdt.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/reset-controller.h>
>>>> #include <linux/types.h>
>>>> #include <linux/watchdog.h>
>>>> +#include <linux/interrupt.h>
>>>>
>>>> #define WDT_MAX_TIMEOUT 31
>>>> #define WDT_MIN_TIMEOUT 1
>>>> @@ -184,15 +185,23 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
>>>> {
>>>> struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
>>>> void __iomem *wdt_base = mtk_wdt->wdt_base;
>>>> + unsigned int timeout_interval = timeout;
>>>> u32 reg;
>>>>
>>>> wdt_dev->timeout = timeout;
>>>> -
>>>> + /*
>>>> + * In dual mode, irq will be triggered at timeout / 2
>>>> + * the real timeout occurs at timeout
>>>> + */
>>>> + if (wdt_dev->pretimeout) {
>>>> + wdt_dev->pretimeout = timeout / 2;
>>>
>>> min_timeout is set to 1. I don't this works well if timeout == 1.
>>> You'll either need to set min_timeout to 2, or handle that case.
>>
>> It is appropriate to change min_timeout to 2.
>>
>>>
>>>> + timeout_interval = wdt_dev->pretimeout;
>>>
>>> timeout_interval is unnecessary. Just update timeout accordingly.
>>> It needs to take the situation of timeout == 1 into account, though.
I plan to remove timeout_interval and directly use (timeout-pretimeout)
update timeout.
>>
>> timeout represents the reset time. When the user calls timeout_show,
>> He hopes to get the configured timeout, not the value changed
>> by pre-timeout.
>> I modify it like this more in line with the original intention.
>>
>>>
>>>> + }
>>>> /*
>>>> * One bit is the value of 512 ticks
>>>> * The clock has 32 KHz
>>>> */
>>>> - reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
>>>> + reg = WDT_LENGTH_TIMEOUT(timeout_interval << 6) | WDT_LENGTH_KEY;
>>>> iowrite32(reg, wdt_base + WDT_LENGTH);
>>>>
>>>> mtk_wdt_ping(wdt_dev);
>>>> @@ -239,13 +248,46 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
>>>> return ret;
>>>>
>>>> reg = ioread32(wdt_base + WDT_MODE);
>>>> - reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> + if (wdt_dev->pretimeout)
>>>> + reg |= (WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> + else
>>>> + reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> reg |= (WDT_MODE_EN | WDT_MODE_KEY);
>>>> iowrite32(reg, wdt_base + WDT_MODE);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> +static int mtk_wdt_set_pretimeout(struct watchdog_device *wdd,
>>>> + unsigned int timeout)
>>>> +{
>>>> + struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdd);
>>>> + void __iomem *wdt_base = mtk_wdt->wdt_base;
>>>> + u32 reg = ioread32(wdt_base + WDT_MODE);
>>>> +
>>>> + if (timeout && !wdd->pretimeout) {
>>>> + wdd->pretimeout = wdd->timeout / 2;
>>>> + reg |= (WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> + } else if (!timeout && wdd->pretimeout) {
>>>> + wdd->pretimeout = 0;
>>>> + reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>>>> + } else
>>>> + return 0;
>>>> +
>>>> + iowrite32(reg, wdt_base + WDT_MODE);
>>>
>>> What is the point of setting the mode here ? It will
>>> be set again in mtk_wdt_set_timeout(). Seems to me all
>>> you need to do here is to set wdd->pretimeout,
>>> then call mtk_wdt_set_timeout().
>>
>> mtk_wdt_set_timeout() only set timeout and ping().
>> Here also need to config to the dualmode or not.
>>
>Ah, you are correct. Sorry, I confused this with the start function.
>That makes me wonder if it would be better to extract a separate
>function, mtk_wdt_set_mode(), for that purpose. Thoughts ?
I have done this, but I found there is no good abstract method,
because wdt mode is used in combination, for example：
WDT_MODE_EN is included in start(), and here is not.
And the judgment of pretimeout here is also different.
Thanks,
Qing
>
>Thanks,
>Guenter
>
>> Thanks,
>> Qing
>>>
>>> Guenter
>>>
>>>> +
>>>> + return mtk_wdt_set_timeout(wdd, wdd->timeout);
>>>> +}
>>>> +
>>>> +static irqreturn_t mtk_wdt_isr(int irq, void *arg)
>>>> +{
>>>> + struct watchdog_device *wdd = arg;
>>>> +
>>>> + watchdog_notify_pretimeout(wdd);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> static const struct watchdog_info mtk_wdt_info = {
>>>> .identity = DRV_NAME,
>>>> .options = WDIOF_SETTIMEOUT |
>>>> @@ -253,12 +295,21 @@ static const struct watchdog_info mtk_wdt_info = {
>>>> WDIOF_MAGICCLOSE,
>>>> };
>>>>
>>>> +static const struct watchdog_info mtk_wdt_pt_info = {
>>>> + .identity = DRV_NAME,
>>>> + .options = WDIOF_SETTIMEOUT |
>>>> + WDIOF_PRETIMEOUT |
>>>> + WDIOF_KEEPALIVEPING |
>>>> + WDIOF_MAGICCLOSE,
>>>> +};
>>>> +
>>>> static const struct watchdog_ops mtk_wdt_ops = {
>>>> .owner = THIS_MODULE,
>>>> .start = mtk_wdt_start,
>>>> .stop = mtk_wdt_stop,
>>>> .ping = mtk_wdt_ping,
>>>> .set_timeout = mtk_wdt_set_timeout,
>>>> + .set_pretimeout = mtk_wdt_set_pretimeout,
>>>> .restart = mtk_wdt_restart,
>>>> };
>>>>
>>>> @@ -267,7 +318,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>>>> struct device *dev = &pdev->dev;
>>>> struct mtk_wdt_dev *mtk_wdt;
>>>> const struct mtk_wdt_data *wdt_data;
>>>> - int err;
>>>> + int err, irq;
>>>>
>>>> mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
>>>> if (!mtk_wdt)
>>>> @@ -279,7 +330,22 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>>>> if (IS_ERR(mtk_wdt->wdt_base))
>>>> return PTR_ERR(mtk_wdt->wdt_base);
>>>>
>>>> - mtk_wdt->wdt_dev.info = &mtk_wdt_info;
>>>> + irq = platform_get_irq(pdev, 0);
>>>> + if (irq > 0) {
>>>> + err = devm_request_irq(&pdev->dev, irq, mtk_wdt_isr, 0, "wdt_bark",
>>>> + &mtk_wdt->wdt_dev);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + mtk_wdt->wdt_dev.info = &mtk_wdt_pt_info;
>>>> + mtk_wdt->wdt_dev.pretimeout = WDT_MAX_TIMEOUT / 2;
>>>> + } else {
>>>> + if (irq == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + mtk_wdt->wdt_dev.info = &mtk_wdt_info;
>>>> + }
>>>> +
>>>> mtk_wdt->wdt_dev.ops = &mtk_wdt_ops;
>>>> mtk_wdt->wdt_dev.timeout = WDT_MAX_TIMEOUT;
>>>> mtk_wdt->wdt_dev.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT * 1000;
>>>>
>>>
>>
>>
>
Powered by blists - more mailing lists