Re: [PATCH 5/5] 2.6.13-rc5-mm1, driver for IBM Automatic Server Restart watchdog

[Andrew Morton <akpm@osdl.org>]

Andrey Panin <pazke@donpac.ru> wrote:
>
> 
> This patch adds driver for IBM Automatic Server Restart watchdog hardware
> found in some IBM eServer xSeries machines. This driver is based on the ugly
> driver provided by IBM. Driver was tested on IBM eServer 226.
> 
> ...
> +
> +	case ASMTYPE_JASPER:
> +		type = "Jaspers ";
> +
> +		/* FIXME: need to use pci_config_lock here, but it's not exported */

That's gregkh material.

> +
> +/*		spin_lock_irqsave(&pci_config_lock, flags);*/
> +
> +		/* Select the SuperIO chip in the PCI I/O port register */
> +		outl(0x8000f858, 0xcf8);
> +
> +		/* 
> +		 * Read the base address for the SuperIO chip.
> +		 * Only the lower 16 bits are valid, but the address is word 
> +		 * aligned so the last bit must be masked off.
> +		 */
> +		asr_base = inl(0xcfc) & 0xfffe;
> +
> +/*		spin_unlock_irqrestore(&pci_config_lock, flags);*/
>
> ...
>
> +static int asr_ioctl(struct inode *inode, struct file *file,
> +		     unsigned int cmd, unsigned long arg)
> +{
> +	static const struct watchdog_info ident = {
> +		.options =	WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> +				WDIOF_MAGICCLOSE,
> +		.identity =	"IBM ASR"
> +	};
> +	void __user *argp = (void __user *)arg;
> +	int __user *p = argp;
> +	int heartbeat;
> +
> +	switch (cmd) {
> +		case WDIOC_GETSUPPORT:
> +			return copy_to_user(argp, &ident, sizeof(ident)) ? 
> +				-EFAULT : 0;
> +
> +		case WDIOC_GETSTATUS:
> +		case WDIOC_GETBOOTSTATUS:
> +			return put_user(0, p);
> +
> +		case WDIOC_KEEPALIVE:
> +			asr_toggle();
> +			return 0;
> +
> +
> +		case WDIOC_SETTIMEOUT:
> +			if (get_user(heartbeat, p))
> +				return -EFAULT;
> +			/* Fall */
> +
> +		case WDIOC_GETTIMEOUT:
> +			heartbeat = 256;
> +			return put_user(heartbeat, p);

Something very wrong is happening with WDIOC_SETTIMEOUT and
WDIOC_GETTIMEOUT.  They're both no-ops and the effect of WDIOC_SETTIMEOUT
is immidiately overwritten.