Skip to contentSkip to conversation

IPMI driver updates, part 1b

PATCH In review Corey Minyard 2 participants 9 messages Feb 24, 2004 – Feb 25, 2004

Subsystems (2)
Networking IPMI
lore
Share & tools
lore

Corey Minyard

author
#0 lore raw

PATCH 1/1 IPMI driver updates, part 1b

It looks like part 1 of the IPMI driver updates was a little too big to get through. So I've split it up. Part 1a and part1b must both be applied. Here's the original message. Also, this is for the 2.6.3 kernel.

It has been far too long since the last IPMI driver updates, but now all the planets have aligned and all the pieces I needed are in and all seem to be working. This update is coming as four parts that must be applied in order, but the later parts do not have to be applied for the former parts to work.

This first part does basic updates to the IPMI infrastructure. It adds support for messaging through an IPMI LAN interface, which is required for some system software that already exists on other IPMI drivers. It also does some renaming (in preparation for one of the later patches) and a lot of little cleanups.

FYI, IPMI is a standard for monitoring and maintaining a system. It provides interfaces for detecting sensors (voltage, temperature, etc.) in the system and monitoring those sensors. Many systems have extended capabilities that allow IPMI to control the system, doing things like lighting leds and controlling hot-swap. This driver allows access to the IPMI system.

-Corey

author
#1 lore raw

It helps to actually attach the code.

Sorry,

-Corey

Quoted 11 lines · Corey Minyard

It looks like part 1 of the IPMI driver updates was a little too big to get through. So I've split it up. Part 1a and part1b must both be applied. Here's the original message. Also, this is for the 2.6.3 kernel.

It has been far too long since the last IPMI driver updates, but now all the planets have aligned and all the pieces I needed are in and all seem to be working. This update is coming as four parts that must be applied in order, but the later parts do not have to be applied for the former parts to work.

This first part does basic updates to the IPMI infrastructure. It adds support for messaging through an IPMI LAN interface, which is required for some system software that already exists on other IPMI drivers. It also does some renaming (in preparation for one of the later patches) and a lot of little cleanups.

FYI, IPMI is a standard for monitoring and maintaining a system. It provides interfaces for detecting sensors (voltage, temperature, etc.) in the system and monitoring those sensors. Many systems have extended capabilities that allow IPMI to control the system, doing things like lighting leds and controlling hot-swap. This driver allows access to the IPMI system.

(quoted diff) +0 -2
-Corey
 
-
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

drivers/char/ipmi/ipmi_kcs_intf.c +152 -102
diff -urN linux.orig/drivers/char/ipmi/ipmi_kcs_intf.c linux-a1/drivers/char/ipmi/ipmi_kcs_intf.c
@@ -55,7 +55,8 @@
 #include <linux/rcupdate.h>
 #include <linux/ipmi_smi.h>
 #include <asm/io.h>
+#include <asm/irq.h>
-#include "ipmi_kcs_sm.h"
+#include "ipmi_si_sm.h"
 #include <linux/init.h>
 
 /* Measure times between events in the driver. */

@@ -73,6 +73,8 @@
 /* This forces a dependency to the config file for this option. */
 #endif
 
+extern struct si_sm_handlers kcs_smi_handlers;
+
 enum kcs_intf_state {
 	KCS_NORMAL,
 	KCS_GETTING_FLAGS,

@@ -88,7 +90,7 @@
 struct kcs_info
 {
 	ipmi_smi_t          intf;
-	struct kcs_data     *kcs_sm;
+	struct si_sm_data   *kcs_sm;
 	spinlock_t          kcs_lock;
 	spinlock_t          msg_lock;
 	struct list_head    xmit_msgs;

@@ -113,8 +115,10 @@
 	   out. */
 	int                 run_to_completion;
 
+	struct si_sm_io     io;
+
 	/* The I/O port of a KCS interface. */
-	int                 port;
+	unsigned int        port;
 
 	/* zero if no irq; */
 	int                 irq;

@@ -165,7 +169,7 @@
 	deliver_recv_msg(kcs_info, msg);
 }
 
-static enum kcs_result start_next_msg(struct kcs_info *kcs_info)
+static enum si_sm_result start_next_msg(struct kcs_info *kcs_info)
 {
 	int              rv;
 	struct list_head *entry = NULL;

@@ -186,7 +190,7 @@
 
 	if (!entry) {
 		kcs_info->curr_msg = NULL;
-		rv = KCS_SM_IDLE;
+		rv = SI_SM_IDLE;
 	} else {
 		int err;
 

@@ -198,14 +202,15 @@
 		do_gettimeofday(&t);
 		printk("**Start2: %d.%9.9d\n", t.tv_sec, t.tv_usec);
 #endif
-		err = start_kcs_transaction(kcs_info->kcs_sm,
-					   kcs_info->curr_msg->data,
-					   kcs_info->curr_msg->data_size);
+		err = kcs_smi_handlers.start_transaction(
+		    kcs_info->kcs_sm,
+		    kcs_info->curr_msg->data,
+		    kcs_info->curr_msg->data_size);
 		if (err) {
 			return_hosed_msg(kcs_info);
 		}
 
-		rv = KCS_CALL_WITHOUT_DELAY;
+		rv = SI_SM_CALL_WITHOUT_DELAY;
 	}
 	spin_unlock(&(kcs_info->msg_lock));
 

@@ -221,7 +226,7 @@
 	msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
 	msg[1] = IPMI_GET_BMC_GLOBAL_ENABLES_CMD;
 
-	start_kcs_transaction(kcs_info->kcs_sm, msg, 2);
+	kcs_smi_handlers.start_transaction(kcs_info->kcs_sm, msg, 2);
 	kcs_info->kcs_state = KCS_ENABLE_INTERRUPTS1;
 }
 

@@ -234,7 +239,7 @@
 	msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;
 	msg[2] = WDT_PRE_TIMEOUT_INT;
 
-	start_kcs_transaction(kcs_info->kcs_sm, msg, 3);
+	kcs_smi_handlers.start_transaction(kcs_info->kcs_sm, msg, 3);
 	kcs_info->kcs_state = KCS_CLEARING_FLAGS;
 }
 

