linux-kernel - Re: [PATCH 3/3] watchdog: Add Congatec CGEB 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: <20120215134122.GC3852@pengutronix.de>
Date:	Wed, 15 Feb 2012 14:41:22 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Wolfram Sang <w.sang@...gutronix.de>
Cc:	linux-kernel@...r.kernel.org, Samuel Ortiz <sameo@...ux.intel.com>,
	Jean Delvare <khali@...ux-fr.org>, linux-i2c@...r.kernel.org,
	linux-watchdog@...r.kernel.org, Ben Dooks <ben-linux@...ff.org>,
	kernel@...gutronix.de
Subject: Re: [PATCH 3/3] watchdog: Add Congatec CGEB watchdog driver

On Tue, Feb 07, 2012 at 04:08:56PM +0100, Wolfram Sang wrote:
> Hi Sascha,
> 
> > +struct cgeb_watchdog_stage {
> > +	unsigned long timeout;
> > +	unsigned long event;
> > +};
> > +
> > +struct cgeb_watchdog_config {
> > +	unsigned long size;
> > +	unsigned long timeout; /* not used in staged mode */
> > +	unsigned long delay;
> > +	unsigned long mode;
> > +	/* optional parameters for staged watchdog */
> > +	unsigned long op_mode;
> > +	unsigned long stage_count;
> > +	struct cgeb_watchdog_stage stages[CGOS_WDOG_EVENT_MAX_STAGES];
> > +};
> > +
> 
> There is some unused stuff in here.

It's the hardware, or more precisely the BIOS that dictates the layout
of this structure. Given that, the members should be u32 rather than
unsigned long.

> 
> > +struct cgeb_watchdog_priv {
> > +	struct cgeb_board_data	*board;
> > +	struct watchdog_device	wdd;
> > +	unsigned int		timeout_s;
> 
> Is wdd->timeout not sufficent?

I wasn't aware such a field exists. Yes, that should work

> 
> > +	int unit;
> > +};
> > +
> > +static int cgeb_watchdog_set_timeout(struct watchdog_device *wdd,
> > +		unsigned int timeout_s)
> > +{
> > +	struct cgeb_watchdog_priv *priv = watchdog_get_drvdata(wdd);
> > +
> > +	if (!timeout_s)
> > +		return -EINVAL;
> 
> Is this possible? You have min_timeout = 1.

Nope, I think we can remove this line.

> 
> > +static int __devinit cgeb_watchdog_probe(struct platform_device *pdev)
> > +{
> > +	struct cgeb_watchdog_priv *priv;
> > +	struct cgeb_pdata *pdata = pdev->dev.platform_data;
> > +	int ret;
> > +
> > +	dev_info(&pdev->dev, "registering\n");
> 
> "registered" on success would be more useful?
> 
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->wdd.ops = &cgeb_watchdog_ops;
> > +	priv->wdd.info = &cgeb_wdd_info;
> > +	priv->wdd.min_timeout = 1;
> > +	priv->wdd.max_timeout = 3600;
> > +	priv->board = pdata->board;
> > +	priv->unit = pdata->unit;
> > +
> > +	watchdog_set_drvdata(&priv->wdd, priv);
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	ret = watchdog_register_device(&priv->wdd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> return watchdog_register_device();

These comments are mutually exclusive ;)

I'll move the dev_info to here.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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/