linux-kernel - Re: [PATCH v4] watchdog: qcom_wdt: set WDOG_HW

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: <23aea80f-265b-8a23-e8ea-f46639dd4129@codeaurora.org>
Date:   Mon, 2 Nov 2020 09:28:41 +0530
From:   Kathiravan T <kathirav@...eaurora.org>
To:     Guenter Roeck <linux@...ck-us.net>,
        Robert Marko <robert.marko@...tura.hr>, agross@...nel.org,
        bjorn.andersson@...aro.org, wim@...ux-watchdog.org,
        linux-arm-msm@...r.kernel.org, linux-watchdog@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     Luka Perkov <luka.perkov@...tura.hr>
Subject: Re: [PATCH v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when
 appropriate


On 10/31/2020 7:38 PM, Guenter Roeck wrote:
> On 10/31/20 5:11 AM, Robert Marko wrote:
>> If the watchdog hardware is enabled/running during boot, e.g.
>> due to a boot loader configuring it, we must tell the
>> watchdog framework about this fact so that it can ping the
>> watchdog until userspace opens the device and takes over
>> control.
>>
>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
>> that use-case.
>>
>> Signed-off-by: Robert Marko <robert.marko@...tura.hr>
> Reviewed-by: Guenter Roeck <linux@...ck-us.net>

Thanks for addressing the comments and now the patch looks good to me. 
One more suggestion, can we make the initcall level of the driver to 
subsys_initcall_sync so that the driver gets registered immediately 
after the watchdog_core is registered and watchdog_core starts pinging 
the WDT?

>> Cc: Luka Perkov <luka.perkov@...tura.hr>
>> ---
>> Changes in v4:
>> * Use QCOM_WDT_ENABLE macro
>>
>> Changes in v3:
>> * Drop call to stop as start already does it
>> * Update commit message
>>
>> Changes in v2:
>> * Correct authorship
>>
>>   drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index ab7465d186fd..07d399c4edc4 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>>   	return 0;
>>   }
>>   
>> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
>> +{
>> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +
>> +	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);
>> +}
>> +
>>   static const struct watchdog_ops qcom_wdt_ops = {
>>   	.start		= qcom_wdt_start,
>>   	.stop		= qcom_wdt_stop,
>> @@ -294,6 +301,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>   	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>>   	watchdog_init_timeout(&wdt->wdd, 0, dev);
>>   
>> +	/*
>> +	 * If WDT is already running, call WDT start which
>> +	 * will stop the WDT, set timeouts as bootloader
>> +	 * might use different ones and set running bit
>> +	 * to inform the WDT subsystem to ping the WDT
>> +	 */
>> +	if (qcom_wdt_is_running(&wdt->wdd)) {
>> +		qcom_wdt_start(&wdt->wdd);
>> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>> +	}
>> +
>>   	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>>   	if (ret)
>>   		return ret;
>>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation