linux-kernel - Re: [PATCH] watchdog: dw: use devm_watchdog_register

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] [day] [month] [year] [list]

Message-ID: <4414c144-ebdd-2c16-7268-1e11d5a682cd@roeck-us.net>
Date:   Fri, 25 Jan 2019 01:01:31 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Jisheng Zhang <Jisheng.Zhang@...aptics.com>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()

On 1/25/19 12:17 AM, Jisheng Zhang wrote:
> Hi,
> 
> On Fri, 25 Jan 2019 00:04:25 -0800 Guenter Roeck wrote:
> 
>> Hi,
>>
>> On 1/24/19 11:52 PM, Jisheng Zhang wrote:
>>> Use devm_watchdog_register_device() to simplify the code.
>>>
>>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@...aptics.com>
>>> ---
>>>    drivers/watchdog/dw_wdt.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>>> index 501aebb5b81f..c053c2de5c2f 100644
>>> --- a/drivers/watchdog/dw_wdt.c
>>> +++ b/drivers/watchdog/dw_wdt.c
>>> @@ -303,7 +303,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>    
>>>    	watchdog_set_restart_priority(wdd, 128);
>>>    
>>> -	ret = watchdog_register_device(wdd);
>>> +	ret = devm_watchdog_register_device(wdd);
>>>    	if (ret)
>>>    		goto out_disable_clk;
>>>    
>>> @@ -318,7 +318,6 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
>>>    {
>>>    	struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
>>>    
>>> -	watchdog_unregister_device(&dw_wdt->wdd);
>>>    	reset_control_assert(dw_wdt->rst);
>>>    	clk_disable_unprepare(dw_wdt->clk);
>>>    
>>>    
>> Unfortunately it isn't that easy. The other two calls have to be executed after
>> unregistering the watchdog, meaning you would have to add devm_add_action()
>> in the probe function to call them.
> 
> do you mean reset_control_asser() and the clk_disable_unprepare()? If yes,

Yes.

> does unregister the watchdog could trigger any register access? Per my
> understanding, there's no register access path in the unregister. Am I
> missing somthing?
> 

If the watchdog is unregistered last, it would be still accessible via its device
nodes, even though the clock has been reset, and even though the HW has been
put in reset. This would be racy.

Guenter