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]
Date:	Fri, 31 May 2013 21:15:23 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Johannes Thumshirn <johannes.thumshirn@....de>
Cc:	wim@...ana.be, guenter@...ck-us.net,
	linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] watchdog: New watchdog driver for MEN A21 watchdogs

On Fri, May 31, 2013 at 02:55:19PM +0200, Johannes Thumshirn wrote:
> 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.
> 

Have a look at Documentation/devicetree/bindings/gpio/gpio-fan.txt and
drivers/hwmon/gpio-fan.c.

> > > +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.
> 
On the other side you expect the user to know about the 1 / 30 limitation, which
I think is worse. The value actually set is returned by the ioctl per API, so
the application can check the actually configured value. As such, it is not
entirely true that the behavior would be unexpected.

> > 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.
> 
Agreed.

> >
> > > +	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...
> 
ok.

> > 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.

Happens to me all the time :)

Guenter
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