linux-kernel - Re: [PATCH 1/2] watchdog: bcm281xx: Watchdog Driver

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: <20131112233911.16eba130@alan.etchedpixels.co.uk>
Date:	Tue, 12 Nov 2013 23:39:11 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	"Markus Mayer" <markus.mayer@...aro.org>
Cc:	"Wim Van Sebroeck" <wim@...ana.be>,
	"Christian Daudt" <bcm@...thebug.org>,
	"Linaro Patches" <patches@...aro.org>,
	"Matt Porter" <matt.porter@...aro.org>,
	"Linux Watchdog List" <linux-watchdog@...r.kernel.org>,
	"ARM Kernel List" <linux-arm-kernel@...ts.infradead.org>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] watchdog: bcm281xx: Watchdog Driver


> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
> +{
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (wdt->resolution > SECWDOG_MAX_RES)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_RES_MASK;
> +		val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;

This is I think the wrong choice of return. If the register fails to read
then presumably the device is b*ggered ? In which case return something
like -EIO and log something nasty.

EAGAIN has fairly specific semantics around signals and/or specific
requests for an I/O operation not to wait.

> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_COUNT_MASK;
> +		val |= SECS_TO_TICKS(wdog->timeout, wdt);
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);

As an aside you could fold most of these functions into one single helper
method that read CTRL_REG, did the and and or and wrote the result back
rather than repeating yourself. But hey if you like typing...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

What if there is no resource present ?

> +	dev_info(dev, "Broadcom Kona Watchdog Timer");

The module load succeeded - the user knows it loaded from that. Likewise
the unload spewage is unwanted and the kernel would just drown in such
messages if we didn't keep them in check. If you need the for debug then
mark them dev_dbg but otherwise bin them.

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