[PATCH] IPMI driver for Linux, version 5

[PATCH] IPMI driver for Linux, version 5

[Corey Minyard]

I have put a new version of the IPMI driver on my home page 
(http://home.attbi.com/~minyard) that removes the Linus-incompatable 
typedefs.  The only typedefs left are "handle" ones.

-Corey

PS - In case you don't know, IPMI is a standard for system management, 
it provides ways to detect the managed devices in the system and sensors 
attached to them.  You can get more information at 
http://www.intel.com/design/servers/ipmi/spec.htm

Re: [PATCH] IPMI driver for Linux, version 5

[Randy.Dunlap]

On Wed, 9 Oct 2002, Corey Minyard wrote:

| I have put a new version of the IPMI driver on my home page
| (http://home.attbi.com/~minyard) that removes the Linus-incompatable
| typedefs.  The only typedefs left are "handle" ones.
|
| PS - In case you don't know, IPMI is a standard for system management,
| it provides ways to detect the managed devices in the system and sensors
| attached to them.  You can get more information at
| http://www.intel.com/design/servers/ipmi/spec.htm
| -


Hi Corey,

Thanks for eliminating most of those typedefs.
What is a handle, and why did you leave those?

I applied and built this on 2.5.41 (in-kernel and modular).
Only one compile warning:
## drivers/char/ipmi/ipmi_kcs_sm.c:234: warning: implicit declaration of function 'memcpy'

Alan has reviewed this driver, right?  (at least an earlier
version of it)

And you are asking for a major device number assignment?

I believe that this driver is in pretty good condition for
addition to the 2.5 kernel.

Specific comments below, each beginning with "##".

Warning to others: long-ish.

~Randy


+++ linux/drivers/char/Config.help	Fri Sep 20 08:37:00 2002

+CONFIG_IPMI_EMULATE_RADISYS
+  This enables emulation of the Radisys IPMI device driver.
+CONFIG_IPMI_EMULATE_INTEL
+  This enables emulation of the Intel IMB device driver.

## These CONFIG_'s aren't used.  Can be removed from help file
## unless there is code to be added for them.

+++ linux/drivers/char/ipmi/Makefile	Thu Aug 22 20:58:47 2002

+O_TARGET	:= built-in.o
## delete O_TARGET for 2.5.lately

+list-multi := ipmi_kcs_drv.o
## delete list-multi for 2.5.lately

+++ linux/drivers/char/ipmi/ipmi_devintf.c	Wed Oct  9 14:38:56 2002

+static int init_ipmi_devintf(void)
## use __init here.
+{
+	int rv;
...
+	rv = ipmi_smi_watcher_register(&smi_watcher);
+	if (rv) {
+		printk(KERN_WARNING "ipmi: can't register smi watcher");
##  need to undo/return/free anything here ?? (see cleanup_ipmi())
+		return rv;
+	}
+}

+#ifdef MODULE
##  don't need this #ifdef
+static void cleanup_ipmi(void)
##  use __exit here
+{
+	ipmi_smi_watcher_unregister(&smi_watcher);
+	devfs_unregister(devfs_handle);
+	unregister_chrdev(ipmi_major, DEVICE_NAME);
+}
+module_exit(cleanup_ipmi);
+#else
## #ifndef MODULE (or nothing since it's __init)
+static int __init ipmi_setup (char *str)
+{
...
+}
+#endif

+++ linux/drivers/char/ipmi/ipmi_kcs_intf.c	Wed Oct  9 14:38:56 2002

+	/* Flags from the last GET_MSG_FLAGS command, used when an ATTN
+	   is set to hold the flags until we are done handling everything
+	   from the flags. */
+	unsigned char       msg_flags;
#define RECEIVE_MSG_AVAIL	0x01
#define EVENT_MSG_BUFFER_FULL	0x02
#define WDT_PRE_TIMEOUT_INT	0x08
## and use these instead of magic numbers when testing <msg_flags> below.

+static int init_ipmi_kcs(void)
## use __init

+#ifdef MODULE
+void cleanup_one_kcs(struct kcs_info *to_clean)
## use __exit

+static void cleanup_ipmi_kcs(void)
## use __exit

+++ linux/drivers/char/ipmi/ipmi_kcs_sm.c	Fri Aug 23 12:03:31 2002

+/* The states the KCS driver may be in. */
+typedef enum kcs_states_e {
...
+} kcs_states_t;
## still have typedefs

+int kcs_get_result(kcs_data_t *kcs, unsigned char *data, int length)
+{
...
+		data[2] = 0xc6; /* Report a truncated error.  We might
+                                   overwrite another error, but that's
+                                   too bad, the user needs to know it
+                                   was truncated. */
## use #defines (with useful names) for magic values

+int kcs_size(void)
+{
+	return sizeof(kcs_data_t);
+}
## why a function ? maybe inline in a header file ?

+++ linux/drivers/char/ipmi/ipmi_kcs_sm.h	Fri Aug 23 11:21:44 2002

+typedef struct kcs_data_s kcs_data_t;
+typedef enum kcs_result_e
+{
...
+} kcs_result_t;
## still have typedefs

+/* Return the size of the KCS structure in bytes. */
+int kcs_size(void);
## put inline here.

+++ linux/drivers/char/ipmi/ipmi_msghandler.c	Wed Oct  9 14:38:56 2002

+int ipmi_request(ipmi_user_t      *user,
+		 struct ipmi_addr *addr,
+		 long             msgid,
+		 struct ipmi_msg  *msg,
+		 int              priority)
+{
+	return i_ipmi_request(user,
+			      user->intf,
+			      addr,
+			      msgid,
+			      msg,
+			      NULL, NULL,
+			      priority);
+}
## why not inline ?

+int ipmi_request_supply_msgs(ipmi_user_t          *user,
+			     struct ipmi_addr     *addr,
+			     long                 msgid,
+			     struct ipmi_msg      *msg,
+			     void                 *supplied_smi,
+			     struct ipmi_recv_msg *supplied_recv,
+			     int                  priority)
+{
+	return i_ipmi_request(user,
+			      user->intf,
+			      addr,
+			      msgid,
+			      msg,
+			      supplied_smi,
+			      supplied_recv,
+			      priority);
+}
## why not inline ?

## Lots of pre-function comments throughout the driver(s) should be in
## typical kernel style, like this one copied from kernel/exit.c:
/*
 * Send signals to all our closest relatives so that they know
 * to properly mourn us..
 */
## as compared to:
+/* Handle a new message.  Return 1 if the message should be requeued,
+   0 if the message should be freed, or -1 if the message should not
+   be freed or requeued. */

+static int has_paniced = 0;
## why are these (data, code) not inside CONFIG_IPMI_PANIC_EVENT ?
## along with notifier_chain_[un]register() calls ?
## except do it _without_ #ifdefs in the middle of functions.
## i.e., hide the #ifdefs in a header file.
+static int panic_event(struct notifier_block *this,
+		       unsigned long         event,
+                       void                  *ptr)
+{
+	int        i;
+	ipmi_smi_t *intf;
+
+	if (has_paniced)
+		return NOTIFY_DONE;
+	has_paniced = 1;
+
+	/* For every registered interface, set it to run to completion. */
+	for (i=0; i<MAX_IPMI_INTERFACES; i++) {
+		intf = ipmi_interfaces[i];
+		if (intf == NULL)
+			continue;
+
+		intf->handlers->set_run_to_completion(intf->send_info, 1);
+	}
+
+#ifdef CONFIG_IPMI_PANIC_EVENT
+	send_panic_events();
+#endif
## hide this #ifdef in a header file.
+
+	return NOTIFY_DONE;
+}

+static int ipmi_init_msghandler(void)
+{
...
+	notifier_chain_register(&panic_notifier_list, &panic_block);
## should be conditional on CONFIG_IPMI_PANIC_EVENT

+#ifdef MODULE
## don't need this #ifdef
+static void cleanup_ipmi(void)
## use __exit

+	notifier_chain_unregister(&panic_notifier_list, &panic_block);
## should be conditional on CONFIG_IPMI_PANIC_EVENT

+++ linux/drivers/char/ipmi/ipmi_watchdog.c	Wed Oct  9 14:38:56 2002

+static void ipmi_wdog_pretimeout_handler(void *handler_data)
+{
+	panic("Watchdog pre-timeout");
## why panic on a pre-timeout condition ?

+#ifdef MODULE
+static void ipmi_unregister_watchdog(void)
## can be __exit

+++ linux/include/linux/ipmi.h	Wed Oct  9 14:38:56 2002

+#ifdef __KERNEL__
## why is this needed?  kernel headers shouldn't be used in userspace.

+typedef struct ipmi_user_s ipmi_user_t;
## remove typedefs

+/*
+ * The userland interface
+ */
## userspace interface in a kernel header file ??  not good.

+++ linux/include/linux/ipmi_smi.h	Wed Oct  9 14:38:56 2002

+typedef struct ipmi_smi_s ipmi_smi_t;
## remove typedefs

+++ linux/kernel/ksyms.c	Fri Sep 20 08:37:16 2002

+EXPORT_SYMBOL(panic_notifier_list);
## don't need this.  (it's already extern; build works without this)

--

Re: [PATCH] IPMI driver for Linux, version 5

[Corey Minyard]

Randy.Dunlap wrote:

>On Wed, 9 Oct 2002, Corey Minyard wrote:
>
>| I have put a new version of the IPMI driver on my home page
>| (http://home.attbi.com/~minyard) that removes the Linus-incompatable
>| typedefs.  The only typedefs left are "handle" ones.
>|
>| PS - In case you don't know, IPMI is a standard for system management,
>| it provides ways to detect the managed devices in the system and sensors
>| attached to them.  You can get more information at
>| http://www.intel.com/design/servers/ipmi/spec.htm
>| -
>
>
>Hi Corey,
>
>Thanks for eliminating most of those typedefs.
>What is a handle, and why did you leave those?
>
Those are the things that I don't want the user to care what they are, 
they just identify the IPMI user to the layer.  In the future, I could 
change them to an integer, or a small structure, or whatever.

I messed up, though, and didn't change the typedef to be
  typedef struct ipmi_user *ipmi_user_t.
I'm going to do that on a release by monday.

>I applied and built this on 2.5.41 (in-kernel and modular).
>Only one compile warning:
>## drivers/char/ipmi/ipmi_kcs_sm.c:234: warning: implicit declaration of function 'memcpy'
>
>Alan has reviewed this driver, right?  (at least an earlier
>version of it)
>
He has at least done a cursory review of the driver.  I'm not sure where 
he stands with it now.

>And you are asking for a major device number assignment?
>
I think I am going to only do dynamic assignment of the major number. 
 Someone else is looking at a filesystem interface for it, and I am also 
looking at doing a socket interface.  There seems to be some resistance 
to having a character driver for this (although I'm not exactly sure how 
to take it) and a socket interface may make more sense.

>
>I believe that this driver is in pretty good condition for
>addition to the 2.5 kernel.
>
We need to at least resolve the interface issues.

>
>Specific comments below, each beginning with "##".
>
>Warning to others: long-ish.
>
>~Randy
>
>
>+++ linux/drivers/char/Config.help	Fri Sep 20 08:37:00 2002
>
>+CONFIG_IPMI_EMULATE_RADISYS
>+  This enables emulation of the Radisys IPMI device driver.
>+CONFIG_IPMI_EMULATE_INTEL
>+  This enables emulation of the Intel IMB device driver.
>
>## These CONFIG_'s aren't used.  Can be removed from help file
>## unless there is code to be added for them.
>
Oops, that was a mistake.  They should only be in the emulation file.

>
>+++ linux/drivers/char/ipmi/Makefile	Thu Aug 22 20:58:47 2002
>
>+O_TARGET	:= built-in.o
>## delete O_TARGET for 2.5.lately
>
>+list-multi := ipmi_kcs_drv.o
>## delete list-multi for 2.5.lately
>
Ok.  I'll look at this for 2.5.

>
>+++ linux/drivers/char/ipmi/ipmi_devintf.c	Wed Oct  9 14:38:56 2002
>
>+static int init_ipmi_devintf(void)
>## use __init here.
>+{
>+	int rv;
>...
>+	rv = ipmi_smi_watcher_register(&smi_watcher);
>+	if (rv) {
>+		printk(KERN_WARNING "ipmi: can't register smi watcher");
>##  need to undo/return/free anything here ?? (see cleanup_ipmi())
>+		return rv;
>+	}
>+}
>
Yes, the devfs handle and char device are now released properly.

>
>+#ifdef MODULE
>##  don't need this #ifdef
>+static void cleanup_ipmi(void)
>##  use __exit here
>+{
>+	ipmi_smi_watcher_unregister(&smi_watcher);
>+	devfs_unregister(devfs_handle);
>+	unregister_chrdev(ipmi_major, DEVICE_NAME);
>+}
>+module_exit(cleanup_ipmi);
>+#else
>## #ifndef MODULE (or nothing since it's __init)
>+static int __init ipmi_setup (char *str)
>+{
>...
>+}
>+#endif
>
Ok.

>
>+++ linux/drivers/char/ipmi/ipmi_kcs_intf.c	Wed Oct  9 14:38:56 2002
>
>+	/* Flags from the last GET_MSG_FLAGS command, used when an ATTN
>+	   is set to hold the flags until we are done handling everything
>+	   from the flags. */
>+	unsigned char       msg_flags;
>#define RECEIVE_MSG_AVAIL	0x01
>#define EVENT_MSG_BUFFER_FULL	0x02
>#define WDT_PRE_TIMEOUT_INT	0x08
>## and use these instead of magic numbers when testing <msg_flags> below.
>
>+static int init_ipmi_kcs(void)
>## use __init
>
>+#ifdef MODULE
>+void cleanup_one_kcs(struct kcs_info *to_clean)
>## use __exit
>
>+static void cleanup_ipmi_kcs(void)
>## use __exit
>
Fixed all the above.

>
>+++ linux/drivers/char/ipmi/ipmi_kcs_sm.c	Fri Aug 23 12:03:31 2002
>
>+/* The states the KCS driver may be in. */
>+typedef enum kcs_states_e {
>...
>+} kcs_states_t;
>## still have typedefs
>
>+int kcs_get_result(kcs_data_t *kcs, unsigned char *data, int length)
>+{
>...
>+		data[2] = 0xc6; /* Report a truncated error.  We might
>+                                   overwrite another error, but that's
>+                                   too bad, the user needs to know it
>+                                   was truncated. */
>## use #defines (with useful names) for magic values
>
>+int kcs_size(void)
>+{
>+	return sizeof(kcs_data_t);
>+}
>## why a function ? maybe inline in a header file ?
>
This was done like this because I don't want the user to have access to 
the contents of the structure.

>
>+++ linux/drivers/char/ipmi/ipmi_kcs_sm.h	Fri Aug 23 11:21:44 2002
>
>+typedef struct kcs_data_s kcs_data_t;
>+typedef enum kcs_result_e
>+{
>...
>+} kcs_result_t;
>## still have typedefs
>
>+/* Return the size of the KCS structure in bytes. */
>+int kcs_size(void);
>## put inline here.
>
>+++ linux/drivers/char/ipmi/ipmi_msghandler.c	Wed Oct  9 14:38:56 2002
>
>+int ipmi_request(ipmi_user_t      *user,
>+		 struct ipmi_addr *addr,
>+		 long             msgid,
>+		 struct ipmi_msg  *msg,
>+		 int              priority)
>+{
>+	return i_ipmi_request(user,
>+			      user->intf,
>+			      addr,
>+			      msgid,
>+			      msg,
>+			      NULL, NULL,
>+			      priority);
>+}
>## why not inline ?
>
>+int ipmi_request_supply_msgs(ipmi_user_t          *user,
>+			     struct ipmi_addr     *addr,
>+			     long                 msgid,
>+			     struct ipmi_msg      *msg,
>+			     void                 *supplied_smi,
>+			     struct ipmi_recv_msg *supplied_recv,
>+			     int                  priority)
>+{
>+	return i_ipmi_request(user,
>+			      user->intf,
>+			      addr,
>+			      msgid,
>+			      msg,
>+			      supplied_smi,
>+			      supplied_recv,
>+			      priority);
>+}
>## why not inline ?
>
Not a big deal, but I made it inline.

>
>## Lots of pre-function comments throughout the driver(s) should be in
>## typical kernel style, like this one copied from kernel/exit.c:
>/*
> * Send signals to all our closest relatives so that they know
> * to properly mourn us..
> */
>## as compared to:
>+/* Handle a new message.  Return 1 if the message should be requeued,
>+   0 if the message should be freed, or -1 if the message should not
>+   be freed or requeued. */
>
I'll fix these later.

>
>+static int has_paniced = 0;
>## why are these (data, code) not inside CONFIG_IPMI_PANIC_EVENT ?
>## along with notifier_chain_[un]register() calls ?
>## except do it _without_ #ifdefs in the middle of functions.
>## i.e., hide the #ifdefs in a header file.
>+static int panic_event(struct notifier_block *this,
>+		       unsigned long         event,
>+                       void                  *ptr)
>+{
>+	int        i;
>+	ipmi_smi_t *intf;
>+
>+	if (has_paniced)
>+		return NOTIFY_DONE;
>+	has_paniced = 1;
>+
>+	/* For every registered interface, set it to run to completion. */
>+	for (i=0; i<MAX_IPMI_INTERFACES; i++) {
>+		intf = ipmi_interfaces[i];
>+		if (intf == NULL)
>+			continue;
>+
>+		intf->handlers->set_run_to_completion(intf->send_info, 1);
>+	}
>+
>+#ifdef CONFIG_IPMI_PANIC_EVENT
>+	send_panic_events();
>+#endif
>## hide this #ifdef in a header file.
>+
>+	return NOTIFY_DONE;
>+}
>
>+static int ipmi_init_msghandler(void)
>+{
>...
>+	notifier_chain_register(&panic_notifier_list, &panic_block);
>## should be conditional on CONFIG_IPMI_PANIC_EVENT
>
Nope.  I need to know if a panic has occurred so I can through the state 
machines into run-to-completion mode.  This way, watchdog messages and 
stuff like that can go out.  The panic event stuff only affects the 
generation of the IPMI event upon a panic.  There are other reasons for 
knowing when a panic happens.

>
>+#ifdef MODULE
>## don't need this #ifdef
>+static void cleanup_ipmi(void)
>## use __exit
>
>+	notifier_chain_unregister(&panic_notifier_list, &panic_block);
>## should be conditional on CONFIG_IPMI_PANIC_EVENT
>
>+++ linux/drivers/char/ipmi/ipmi_watchdog.c	Wed Oct  9 14:38:56 2002
>
>+static void ipmi_wdog_pretimeout_handler(void *handler_data)
>+{
>+	panic("Watchdog pre-timeout");
>## why panic on a pre-timeout condition ?
>
>+#ifdef MODULE
>+static void ipmi_unregister_watchdog(void)
>## can be __exit
>
>+++ linux/include/linux/ipmi.h	Wed Oct  9 14:38:56 2002
>
>+#ifdef __KERNEL__
>## why is this needed?  kernel headers shouldn't be used in userspace.
>
It doesn't hurt anything, and it's nice to be able to use the same 
include files.

>
>+typedef struct ipmi_user_s ipmi_user_t;
>## remove typedefs
>
This will become a typedef of a pointer.

>
>+/*
>+ * The userland interface
>+ */
>## userspace interface in a kernel header file ??  not good.
>
You have to define the userland interface somewhere so the kernel knows 
what to export :-).

>
>+++ linux/include/linux/ipmi_smi.h	Wed Oct  9 14:38:56 2002
>
>+typedef struct ipmi_smi_s ipmi_smi_t;
>## remove typedefs
>
This will become a typedef of a pointer.

>
>+++ linux/kernel/ksyms.c	Fri Sep 20 08:37:16 2002
>
>+EXPORT_SYMBOL(panic_notifier_list);
>## don't need this.  (it's already extern; build works without this)
>
The module will not load without this.  It is quite necessary.


Thanks for the input, I will fix the ones I identified as problems.

-Corey