@@ -281,9 +286,10 @@
 		kcs_info->curr_msg->data[1] = IPMI_GET_MSG_CMD;
 		kcs_info->curr_msg->data_size = 2;
 
-		start_kcs_transaction(kcs_info->kcs_sm,
-				      kcs_info->curr_msg->data,
-				      kcs_info->curr_msg->data_size);
+		kcs_smi_handlers.start_transaction(
+		    kcs_info->kcs_sm,
+		    kcs_info->curr_msg->data,
+		    kcs_info->curr_msg->data_size);
 		kcs_info->kcs_state = KCS_GETTING_MESSAGES;
 	} else if (kcs_info->msg_flags & EVENT_MSG_BUFFER_FULL) {
 		/* Events available. */

@@ -299,9 +305,10 @@
 		kcs_info->curr_msg->data[1] = IPMI_READ_EVENT_MSG_BUFFER_CMD;
 		kcs_info->curr_msg->data_size = 2;
 
-		start_kcs_transaction(kcs_info->kcs_sm,
-				      kcs_info->curr_msg->data,
-				      kcs_info->curr_msg->data_size);
+		kcs_smi_handlers.start_transaction(
+		    kcs_info->kcs_sm,
+		    kcs_info->curr_msg->data,
+		    kcs_info->curr_msg->data_size);
 		kcs_info->kcs_state = KCS_GETTING_EVENTS;
 	} else {
 		kcs_info->kcs_state = KCS_NORMAL;

@@ -323,9 +330,10 @@
 			break;
 			
 		kcs_info->curr_msg->rsp_size
-			= kcs_get_result(kcs_info->kcs_sm,
-					 kcs_info->curr_msg->rsp,
-					 IPMI_MAX_MSG_LENGTH);
+			= kcs_smi_handlers.get_result(
+			    kcs_info->kcs_sm,
+			    kcs_info->curr_msg->rsp,
+			    IPMI_MAX_MSG_LENGTH);
 		
 		/* Do this here becase deliver_recv_msg() releases the
 		   lock, and a new message can be put in during the

@@ -341,7 +349,7 @@
 		unsigned int  len;
 
 		/* We got the flags from the KCS, now handle them. */
-		len = kcs_get_result(kcs_info->kcs_sm, msg, 4);
+		len = kcs_smi_handlers.get_result(kcs_info->kcs_sm, msg, 4);
 		if (msg[2] != 0) {
 			/* Error fetching flags, just give up for
 			   now. */

@@ -363,7 +371,7 @@
 		unsigned char msg[3];
 
 		/* We cleared the flags. */
-		kcs_get_result(kcs_info->kcs_sm, msg, 3);
+		kcs_smi_handlers.get_result(kcs_info->kcs_sm, msg, 3);
 		if (msg[2] != 0) {
 			/* Error clearing flags */
 			printk(KERN_WARNING

@@ -380,9 +388,9 @@
 	case KCS_GETTING_EVENTS:
 	{
 		kcs_info->curr_msg->rsp_size
-			= kcs_get_result(kcs_info->kcs_sm,
-					 kcs_info->curr_msg->rsp,
-					 IPMI_MAX_MSG_LENGTH);
+			= kcs_smi_handlers.get_result(kcs_info->kcs_sm,
+						      kcs_info->curr_msg->rsp,
+						      IPMI_MAX_MSG_LENGTH);
 
 		/* Do this here becase deliver_recv_msg() releases the
 		   lock, and a new message can be put in during the

@@ -405,9 +413,9 @@
 	case KCS_GETTING_MESSAGES:
 	{
 		kcs_info->curr_msg->rsp_size
-			= kcs_get_result(kcs_info->kcs_sm,
-					 kcs_info->curr_msg->rsp,
-					 IPMI_MAX_MSG_LENGTH);
+			= kcs_smi_handlers.get_result(kcs_info->kcs_sm,
+						      kcs_info->curr_msg->rsp,
+						      IPMI_MAX_MSG_LENGTH);
 
 		/* Do this here becase deliver_recv_msg() releases the
 		   lock, and a new message can be put in during the

@@ -432,7 +440,7 @@
 		unsigned char msg[4];
 
 		/* We got the flags from the KCS, now handle them. */
-		kcs_get_result(kcs_info->kcs_sm, msg, 4);
+		kcs_smi_handlers.get_result(kcs_info->kcs_sm, msg, 4);
 		if (msg[2] != 0) {
 			printk(KERN_WARNING
 			       "ipmi_kcs: Could not enable interrupts"

@@ -442,7 +450,8 @@
 			msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
 			msg[1] = IPMI_SET_BMC_GLOBAL_ENABLES_CMD;
 			msg[2] = msg[3] | 1; /* enable msg queue int */
-			start_kcs_transaction(kcs_info->kcs_sm, msg,3);
+			kcs_smi_handlers.start_transaction(
+			    kcs_info->kcs_sm, msg,3);
 			kcs_info->kcs_state = KCS_ENABLE_INTERRUPTS2;
 		}
 		break;

@@ -453,7 +462,7 @@
 		unsigned char msg[4];
 
 		/* We got the flags from the KCS, now handle them. */
-		kcs_get_result(kcs_info->kcs_sm, msg, 4);
+		kcs_smi_handlers.get_result(kcs_info->kcs_sm, msg, 4);
 		if (msg[2] != 0) {
 			printk(KERN_WARNING
 			       "ipmi_kcs: Could not enable interrupts"

@@ -467,9 +476,10 @@
 
 /* Called on timeouts and events.  Timeouts should pass the elapsed
    time, interrupts should pass in zero. */
-static enum kcs_result kcs_event_handler(struct kcs_info *kcs_info, int time)
+static enum si_sm_result kcs_event_handler(struct kcs_info *kcs_info,
+					   int time)
 {
-	enum kcs_result kcs_result;
+	enum si_sm_result kcs_result;
 
  restart:
 	/* There used to be a loop here that waited a little while

@@ -478,19 +488,19 @@
 	   range, which is far too long to wait in an interrupt.  So
 	   we just run until the state machine tells us something
 	   happened or it needs a delay. */
-	kcs_result = kcs_event(kcs_info->kcs_sm, time);
+	kcs_result = kcs_smi_handlers.event(kcs_info->kcs_sm, time);
 	time = 0;
-	while (kcs_result == KCS_CALL_WITHOUT_DELAY)
+	while (kcs_result == SI_SM_CALL_WITHOUT_DELAY)
 	{
-		kcs_result = kcs_event(kcs_info->kcs_sm, 0);
+		kcs_result = kcs_smi_handlers.event(kcs_info->kcs_sm, 0);
 	}
 
-	if (kcs_result == KCS_TRANSACTION_COMPLETE)
+	if (kcs_result == SI_SM_TRANSACTION_COMPLETE)
 	{
 		handle_transaction_done(kcs_info);
-		kcs_result = kcs_event(kcs_info->kcs_sm, 0);
+		kcs_result = kcs_smi_handlers.event(kcs_info->kcs_sm, 0);
 	}
-	else if (kcs_result == KCS_SM_HOSED)
+	else if (kcs_result == SI_SM_HOSED)
 	{
 		if (kcs_info->curr_msg != NULL) {
 			/* If we were handling a user message, format

@@ -498,12 +508,12 @@
                            tell it about the error. */
 			return_hosed_msg(kcs_info);
 		}
-		kcs_result = kcs_event(kcs_info->kcs_sm, 0);
+		kcs_result = kcs_smi_handlers.event(kcs_info->kcs_sm, 0);
 		kcs_info->kcs_state = KCS_NORMAL;
 	}
 
 	/* We prefer handling attn over new messages. */
-	if (kcs_result == KCS_ATTN)
+	if (kcs_result == SI_SM_ATTN)
 	{
 		unsigned char msg[2];
 

@@ -515,19 +525,19 @@
 		msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
 		msg[1] = IPMI_GET_MSG_FLAGS_CMD;
 
-		start_kcs_transaction(kcs_info->kcs_sm, msg, 2);
+		kcs_smi_handlers.start_transaction(kcs_info->kcs_sm, msg, 2);
 		kcs_info->kcs_state = KCS_GETTING_FLAGS;
 		goto restart;
 	}
 
 	/* If we are currently idle, try to start the next message. */
-	if (kcs_result == KCS_SM_IDLE) {
+	if (kcs_result == SI_SM_IDLE) {
 		kcs_result = start_next_msg(kcs_info);
-		if (kcs_result != KCS_SM_IDLE)
+		if (kcs_result != SI_SM_IDLE)
 			goto restart;
         }
 
-	if ((kcs_result == KCS_SM_IDLE)
+	if ((kcs_result == SI_SM_IDLE)
 	    && (atomic_read(&kcs_info->req_events)))
 	{
 		/* We are idle and the upper layer requested that I fetch

@@ -538,7 +548,7 @@
 		msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
 		msg[1] = IPMI_GET_MSG_FLAGS_CMD;
 
-		start_kcs_transaction(kcs_info->kcs_sm, msg, 2);
+		kcs_smi_handlers.start_transaction(kcs_info->kcs_sm, msg, 2);
 		kcs_info->kcs_state = KCS_GETTING_FLAGS;
 		goto restart;
 	}

@@ -550,11 +560,11 @@
 		   struct ipmi_smi_msg *msg,
 		   int                 priority)
 {
-	struct kcs_info *kcs_info = (struct kcs_info *) send_info;
-	enum kcs_result result;
-	unsigned long   flags;
+	struct kcs_info    *kcs_info = (struct kcs_info *) send_info;
+	enum si_sm_result  result;
+	unsigned long      flags;
 #ifdef DEBUG_TIMING
-	struct timeval t;
+	struct timeval     t;
 #endif
 
 	spin_lock_irqsave(&(kcs_info->msg_lock), flags);

@@ -575,7 +585,7 @@
 
 		spin_lock_irqsave(&(kcs_info->kcs_lock), flags);
 		result = kcs_event_handler(kcs_info, 0);
-		while (result != KCS_SM_IDLE) {
+		while (result != SI_SM_IDLE) {
 			udelay(KCS_SHORT_TIMEOUT_USEC);
 			result = kcs_event_handler(kcs_info,
 						   KCS_SHORT_TIMEOUT_USEC);

@@ -603,16 +613,16 @@
 
 static void set_run_to_completion(void *send_info, int i_run_to_completion)
 {
-	struct kcs_info *kcs_info = (struct kcs_info *) send_info;
-	enum kcs_result result;
-	unsigned long   flags;
+	struct kcs_info    *kcs_info = (struct kcs_info *) send_info;
+	enum si_sm_result  result;
+	unsigned long      flags;
 
 	spin_lock_irqsave(&(kcs_info->kcs_lock), flags);
 
 	kcs_info->run_to_completion = i_run_to_completion;
 	if (i_run_to_completion) {
 		result = kcs_event_handler(kcs_info, 0);
-		while (result != KCS_SM_IDLE) {
+		while (result != SI_SM_IDLE) {
 			udelay(KCS_SHORT_TIMEOUT_USEC);
 			result = kcs_event_handler(kcs_info,
 						   KCS_SHORT_TIMEOUT_USEC);

@@ -667,13 +677,13 @@
 
 static void kcs_timeout(unsigned long data)
 {
-	struct kcs_info *kcs_info = (struct kcs_info *) data;
-	enum kcs_result kcs_result;
-	unsigned long   flags;
-	unsigned long   jiffies_now;
-	unsigned long   time_diff;
+	struct kcs_info    *kcs_info = (struct kcs_info *) data;
+	enum si_sm_result  kcs_result;
+	unsigned long      flags;
+	unsigned long      jiffies_now;
+	unsigned long      time_diff;
 #ifdef DEBUG_TIMING
-	struct timeval t;
+	struct timeval     t;
 #endif
 
 	if (kcs_info->stop_operation) {

@@ -703,7 +713,7 @@
 	/* If the state machine asks for a short delay, then shorten
            the timer timeout. */
 #ifdef CONFIG_HIGH_RES_TIMERS
-	if (kcs_result == KCS_CALL_WITH_DELAY) {
+	if (kcs_result == SI_SM_CALL_WITH_DELAY) {
 		kcs_info->kcs_timer.sub_expires
 			+= usec_to_arch_cycles(KCS_SHORT_TIMEOUT_USEC);
 		while (kcs_info->kcs_timer.sub_expires >= cycles_per_jiffies) {

@@ -716,7 +726,7 @@
 	}
 #else
 	/* If requested, take the shortest delay possible */
-	if (kcs_result == KCS_CALL_WITH_DELAY) {
+	if (kcs_result == SI_SM_CALL_WITH_DELAY) {
 		kcs_info->kcs_timer.expires = jiffies + 1;
 	} else {
 		kcs_info->kcs_timer.expires = jiffies + KCS_TIMEOUT_JIFFIES;

@@ -767,12 +777,12 @@
 extern int kcs_dbg;
 static int ipmi_kcs_detect_hardware(unsigned int port,
 				    unsigned char *addr,
-				    struct kcs_data *data)
+				    struct si_sm_data *data)
 {
-	unsigned char   msg[2];
-	unsigned char   resp[IPMI_MAX_MSG_LENGTH];
-	unsigned long   resp_len;
-	enum kcs_result kcs_result;
+	unsigned char      msg[2];
+	unsigned char      resp[IPMI_MAX_MSG_LENGTH];
+	unsigned long      resp_len;
+	enum si_sm_result  kcs_result;
 
 	/* It's impossible for the KCS status register to be all 1's,
 	   (assuming a properly functioning, self-initialized BMC)

@@ -789,29 +799,31 @@
 	   useful info. */
 	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
 	msg[1] = IPMI_GET_DEVICE_ID_CMD;
-	start_kcs_transaction(data, msg, 2);
+	kcs_smi_handlers.start_transaction(data, msg, 2);
 	
-	kcs_result = kcs_event(data, 0);
+	kcs_result = kcs_smi_handlers.event(data, 0);
 	for (;;)
 	{
-		if (kcs_result == KCS_CALL_WITH_DELAY) {
-			udelay(100);
-			kcs_result = kcs_event(data, 100);
+		if (kcs_result == SI_SM_CALL_WITH_DELAY) {
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			schedule_timeout(1);
+			kcs_result = kcs_smi_handlers.event(data, 100);
 		}
-		else if (kcs_result == KCS_CALL_WITHOUT_DELAY)
+		else if (kcs_result == SI_SM_CALL_WITHOUT_DELAY)
 		{
-			kcs_result = kcs_event(data, 0);
+			kcs_result = kcs_smi_handlers.event(data, 0);
 		}
 		else
 			break;
 	}
-	if (kcs_result == KCS_SM_HOSED) {
+	if (kcs_result == SI_SM_HOSED) {
 		/* We couldn't get the state machine to run, so whatever's at
 		   the port is probably not an IPMI KCS interface. */
 		return -ENODEV;
 	}
 	/* Otherwise, we got some data. */
-	resp_len = kcs_get_result(data, resp, IPMI_MAX_MSG_LENGTH);
+	resp_len = kcs_smi_handlers.get_result(data, resp,
+					       IPMI_MAX_MSG_LENGTH);
 	if (resp_len < 6)
 		/* That's odd, it should be longer. */
 		return -EINVAL;

@@ -851,6 +863,36 @@
 MODULE_PARM(kcs_irqs, "1-4i");
 MODULE_PARM(kcs_ports, "1-4i");
 
+static unsigned char port_inb(struct si_sm_io *io, unsigned int offset)
+{
+	struct kcs_info *info = io->info;
+
+	return inb(info->port+offset);
+}
+
+static void port_outb(struct si_sm_io *io, unsigned int offset,
+		      unsigned char b)
+{
+	struct kcs_info *info = io->info;
+
+	outb(b, info->port+offset);
+}
+
+static unsigned char mem_inb(struct si_sm_io *io, unsigned int offset)
+{
+	struct kcs_info *info = io->info;
+
+	return readb(info->addr+offset);
+}
+
+static void mem_outb(struct si_sm_io *io, unsigned int offset,
+		     unsigned char b)
+{
+	struct kcs_info *info = io->info;
+
+	writeb(b, info->addr+offset);
+}
+
 /* Returns 0 if initialized, or negative on an error. */
 static int init_one_kcs(int kcs_port, 
 			int irq, 

@@ -893,6 +935,9 @@
 		       	       kcs_port);
 			return -EIO;
 		}
+		new_kcs->io.outputb = port_outb;
+		new_kcs->io.inputb = port_inb;
+		new_kcs->io.info = new_kcs;
 	} else {
 		if (request_mem_region(kcs_physaddr, 2, DEVICE_NAME) == NULL) {
 			kfree(new_kcs);

@@ -908,15 +953,18 @@
 		       	       kcs_physaddr);
 			return -EIO;
 		}
+		new_kcs->io.outputb = mem_outb;
+		new_kcs->io.inputb = mem_inb;
+		new_kcs->io.info = new_kcs;
 	}
 
-	new_kcs->kcs_sm = kmalloc(kcs_size(), GFP_KERNEL);
+	new_kcs->kcs_sm = kmalloc(kcs_smi_handlers.size(), GFP_KERNEL);
 	if (!new_kcs->kcs_sm) {
 		printk(KERN_ERR "ipmi_kcs: out of memory\n");
 		rv = -ENOMEM;
 		goto out_err;
 	}
-	init_kcs_data(new_kcs->kcs_sm, kcs_port, new_kcs->addr);
+	kcs_smi_handlers.init_data(new_kcs->kcs_sm, &(new_kcs->io));
 	spin_lock_init(&(new_kcs->kcs_lock));
 	spin_lock_init(&(new_kcs->msg_lock));
 

@@ -1021,47 +1069,47 @@
 #include <linux/acpi.h>
 
 struct SPMITable {
-	s8      Signature[4];
-	u32     Length;
-	u8      Revision;
-	u8      Checksum;
-	s8      OEMID[6];
-	s8      OEMTableID[8];
-	s8      OEMRevision[4];
-	s8      CreatorID[4];
-	s8      CreatorRevision[4];
-	u8      InterfaceType[2];
-	s16     SpecificationRevision;
+	s8	Signature[4];
+	u32	Length;
+	u8	Revision;
+	u8	Checksum;
+	s8	OEMID[6];
+	s8	OEMTableID[8];
+	s8	OEMRevision[4];
+	s8	CreatorID[4];
+	s8	CreatorRevision[4];
+	u8	InterfaceType[2];
+	s16	SpecificationRevision;
 
 	/*
 	 * Bit 0 - SCI interrupt supported
 	 * Bit 1 - I/O APIC/SAPIC
 	 */
-	u8      InterruptType;
+	u8	InterruptType;
 
 	/* If bit 0 of InterruptType is set, then this is the SCI
 	   interrupt in the GPEx_STS register. */
-	u8      GPE;
+	u8	GPE;
 
-	s16     Reserved;
+	s16	Reserved;
 
 	/* If bit 1 of InterruptType is set, then this is the I/O
-	   APIC/SAPIC interrupt. */
-	u32     GlobalSystemInterrupt;
+           APIC/SAPIC interrupt. */
+	u32	GlobalSystemInterrupt;
 
 	/* The actual register address. */
 	struct acpi_generic_address addr;
 
-	u8      UID[4];
+	u8	UID[4];
 
 	s8      spmi_id[1]; /* A '\0' terminated array starts here. */
 };
 
 static int acpi_find_bmc(unsigned long *physaddr, int *port)
 {
-	acpi_status          status;
-	struct SPMITable     *spmi;
-	
+	acpi_status status;
+	struct SPMITable  *spmi;
+
 	status = acpi_get_firmware_table("SPMI", 1,
 					 ACPI_LOGICAL_ADDRESSING,
 					 (struct acpi_table_header **) &spmi);

@@ -1084,8 +1132,8 @@
 	} else if (spmi->addr.address_space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
 		*port = spmi->addr.address;
 		printk("ipmi_kcs_intf: Found ACPI-specified state machine"
-		       " at I/O address 0x%lx\n",
-		       (unsigned long) spmi->addr.address);
+		       " at I/O address 0x%x\n",
+		       (unsigned int) spmi->addr.address);
 	} else
 		goto not_found; /* Not an address type we recognise. */
 

@@ -1134,7 +1182,7 @@
 	if (kcs_trydefaults && (pos == 0)) {
 		rv = -EINVAL;
 #ifdef CONFIG_ACPI_INTERPRETER
-		if (rv && (physaddr = acpi_find_bmc(&physaddr, &port) == 0)) {
+		if (rv && (acpi_find_bmc(&physaddr, &port) == 0)) {
 			rv = init_one_kcs(port, 
 					  0, 
 					  physaddr, 

@@ -1205,6 +1253,7 @@
 	/* Wait for the timer to stop.  This avoids problems with race
 	   conditions removing the timer here. */
 	while (!to_clean->timer_stopped) {
+		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(1);
 	}
drivers/char/ipmi/ipmi_kcs_sm.c +84 -69

304 lines not on this page · raw message

drivers/char/ipmi/ipmi_kcs_sm.h +0 -70

70 lines not on this page · raw message

drivers/char/ipmi/ipmi_msghandler.c +1076 -88

1,694 lines not on this page · raw message

drivers/char/ipmi/ipmi_si_sm.h +112 -0

112 lines not on this page · raw message

#2 lore raw
Quoted 2 lines · Corey Minyard

It helps to actually attach the code.

Got there eventually.

Patches seem reasonable, thanks. I'm not sure how to judge the suitability of the socket interface but it only touches your code..

- Several instances of IPMI_MAX_MSG_LENGTH-sized local arrays. It's not toooo large, but watch out for the stack space.

- You should convert the MODULE_PARMs to module_param() sometime.

- Be aware that the bitfields in struct seq_table will all fall into the same word and the compiler doesn't access them atomically. You must provide your own locking to prevent updates to them from scribbling on each other. Or make them integers.

- You misspelt breadcrumbs!

- This:

	extern struct si_sm_handlers kcs_smi_handlers;

should be in a header file.

- kcs_event_handler() sets `time = 0;' but never uses it again.

- status2txt() should take the caller's char* rather than use a static buffer.

- In ipmi_bt_sm.c:

	volatile unsigned char status;

The volatile is a red flag. It seems to be unneeded.

- We have #ifdef CONFIG_HIGH_RES_TIMERS code in there?

- drivers/char/ipmi/ipmi_si.c has lots of

        struct smi_info *smi_info = (struct smi_info *) send_info;

You don't need to cast a void* and it's actually a harmful thing to do:
it suppresses useful warnings if someone goes and changes the type of the rhs.

- ipmi_wait_for_queue() doesn't need set_current_state(TASK_RUNNING); after schedule_timeout() (I removed it)

- There's a locking bug in ipmi_recvmsg(): it can unlock i_lock when it
isn't held. I added this:

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
 
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);

which may or may not be correct.

Anyway, that's all fairly trivial. Please retest this code in the next -mm, send me any updates against that as appropriate and we'll get this merged up, thanks.

author
#3 lore raw
Quoted 12 lines · Andrew Morton
Quoted 3 lines · Corey Minyard

It helps to actually attach the code.

Got there eventually.

Patches seem reasonable, thanks. I'm not sure how to judge the suitability of the socket interface but it only touches your code..

Let's leave the af_ipmi code out for now. I'd like to get some opinions on it, though.

Quoted 4 lines

- Several instances of IPMI_MAX_MSG_LENGTH-sized local arrays. It's not toooo large, but watch out for the stack space.

I will rework these. These used to be much smaller, but newer hardware needed larger sizes.

Quoted 3 lines

- You should convert the MODULE_PARMs to module_param() sometime.

Will do.

Quoted 6 lines

- Be aware that the bitfields in struct seq_table will all fall into the same word and the compiler doesn't access them atomically. You must provide your own locking to prevent updates to them from scribbling on each other. Or make them integers.

These are only accessed when the sequence number lock is held, so they should be ok.

Quoted 3 lines

- You misspelt breadcrumbs!

Argh! I'll get that fixed one of these days :)

Quoted 7 lines

- This:

	extern struct si_sm_handlers kcs_smi_handlers;

should be in a header file.

ok.

Quoted 3 lines

- kcs_event_handler() sets `time = 0;' but never uses it again.

Actually, that's not true. There's an evil goto that goes back to "restart:", thus the time needs to be cleared.

Quoted 3 lines

- status2txt() should take the caller's char* rather than use a static buffer.

Ok, I'll fix that.

Quoted 7 lines

- In ipmi_bt_sm.c:

	volatile unsigned char status;

The volatile is a red flag. It seems to be unneeded.

I'll ask the person who wrote this about it.

Quoted 3 lines

- We have #ifdef CONFIG_HIGH_RES_TIMERS code in there?

Well, yes. That's so if people add the high-res timer patch, this driver can take advantage of it. Is that a problem?

Quoted 9 lines

- drivers/char/ipmi/ipmi_si.c has lots of

       struct smi_info *smi_info = (struct smi_info *) send_info;

You don't need to cast a void* and it's actually a harmful thing to do:
it suppresses useful warnings if someone goes and changes the type of the rhs.

Yes, true.

Quoted 4 lines

- ipmi_wait_for_queue() doesn't need set_current_state(TASK_RUNNING); after schedule_timeout() (I removed it)

Ok.

Quoted 17 lines

- There's a locking bug in ipmi_recvmsg(): it can unlock i_lock when it
isn't held. I added this:

diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
 
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
Quoted 2 lines

which may or may not be correct.

I'll look at this.

Quoted 6 lines

Anyway, that's all fairly trivial. Please retest this code in the next -mm, send me any updates against that as appropriate and we'll get this merged up, thanks.

Ok.

Thank you for your help.

-Corey

author
#5 lore raw
In reply to Andrew Morton
Quoted 12 lines · Andrew Morton
Quoted 3 lines · Corey Minyard

It helps to actually attach the code.

Got there eventually.

Patches seem reasonable, thanks. I'm not sure how to judge the suitability of the socket interface but it only touches your code..

Actually, since you already have it in, just leave it in. It's easy to pull out.

Quoted 17 lines

- There's a locking bug in ipmi_recvmsg(): it can unlock i_lock when it
isn't held. I added this:

diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
 
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
Quoted 2 lines

which may or may not be correct.

Actually, I believe the code is correct, and your change will break it. This is in a "while (1)" loop, and the only way to get out of this loop is to return with the lock not held or to break out of the loop with the lock held (and later code will unlock it). Am I correct here?

-Corey

#4 lore raw
In reply to Corey Minyard
Quoted 6 lines · Corey Minyard
Quoted 4 lines

- We have #ifdef CONFIG_HIGH_RES_TIMERS code in there?

Well, yes. That's so if people add the high-res timer patch, this driver can take advantage of it. Is that a problem?

Generally we prefer to not carry such code in the kernel tree. But it's relatively localised and not in a place where a lot of people go. If it makes your life easier and is actually maintained it is hardly the end of the world.

#6 lore raw
In reply to Corey Minyard
Quoted 14 lines · Corey Minyard
Quoted 14 lines

diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
 
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
Quoted 6 lines
Quoted 2 lines

which may or may not be correct.

Actually, I believe the code is correct, and your change will break it. This is in a "while (1)" loop, and the only way to get out of this loop is to return with the lock not held or to break out of the loop with the lock held (and later code will unlock it). Am I correct here?

With a little more context:

+ spin_unlock_irqrestore(&i->lock, flags);
+ if (!timeo) {
+ return -EAGAIN;
+ } else if (signal_pending (current)) {
+ dbg("Signal pending: %d", 1);
+ return -EINTR;
+ }
+
+ timeo = ipmi_wait_for_queue(i, timeo);
+ }
+
+ rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
+ list_del(&rcvmsg->link);
+ spin_unlock_irqrestore(&i->lock, flags);

See, there's a direct code path from one spin_unlock() to the other. And ipmi_wait_for_queue() does not retake the lock.

#8 lore raw
In reply to Corey Minyard
Quoted 17 lines · Corey Minyard
Quoted 17 lines

- There's a locking bug in ipmi_recvmsg(): it can unlock i_lock when it
isn't held. I added this:

diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
 
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
Quoted 6 lines
Quoted 2 lines

which may or may not be correct.

Actually, I believe the code is correct, and your change will break it. This is in a "while (1)" loop, and the only way to get out of this loop is to return with the lock not held or to break out of the loop with the lock held (and later code will unlock it). Am I correct here?

Ah, you are of course correct. Consider me thwapped.

author
#7 lore raw
In reply to Andrew Morton
Quoted 41 lines · Andrew Morton
Quoted 12 lines · Corey Minyard
Quoted 12 lines

diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c

net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +1 -0
@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
 		}
 
 		timeo = ipmi_wait_for_queue(i, timeo);
+		spin_lock_irqsave(&i->lock, flags);
 	}
Quoted 8 lines
Quoted 5 lines
net/ipmi/af_ipmi.c renamed from net/ipmi/af_ipmi.c~af_ipmi-locking-fix +0 -0
 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);

which may or may not be correct.

Actually, I believe the code is correct, and your change will break it. This is in a "while (1)" loop, and the only way to get out of this loop is to return with the lock not held or to break out of the loop with the lock held (and later code will unlock it). Am I correct here?

With a little more context:

(quoted diff) +14 -0
+		spin_unlock_irqrestore(&i->lock, flags);
+		if (!timeo) {
+			return -EAGAIN;
+		} else if (signal_pending (current)) {
+			dbg("Signal pending: %d", 1);
+			return -EINTR;
+		}
+
+		timeo = ipmi_wait_for_queue(i, timeo);
+	}
+
+	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
+	list_del(&rcvmsg->link);
+	spin_unlock_irqrestore(&i->lock, flags);
Quoted 4 lines

See, there's a direct code path from one spin_unlock() to the other. And ipmi_wait_for_queue() does not retake the lock.

With even more context:

   while (1) {
       spin_lock_irqsave(&i->lock, flags);
       if (!list_empty(&i->msg_list))
           break;
       spin_unlock_irqrestore(&i->lock, flags);
       if (!timeo) {
           return -EAGAIN;
       } else if (signal_pending (current)) {
           dbg("Signal pending: %d", 1);
           return -EINTR;
       }
       timeo = ipmi_wait_for_queue(i, timeo);

}

   rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
   list_del(&rcvmsg->link);   
   spin_unlock_irqrestore(&i->lock, flags);

So it will always go back to the top of the while loop and claim the lock again. It's kind of wierd looking, but I still believe it is correct. It's ugly, true, I can work on rewriting this piece.

-Corey