Message-ID: <20091215164950.1fd41caf@bike.lwn.net>
Date:	Tue, 15 Dec 2009 16:49:50 -0700
From:	Jonathan Corbet <corbet@....net>
To:	Keith Mannthey <kmannth@...ibm.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	John Stultz <johnstul@...ibm.com>
Subject: Re: [RFC][Patch] IBM Real-Time "SMI Free" mode drive -v2

On Tue, 15 Dec 2009 12:09:48 -0800
Keith Mannthey <kmannth@...ibm.com> wrote:

> This driver supports the Real-Time Linux (RTL) BIOS feature.  The RTL
> feature allows non-fatal System Management Interrupts (SMIs) to be
> disabled on supported IBM platforms. 

...and that seems like a really useful thing to be able to do.

A few notes, while I was in the neighborhood...


> --- linux-2.6.32/drivers/misc/ibmrtl/ibm_rtl.c	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.32-rtl/drivers/misc/ibmrtl/ibm_rtl.c	2009-12-14 16:37:19.000000000 -0800

Do you need to create a new directory for a single .c file?

> @@ -0,0 +1,189 @@
> +/*
> + * IBM Premium Real-Time Mode Device Driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) IBM Corporation, 2008, 2009
> + *
> + * Author: Keith Mannthey <kmannth@...ibm.com>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/sysdev.h>
> +#include <asm/byteorder.h>
> +#include "rtl.h"
> +
> +static spinlock_t rtl_lock;

You should probably include <linux/spinlock.h> for this.

> +static void __iomem *rtl_table;
> +static void __iomem *edba_map;
> +
> +static int ibm_rtl_write(u8 value)
> +{
> +	int count = 0, ret = 0;
> +	u32 cmd_prt_value, cmd_port_addr;
> +
> +	if ((value != RTL_ENABLE) && (value != RTL_DISABLE))
> +		return -EINVAL;
> +
> +	spin_lock(&rtl_lock);
> +
> +	if (readb(rtl_table+RTL_STATE) != value) {
> +		writeb(value, (rtl_table+RTL_CMD));
> +
> +		/* Trigger the command to be processed*/
> +		cmd_prt_value = readw(rtl_table + RTL_CMD_PORT_VALUE);
> +		cmd_port_addr = readw(rtl_table + RTL_CMD_PORT_ADDR);
> +		outb(cmd_prt_value, cmd_port_addr);
> +
> +		/* Wait for the command to finish */
> +		while (readb(rtl_table+RTL_CMD)) {
> +			mdelay(10);

For a driver which is intended to help reduce latencies, a 10ms delay with
a spinlock held seems a little harsh.  It seems like maybe you could drop
the lock and use msleep()?

> +			if (count++ > 10000) {

...that's 100 seconds total - a *long* time...

> +				printk(KERN_ERR "IBM_RTL: Hardware not "
> +					"responding to mode switch request\n");
> +				spin_unlock(&rtl_lock);
> +				return -EIO;
> +			}
> +		}
> +
> +		if (readb(rtl_table + RTL_CMD_STATUS))
> +			ret = -EIO;
> +	}
> +
> +	spin_unlock(&rtl_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t rtl_show_version(struct sysdev_class *dev, char *buf)
> +{
> +	return sprintf(buf, "%d\n", (int) readb(rtl_table+RTL_VERSION));
> +}
> +
> +static ssize_t rtl_show_state(struct sysdev_class *dev, char *buf)
> +{
> +	return sprintf(buf, "%d\n", readb(rtl_table+RTL_STATE));
> +}
> +
> +static ssize_t rtl_set_state(struct sysdev_class *dev, const char *buf,
> +				 size_t size)
> +{
> +	ssize_t ret;
> +
> +	if (size != 2)
> +		return -EINVAL;

People do use "echo -n" to write to sysfs files sometimes.

> +
> +	switch (buf[0]) {
> +	case '0':
> +		ret = ibm_rtl_write(RTL_DISABLE);
> +		break;
> +	case '1':
> +		ret = ibm_rtl_write(RTL_ENABLE);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	if (ret >= 0)
> +		ret = size;
> +
> +	return ret;
> +}
> +
> +static struct sysdev_class class_rtl = {
> +	.name = "ibm_rtl",
> +};
> +
> +static SYSDEV_CLASS_ATTR(version, S_IRUGO, rtl_show_version, NULL);
> +static SYSDEV_CLASS_ATTR(state, 0600, rtl_show_state, rtl_set_state);
> +
> +static struct sysdev_class_attribute *rtl_attributes[] = {
> +	&attr_version,
> +	&attr_state,
> +	NULL
> +};
> +
> +static int rtl_setup_sysfs(void)
> +{
> +	int ret, i;
> +
> +	ret = sysdev_class_register(&class_rtl);
> +
> +	if (!ret) {
> +		for (i = 0; rtl_attributes[i]; i++)
> +			sysdev_class_create_file(&class_rtl, rtl_attributes[i]);
> +	}
> +	return ret;
> +}
> +
> +static void rtl_teardown_sysfs(void)
> +{
> +	int i;
> +
> +	for (i = 0; rtl_attributes[i]; i++)
> +		sysdev_class_remove_file(&class_rtl, rtl_attributes[i]);
> +
> +	sysdev_class_unregister(&class_rtl);
> +	return;
> +}
> +
> +/* Only allow the modules to load if the _RTL_ table can be found */
> +int init_module(void)

This needs to be static.

> +{
> +	unsigned long ebda_size, ebda_addr;
> +	unsigned int ebda_kb;
> +	u32 *table;
> +	int ret;
> +
> +	/* Get the address for the Extende Biso Data Area */
> +	ebda_addr = *(u16 *) phys_to_virt(EDBA_ADDR);
> +	ebda_addr <<= 4;
> +	edba_map = ioremap(ebda_addr, 4);

Me confused.  Where is this data area, and how are you remapping it?  This
could use an explanatory comment.

> +	if (!edba_map)
> +		return -ENOMEM;
> +
> +	/* First word in the EDBA is the Size in KB */
> +	ebda_kb = *(u32 *) edba_map;

Probably better to use ioread32() here.

> +	ebda_size = ebda_kb*1024;
> +	iounmap(edba_map);
> +
> +	/* Remap the whole table */
> +	edba_map = ioremap(ebda_addr, ebda_size);
> +	if (!edba_map)
> +		return -ENOMEM;
> +
> +	for (table = (u32 *) edba_map ; \
> +			table < (u32 *) edba_map + ebda_size/4; table++)
> +		if (*table == RTL_MAGIC_IDENT) {

Again, you should use ioread32() here.

> +			rtl_table = table;
> +			ret = rtl_setup_sysfs();
> +			return ret;
> +		}
> +
> +	ret = -ENODEV;
> +	iounmap(edba_map);
> +	return ret;
> +}
> +
> +void cleanup_module(void)

static

> +{
> +	rtl_teardown_sysfs();
> +	iounmap(edba_map);
> +}
> +
> +MODULE_LICENSE("GPL");
> diff -urN linux-2.6.32/drivers/misc/ibmrtl/Makefile linux-2.6.32-rtl/drivers/misc/ibmrtl/Makefile
> --- linux-2.6.32/drivers/misc/ibmrtl/Makefile	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.32-rtl/drivers/misc/ibmrtl/Makefile	2009-12-11 10:24:23.000000000 -0800
> @@ -0,0 +1 @@
> +obj-$(CONFIG_IBM_RTL) := ibm_rtl.o
> diff -urN linux-2.6.32/drivers/misc/ibmrtl/rtl.h linux-2.6.32-rtl/drivers/misc/ibmrtl/rtl.h
> --- linux-2.6.32/drivers/misc/ibmrtl/rtl.h	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.32-rtl/drivers/misc/ibmrtl/rtl.h	2009-12-11 10:24:23.000000000 -0800
> @@ -0,0 +1,27 @@
> +#include <linux/io.h>
> +
> +/* The RTL table looks something like
> +	u8 signature[5];
> +	u8 version;
> +	u8 RT_Status;
> +	u8 Command;
> +	u8 CommandStatus;
> +	u8 CMDAddressType;
> +	u8 CmdGranularity;
> +	u8 CmdOffset;
> +	u16 Reserve1;
> +	u8 CmdPortAddress[4];
> +	u8 CmdPortValue[4];
> +*/
> +#define RTL_TABLE_SIZE 0x16
> +#define RTL_MAGIC_IDENT (('L'<<24)|('T'<<16)|('R'<<8)|'_')
> +#define RTL_VERSION 0x5
> +#define RTL_STATE 0x6
> +#define RTL_CMD 0x7
> +#define RTL_CMD_STATUS 0x8
> +#define RTL_CMD_PORT_ADDR 0xE
> +#define RTL_CMD_PORT_VALUE 0x12
> +
> +#define EDBA_ADDR 0x40E
> +#define RTL_ENABLE 1
> +#define RTL_DISABLE 2

These seem like a bit of a strange obfuscation; why not just use a normal,
zero-or-one int value?

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