linux-kernel - RE: [PATCH v6 3/8] watchdog: hpe-wdt: Introduce HPE GXP 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]

Message-ID: <PH0PR84MB1718D28F1846F54DB5766E6D88C09@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Tue, 3 May 2022 16:22:35 +0000
From:   "Hawkins, Nick" <nick.hawkins@....com>
To:     Guenter Roeck <linux@...ck-us.net>,
        "Verdun, Jean-Marie" <verdun@....com>,
        "joel@....id.au" <joel@....id.au>, "arnd@...db.de" <arnd@...db.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
Subject: RE: [PATCH v6 3/8] watchdog: hpe-wdt: Introduce HPE GXP Watchdog

On 5/2/22 13:40, nick.hawkins@....com wrote:
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>

> Where are those of_ includes used ?

They were not used anymore with latest changes. Thank you for pointing this out. I will remember to check in the future for each new commit to double check this.

> > +#define WDT_MAX_TIMEOUT_MS	655000

> Shouldn't that be 655350 ?

Yes it should be. I will correct this.

> > +static int gxp_wdt_set_timeout(struct watchdog_device *wdd,
> > +			       unsigned int timeout)
> > +{
> > +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
> > +	u32 actual;
> > +
> > +	wdd->timeout = timeout;
> > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +	writew(SECS_TO_WDOG_TICKS(actual), drvdata->base + GXP_WDT_CNT_OFS);

> First, the accuracy of actual is reduced to 1 second, then SECS_TO_WDOG_TICKS() multiplies the result with 100, meaning the actual accuracy is 10ms. Why not just use 10 ms ?

>	actual = min(timeout * 100, wdd->max_hw_heartbeat_ms / 10);
>	writew(actual, drvdata->base + GXP_WDT_CNT_OFS);

I have replaced the mention code with what you recommended above.

> > +
> > +static int gxp_restart(struct watchdog_device *wdd, unsigned long action,
> > +		       void *data)
> > +{
> > +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
> > +
> > +	writew(10, drvdata->base + GXP_WDT_CNT_OFS);

> Doesn't that translate to 100 ms timeout ? Why such a large reboot delay instead of writing 1 ?

This has been changed to 1.

> > +	gxp_wdt_enable_reload(drvdata);
> > +	mdelay(100);
> > +	return 0;
> > +}
> > +
> > +static int gxp_wdt_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct gxp_wdt *drvdata;
> > +	int err;
> > +	u8 val;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(struct gxp_wdt), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	drvdata->base = (void __iomem *)dev->platform_data;

> I'd personaly prefer if the address was passed as resource.

Just to clarify for my understanding are you asking that in the device structure I use the "void *platform_data" to pass "struct *resource"? If I am incorrect here can you elaborate on what you would like to be done? Based on feedback in review for the device tree; the watchdog is being created as a child to the timer. Therefore the conclusion reached was there should not be a gxp-wdt listed in the device tree files. I took this implementation based on what I found in ixp4xx_wdt.c.

Thank you for your time and feedback Guenter,

-Nick Hawkins