linux-kernel - Re: [PATCH v2 5/7] watchdog: Add Nuvoton NCT6694 WDT support

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: <CAOoeyxUMGhvNukqc0ufT1UrCi4WK60cNaOpyx1ngqOxm5OsT4Q@mail.gmail.com>
Date: Fri, 22 Nov 2024 16:11:50 +0800
From: Ming Yu <a0282524688@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl, 
	andi.shyti@...nel.org, mkl@...gutronix.de, mailhol.vincent@...adoo.fr, 
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, wim@...ux-watchdog.org, jdelvare@...e.com, 
	alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH v2 5/7] watchdog: Add Nuvoton NCT6694 WDT support

Dear Guenter,

Thank you for your comments,

Guenter Roeck <linux@...ck-us.net> 於 2024年11月21日 週四 下午10:15寫道：
> > +config NCT6694_WATCHDOG
> > +     tristate "Nuvoton NCT6694 watchdog support"
> > +     depends on MFD_NCT6694
> > +     select WATCHDOG_CORE
> > +     help
> > +       If you say yes to this option, support will be included for Nuvoton
> > +       NCT6694, a USB device to watchdog timer.
> > +
>
> It is a peripheral expander, not a "USB device to watchdog timer". Watchdog is only
> a small part of its functionality.
>

Understood. I will make the modifications in v3.

> > +       This driver can also be built as a module. If so, the module will
> > +       be called nct6694_wdt.
...
> > +     ret = nct6694_wdt_setting(wdev, wdev->timeout, NCT6694_ACTION_GPO,
> > +                               wdev->pretimeout, NCT6694_ACTION_GPO);
> > +     if (ret)
> > +             return ret;
> > +
> > +     dev_info(data->dev, "Setting WDT(%d): timeout = %d, pretimeout = %d\n",
> > +              data->wdev_idx, wdev->timeout, wdev->pretimeout);
> > +
>
> This is logging noise. Drop or set as debug message.
>

Okay, I'll drop it in v3.

> > +     return ret;
> > +}
...
> > +static int nct6694_wdt_set_timeout(struct watchdog_device *wdev,
> > +                                unsigned int timeout)
> > +{
> > +     struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev);
> > +     int ret;
> > +
> > +     if (timeout < wdev->pretimeout) {
> > +             dev_warn(data->dev, "pretimeout < timeout. Setting to zero\n");
> > +             wdev->pretimeout = 0;
> > +     }
> > +
> This is only necessary if the pretimeout was not validated during probe
> since otherwise the watchdog core does the check. Please validate it there.
>

Understood. I will make the modifications in v3.

> > +     ret = nct6694_wdt_setting(wdev, timeout, NCT6694_ACTION_GPO,
> > +                               wdev->pretimeout, NCT6694_ACTION_GPO);
> > +     if (ret)
> > +             return ret;
> > +
> > +     wdev->timeout = timeout;
> > +
> > +     return ret;
>
> ret == 0 here, so return 0.
>

Okay, fix it in v3.

> > +}
> > +
> > +static int nct6694_wdt_set_pretimeout(struct watchdog_device *wdev,
> > +                                   unsigned int pretimeout)
> > +{
> > +     int ret;
> > +
> > +     ret = nct6694_wdt_setting(wdev, wdev->timeout, NCT6694_ACTION_GPO,
> > +                               pretimeout, NCT6694_ACTION_GPO);
> > +     if (ret)
> > +             return ret;
> > +
> > +     wdev->pretimeout = pretimeout;
> > +
> > +     return ret;
>
> ret == 0 here, so return 0.
>

Okay, fix it in v3.

> > +}
> > +
> > +static unsigned int nct6694_wdt_get_time(struct watchdog_device *wdev)
> > +{
> > +     struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev);
> > +     struct nct6694_wdt_cmd0 *buf = (struct nct6694_wdt_cmd0 *)data->xmit_buf;
> > +     struct nct6694 *nct6694 = data->nct6694;
> > +     unsigned int timeleft_ms;
> > +     int ret;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = nct6694_read_msg(nct6694, NCT6694_WDT_MOD,
> > +                            NCT6694_WDT_CMD0_OFFSET(data->wdev_idx),
> > +                            NCT6694_WDT_CMD0_LEN, buf);
> > +     if (ret)
> > +             return ret;
>
> The function does not return an error code. Return 0 instead.

Okay, fix it in v3.

> > +
> > +     timeleft_ms = le32_to_cpu(buf->countdown);
> > +
> > +     return timeleft_ms / 1000;
> > +}
...
> > +     wdev = &data->wdev;
> > +     wdev->info = &nct6694_wdt_info;
> > +     wdev->ops = &nct6694_wdt_ops;
> > +     wdev->timeout = timeout;
> > +     wdev->pretimeout = pretimeout;
>
> pretimeout should be validated here.
>

Understood. I will make the modifications in v3.

Best regards,
Ming