linux-kernel - Re: [PATCH v3] Watchdog: New module for ITE 5632 watchdog

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]

Date: Fri, 23 Feb 2024 19:43:17 -0500
From: "Mark Pearson" <mpearson@...ebb.ca>
To: "Guenter Roeck" <linux@...ck-us.net>, "David Ober" <dober6023@...il.com>,
 wim@...ux-watchdog.org
Cc: linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
 "David Ober" <dober@...ovo.com>
Subject: Re: [PATCH v3] Watchdog: New module for ITE 5632 watchdog

Thanks Guenter

On Fri, Feb 23, 2024, at 3:21 PM, Guenter Roeck wrote:
> On 2/23/24 11:58, Mark Pearson wrote:
>> On Fri, Jul 21, 2023, at 8:29 AM, David Ober wrote:
>>> This modules is to allow for the new ITE 5632 EC chip
>>> to support the watchdog for initial use in the Lenovo SE10
>>>
>>> Signed-off-by: David Ober <dober6023@...il.com>
>>>
>>> V2 Fix stop to deactivate wdog on unload of module
>>> V2 Remove extra logging that is not needed
>>> V2 change udelays to usleep_range
>>> V2 Changed to now request region on start and release on stop instead
>>>     of for every ping and read/write
>>> V3 add counter to while loops so it will not hang
>>> V3 rework code to use platform_device_register_simple
>>> V3 rework getting the Chip ID to remove duplicate code and close IO
>>> V3 change some extra logging to be debug only
>>> ---
> [ ... ]
>>> +config ITE5632_WDT
>>> +        tristate "ITE 5632"
>>> +        select WATCHDOG_CORE
>>> +        help
>>> +          If you say yes here you get support for the watchdog
>>> +          functionality of the ITE 5632 eSIO chip.
>>> +
>>> +          This driver can also be built as a module. If so, the module
>>> +          will be called ite5632_wdt.
>>> +
>
> [ ... ]
>
>>
>> 
>> Please let us know if there is anything else needed to get this accepted. Happy to address any feedback.
>> 
>
> I am sure I commented on this before. The fact that the Lenovo SE10 uses an
> ITE 5632 controller is completely irrelevant. Lenovo could decide tomorrow to
> replace the ITE chip with a Nuvoton chip, use the same API to talk with it,
> and the watchdog would work perfectly fine.
>
> This is a driver for the watchdog implemented in the embedded controller
> on Lenovo SE10. It is not a watchdog driver for ITE5632. Again, the EC chip
> used in that Lenovo system is completely irrelevant, even more so since
> this seems to be one of those undocumented ITE chips which officially
> don't even exist. Claiming that this would be a watchdog driver for ITE5632
> would be not only misleading but simply wrong.
>
> It seems that we can not agree on this. That means that, from my perspective,
> there is no real path to move forward. Wim will have to decide if and how
> to proceed.
>
My apologies - I hadn't realised that was the issue (my fault for missing it). Appreciate the clarification.

Is this as simple as renaming this driver as (for example) a lenovo_se_wdt device, and adding in the appropriate checking during the init that it is only used on Lenovo SE10 platforms?

I don't understand the concern if a different chip was used - wouldn't that need a different driver at that point?

If it's more subtle than that, is there something you propose instead?

Thanks
Mark