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

[Corey Minyard <minyard@acm.org>]

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