linux-kernel - Re: [PATCH v6] watchdog: New watchdog driver for MEN A21 watchdogs

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: <20130531125519.GB8983@jtlinux>
Date:	Fri, 31 May 2013 14:55:19 +0200
From:	Johannes Thumshirn <johannes.thumshirn@....de>
To:	Guenter Roeck <linux@...ck-us.net>
CC:	<wim@...ana.be>, <guenter@...ck-us.net>,
	<linux-watchdog@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<johannes.thumshirn@....de>
Subject: Re: [PATCH v6] watchdog: New watchdog driver for MEN A21 watchdogs

Hi Guenther,
On Fri, May 31, 2013 at 04:40:37AM -0700, Guenter Roeck wrote:
> > +#define GPIO_WD_ENAB	169
> > +#define GPIO_WD_FAST	170
> > +#define GPIO_WD_TRIG	171
> > +
> > +#define GPIO_RST_CAUSE_BASE 166
> > +
>
> I think I asked that before ... as you are supporting devicetree, gpio pins
> should really be provided through devicetree properties and not be hardcoded.
>
Yes you did and I didn't come up with a solution to this problem yet. I understand
and agree to your concerns but I'm lacking example code/documentation for it, maybe
you can point me to an example on that and then I'll update my code accordingly.

> > +struct a21_wdt_drv {
> > +	struct watchdog_device wdt;
> > +	struct mutex lock;
> > +	}
> > +
> > +	if (timeout == 30 && wdt->timeout == 1) {
> > +		dev_err(wdt->dev,
> > +			"Transition from fast to slow mode not allowed\n");
> > +		return -EINVAL;
> > +	}
> > +
> I dislike that [2 .. 29] timeout values are not supported, and would rather have
> you accept that range and set the timeout to 30. Also, it is somewhat undesirable
> that the timeout can not be changed back to 30 after being set to 1.

The not changing back issue was a hardware design criterion and I have no
influence on that one. I've already checked with the IC developer and he said,
this is intended. I don't quite understand why I should accept the timeout and
set it to 30? In my opinion this only introduces some magic the user won't
understand. Instead failing and giving an error message is something a user can
take as a hint for investigating.

> As Wim recommended, a softdog on top of the hardware watchdog would be the best
> solution. I'll leave it up to him to decide if he wants to accept the code
> as-is.

Yes let's see what he's saying.

>
> > +	mutex_lock(&drv->lock);
> > +
> > +	ret = watchdog_register_device(&a21_wdt);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Cannot register watchdog device\n");
> > +		goto err_register_wd;
> > +	}
> > +
> > +	ret = a21_wdt_get_bootstatus(&reset);
> > +	if (ret)
> > +		dev_warn(&pdev->dev, "Reset Cause contains invalid data\n");
> > +	else {
> > +		if (reset == 2)
> > +			a21_wdt.bootstatus |= WDIOF_EXTERN1;
> > +		else if (reset == 4)
> > +			a21_wdt.bootstatus |= WDIOF_CARDRESET;
> > +		else if (reset == 5)
> > +			a21_wdt.bootstatus |= WDIOF_POWERUNDER;
> > +		else if (reset == 7)
> > +			a21_wdt.bootstatus |= WDIOF_EXTERN2;
>
> What about other causes ? No useful match ?
>

None I could find. Actually WDIOF_EXTERN[12] already are a pretty creative
mapping in my opinion. EXTERN1 is a "Push Button" event, EXTERN2 is a reset
caused by a JTAG/BDM adapter...

> I think bootstatus should be set prior to registering the watchdog device
> to avoid race conditions where an application reads it prior to being set.
>
Agreed, didn't see that one *doh*, must be fixed before inclusion.
> > +	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/