From: Guenter Roeck <linux@roeck-us.net>
To: Mark Pearson <mpearson-lenovo@squebb.ca>,
David Ober <dober6023@gmail.com>,
wim@linux-watchdog.org
Cc: linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
David Ober <dober@lenovo.com>
Subject: Re: [PATCH v2] watchdog: add in watchdog for nct6686
Date: Sat, 24 Feb 2024 08:19:27 -0800 [thread overview]
Message-ID: <e6d0fda7-09db-4955-92dd-def2df768da6@roeck-us.net> (raw)
In-Reply-To: <8458d626-8862-44ed-8966-2f60852c92bb@app.fastmail.com>
On 2/23/24 11:49, Mark Pearson wrote:
> Hi,
>
> On Tue, Dec 19, 2023, at 3:05 PM, Mark Pearson wrote:
>> On Tue, Sep 12, 2023, at 7:27 AM, David Ober wrote:
>>> This change adds in the watchdog timer support for the nct6686
>>> chip so that it can be used on the Lenovo m90n IOT device
>>>
[ ... ]
>
> Would it be possible to get feedback on if anything else is needed for this patch please?
> We have customers wanting to use it, and we've ended up having to provide it as an out-of-tree module. Would love to get this integrated into the kernel properly.
>
The locking in the driver is pretty much pointless since accesses
will be serialized by the watchdog core. At the same time, all
driver-local locking will not prevent access to the chip from the
nct6683 hwmon driver (which also supports nt6686). If both are
instantiated, I don't immediately see how they would not corrupt
each other.
Other than that, there is unexplained code such as nct6686_wdt_set_bank(),
which writes two bytes into the chip, and nct6686_wdt_reset_bank(),
which only writes one byte, but only conditionally if the bank is != 0.
That doesn't really "reset" the bank; at best it selects bank 0xff
unless bank 0 was requested. I don't really understand how that would be
"play safe" since it is not explained what that means. Besides, the hwmon
driver doesn't do that, so I don't understand the implications.
Actually, looking into both the this patch and the hwmon driver, it seems
that they are locking out each other, with "first driver to probe wins",
by reserving the access memory range for themselves. That is not acceptable.
I'll have to study the chip datasheets in detail to understand if
there are other potential issues, and I have not had the time to do that.
Guenter
next prev parent reply other threads:[~2024-02-24 16:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-12 11:27 [PATCH v2] watchdog: add in watchdog for nct6686 David Ober
2023-12-19 20:05 ` Mark Pearson
2024-02-23 19:49 ` Mark Pearson
2024-02-24 16:19 ` Guenter Roeck [this message]
2024-02-24 17:00 ` [External] " David Ober
2024-02-24 17:25 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e6d0fda7-09db-4955-92dd-def2df768da6@roeck-us.net \
--to=linux@roeck-us.net \
--cc=dober6023@gmail.com \
--cc=dober@lenovo.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=mpearson-lenovo@squebb.ca \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).