linux-kernel - Re: [RESEND PATCH v3 2/3] watchdog: f71808e_wdt: migrate to new kernel watchdog API

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 3 May 2021 20:33:29 +0200
From:   Ahmad Fatoum <a.fatoum@...gutronix.de>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        linux-watchdog@...r.kernel.org, kernel@...gutronix.de,
        linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH v3 2/3] watchdog: f71808e_wdt: migrate to new
 kernel watchdog API

Hello Guenter,

On 03.05.21 17:28, Guenter Roeck wrote:
> On Mon, May 03, 2021 at 04:39:25PM +0200, Ahmad Fatoum wrote:
>> Hello Guenter,
>>
>> On 03.05.21 16:31, Guenter Roeck wrote:
>>>> -static struct fintek_wdt watchdog = {
>>>> -	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>>>> -};
>>>> +static struct fintek_wdt watchdog;
>>>
>>> This should really be allocated, and "watchdog" is a terrible variable name
>>> even if static, especially given the previous patch.
>>
>> I can add a follow up patch to change this. I maintained the old
>> state of things here to make review easier.
>>
> Odd argument. You changed all the function names (unnecessarily,
> if you ask me) in the first patch of the series because they were
> too generic in your opinion.

I find it surprising to have a function called watchdog_set_timeout
defined locally in a driver.

But yes, the first patch was more useful in v2, where new functions
were added for pinmuxing each variant. Having f71808e_ for generic
code and f8186fg_ for a variant would not help code comprehension.

> That by itself added a lot of unnecessary complexity to the review.

This was not my intention. I figured having a separate patch that just
does the rename should make review of follow-up commits easier, not harder...

> And pretty much everything else changed
> in this patch anyway.

Feedback on v2 was that I change too much and that this made review harder.
I thus left name changes in a separate patch and limited changes in follow-up
commit to signatures where appropriate.

Thanks,
Ahmad

> 
> Guenter
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |