linux-kernel - RE: [PATCH v6 2/2] watchdog: Add watchdog driver for Sunplus SP7021

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: <f61d99bff64e4a49b2a2943d01d9b6d2@cqplus1.com>
Date:   Mon, 28 Feb 2022 10:04:34 +0000
From:   xt.hu[胡先韬] <xt.hu@...lus1.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        qinjian[覃健] <qinjian@...lus1.com>
Subject: RE: [PATCH v6 2/2] watchdog: Add watchdog driver for Sunplus SP7021

...
> > +
> > +/* TIMEOUT_MAX = ffff0/90kHz =11.65,so longer than 11 seconds will time out */
> > +static int sp_wdt_ping(struct watchdog_device *wdev)
> > +{
> > +	struct sp_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	void __iomem *base = priv->base;
> > +	u32 count;
> > +	u32 actual;
> > +
> > +	actual = min(wdev->timeout, SP_WDT_MAX_TIMEOUT);
> > +
> > +	if (actual > SP_WDT_MAX_TIMEOUT) {
> 
> This is never true. because actual is set to the minimum of wdev->timeout
> and SP_WDT_MAX_TIMEOUT. It is also not clear to me what this code path
> is supposed to be doing, compared to the code below. Please fix and add
> a comment explaining the code (ie what does writing WDT_CONMAX into
> the control register do ?)
> 

I will fix "if (actual > SP_WDT_MAX_TIMEOUT)" to "if (wdev->timeout > 
SP_WDT_MAX_TIMEOUT)". I will add comment "WDT_CONMAX sets the 
count to the maximum (down-counting)."
When timeout > SP_WDT_MAX_TIMEOUT, I try to set a smaller value or 
do nothing. Watchdog driver don't work properly.

> > +		writel(WDT_CONMAX, base + WDT_CTRL);
> > +	} else {
> > +		writel(WDT_UNLOCK, base + WDT_CTRL);
> > +		/* tiemrw_cnt[3:0]can't be write,only [19:4] can be write. */
> 
> timerw_cnd ?, space before can't, and s/write/written/g.
> 
> > +		count = (actual * STC_CLK) >> 4;
> 
> so this is (actual * 90000) >> 4 and does set bit 0..3. Is that intentional
> or is it supposed to be << 4 ? The comment above does not really explain
> the calculation; if anything, it just creates confusion.
> 

I will add comment "Watchdog timer is a 20-bit down-counting based 
on STC_CLK. This register bits[16]~[0] is from bit[19]~bit[4] of the 
watchdog timer counter. "

Thanks,
Best Regards