Re: [PATCH] drivers: char: ipmi: Replaced kmalloc and strcpy with kstrdup
On 03/16/2013 09:16 AM, Alexandru Gheorghiu wrote:
Replaced calls to kmalloc followed by strcpy with a sincle call to kstrdup.
Patch found using coccinelle.
Signed-off-by: Alexandru Gheorghiu
---
drivers/char/ipmi/ipmi_msghandler.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 053201b..617b443 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2037,12 +2037,11 @@ int ipmi_smi_add_proc_entry(ipmi_smi_t smi, char *name,
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
return -ENOMEM;
- entry->name = kmalloc(strlen(name)+1, GFP_KERNEL);
+ entry->name = kstrdup(name, GFP_KERNEL);
if (!entry->name) {
kfree(entry);
return -ENOMEM;
}
- strcpy(entry->name, name);
file = proc_create_data(name, 0, smi->proc_dir, proc_ops, data);
if (!file) {
Looks good to me.
Acked-by: Corey Minyard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: char: ipmi: Replaced kmalloc and strcpy with kstrdup
On 03/16/2013 09:16 AM, Alexandru Gheorghiu wrote:
Replaced calls to kmalloc followed by strcpy with a sincle call to kstrdup.
Patch found using coccinelle.
Signed-off-by: Alexandru Gheorghiu
---
drivers/char/ipmi/ipmi_msghandler.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 053201b..617b443 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2037,12 +2037,11 @@ int ipmi_smi_add_proc_entry(ipmi_smi_t smi, char *name,
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
return -ENOMEM;
- entry->name = kmalloc(strlen(name)+1, GFP_KERNEL);
+ entry->name = kstrdup(name, GFP_KERNEL);
if (!entry->name) {
kfree(entry);
return -ENOMEM;
}
- strcpy(entry->name, name);
file = proc_create_data(name, 0, smi->proc_dir, proc_ops, data);
if (!file) {
Actually, I should say that I've pulled this into my tree.
Thanks,
-corey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Linux IPMI subsystem hang
On 03/15/2013 01:57 PM, Daniel Kahn Gillmor wrote:
On Tue 2013-03-12 22:23:37 -0400, Daniel Kahn Gillmor wrote:
I am working with a Lenovo ThinkCentre M78, model 4865-A14, and it seems
to have trouble with the IPMI subsystem.
udev seems to hang for about 3 minutes at startup, ultimately failing
with the following messages:
udevd[416]: worker [495] unexpectedly returned with status 0x0100
udevd[416]: worker [495] failed while handling
'/devices/pci:00/:00:15.2/:03:00.3'
This hang happens whether i'm running linux kernel 3.2 or 3.8, using
either x86 or x86_64 kernels.
trying with udev 175-7.1 (from debian unstable) and kernel 3.2, i see
that the failure message is:
udevd[548]: timeout: killing '/sbin/modprobe -b
pci:v10ECd816Csv17AAsd3089bc0Csc07i01' [623]
and:
[5.650931] ipmi message handler version 39.2
[5.916958] IPMI System Interface driver.
[5.921153] ipmi_si :03:00.3: probing via PCI
[5.925851] ipmi_si :03:00.3: [io 0xe000-0xe0ff] regsize 1 spacing 1
irq 17
[5.933727] ipmi_si: Adding PCI-specified kcs state machine
[5.939554] ipmi_si: Trying PCI-specified kcs state machine at i/o address
0xe000, slave address 0x0, irq 17
[ 406.916061] ipmi_si: There appears to be no BMC at this location
with kernel 3.8, the last line ("There appears to be no BMC at this
location") isn't emitted, but the delay/hang with modprobe still
happens.
I think the first alias in ipmi_si.ko is what is causing this to be triggered:
0 krazy:~# modinfo ipmi_si | grep ^alias
alias: pci:v*d*sv*sd*bc0Csc07i*
alias: pci:v103Cd121Asv*sd*bc*sc*i*
0 krazy:~#
since the bc0Csc07 matches the [0c07] identifier from lspci:
03:00.3 IPMI SMIC interface [0c07]: Realtek Semiconductor Co., Ltd. Device
[10ec:816c] (rev 01) (prog-if 01)
It seems like there are four plausible cases:
0) this is actually an IPMI device, but the hardware is broken.
1) this is an IPMI device, but it does not implement some part of the
IPMI spec that ipmi_si.ko expects to be implemented, and ipmi_si.ko
cannot detect this cleanly.
2) this device is not an IPMI device at all, and is mislabeled in its
PCI identifiers somehow.
3) this device is not an IPMI device at all, it is properly labeled,
and the module's internal aliasing (and lspci's index?) is
overgeneral and misidentifies the device.
How can i distinguish between these cases?
I would guess that the register spacing is wrong. The spec has a
protocol for determining register spacing, but according to the spec it
only works for KCS interfaces. Since this is a SMIC interface, it's not
implemented.
You can hardcode values in ipmi_pci_probe_regspacing() in
drivers/char/ipmi/ipmi_si_intf.c to see if that makes a difference. I'd
guess 4, but it might be 16. I can think about trying the protocol on
SMIC, perhaps it will work there, too.
-corey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/8] IPMI: Change device node ordering to reflect probe order
From: Carol Hebert <[EMAIL PROTECTED]> Currently, on systems with multiple BMC interfaces, the ipmi device names are being created in reverse order relative to how they are discovered on the system (e.g. on an IBM x3950 multinode server with N nodes, the device name for the BMC in the first node is /dev/ipmiN-1 and the device name for the BMC in the last node is /dev/ipmi0, etc.). The problem is caused by the list handling routines chosen in dmi_scan.c. Using list_add() causes the multiple ipmi devices to be added to the device list using a stack-paradigm and so the ipmi driver subsequently pulls them off during initialization in LIFO order. This patch changes the dmi_save_ipmi_device() list handling paradigm to a queue, thereby allowing the ipmi driver to build the ipmi device names in the order in which they are found on the system. Signed-off-by: Carol Hebert <[EMAIL PROTECTED]> Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> --- --- linux-2.6.24.orig/drivers/firmware/dmi_scan.c 2008-01-30 11:21:55.0 -0800 +++ linux-2.6.24/drivers/firmware/dmi_scan.c2008-01-30 11:18:05.0 -0800 @@ -219,7 +219,7 @@ static void __init dmi_save_ipmi_device( dev->name = "IPMI controller"; dev->device_data = data; - list_add(&dev->list, &dmi_devices); + list_add_tail(&dev->list, &dmi_devices); } /* -- 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/
[PATCH 7/8] IPMI: convert locked counters to atomics
From: Konstantin Baydarov <[EMAIL PROTECTED]>
Atomics are a lot more efficient and neat than using a lock.
Signed-off-by: Konstantin Baydarov <[EMAIL PROTECTED]>
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
@@ -67,7 +67,6 @@ static struct proc_dir_entry *proc_ipmi_
the max message timer. This is in milliseconds. */
#define MAX_MSG_TIMEOUT6
-
/*
* The main "user" data structure.
*/
@@ -186,6 +185,76 @@ struct bmc_device
struct device_attribute aux_firmware_rev_attr;
};
+struct ipmi_stats
+{
+ /* Commands we got that were invalid. */
+ atomic_t sent_invalid_commands;
+
+ /* Commands we sent to the MC. */
+ atomic_t sent_local_commands;
+ /* Responses from the MC that were delivered to a user. */
+ atomic_t handled_local_responses;
+ /* Responses from the MC that were not delivered to a user. */
+ atomic_t unhandled_local_responses;
+
+ /* Commands we sent out to the IPMB bus. */
+ atomic_t sent_ipmb_commands;
+ /* Commands sent on the IPMB that had errors on the SEND CMD */
+ atomic_t sent_ipmb_command_errs;
+ /* Each retransmit increments this count. */
+ atomic_t retransmitted_ipmb_commands;
+ /* When a message times out (runs out of retransmits) this is
+ incremented. */
+ atomic_t timed_out_ipmb_commands;
+
+ /* This is like above, but for broadcasts. Broadcasts are
+ *not* included in the above count (they are expected to
+ time out). */
+ atomic_t timed_out_ipmb_broadcasts;
+
+ /* Responses I have sent to the IPMB bus. */
+ atomic_t sent_ipmb_responses;
+
+ /* The response was delivered to the user. */
+ atomic_t handled_ipmb_responses;
+ /* The response had invalid data in it. */
+ atomic_t invalid_ipmb_responses;
+ /* The response didn't have anyone waiting for it. */
+ atomic_t unhandled_ipmb_responses;
+
+ /* Commands we sent out to the IPMB bus. */
+ atomic_t sent_lan_commands;
+ /* Commands sent on the IPMB that had errors on the SEND CMD */
+ atomic_t sent_lan_command_errs;
+ /* Each retransmit increments this count. */
+ atomic_t retransmitted_lan_commands;
+ /* When a message times out (runs out of retransmits) this is
+ incremented. */
+ atomic_t timed_out_lan_commands;
+
+ /* Responses I have sent to the IPMB bus. */
+ atomic_t sent_lan_responses;
+
+ /* The response was delivered to the user. */
+ atomic_t handled_lan_responses;
+ /* The response had invalid data in it. */
+ atomic_t invalid_lan_responses;
+ /* The response didn't have anyone waiting for it. */
+ atomic_t unhandled_lan_responses;
+
+ /* The command was delivered to the user. */
+ atomic_t handled_commands;
+ /* The command had invalid data in it. */
+ atomic_t invalid_commands;
+ /* The command didn't have anyone waiting for it. */
+ atomic_t unhandled_commands;
+
+ /* Invalid data in an event. */
+ atomic_t invalid_events;
+ /* Events that were received with the proper format. */
+ atomic_t events;
+};
+
#define IPMI_IPMB_NUM_SEQ 64
#define IPMI_MAX_CHANNELS 16
struct ipmi_smi
@@ -286,75 +355,7 @@ struct ipmi_smi
struct proc_dir_entry *proc_dir;
char proc_dir_name[10];
- spinlock_t counter_lock; /* For making counters atomic. */
-
- /* Commands we got that were invalid. */
- unsigned int sent_invalid_commands;
-
- /* Commands we sent to the MC. */
- unsigned int sent_local_commands;
- /* Responses from the MC that were delivered to a user. */
- unsigned int handled_local_responses;
- /* Responses from the MC that were not delivered to a user. */
- unsigned int unhandled_local_responses;
-
- /* Commands we sent out to the IPMB bus. */
- unsigned int sent_ipmb_commands;
- /* Commands sent on the IPMB that had errors on the SEND CMD */
- unsigned int sent_ipmb_command_errs;
- /* Each retransmit increments this count. */
- unsigned int retransmitted_ipmb_commands;
- /* When a message times out (runs out of retransmits) this is
- incremented. */
- unsigned int timed_out_ipmb_commands;
-
- /* This is like above, but for broadcasts. Broadcasts are
- *not* included in the above count (they are expected to
- time out). */
- unsigned int timed_out_ipmb_broadcasts;
-
- /* Responses I have sent to the IPMB bus. */
- unsigned int sent_ipmb_responses;
-
-
[PATCH 6/8] IPMI: Update driver version
From: Corey Minyard <[EMAIL PROTECTED]> Enough bug fixes and changes that we need a new driver version. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Index: linux-2.6.23/drivers/char/ipmi/ipmi_msghandler.c === --- linux-2.6.23.orig/drivers/char/ipmi/ipmi_msghandler.c +++ linux-2.6.23/drivers/char/ipmi/ipmi_msghandler.c @@ -47,7 +47,7 @@ #define PFX "IPMI message handler: " -#define IPMI_DRIVER_VERSION "39.1" +#define IPMI_DRIVER_VERSION "39.2" static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); static int ipmi_init_msghandler(void); -- 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/
[PATCH 1/8] IPMI: Hold ATTN until upper layer ready
From: Corey Minyard <[EMAIL PROTECTED]>
Hold handling of ATTN until the upper layer has reported that it is
ready.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Cc: Patrick Schoeller <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
@@ -723,8 +723,11 @@ static enum si_sm_result smi_event_handl
si_sm_result = smi_info->handlers->event(smi_info->si_sm, 0);
}
- /* We prefer handling attn over new messages. */
- if (si_sm_result == SI_SM_ATTN)
+ /*
+* We prefer handling attn over new messages. But don't do
+* this if there is not yet an upper layer to handle anything.
+*/
+ if (likely(smi_info->intf) && si_sm_result == SI_SM_ATTN)
{
unsigned char msg[2];
--
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/
[PATCH 3/8] IPMI: Run to completion fixes
From: Corey Minyard <[EMAIL PROTECTED]>
The "run_to_completion" mode was somewhat broken. Locks need to be
avoided in run_to_completion mode, and it shouldn't be used by normal
users, just internally for panic situations.
This patch removes locks in run_to_completion mode and removes the
user call for setting the mode. The only user was the poweroff
code, but it was easily converted to use the polling interface.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.21/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.21.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.21/drivers/char/ipmi/ipmi_msghandler.c
@@ -1197,13 +1197,6 @@ int ipmi_unregister_for_cmd(ipmi_user_t
return rv;
}
-void ipmi_user_set_run_to_completion(ipmi_user_t user, int val)
-{
- ipmi_smi_t intf = user->intf;
- if (intf->handlers)
- intf->handlers->set_run_to_completion(intf->send_info, val);
-}
-
static unsigned char
ipmb_checksum(unsigned char *data, int size)
{
@@ -4201,5 +4194,4 @@ EXPORT_SYMBOL(ipmi_get_my_address);
EXPORT_SYMBOL(ipmi_set_my_LUN);
EXPORT_SYMBOL(ipmi_get_my_LUN);
EXPORT_SYMBOL(ipmi_smi_add_proc_entry);
-EXPORT_SYMBOL(ipmi_user_set_run_to_completion);
EXPORT_SYMBOL(ipmi_free_recv_msg);
Index: linux-2.6.21/drivers/char/ipmi/ipmi_poweroff.c
===
--- linux-2.6.21.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.21/drivers/char/ipmi/ipmi_poweroff.c
@@ -99,11 +99,14 @@ static unsigned char ipmi_version;
allocate them, since we may be in a panic situation. The whole
thing is single-threaded, anyway, so multiple messages are not
required. */
+static atomic_t dummy_count = ATOMIC_INIT(0);
static void dummy_smi_free(struct ipmi_smi_msg *msg)
{
+ atomic_dec(&dummy_count);
}
static void dummy_recv_free(struct ipmi_recv_msg *msg)
{
+ atomic_dec(&dummy_count);
}
static struct ipmi_smi_msg halt_smi_msg =
{
@@ -152,17 +155,28 @@ static int ipmi_request_wait_for_respons
return halt_recv_msg.msg.data[0];
}
-/* We are in run-to-completion mode, no completion is desired. */
+/* Wait for message to complete, spinning. */
static int ipmi_request_in_rc_mode(ipmi_user_tuser,
struct ipmi_addr *addr,
struct kernel_ipmi_msg *send_msg)
{
int rv;
+ atomic_set(&dummy_count, 2);
rv = ipmi_request_supply_msgs(user, addr, 0, send_msg, NULL,
&halt_smi_msg, &halt_recv_msg, 0);
- if (rv)
+ if (rv) {
+ atomic_set(&dummy_count, 0);
return rv;
+ }
+
+ /*
+* Spin until our message is done.
+*/
+ while (atomic_read(&dummy_count) > 0) {
+ ipmi_poll_interface(user);
+ barrier();
+ }
return halt_recv_msg.msg.data[0];
}
@@ -531,9 +545,7 @@ static void ipmi_poweroff_function (void
return;
/* Use run-to-completion mode, since interrupts may be off. */
- ipmi_user_set_run_to_completion(ipmi_user, 1);
specific_poweroff_func(ipmi_user);
- ipmi_user_set_run_to_completion(ipmi_user, 0);
}
/* Wait for an IPMI interface to be installed, the first one installed
Index: linux-2.6.21/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.21.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.21/drivers/char/ipmi/ipmi_si_intf.c
@@ -809,56 +809,54 @@ static void sender(void*
return;
}
- spin_lock_irqsave(&(smi_info->msg_lock), flags);
#ifdef DEBUG_TIMING
do_gettimeofday(&t);
printk("**Enqueue: %d.%9.9d\n", t.tv_sec, t.tv_usec);
#endif
if (smi_info->run_to_completion) {
- /* If we are running to completion, then throw it in
- the list and run transactions until everything is
- clear. Priority doesn't matter here. */
+ /*
+* If we are running to completion, then throw it in
+* the list and run transactions until everything is
+* clear. Priority doesn't matter here.
+*/
+
+ /*
+* Run to completion means we are single-threaded, no
+* need for locks.
+*/
list_add_tail(&(msg->link), &(smi_info->xmit_msgs));
- /* We have to release the msg lock and claim the smi
- lock in this case, because of race conditions. */
- spin_unlock_irqrestore(&(smi_info->msg_lock), flags);
-
- spin_lock_irqsave(&a
[PATCH 4/8] IPMI: Don't grab locks in run-to-completion mode
From: Konstantin Baydarov <[EMAIL PROTECTED]>
This patch prevents deadlocks in IPMI panic handler caused by msg_lock
in smi_info structure and waiting_msgs_lock in ipmi_smi structure.
Signed-off-by: Konstantin Baydarov <[EMAIL PROTECTED]>
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
@@ -289,7 +289,8 @@ static enum si_sm_result start_next_msg(
/* No need to save flags, we aleady have interrupts off and we
already hold the SMI lock. */
- spin_lock(&(smi_info->msg_lock));
+ if (!smi_info->run_to_completion)
+ spin_lock(&(smi_info->msg_lock));
/* Pick the high priority queue first. */
if (!list_empty(&(smi_info->hp_xmit_msgs))) {
@@ -329,7 +330,8 @@ static enum si_sm_result start_next_msg(
rv = SI_SM_CALL_WITHOUT_DELAY;
}
out:
- spin_unlock(&(smi_info->msg_lock));
+ if (!smi_info->run_to_completion)
+ spin_unlock(&(smi_info->msg_lock));
return rv;
}
Index: linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
@@ -351,8 +351,16 @@ struct ipmi_smi
/* Invalid data in an event. */
unsigned int invalid_events;
+
/* Events that were received with the proper format. */
unsigned int events;
+
+ /*
+* run_to_completion duplicate of smb_info, smi_info
+* and ipmi_serial_info structures. Used to decrease numbers of
+* parameters passed by "low" level IPMI code.
+*/
+ int run_to_completion;
};
#define to_si_intf_from_dev(device) container_of(device, struct ipmi_smi, dev)
@@ -3451,8 +3459,9 @@ static int handle_new_recv_msg(ipmi_smi_
void ipmi_smi_msg_received(ipmi_smi_t intf,
struct ipmi_smi_msg *msg)
{
- unsigned long flags;
+ unsigned long flags = 0; /* keep us warning-free. */
int rv;
+ int run_to_completion;
if ((msg->data_size >= 2)
@@ -3501,21 +3510,30 @@ void ipmi_smi_msg_received(ipmi_smi_t
/* To preserve message order, if the list is not empty, we
tack this message onto the end of the list. */
- spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
+ run_to_completion = intf->run_to_completion;
+ barrier();
+ if (!run_to_completion)
+ spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
if (!list_empty(&intf->waiting_msgs)) {
list_add_tail(&msg->link, &intf->waiting_msgs);
- spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
+ if (!run_to_completion)
+ spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
goto out;
}
- spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
+ if (!run_to_completion)
+ spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
rv = handle_new_recv_msg(intf, msg);
if (rv > 0) {
/* Could not handle the message now, just add it to a
list to handle later. */
- spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
+ run_to_completion = intf->run_to_completion;
+ barrier();
+ if (!run_to_completion)
+ spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
list_add_tail(&msg->link, &intf->waiting_msgs);
- spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
+ if (!run_to_completion)
+ spin_unlock_irqrestore(&intf->waiting_msgs_lock, flags);
} else if (rv == 0) {
ipmi_free_smi_msg(msg);
}
@@ -3884,6 +3902,7 @@ static void send_panic_events(char *str)
/* Interface is not ready. */
continue;
+ intf->run_to_completion = 1;
/* Send the event announcing the panic. */
intf->handlers->set_run_to_completion(intf->send_info, 1);
i_ipmi_request(NULL,
@@ -4059,6 +4078,7 @@ static int panic_event(struct notifier_b
/* Interface is not ready. */
continue;
+ intf->run_to_completion = 1;
intf->handlers->set_run_to_completion(intf->send_info, 1);
}
--
[PATCH 5/8] IPMI: Don't print event queue full on every event
From: Corey Minyard <[EMAIL PROTECTED]>
Don't print out that the event queue is full on every event, only
print something out when it becomes full or becomes not full.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.23/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.23.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.23/drivers/char/ipmi/ipmi_msghandler.c
@@ -253,7 +253,8 @@ struct ipmi_smi
spinlock_t events_lock; /* For dealing with event stuff. */
struct list_head waiting_events;
unsigned int waiting_events_count; /* How many events in queue? */
- int delivering_events;
+ char delivering_events;
+ char event_msg_printed;
/* The event receiver for my BMC, only really used at panic
shutdown as a place to store this. */
@@ -1075,6 +1076,11 @@ int ipmi_set_gets_events(ipmi_user_t use
list_for_each_entry_safe(msg, msg2, &intf->waiting_events, link)
list_move_tail(&msg->link, &msgs);
intf->waiting_events_count = 0;
+ if (intf->event_msg_printed) {
+ printk(KERN_WARNING PFX "Event queue no longer"
+ " full\n");
+ intf->event_msg_printed = 0;
+ }
intf->delivering_events = 1;
spin_unlock_irqrestore(&intf->events_lock, flags);
@@ -3253,11 +3259,12 @@ static int handle_read_event_rsp(ipmi_sm
copy_event_into_recv_msg(recv_msg, msg);
list_add_tail(&(recv_msg->link), &(intf->waiting_events));
intf->waiting_events_count++;
- } else {
+ } else if (!intf->event_msg_printed) {
/* There's too many things in the queue, discard this
message. */
- printk(KERN_WARNING PFX "Event queue full, discarding an"
- " incoming event\n");
+ printk(KERN_WARNING PFX "Event queue full, discarding"
+ " incoming events\n");
+ intf->event_msg_printed = 1;
}
out:
--
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/
[PATCH 8/8] IPMI: Convert locked counters to atomics in the system interface
From: Corey Minyard <[EMAIL PROTECTED]>
Atomics are faster and neater than locked counters.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
@@ -120,6 +120,22 @@ static struct device_driver ipmi_driver
.bus = &platform_bus_type
};
+struct smi_stats
+{
+ atomic_t short_timeouts;
+ atomic_t long_timeouts;
+ atomic_t timeout_restarts;
+ atomic_t idles;
+ atomic_t interrupts;
+ atomic_t attentions;
+ atomic_t flag_fetches;
+ atomic_t hosed_count;
+ atomic_t complete_transactions;
+ atomic_t events;
+ atomic_t watchdog_pretimeouts;
+ atomic_t incoming_messages;
+};
+
struct smi_info
{
intintf_num;
@@ -216,25 +232,17 @@ struct smi_info
unsigned char slave_addr;
/* Counters and things for the proc filesystem. */
- spinlock_t count_lock;
- unsigned long short_timeouts;
- unsigned long long_timeouts;
- unsigned long timeout_restarts;
- unsigned long idles;
- unsigned long interrupts;
- unsigned long attentions;
- unsigned long flag_fetches;
- unsigned long hosed_count;
- unsigned long complete_transactions;
- unsigned long events;
- unsigned long watchdog_pretimeouts;
- unsigned long incoming_messages;
+ struct smi_stats stats;
struct task_struct *thread;
struct list_head link;
};
+#define smi_inc_stat(smi, stat) atomic_inc(&(smi)->stats.stat)
+#define smi_get_stat(smi, stat) \
+ ((unsigned int) atomic_read(&smi->stats.stat))
+
#define SI_MAX_PARMS 4
static int force_kipmid[SI_MAX_PARMS];
@@ -398,9 +406,7 @@ static void handle_flags(struct smi_info
retry:
if (smi_info->msg_flags & WDT_PRE_TIMEOUT_INT) {
/* Watchdog pre-timeout */
- spin_lock(&smi_info->count_lock);
- smi_info->watchdog_pretimeouts++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, watchdog_pretimeouts);
start_clear_flags(smi_info);
smi_info->msg_flags &= ~WDT_PRE_TIMEOUT_INT;
@@ -545,9 +551,7 @@ static void handle_transaction_done(stru
smi_info->msg_flags &= ~EVENT_MSG_BUFFER_FULL;
handle_flags(smi_info);
} else {
- spin_lock(&smi_info->count_lock);
- smi_info->events++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, events);
/* Do this before we deliver the message
because delivering the message releases the
@@ -581,9 +585,7 @@ static void handle_transaction_done(stru
smi_info->msg_flags &= ~RECEIVE_MSG_AVAIL;
handle_flags(smi_info);
} else {
- spin_lock(&smi_info->count_lock);
- smi_info->incoming_messages++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, incoming_messages);
/* Do this before we deliver the message
because delivering the message releases the
@@ -700,18 +702,14 @@ static enum si_sm_result smi_event_handl
if (si_sm_result == SI_SM_TRANSACTION_COMPLETE)
{
- spin_lock(&smi_info->count_lock);
- smi_info->complete_transactions++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, complete_transactions);
handle_transaction_done(smi_info);
si_sm_result = smi_info->handlers->event(smi_info->si_sm, 0);
}
else if (si_sm_result == SI_SM_HOSED)
{
- spin_lock(&smi_info->count_lock);
- smi_info->hosed_count++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, hosed_count);
/* Do the before return_hosed_msg, because that
releases the lock. */
@@ -733,9 +731,7 @@ static enum si_sm_result smi_event_handl
{
unsigned char msg[2];
- spin_lock(&smi_info->count_lock);
- smi_info->attentions++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, attentions);
/* Got a attn, send down a get message flags to see
what's causing it. It would be better to handl
Re: [PATCH 7/8] IPMI: convert locked counters to atomics
Andrew Morton wrote:
The code forgot to initialise all of these.
It just so happens that the all-bits-zero pattern works correctly for all
current architectures, so the code should work OK. But there is no reason
(I hope) why an architecture cannot implement atomic_t as
struct atomic_t {
int counter;
spinlock_t lock;
};
in which case the results of ATOMIC_INIT() may _not_ be all-zeroes, in
which case the code will deadlock.
So. It works, but it's grubby. Do you still wish to proceed?
Thanks. Don't proceed for now, I'll work up new patches.
-corey
--
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/
Re: [PATCH 3/8] IPMI: Run to completion fixes
Andrew Morton wrote:
On Wed, 13 Feb 2008 10:23:42 -0600
Corey Minyard <[EMAIL PROTECTED]> wrote:
From: Corey Minyard <[EMAIL PROTECTED]>
The "run_to_completion" mode was somewhat broken. Locks need to be
avoided in run_to_completion mode, and it shouldn't be used by normal
users, just internally for panic situations.
This patch removes locks in run_to_completion mode and removes the
user call for setting the mode. The only user was the poweroff
code, but it was easily converted to use the polling interface.
+ /*
+* Spin until our message is done.
+*/
+ while (atomic_read(&dummy_count) > 0) {
+ ipmi_poll_interface(user);
+ barrier();
+ }
we'd normally use cpu_relax() here.
Yes, that's what I should have used. I'll submit a new patch to fix this.
-corey
--
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/
Re: [PATCH 6/8] IPMI: Update driver version
Andrew Morton wrote: On Wed, 13 Feb 2008 10:30:48 -0600 Corey Minyard <[EMAIL PROTECTED]> wrote: Enough bug fixes and changes that we need a new driver version. Are none of them serious enough to warrant a 2.6.24.x backport? No, nothing really terribly urgent. Just minor stuff. -corey -- 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/
[PATCH 3/4] IPMI: convert locked counters to atomics
From: Konstantin Baydarov <[EMAIL PROTECTED]> Atomics are a lot more efficient and neat than using a lock. Signed-off-by: Konstantin Baydarov <[EMAIL PROTECTED]> Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> --- Index: linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c === --- linux-2.6.24.orig/drivers/char/ipmi/ipmi_msghandler.c +++ linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c @@ -67,7 +67,6 @@ static struct proc_dir_entry *proc_ipmi_ the max message timer. This is in milliseconds. */ #define MAX_MSG_TIMEOUT6 - /* * The main "user" data structure. */ @@ -186,6 +185,96 @@ struct bmc_device struct device_attribute aux_firmware_rev_attr; }; +/* + * Various statistics for IPMI, these index stats[] in the ipmi_smi + * structure. + */ +/* Commands we got from the user that were invalid. */ +#define IPMI_STAT_sent_invalid_commands0 + +/* Commands we sent to the MC. */ +#define IPMI_STAT_sent_local_commands 1 + +/* Responses from the MC that were delivered to a user. */ +#define IPMI_STAT_handled_local_responses 2 + +/* Responses from the MC that were not delivered to a user. */ +#define IPMI_STAT_unhandled_local_responses3 + +/* Commands we sent out to the IPMB bus. */ +#define IPMI_STAT_sent_ipmb_commands 4 + +/* Commands sent on the IPMB that had errors on the SEND CMD */ +#define IPMI_STAT_sent_ipmb_command_errs 5 + +/* Each retransmit increments this count. */ +#define IPMI_STAT_retransmitted_ipmb_commands 6 + +/* When a message times out (runs out of retransmits) this is incremented. */ +#define IPMI_STAT_timed_out_ipmb_commands 7 + +/* + * This is like above, but for broadcasts. Broadcasts are + * *not* included in the above count (they are expected to + * time out). + */ +#define IPMI_STAT_timed_out_ipmb_broadcasts8 + +/* Responses I have sent to the IPMB bus. */ +#define IPMI_STAT_sent_ipmb_responses 9 + +/* The response was delivered to the user. */ +#define IPMI_STAT_handled_ipmb_responses 10 + +/* The response had invalid data in it. */ +#define IPMI_STAT_invalid_ipmb_responses 11 + +/* The response didn't have anyone waiting for it. */ +#define IPMI_STAT_unhandled_ipmb_responses 12 + +/* Commands we sent out to the IPMB bus. */ +#define IPMI_STAT_sent_lan_commands13 + +/* Commands sent on the IPMB that had errors on the SEND CMD */ +#define IPMI_STAT_sent_lan_command_errs14 + +/* Each retransmit increments this count. */ +#define IPMI_STAT_retransmitted_lan_commands 15 + +/* When a message times out (runs out of retransmits) this is incremented. */ +#define IPMI_STAT_timed_out_lan_commands 16 + +/* Responses I have sent to the IPMB bus. */ +#define IPMI_STAT_sent_lan_responses 17 + +/* The response was delivered to the user. */ +#define IPMI_STAT_handled_lan_responses18 + +/* The response had invalid data in it. */ +#define IPMI_STAT_invalid_lan_responses19 + +/* The response didn't have anyone waiting for it. */ +#define IPMI_STAT_unhandled_lan_responses 20 + +/* The command was delivered to the user. */ +#define IPMI_STAT_handled_commands 21 + +/* The command had invalid data in it. */ +#define IPMI_STAT_invalid_commands 22 + +/* The command didn't have anyone waiting for it. */ +#define IPMI_STAT_unhandled_commands 23 + +/* Invalid data in an event. */ +#define IPMI_STAT_invalid_events 24 + +/* Events that were received with the proper format. */ +#define IPMI_STAT_events 25 + +/* When you add a statistic, you must update this value. */ +#define IPMI_NUM_STATS 26 + + #define IPMI_IPMB_NUM_SEQ 64 #define IPMI_MAX_CHANNELS 16 struct ipmi_smi @@ -286,75 +375,7 @@ struct ipmi_smi struct proc_dir_entry *proc_dir; char proc_dir_name[10]; - spinlock_t counter_lock; /* For making counters atomic. */ - - /* Commands we got that were invalid. */ - unsigned int sent_invalid_commands; - - /* Commands we sent to the MC. */ - unsigned int sent_local_commands; - /* Responses from the MC that were delivered to a user. */ - unsigned int handled_local_responses; - /* Responses from the MC that were not delivered to a user. */ - unsigned int unhandled_local_responses; - - /* Commands we sent out to the IPMB bus. */ - unsigned int sent_ipmb_commands; - /* Commands sent on the IPMB that had errors on the SEND CMD */ - unsigned int sent_ipmb_command_errs; -
[PATCH 1/4] IPMI: Change barrier to cpu_relax in poweroff code
From: Corey Minyard <[EMAIL PROTECTED]>
Change a barrier to cpu_relax(), which is more appropriate for a
polling loop.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_poweroff.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_poweroff.c
@@ -175,7 +175,7 @@ static int ipmi_request_in_rc_mode(ipmi_
*/
while (atomic_read(&dummy_count) > 0) {
ipmi_poll_interface(user);
- barrier();
+ cpu_relax();
}
return halt_recv_msg.msg.data[0];
--
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/
[PATCH 4/4] IPMI: Convert locked counters to atomics in the system interface
From: Corey Minyard <[EMAIL PROTECTED]>
Atomics are faster and neater than locked counters.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
Index: linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
@@ -120,6 +120,27 @@ static struct device_driver ipmi_driver
.bus = &platform_bus_type
};
+
+/*
+ * Indexes into stats[] in smi_info below.
+ */
+
+#define SI_STAT_short_timeouts 0
+#define SI_STAT_long_timeouts 1
+#define SI_STAT_timeout_restarts 2
+#define SI_STAT_idles 3
+#define SI_STAT_interrupts 4
+#define SI_STAT_attentions 5
+#define SI_STAT_flag_fetches 6
+#define SI_STAT_hosed_count7
+#define SI_STAT_complete_transactions 8
+#define SI_STAT_events 9
+#define SI_STAT_watchdog_pretimeouts 10
+#define SI_STAT_incoming_messages 11
+
+/* If you add a stat, you must update this value. */
+#define SI_NUM_STATS 12
+
struct smi_info
{
intintf_num;
@@ -216,25 +237,18 @@ struct smi_info
unsigned char slave_addr;
/* Counters and things for the proc filesystem. */
- spinlock_t count_lock;
- unsigned long short_timeouts;
- unsigned long long_timeouts;
- unsigned long timeout_restarts;
- unsigned long idles;
- unsigned long interrupts;
- unsigned long attentions;
- unsigned long flag_fetches;
- unsigned long hosed_count;
- unsigned long complete_transactions;
- unsigned long events;
- unsigned long watchdog_pretimeouts;
- unsigned long incoming_messages;
+ atomic_t stats[SI_NUM_STATS];
struct task_struct *thread;
struct list_head link;
};
+#define smi_inc_stat(smi, stat) \
+ atomic_inc(&(smi)->stats[SI_STAT_ ## stat])
+#define smi_get_stat(smi, stat) \
+ ((unsigned int) atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
+
#define SI_MAX_PARMS 4
static int force_kipmid[SI_MAX_PARMS];
@@ -398,9 +412,7 @@ static void handle_flags(struct smi_info
retry:
if (smi_info->msg_flags & WDT_PRE_TIMEOUT_INT) {
/* Watchdog pre-timeout */
- spin_lock(&smi_info->count_lock);
- smi_info->watchdog_pretimeouts++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, watchdog_pretimeouts);
start_clear_flags(smi_info);
smi_info->msg_flags &= ~WDT_PRE_TIMEOUT_INT;
@@ -545,9 +557,7 @@ static void handle_transaction_done(stru
smi_info->msg_flags &= ~EVENT_MSG_BUFFER_FULL;
handle_flags(smi_info);
} else {
- spin_lock(&smi_info->count_lock);
- smi_info->events++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, events);
/* Do this before we deliver the message
because delivering the message releases the
@@ -581,9 +591,7 @@ static void handle_transaction_done(stru
smi_info->msg_flags &= ~RECEIVE_MSG_AVAIL;
handle_flags(smi_info);
} else {
- spin_lock(&smi_info->count_lock);
- smi_info->incoming_messages++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, incoming_messages);
/* Do this before we deliver the message
because delivering the message releases the
@@ -700,18 +708,14 @@ static enum si_sm_result smi_event_handl
if (si_sm_result == SI_SM_TRANSACTION_COMPLETE)
{
- spin_lock(&smi_info->count_lock);
- smi_info->complete_transactions++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, complete_transactions);
handle_transaction_done(smi_info);
si_sm_result = smi_info->handlers->event(smi_info->si_sm, 0);
}
else if (si_sm_result == SI_SM_HOSED)
{
- spin_lock(&smi_info->count_lock);
- smi_info->hosed_count++;
- spin_unlock(&smi_info->count_lock);
+ smi_inc_stat(smi_info, hosed_count);
/* Do the before return_hosed_msg, because that
releases the lock. */
@@ -733,9 +737,7 @@ static enum si_sm_result smi_event_handl
{
unsigned char msg[2];
- spin_lock(&smi_info->count_lock);
-
[PATCH 2/4] IPMI: Remove unnecessary memory barriers
From: Corey Minyard <[EMAIL PROTECTED]>
Remove some unnecessary barriers.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
This can be folded into ipmi-dont-grab-locks-in-run-to-completion-mode.patch
Index: linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
@@ -3518,7 +3518,6 @@ void ipmi_smi_msg_received(ipmi_smi_t
/* To preserve message order, if the list is not empty, we
tack this message onto the end of the list. */
run_to_completion = intf->run_to_completion;
- barrier();
if (!run_to_completion)
spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
if (!list_empty(&intf->waiting_msgs)) {
@@ -3535,7 +3534,6 @@ void ipmi_smi_msg_received(ipmi_smi_t
/* Could not handle the message now, just add it to a
list to handle later. */
run_to_completion = intf->run_to_completion;
- barrier();
if (!run_to_completion)
spin_lock_irqsave(&intf->waiting_msgs_lock, flags);
list_add_tail(&msg->link, &intf->waiting_msgs);
--
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/
Re: [PATCH 3/4] IPMI: convert locked counters to atomics
Andrew Morton wrote:
+ for (i = 0; i < IPMI_NUM_STATS; i++)
+ atomic_set(&intf->stats[i], 0);
And this is why it would be very hard for any architecture to ever
implement atomic_t as
struct atomic_t {
int counter;
spinlock_t lock;
};
The interface assumes that atomic_set() fully initialises the atomic_t, and
that atomic_set() can be used agaisnt both an uninitialised atomic_t and
against an already-initialised atomic_t. IOW, we don't have atomic_init().
So would our hypothetical future architcture's atomic_set() do spin_lock(),
or would it do spin_lock_init()? Either one is wrong in many atomic_set
callsites.
Oh well.
Yeah, I thought the same thing when I did this. Do we start working
on an atomic_init()? It would be easy enough to set it to atomic_set()
for current architectures.
-corey
--
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/
Re: [Openipmi-developer] [PATCH 3/4] IPMI: convert locked counters to atomics
Peter Zijlstra wrote: On Thu, 2008-02-14 at 12:30 -0600, Corey Minyard wrote: +/* The command didn't have anyone waiting for it. */ +#define IPMI_STAT_unhandled_commands 23 + +/* Invalid data in an event. */ +#define IPMI_STAT_invalid_events 24 + +/* Events that were received with the proper format. */ +#define IPMI_STAT_events 25 + +/* When you add a statistic, you must update this value. */ +#define IPMI_NUM_STATS 26 This shouts enum to me.. Someone else asked about this, and I wasn't too sure. It seems from the CodingStyle document that enums are preferred for things like this, but I current kernel code seems a mixed bag. An enum is cleaner and more reliable, IMHO. I'll convert it, I think. Thanks, -corey -- 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/
[PATCH 1/5] IPMI: convert message handler defines to an enum
From: Corey Minyard <[EMAIL PROTECTED]>
Convert the #defines for statistics into an enum in the IPMI message
handler.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Cc: Peter Zijlstra <[EMAIL PROTECTED]>
---
Ok to merge into ipmi-convert-locked-counters-to-atomics.patch
Index: linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_msghandler.c
@@ -189,90 +189,99 @@ struct bmc_device
* Various statistics for IPMI, these index stats[] in the ipmi_smi
* structure.
*/
-/* Commands we got from the user that were invalid. */
-#define IPMI_STAT_sent_invalid_commands0
+enum ipmi_stat_indexes {
+ /* Commands we got from the user that were invalid. */
+ IPMI_STAT_sent_invalid_commands = 0,
-/* Commands we sent to the MC. */
-#define IPMI_STAT_sent_local_commands 1
+ /* Commands we sent to the MC. */
+ IPMI_STAT_sent_local_commands,
-/* Responses from the MC that were delivered to a user. */
-#define IPMI_STAT_handled_local_responses 2
+ /* Responses from the MC that were delivered to a user. */
+ IPMI_STAT_handled_local_responses,
-/* Responses from the MC that were not delivered to a user. */
-#define IPMI_STAT_unhandled_local_responses3
+ /* Responses from the MC that were not delivered to a user. */
+ IPMI_STAT_unhandled_local_responses,
-/* Commands we sent out to the IPMB bus. */
-#define IPMI_STAT_sent_ipmb_commands 4
+ /* Commands we sent out to the IPMB bus. */
+ IPMI_STAT_sent_ipmb_commands,
-/* Commands sent on the IPMB that had errors on the SEND CMD */
-#define IPMI_STAT_sent_ipmb_command_errs 5
+ /* Commands sent on the IPMB that had errors on the SEND CMD */
+ IPMI_STAT_sent_ipmb_command_errs,
-/* Each retransmit increments this count. */
-#define IPMI_STAT_retransmitted_ipmb_commands 6
+ /* Each retransmit increments this count. */
+ IPMI_STAT_retransmitted_ipmb_commands,
-/* When a message times out (runs out of retransmits) this is incremented. */
-#define IPMI_STAT_timed_out_ipmb_commands 7
+ /*
+* When a message times out (runs out of retransmits) this is
+* incremented.
+*/
+ IPMI_STAT_timed_out_ipmb_commands,
-/*
- * This is like above, but for broadcasts. Broadcasts are
- * *not* included in the above count (they are expected to
- * time out).
- */
-#define IPMI_STAT_timed_out_ipmb_broadcasts8
+ /*
+* This is like above, but for broadcasts. Broadcasts are
+* *not* included in the above count (they are expected to
+* time out).
+*/
+ IPMI_STAT_timed_out_ipmb_broadcasts,
-/* Responses I have sent to the IPMB bus. */
-#define IPMI_STAT_sent_ipmb_responses 9
+ /* Responses I have sent to the IPMB bus. */
+ IPMI_STAT_sent_ipmb_responses,
-/* The response was delivered to the user. */
-#define IPMI_STAT_handled_ipmb_responses 10
+ /* The response was delivered to the user. */
+ IPMI_STAT_handled_ipmb_responses,
-/* The response had invalid data in it. */
-#define IPMI_STAT_invalid_ipmb_responses 11
+ /* The response had invalid data in it. */
+ IPMI_STAT_invalid_ipmb_responses,
-/* The response didn't have anyone waiting for it. */
-#define IPMI_STAT_unhandled_ipmb_responses 12
+ /* The response didn't have anyone waiting for it. */
+ IPMI_STAT_unhandled_ipmb_responses,
-/* Commands we sent out to the IPMB bus. */
-#define IPMI_STAT_sent_lan_commands13
+ /* Commands we sent out to the IPMB bus. */
+ IPMI_STAT_sent_lan_commands,
-/* Commands sent on the IPMB that had errors on the SEND CMD */
-#define IPMI_STAT_sent_lan_command_errs14
+ /* Commands sent on the IPMB that had errors on the SEND CMD */
+ IPMI_STAT_sent_lan_command_errs,
-/* Each retransmit increments this count. */
-#define IPMI_STAT_retransmitted_lan_commands 15
+ /* Each retransmit increments this count. */
+ IPMI_STAT_retransmitted_lan_commands,
-/* When a message times out (runs out of retransmits) this is incremented. */
-#define IPMI_STAT_timed_out_lan_commands 16
+ /*
+* When a message times out (runs out of retransmits) this is
+* incremented.
+*/
+ IPMI_STAT_timed_out_lan_commands,
-/* Responses I have sent to the IPMB bus. */
-#define IPMI_STAT_sent_lan_responses 17
+ /* Responses I have sent to the IPMB bus. */
+ IPMI_STAT_sent_lan_responses,
-/* The response was delivered to the user. */
-#define IPMI_STAT_handled_lan_respons
[PATCH 2/5] IPMI: Convert system interface defines to an enum
From: Corey Minyard <[EMAIL PROTECTED]>
Convert the #defines for statistics into an enum in the IPMI system
interface and remove the unused timeout_restart statistic. And comment
what these statistics mean.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Cc: Peter Zijlstra <[EMAIL PROTECTED]>
---
Ok to merge into
ipmi-convert-locked-counters-to-atomics-in-the-system-interface.patch
Index: linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.24.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.24/drivers/char/ipmi/ipmi_si_intf.c
@@ -124,22 +124,50 @@ static struct device_driver ipmi_driver
/*
* Indexes into stats[] in smi_info below.
*/
+enum si_stat_indexes {
+ /*
+* Number of times the driver requested a timer while an operation
+* was in progress.
+*/
+ SI_STAT_short_timeouts = 0,
+
+ /*
+* Number of times the driver requested a timer while nothing was in
+* progress.
+*/
+ SI_STAT_long_timeouts,
+
+ /* Number of times the interface was idle while being polled. */
+ SI_STAT_idles,
+
+ /* Number of interrupts the driver handled. */
+ SI_STAT_interrupts,
+
+ /* Number of time the driver got an ATTN from the hardware. */
+ SI_STAT_attentions,
-#define SI_STAT_short_timeouts 0
-#define SI_STAT_long_timeouts 1
-#define SI_STAT_timeout_restarts 2
-#define SI_STAT_idles 3
-#define SI_STAT_interrupts 4
-#define SI_STAT_attentions 5
-#define SI_STAT_flag_fetches 6
-#define SI_STAT_hosed_count7
-#define SI_STAT_complete_transactions 8
-#define SI_STAT_events 9
-#define SI_STAT_watchdog_pretimeouts 10
-#define SI_STAT_incoming_messages 11
+ /* Number of times the driver requested flags from the hardware. */
+ SI_STAT_flag_fetches,
-/* If you add a stat, you must update this value. */
-#define SI_NUM_STATS 12
+ /* Number of times the hardware didn't follow the state machine. */
+ SI_STAT_hosed_count,
+
+ /* Number of completed messages. */
+ SI_STAT_complete_transactions,
+
+ /* Number of IPMI events received from the hardware. */
+ SI_STAT_events,
+
+ /* Number of watchdog pretimeouts. */
+ SI_STAT_watchdog_pretimeouts,
+
+ /* Number of asyncronous messages received. */
+ SI_STAT_incoming_messages,
+
+
+ /* This *must* remain last, add new values above this. */
+ SI_NUM_STATS
+};
struct smi_info
{
@@ -2400,8 +2428,6 @@ static int stat_file_read_proc(char *pag
smi_get_stat(smi, short_timeouts));
out += sprintf(out, "long_timeouts: %u\n",
smi_get_stat(smi, long_timeouts));
- out += sprintf(out, "timeout_restarts: %u\n",
- smi_get_stat(smi, timeout_restarts));
out += sprintf(out, "idles: %u\n",
smi_get_stat(smi, idles));
out += sprintf(out, "interrupts:%u\n",
--
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/
[Patch 3/5] IPMI: Style fixes in the base code
From: Corey Minyard <[EMAIL PROTECTED]> Lots of style fixes for the base IPMI driver. No functional changes. Basically fixes everything reported by checkpatch and fixes the comment style. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Patch is too big for the list, per the Submitting Patches document. It is available at: http://mysite.verizon.net/tcminyard/ipmi-style-fixes.patch -- 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/
[Patch 4/5] IPMI: Style fixes in the system interface code
From: Corey Minyard <[EMAIL PROTECTED]> Lots of style fixes for the IPMI system interface driver. No functional changes. Basically fixes everything reported by checkpatch and fixes the comment style. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Cc: Rocky Craig <[EMAIL PROTECTED]> Cc: Hannes Schulz <[EMAIL PROTECTED]> Patch is too big for the list, per the Submitting Patches document. It is available at: http://mysite.verizon.net/tcminyard/ipmi-system-interface-style-fixes.patch -- 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/
[Patch 5/5] IPMI: Style fixes in the misc code
From: Corey Minyard <[EMAIL PROTECTED]> Lots of style fixes for the miscellaneous IPMI files. No functional changes. Basically fixes everything reported by checkpatch and fixes the comment style. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Patch is too big for the list, per the Submitting Patches document. It is available at: http://mysite.verizon.net/tcminyard/ipmi-misc-style-fixes.patch -- 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/
Re: [PATCH 1/2] ACPI: Reorder IPMI driver before any other ACPI drivers
On 09/20/2012 08:26 PM, Matthew Garrett wrote: On Thu, Sep 20, 2012 at 08:19:48PM -0500, Corey Minyard wrote: On 09/20/2012 04:46 PM, Matthew Garrett wrote: Drivers may make calls that require the ACPI IPMI driver to have been initialised already, so make sure that it appears earlier in the build order. The IPMI driver uses the ACPI namespace as an option to know the address and characteristics of the device. Does that still work? Yes, the ACPI interpreter is initialised earlier than the IPMI driver discovery. Cool, I'm good with these, then. Thanks, -corey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] IPMI: Fix some uninitialized warning
On 10/22/2012 06:49 PM, Andrew Morton wrote:
On Tue, 16 Oct 2012 15:53:39 -0500
miny...@acm.org wrote:
From: Corey Minyard
There was a spot where the compiler couldn't tell some variables
would be set. So initialize them to make the warning go away.
Signed-off-by: Corey Minyard
---
drivers/char/ipmi/ipmi_msghandler.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 2c29942..a0c84bb 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -1880,7 +1880,7 @@ int ipmi_request_supply_msgs(ipmi_user_t user,
struct ipmi_recv_msg *supplied_recv,
int priority)
{
- unsigned char saddr, lun;
+ unsigned char saddr = 0, lun = 0;
int rv;
if (!user)
The kernel build actually generates quite a lot of "bar uninitialised"
warnings for foo(&bar) expressions. I just ignore them, because later
versions of gcc stopped doing that.
The fix is OK, I suppose. But it will cause additional code to be
emitted. Using uninitialized_var() avoids that, and makes things
clearer to the reader.
Well, warnings annoy me, especially when I get the emails about them.
But I suppose that's the point of the emails :).
I'll do a patch to switch it to uninitialized_var() for the next set of
things I send. Thanks.
-corey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v2] ipmi: info leak in compat_ipmi_ioctl()
On 05/31/2013 07:46 AM, Dan Carpenter wrote: On x86_64 there is a 4 byte hole between ->recv_type and ->addr. Got it, in my tree now. Thanks. Signed-off-by: Dan Carpenter --- v2: fixed the changelog a little. Also added LKML because the openipmi is a moderated list (and the moderator thought my email was spam). I apologize, the list gets a lot of spam, and I must have made a mistake. -corey diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c index 9eb360f..8e306ac 100644 --- a/drivers/char/ipmi/ipmi_devintf.c +++ b/drivers/char/ipmi/ipmi_devintf.c @@ -810,6 +810,7 @@ static long compat_ipmi_ioctl(struct file *filep, unsigned int cmd, struct ipmi_recv __user *precv64; struct ipmi_recv recv64; + memset(&recv64, 0, sizeof(recv64)); if (get_compat_ipmi_recv(&recv64, compat_ptr(arg))) return -EFAULT; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
On 07/25/2013 07:06 AM, Rafael J. Wysocki wrote:
On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote:
-stable according to the previous conversation.
From: Rafael J. Wysocki [mailto:r...@sisk.pl]
Sent: Thursday, July 25, 2013 7:38 AM
On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
This patch fixes races caused by unprotected ACPI IPMI transfers.
We can see the following crashes may occur:
1. There is no tx_msg_lock held for iterating tx_msg_list in
ipmi_flush_tx_msg() while it is parellel unlinked on failure in
acpi_ipmi_space_handler() under protection of tx_msg_lock.
2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler()
while it is parellel accessed in ipmi_flush_tx_msg() and
ipmi_msg_handler().
This patch enhances tx_msg_lock to protect all tx_msg accesses to
solve this issue. Then tx_msg_lock is always held around complete()
and tx_msg accesses.
Calling smp_wmb() before setting msg_done flag so that messages
completed due to flushing will not be handled as 'done' messages while
their contents are not vaild.
Signed-off-by: Lv Zheng
Cc: Zhao Yakui
Reviewed-by: Huang Ying
---
drivers/acpi/acpi_ipmi.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
b37c189..527ee43 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
acpi_ipmi_device *ipmi)
struct acpi_ipmi_msg *tx_msg, *temp;
int count = HZ / 10;
struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+ unsigned long flags;
+ spin_lock_irqsave(&ipmi->tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
/* wake up the sleep thread on the Tx msg */
complete(&tx_msg->tx_complete);
}
+ spin_unlock_irqrestore(&ipmi->tx_msg_lock, flags);
/* wait for about 100ms to flush the tx message list */
while (count--) {
@@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
ipmi_recv_msg *msg, void *user_msg_data)
break;
}
}
- spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
if (!msg_found) {
dev_warn(&pnp_dev->dev,
"Unexpected response (msg id %ld) is returned.\n",
msg->msgid);
- goto out_msg;
+ goto out_lock;
}
/* copy the response data to Rx_data buffer */ @@ -286,10 +288,14 @@
static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void
*user_msg_data)
}
tx_msg->rx_len = msg->msg.data_len;
memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);
+ /* tx_msg content must be valid before setting msg_done flag */
+ smp_wmb();
That's suspicious.
If you need the write barrier here, you'll most likely need a read barrier
somewhere else. Where's that?
It might depend on whether the content written before the smp_wmb() is used or
not by the other side codes under the condition set after the smp_wmb().
So comment could be treated as 2 parts:
1. do we need a paired smp_rmb().
2. do we need a smp_wmb().
For 1.
If we want a paired smp_rmb(), then it will appear in this function:
186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
187 acpi_integer *value, int rem_time)
188 {
189 struct acpi_ipmi_buffer *buffer;
190
191 /*
192 * value is also used as output parameter. It represents the
response
193 * IPMI message returned by IPMI command.
194 */
195 buffer = (struct acpi_ipmi_buffer *)value;
196 if (!rem_time && !msg->msg_done) {
197 buffer->status = ACPI_IPMI_TIMEOUT;
198 return;
199 }
200 /*
201 * If the flag of msg_done is not set or the recv length is zero, it
202 * means that the IPMI command is not executed correctly.
203 * The status code will be ACPI_IPMI_UNKNOWN.
204 */
205 if (!msg->msg_done || !msg->rx_len) {
206 buffer->status = ACPI_IPMI_UNKNOWN;
207 return;
208 }
+ smp_rmb();
209 /*
210 * If the IPMI response message is obtained correctly, the status
code
211 * will be ACPI_IPMI_OK
212 */
213 buffer->status = ACPI_IPMI_OK;
214 buffer->length = msg->rx_len;
215 memcpy(buffer->data, msg->rx_data, msg->rx_len);
216 }
If we don't then there will only be msg content not correctly read from
msg->rx_data.
Note that the rx_len is 0 during initialization and will never exceed the
sizeof(buffer->data), so the read is safe.
Being without smp_rmb() is also OK in this case, since:
1. buffer->data will never be used when buffer->status is not ACPI_IPMI_OK and
2. the smp_rmb()/smp_wmb() added in this patch will be
Re: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
On 07/25/2013 07:16 PM, Zheng, Lv wrote: If I understand this correctly, the problem would be if: rem_time = wait_for_completion_timeout(&tx_msg->tx_complete, IPMI_TIMEOUT); returns on a timeout, then checks msg_done and races with something setting msg_done. If that is the case, you would need the smp_rmb() before checking msg_done. However, the timeout above is unnecessary. You are using ipmi_request_settime(), so you can set the timeout when the IPMI command fails and returns a failure message. The driver guarantees a return message for each request. Just remove the timeout from the completion, set the timeout and retries in the ipmi request, and the completion should handle the barrier issues. It's just difficult for me to determine retry count and timeout value, maybe retry=0, timeout=IPMI_TIMEOUT is OK. The code of the timeout completion is already there, I think the quick fix code should not introduce this logic. I'll add a new patch to apply your comment. Since it is a local BMC, I doubt a retry is required. That is probably fine. Or you could set retry=1 and timeout=IPMI_TIMEOUT/2 if you wanted to be more sure, but I doubt it would make a difference. The only time you really need to worry about retries is if you are resetting the BMC or it is being overloaded. Plus, from a quick glance at the code, it doesn't look like it will properly handle a situation where the timeout occurs and is handled then the response comes in later. PATCH 07 fixed this issue. Here we just need the smp_rmb() or holding tx_msg_lock() around the acpi_format_ipmi_response(). If you apply the fix like I suggest, then the race goes away. If there's no timeout and it just waits for the completion, things get a lot simpler. Thanks for commenting. No problem, thanks for working on this. -corey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipmi: ipmi_devintf: compat_ioctl method failes to take ipmi_mutex
Yes, you are right. I've pulled this in to my tree. Looking at this,
ipmi_mutex really should go away and be replaced bu something that
scales better, but I guess it's not that critical for IPMI.
-corey
On 05/13/2013 02:39 PM, Benjamin LaHaise wrote:
When a 32 bit version of ipmitool is used on a 64 bit kernel, the
ipmi_devintf code fails to correctly acquire ipmi_mutex. This results in
incomplete data being retrieved in some cases, or other possible failures.
Add a wrapper around compat_ipmi_ioctl() to take ipmi_mutex to fix this.
This is probably a candidate for -stable as well.
Signed-off-by: Benjamin LaHaise
---
drivers/char/ipmi/ipmi_devintf.c | 14 +-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
index 9eb360f..d5a5f02 100644
--- a/drivers/char/ipmi/ipmi_devintf.c
+++ b/drivers/char/ipmi/ipmi_devintf.c
@@ -837,13 +837,25 @@ static long compat_ipmi_ioctl(struct file *filep,
unsigned int cmd,
return ipmi_ioctl(filep, cmd, arg);
}
}
+
+static long unlocked_compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret;
+
+ mutex_lock(&ipmi_mutex);
+ ret = compat_ipmi_ioctl(filep, cmd, arg);
+ mutex_unlock(&ipmi_mutex);
+
+ return ret;
+}
#endif
static const struct file_operations ipmi_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = ipmi_unlocked_ioctl,
#ifdef CONFIG_COMPAT
- .compat_ioctl = compat_ipmi_ioctl,
+ .compat_ioctl = unlocked_compat_ipmi_ioctl,
#endif
.open = ipmi_open,
.release= ipmi_release,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] ipmi: Improve error messages on failed irq enable
When the interrupt enable message returns an error, the messages are
not entirely accurate nor helpful. So improve them.
Signed-off-by: Corey Minyard
Cc: Andy Lutomirski
---
drivers/char/ipmi/ipmi_si_intf.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 313538a..af4b23f 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -663,8 +663,10 @@ static void handle_transaction_done(struct smi_info
*smi_info)
/* We got the flags from the SMI, now handle them. */
smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
if (msg[2] != 0) {
- dev_warn(smi_info->dev, "Could not enable interrupts"
-", failed get, using polled mode.\n");
+ dev_warn(smi_info->dev,
+"Couldn't get irq info: %x.\n", msg[2]);
+ dev_warn(smi_info->dev,
+"Maybe ok, but ipmi might run very slowly.\n");
smi_info->si_state = SI_NORMAL;
} else {
msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
@@ -685,10 +687,12 @@ static void handle_transaction_done(struct smi_info
*smi_info)
/* We got the flags from the SMI, now handle them. */
smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
- if (msg[2] != 0)
- dev_warn(smi_info->dev, "Could not enable interrupts"
-", failed set, using polled mode.\n");
- else
+ if (msg[2] != 0) {
+ dev_warn(smi_info->dev,
+"Couldn't set irq info: %x.\n", msg[2]);
+ dev_warn(smi_info->dev,
+"Maybe ok, but ipmi might run very slowly.\n");
+ } else
smi_info->interrupt_disabled = 0;
smi_info->si_state = SI_NORMAL;
break;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/4] drivers: char: ipmi: Replaced kmalloc and strcpy with kstrdup
From: Alexandru Gheorghiu
Replaced calls to kmalloc followed by strcpy with a sincle call to kstrdup.
Patch found using coccinelle.
Signed-off-by: Alexandru Gheorghiu
Signed-off-by: Corey Minyard
---
drivers/char/ipmi/ipmi_msghandler.c |3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 4d439d2..4445fa1 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2037,12 +2037,11 @@ int ipmi_smi_add_proc_entry(ipmi_smi_t smi, char *name,
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
return -ENOMEM;
- entry->name = kmalloc(strlen(name)+1, GFP_KERNEL);
+ entry->name = kstrdup(name, GFP_KERNEL);
if (!entry->name) {
kfree(entry);
return -ENOMEM;
}
- strcpy(entry->name, name);
file = proc_create_data(name, 0, smi->proc_dir, proc_ops, data);
if (!file) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/4] ipmi: Some minor fixes
Some minor fixes I had queued up. The last one came in recently (patch 4) and it and patch 2 are candidates for stable-kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] drivers/char/ipmi: memcpy, need additional 2 bytes to avoid memory overflow
From: Chen Gang
when calling memcpy, read_data and write_data need additional 2 bytes.
write_data:
for checking: "if (size > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(bt->write_data + 3, data + 1, size - 1)"
read_data:
for checking: "if (msg_len < 3 || msg_len > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(data + 2, bt->read_data + 4, msg_len - 2)"
Signed-off-by: Chen Gang
Signed-off-by: Corey Minyard
Cc: sta...@vger.kernel.org
---
drivers/char/ipmi/ipmi_bt_sm.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c
index cdd4c09f..a22a7a5 100644
--- a/drivers/char/ipmi/ipmi_bt_sm.c
+++ b/drivers/char/ipmi/ipmi_bt_sm.c
@@ -95,9 +95,9 @@ struct si_sm_data {
enum bt_states state;
unsigned char seq;/* BT sequence number */
struct si_sm_io *io;
- unsigned char write_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char write_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int write_count;
- unsigned char read_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char read_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int read_count;
int truncated;
longtimeout;/* microseconds countdown */
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] ipmi: ipmi_devintf: compat_ioctl method fails to take ipmi_mutex
From: Benjamin LaHaise
When a 32 bit version of ipmitool is used on a 64 bit kernel, the
ipmi_devintf code fails to correctly acquire ipmi_mutex. This results in
incomplete data being retrieved in some cases, or other possible failures.
Add a wrapper around compat_ipmi_ioctl() to take ipmi_mutex to fix this.
Signed-off-by: Benjamin LaHaise
Signed-off-by: Corey Minyard
Cc: sta...@vger.kernel.org
---
drivers/char/ipmi/ipmi_devintf.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
index 9eb360f..d5a5f02 100644
--- a/drivers/char/ipmi/ipmi_devintf.c
+++ b/drivers/char/ipmi/ipmi_devintf.c
@@ -837,13 +837,25 @@ static long compat_ipmi_ioctl(struct file *filep,
unsigned int cmd,
return ipmi_ioctl(filep, cmd, arg);
}
}
+
+static long unlocked_compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret;
+
+ mutex_lock(&ipmi_mutex);
+ ret = compat_ipmi_ioctl(filep, cmd, arg);
+ mutex_unlock(&ipmi_mutex);
+
+ return ret;
+}
#endif
static const struct file_operations ipmi_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = ipmi_unlocked_ioctl,
#ifdef CONFIG_COMPAT
- .compat_ioctl = compat_ipmi_ioctl,
+ .compat_ioctl = unlocked_compat_ipmi_ioctl,
#endif
.open = ipmi_open,
.release= ipmi_release,
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] ipmi: Improve error messages on failed irq enable
On 05/16/2013 05:23 PM, Andy Lutomirski wrote:
/* We got the flags from the SMI, now handle them. */
smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
- if (msg[2] != 0)
- dev_warn(smi_info->dev, "Could not enable interrupts"
-", failed set, using polled mode.\n");
- else
+ if (msg[2] != 0) {
+ dev_warn(smi_info->dev,
+"Couldn't set irq info: %x.\n", msg[2]);
+ dev_warn(smi_info->dev,
+"Maybe ok, but ipmi might run very slowly.\n");
+ } else
Minor nit: it would be nice if these warnings were collapsed into a
single printk -- that would save me a whitelist entry of acceptable
KERN_WARNING messages :)
Yeah, the trouble is that checkpatch will give a warning if you split a
string
between two lines or if a line is longer than 80 characters. I'm not
creative
enough to fit it into a single line. Maybe I'm trying to be too literal
here,
but I split it into two prints to avoid the warning.
My Dell 12g server says:
[97627.407724] ipmi_si ipmi_si.0: Using irq 10
[97627.421369] ipmi_si ipmi_si.0: Couldn't set irq info: cc.
[97627.427389] ipmi_si ipmi_si.0: Maybe ok, but ipmi might run very slowly.
Tested-by: Andy Lutomirski
Thanks a bunch.
-corey
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/char/ipmi: memcpy, need additional 2 bytes to avoid memory overflow
Yes, this is correct, I've pulled it into my tree.
-corey
On 03/29/2013 02:18 AM, Chen Gang wrote:
when calling memcpy, read_data and write_data need additional 2 bytes.
write_data:
for checking: "if (size > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(bt->write_data + 3, data + 1, size - 1)"
read_data:
for checking: "if (msg_len < 3 || msg_len > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(data + 2, bt->read_data + 4, msg_len - 2)"
Signed-off-by: Chen Gang
---
drivers/char/ipmi/ipmi_bt_sm.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c
index cdd4c09f..a22a7a5 100644
--- a/drivers/char/ipmi/ipmi_bt_sm.c
+++ b/drivers/char/ipmi/ipmi_bt_sm.c
@@ -95,9 +95,9 @@ struct si_sm_data {
enum bt_states state;
unsigned char seq;/* BT sequence number */
struct si_sm_io *io;
- unsigned char write_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char write_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int write_count;
- unsigned char read_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char read_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int read_count;
int truncated;
longtimeout;/* microseconds countdown */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CRIS: Fix I/O macros
Can we get something like this in? It would fix a lot of build regressions.
-corey
On 07/09/2012 03:22 PM, miny...@acm.org wrote:
From: Corey Minyard
The inb/outb macros for CRIS are broken from a number of points of
view, missing () around parameters and they have an unprotected if
statement in them. This was breaking the compile of IPMI on CRIS
and thus I was being annoyed by build regressions, so I fixed them.
Plus I don't think they would have worked at all, since the data
values were missing "&" and the outsl had a "3" instead of a "4"
for the size. From what I can tell, this stuff is not used at all,
so this can't be any more broken than it was before, anyway.
Signed-off-by: Corey Minyard
---
arch/cris/include/asm/io.h | 39 +--
1 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/arch/cris/include/asm/io.h b/arch/cris/include/asm/io.h
index 32567bc..ac12ae2 100644
--- a/arch/cris/include/asm/io.h
+++ b/arch/cris/include/asm/io.h
@@ -133,12 +133,39 @@ static inline void writel(unsigned int b, volatile void
__iomem *addr)
#define insb(port,addr,count) (cris_iops ?
cris_iops->read_io(port,addr,1,count) : 0)
#define insw(port,addr,count) (cris_iops ?
cris_iops->read_io(port,addr,2,count) : 0)
#define insl(port,addr,count) (cris_iops ?
cris_iops->read_io(port,addr,4,count) : 0)
-#define outb(data,port) if (cris_iops)
cris_iops->write_io(port,(void*)(unsigned)data,1,1)
-#define outw(data,port) if (cris_iops)
cris_iops->write_io(port,(void*)(unsigned)data,2,1)
-#define outl(data,port) if (cris_iops)
cris_iops->write_io(port,(void*)(unsigned)data,4,1)
-#define outsb(port,addr,count) if(cris_iops)
cris_iops->write_io(port,(void*)addr,1,count)
-#define outsw(port,addr,count) if(cris_iops)
cris_iops->write_io(port,(void*)addr,2,count)
-#define outsl(port,addr,count) if(cris_iops)
cris_iops->write_io(port,(void*)addr,3,count)
+static inline void outb(unsigned char data, unsigned int port)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *) &data, 1, 1);
+}
+static inline void outw(unsigned short data, unsigned int port)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *) &data, 2, 1);
+}
+static inline void outl(unsigned int data, unsigned int port)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *) &data, 4, 1);
+}
+static inline void outsb(unsigned int port, const void *addr,
+unsigned long count)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *)addr, 1, count);
+}
+static inline void outsw(unsigned int port, const void *addr,
+unsigned long count)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *)addr, 2, count);
+}
+static inline void outsl(unsigned int port, const void *addr,
+unsigned long count)
+{
+ if (cris_iops)
+ cris_iops->write_io(port, (void *)addr, 4, count);
+}
/*
* Convert a physical pointer to a virtual kernel pointer for /dev/mem
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ACPI: Reorder IPMI driver before any other ACPI drivers
On 09/20/2012 04:46 PM, Matthew Garrett wrote: Drivers may make calls that require the ACPI IPMI driver to have been initialised already, so make sure that it appears earlier in the build order. The IPMI driver uses the ACPI namespace as an option to know the address and characteristics of the device. Does that still work? -corey Signed-off-by: Matthew Garrett --- drivers/acpi/Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 47199e2..82422fe 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -47,6 +47,10 @@ acpi-y += video_detect.o endif # These are (potentially) separate modules + +# IPMI may be used by other drivers, so it has to initialise before them +obj-$(CONFIG_ACPI_IPMI)+= acpi_ipmi.o + obj-$(CONFIG_ACPI_AC) += ac.o obj-$(CONFIG_ACPI_BUTTON) += button.o obj-$(CONFIG_ACPI_FAN)+= fan.o @@ -70,6 +74,5 @@ processor-y += processor_idle.o processor_thermal.o processor-$(CONFIG_CPU_FREQ) += processor_perflib.o obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o -obj-$(CONFIG_ACPI_IPMI)+= acpi_ipmi.o obj-$(CONFIG_ACPI_APEI) += apei/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Openipmi-developer] [PATCH 3/4] IPMI: convert locked counters to atomics
Matt Domsch wrote: On Thu, Feb 14, 2008 at 12:30:51PM -0600, Corey Minyard wrote: From: Konstantin Baydarov <[EMAIL PROTECTED]> Atomics are a lot more efficient and neat than using a lock. per_cpu variables are a lot more efficient and neat than using locks for simple statistics. no cache line bouncing to increment the counter. Are these read so often that atomics are really better? I agree, I'll put this in the plan for the future. I don't have time to do it at this point, but the changes made in these patches should make it easy to change in the future. Thanks, -corey -- 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/
Re: [PATCH] Move console redirect to pid namespace
On 02/13/2013 01:08 PM, Eric W. Biederman wrote: Bruno Prémont writes: CCing containers list On Fri, 08 February 2013 miny...@acm.org wrote: From: Corey Minyard The console redirect - ioctl(fd, TIOCCONS) - is not in a namespace, thus a container can do a redirect and grab all the I/O on the host and all container consoles. This change puts the redirect in the pid namespace. Signed-off-by: Corey Minyard --- I'm pretty sure this patch is not correct, but I'm not quite sure the best way to fix this. I'm not 100% sure that the pid namespace is the right place, but it seemed the most reasonable of all the choices. The other obvious choice is the mount namespace, but it didn't seem as good a fit. With recent changes, tying it to init user namespace might even be better. With recent changes this is tied to the initial user namespace. So the simple solution to this and so many other similiar security problems is to run your container in a user namespace. The permission check currently is capable(CAP_SYS_ADMIN) which requires the caller to have the CAP_SYS_ADMIN in the initial user namespace. I'm not sure I follow. Are these changes in k.org, or in another repository someplace? Is there a desire to have TIOCCONS not just fail in a container but to have TIOCCONS work in a container specific way? Well, my desire is for the host console to work properly if a container uses TIOCCONS :-). It seems to me that the most consistent way to handle this is to have TIOCCONS in a container redirect the container's console. The other problem is that I don't think you can call fput() from destroy_pid_namespace(). That can be called from interrupt context, and I don't think fput() is safe there. I know it's not safe in 3.4 with the RT patch applied. However, the only way I've come up with to fix it is to add a workqueue, and that seems a bit heavy for this. Actually getting destroy_pid_namespace out of interrupt context wouldn't be the worst thing in the world. I would agree, but it would still require something like a workqueue. Is there a better mechanism? -corey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Move console redirect to pid namespace
On 02/14/2013 10:23 PM, Eric W. Biederman wrote: With recent changes this is tied to the initial user namespace. So the simple solution to this and so many other similiar security problems is to run your container in a user namespace. The permission check currently is capable(CAP_SYS_ADMIN) which requires the caller to have the CAP_SYS_ADMIN in the initial user namespace. I'm not sure I follow. Are these changes in k.org, or in another repository someplace? In k.org. 3.7 would work. 3.8-rcX would work even better. root in a user namespace does not have permission to call TIOCCONS. Ok, that's good enough for me. I don't have a compelling reason to make it work, beyond liking consistency. Thank you, -corey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipmi: add new kernel options to prevent automatic ipmi init
Well, the built-in driver works on systems that have more than one interface
and more than one BMC, and multiple IPMBs (and all of the other channel
types for that matter, and the driver handles all the multiplexing and nasty
addressing). There is, in fact, no arbitrary limit, and IBM tested
this fairly extensively with some of their systems. I'm not sure why you
would need a custom driver, and if there are some custom things that need
to be done for your servers, I'd be happy to add that. I've worked with a
number of other vendors to get changes like this in. And then ipmitool,
freeipmi, openipmi, etc. will work with the device.
I don't have a big problem with this patch. I wonder why you would want
to compile the standard driver into your kernel if you expected to load a
module with a custom driver later, though. None of the distros I know of
compile it in, it's always a module.
You can also dynamically remove the device from the driver using the hot
add/remove capability. To remove it, you can do:
echo remove,`cat /proc/ipmi/0/params`
and it will disassociate that device (IPMI interface 0 in this case)
from the
driver. So you can iterate through all the devices in /proc/ipmi and
remove them all at startup.
If none of the above options work for you, we can go ahead with this
patch. Just wanted to let you know that current options exist, and see
if you wanted to take a different direction.
-corey
On 12/13/2012 12:40 PM, Tony Camuso wrote:
The configuration change building ipmi_si into the kernel precludes the
use of a custom driver that can utilize more than one KCS interface,
multiple IPMBs, and more than one BMC. This capability is important for
fault-tolerant systems.
Even if the kernel option ipmi_si.trydefaults=0 is specified, ipmi_si
discovers and claims one of the KCS interfaces on a Stratus server.
The inability to now prevent the kernel from managing this device is a
regression from previous kernels. The regression breaks a capability
fault-tolerant vendors have relied upon.
To support both ACPI opregion access and the need to avoid activation
of ipmi_si on some platforms, we've added two new kernel options,
ipmi_si.tryacpi and ipmi_si.trydmi be added to prevent ipmi_si from
initializing when these options are set to 0 on the kernel command line.
With these options at the default value of 1, ipmi_si init proceeds
according to the kernel default.
Tested-by: Jim Paradis
Signed-off-by: Robert Evans
Signed-off-by: Jim Paradis
Signed-off-by: Tony Camuso
---
drivers/char/ipmi/ipmi_si_intf.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 20ab5b3..0a441cf 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1208,6 +1208,12 @@ static int smi_num; /* Used to sequence the SMIs */
#define DEFAULT_REGSPACING1
#define DEFAULT_REGSIZE 1
+#ifdef CONFIG_ACPI
+static int si_tryacpi = 1;
+#endif
+#ifdef CONFIG_DMI
+static int si_trydmi = 1;
+#endif
static bool si_trydefaults = 1;
static char *si_type[SI_MAX_PARMS];
#define MAX_SI_TYPE_STR 30
@@ -1238,6 +1244,16 @@ MODULE_PARM_DESC(hotmod, "Add and remove interfaces.
See"
" Documentation/IPMI.txt in the kernel sources for the"
" gory details.");
+#ifdef CONFIG_ACPI
+module_param_named(tryacpi, si_tryacpi, bool, 0);
+MODULE_PARM_DESC(tryacpi, "Setting this to zero will disable the"
+" default scan of the interfaces identified via ACPI");
+#endif
+#ifdef CONFIG_DMI
+module_param_named(trydmi, si_trydmi, bool, 0);
+MODULE_PARM_DESC(trydmi, "Setting this to zero will disable the"
+" default scan of the interfaces identified via DMI");
+#endif
module_param_named(trydefaults, si_trydefaults, bool, 0);
MODULE_PARM_DESC(trydefaults, "Setting this to 'false' will disable the"
" default scan of the KCS and SMIC interface at the standard"
@@ -3408,16 +3424,20 @@ static int init_ipmi_si(void)
#endif
#ifdef CONFIG_ACPI
- pnp_register_driver(&ipmi_pnp_driver);
- pnp_registered = 1;
+ if (si_tryacpi) {
+ pnp_register_driver(&ipmi_pnp_driver);
+ pnp_registered = 1;
+ }
#endif
#ifdef CONFIG_DMI
- dmi_find_bmc();
+ if (si_trydmi)
+ dmi_find_bmc();
#endif
#ifdef CONFIG_ACPI
- spmi_find_bmc();
+ if (si_tryacpi)
+ spmi_find_bmc();
#endif
/* We prefer devices with interrupts, but in the case of a machine
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipmi: add new kernel options to prevent automatic ipmi init
On 12/14/2012 10:25 AM, Evans, Robert wrote:
Corey,
Thanks for the thoughtful reply. Below I respond in detail to
these three points.
1) Why building a variant kernel with ipmi_si as a module is not
feasible.
2) User mode access to IPMI on Stratus systems (e.g. ipmitool).
3) ipmi_si hot removal seems to not work as needed.
Stratus might be able to use the hot removal option instead of the
proposed patch if hot removal can remove all interfaces from usage
by ipmi_si. Our testing of this option was not successful as
shown below.
- - -
1) Why building a variant kernel with ipmi_si as a module is not
feasible:
Stratus sells servers based upon Red Hat Enterprise Linux (RHEL).
In the next release of RHEL, ipmi_si will be built into the kernel
so that access to ACPI opregion is available early in kernel
startup. Stratus systems run the Red Hat kernel so that the
system is certified and supported by Red Hat. For this reason
using a custom kernel configured to build ipmi_si as a module is
not an option.
Yes, the RHEL engineer explained this to me, and it makes sense now. Thanks.
2) User mode access to IPMI on Stratus systems:
Although Stratus provides a replacement for ipmi_si, we depend
on ipmi_msghandler and ipmi_devintf. The device /dev/ipmi0 is
present and this device is utilized by the user-mode system
management software Stratus supplies.
Therefore other programs like ipmitool can send IPMI commands and
get responses on Stratus systems.
Ah, ok. That's good.
3) Hot removal of the KCS interfaces discovered by ipmi_si seems
to not do enough... One KCS cannot successfully be removed:
Based upon your suggestion, we tried to use hot removal. With
RHEL 6.4 Beta (kernel-2.6.32-343.el6), Stratus attempted to hot
remove the IPMI interfaces. This was booted with
"ipmi_si.trydefaults=0"
although we expect that kernel option to have no effect since a
BMC is found before the defaults would be tried.
This is logged when ipmi_si initializes indicating that both BMCs
were discovered:
ipmi message handler version 39.2
IPMI System Interface driver.
ipmi_si: Trying ACPI-specified kcs state machine at i/o address 0xca2,
slave address 0x0, irq 0
ipmi: Found new BMC (man_id: 0x77, prod_id: 0x05c6, dev_id: 0x41)
IPMI kcs interface initialized
ipmi_si: Adding SMBIOS-specified kcs state machine
ipmi_si: Trying SMBIOS-specified kcs state machine at i/o address 0xda2,
slave address 0x20, irq 0
ipmi: interfacing existing BMC (man_id: 0x77, prod_id: 0x05c6,
dev_id: 0x41)
IPMI kcs interface initialized
Although there are two different BMCs, because it says
"interfacing existing BMC"
it appears that ipmi_si assumes they are the same BMC.
That's happening in the message handler and it happens because the
manufacturer, product, and device id all match. From the spec:
The Device ID is typically used in combination with the Product ID
field such
that the Device IDs for different controllers are unique under a
given Product
ID. A controller can optionally use the Device ID as an ‘instance’
identifier if
more than one controller of that kind is used in the system.
(Section 20.1)
Different controllers in the same system are supposed to have different
device
IDs.
Also, I notice the slave address for the first KCS (port CA2) seems
wrong. Maybe these findings are relevant to what happens next.
Probably not relevant. It's not correct because, for some bizarre
reason, the slave address is not present in the ACPI information.
The slave address is only used by the message handler for the
IPMB return address on messages routed over IPMB.
It is odd that one interface is specified in ACPI and the other in DMI.
You can specify all of them in both tables.
After ipmi_si has been initialized, a script runs to load ftmod, the
module that contains the Stratus IPMI driver. This code was added to
hot remove the interfaces discovered by ipmi_si before loading ftmod:
for i in $(cd /proc/ipmi; ls)
do
dev="IPMI${i}"
params="$(cat /proc/ipmi/${i}/params)"
msg="Considering removal of dev: ${dev} ${params}"
logger -p kern.info -t `basename ${0}` "${msg}"
echo "${msg}" > /dev/console
[ -n "${params}" ] &&
echo "remove,`cat /proc/ipmi/${i}/params`" \
> /sys/module/ipmi_si/parameters/hotmod
done
In the console log we can see this script run prior to loading the
Stratus ftmod.ko and we also see that ftmod exposes a BMC:
Considering removal of dev: IPMI0
kcs,i/o,0xca2,rsp=1,rsi=1,rsh=0,irq=0,ipmb=0
Considering removal of dev: IPMI1
kcs,i/o,0xda2,rsp=1,rsi=1,rsh=0,irq=0,ipmb=32
ftmod: module license 'LGPL' taints kernel.
Disabling lock debugging due to kernel taint
FTMOD version lsb-ft-ftmod-9.0.4-209
ftmod: GLOBAL_SIZE=4194304
ftmod: global_cc_memory 0x88003740
ipmi: Found new BMC (man_id: 0x00, prod_id: 0x, dev_id: 0x00)
ipmi device interface
The KCS at port DA2 is removed from use by ipmi_si. However, the
Re: [PATCH] namespace: Add an identifier to the namespace file
On 09/26/2012 07:48 PM, Eric W. Biederman wrote: > I have fixed up Serge's email address and the linux-kernel email > address. > > Corey Minyard writes: > >> On 09/26/2012 07:16 PM, Eric W. Biederman wrote: >>> miny...@acm.org writes: >>> >>>> From: Corey Minyard >>>> >>>> Add a unique identifier to the network namespace file. Each namespace >>>> will have an identifier, making it possible to find if two programs are >>>> in the same namespace, or if a program is using a persistent >>>> namespace. >>> Corey. >>> >>> Please have a peak inside of >>> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git >>> userns-always-map-user-v59 >>> >>> That implements consistent inode numbers across all of the >>> /proc//ns/net files in the same network namespace so it is possible >>> to detect if one task is in the same network namespace as another. >>> >>> I believe that should have what you need. >> I did think about that, but I didn't do that for two reasons: 1) It >> seemed to go against the grain of procfs (well, and it seemed harder, >> too) and 2) I would like to add a netfilter based upon namespace. >> >> The first one is now moot, of course, but I'm not sure what to do about >> the second one. I know we have customers that are using our current VRF >> implementation to do exactly that, but I couldn't think of a way to do >> that. Perhaps I can use the proc inode number, because it will be fixed. >> I'll have to look into that. > For netfilter or any other interface what should happen is that a file > descriptor is passed in and the network namespace is derived from the > file descriptor. > > Given that netfilter is per network namespace I'm not certain how > filtering per network namespace would make sense. > > When I have poked my nose into netfilter I have had the hard problem > that I have not figured out how to translate the parameters supplied > by userspace into a more appropriate in kernel form, so I'm not certain > how to implement passing a file descriptor into netfilter efficiently. Yeah, that's kind of where I am stuck, too. netfilter used to be global, so if it's not, this may be a "You can't do that any more". >>> I need to get my act together on a merge strategy for that bit of code. >>> >>> >>>> Signed-off-by: Corey Minyard >>>> --- >>>> The persistent network namespaces are quite useful, but I really need >>>> a way to determine if a program is running in one of these namespaces. >>>> I'm not sure if this is the best way, but I couldn't come up with >>>> much else that sounded feasible to me. >>> A globally unique name that I need another to implement yet another >>> namespace to deal with is something I don't even want to think about. >> I'm not sure it's quite that bad :-). > It is. > > Imagine a system with one netork namespace per user, running containers > using vrfs. It isn't hard to imagine plausible solutions that get you > nested 3 deep. I read through all the patches you had for this, and I realized what you meant. Yeah, it's that bad. Thanks again. -corey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PM / IPMI: Remove empty legacy PCI PM callbacks
Looks fine to me.
Acked-by: Corey Minyard
On 07/06/2012 03:02 PM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki
The legacy PM callbacks provided by the IPMI PCI driver are
empty routines returning 0, so they can be safely dropped.
Signed-off-by: Rafael J. Wysocki
---
drivers/char/ipmi/ipmi_si_intf.c | 16
1 file changed, 16 deletions(-)
Index: linux/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux/drivers/char/ipmi/ipmi_si_intf.c
@@ -2503,18 +2503,6 @@ static void __devexit ipmi_pci_remove(st
cleanup_one_si(info);
}
-#ifdef CONFIG_PM
-static int ipmi_pci_suspend(struct pci_dev *pdev, pm_message_t state)
-{
- return 0;
-}
-
-static int ipmi_pci_resume(struct pci_dev *pdev)
-{
- return 0;
-}
-#endif
-
static struct pci_device_id ipmi_pci_devices[] = {
{ PCI_DEVICE(PCI_HP_VENDOR_ID, PCI_MMC_DEVICE_ID) },
{ PCI_DEVICE_CLASS(PCI_ERMC_CLASSCODE, PCI_ERMC_CLASSCODE_MASK) },
@@ -2527,10 +2515,6 @@ static struct pci_driver ipmi_pci_driver
.id_table = ipmi_pci_devices,
.probe =ipmi_pci_probe,
.remove = __devexit_p(ipmi_pci_remove),
-#ifdef CONFIG_PM
- .suspend = ipmi_pci_suspend,
- .resume = ipmi_pci_resume,
-#endif
};
#endif /* CONFIG_PCI */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Better precision in rusage and virtual itimers?
I've looked through the scheduling code and searched pretty much everywhere I could think of and I've found nothing on this, so it's time to escalate :-). The measurement of CPU usage and virtual itimers is crude at best. It's actually possible to set a relatively short virtual itimer (say, 50ms) and have it use almost no CPU before going off. I have actually had this happen. For larger values and for gross rusage measurements, this current scheme is probably ok, but for more fine-grained stuff it is not ok. Most, if not all, modern CPUs have a high-precision clock that could easily track CPU usage down to the microsecond level. My questions are: has anyone done any work on this? If not, and, if I was willing to do the work, would it be likely to be accepted? What I would like to do is the following: * Add a way to get to a high-precision clock that architectures can provide. * Modify fields in task_struct to optionally support higher precision. * Add some optional code before and after switch_to() to update the timer values and rusage values. * Add a way for architectures to report when they enter and leave system code (for tracking system verses user usage). All of this, of course, would be optional and the old way would still be available. Corey - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Problem autonegotiation with tulip driver?
In the current version (and may previous versions) of the tulip driver
in media.c in the function tulip_select_media() with mleaf->type being
2 or 4, we have the following code:
if (p[1] & 0x40) { /* SIA (CSR13-15) setup values are provided. */
csr13val = setup[0];
csr14val = setup[1];
csr15dir = (setup[3]<<16) | setup[2];
csr15val = (setup[4]<<16) | setup[2];
outl(0, ioaddr + CSR13);
outl(csr14val, ioaddr + CSR14);
outl(csr15dir, ioaddr + CSR15); /* Direction */
outl(csr15val, ioaddr + CSR15); /* Data */
outl(csr13val, ioaddr + CSR13);
} else {
csr13val = 1;
csr14val = 0x0003FF7F;
^^
csr15dir = (setup[0]<<16) | 0x0008;
csr15val = (setup[1]<<16) | 0x0008;
In the value underscored above, autonegotiation of the media is turned
off if the eeprom doesn't provide the CSR14 value. This doesn't seem
right (and doesn't work on our Ramix cards), it seems like you would
want to leave autonegotiation on here (the value would be 0x0003).
Without this, our Ramix cards will not autonegotiate. With the
change, they work great.
Corey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Problem autonegotiation with tulip driver?
In the current version (and may previous versions) of the tulip driver
in media.c in the function tulip_select_media() with mleaf->type being
2 or 4, we have the following code:
if (p[1] & 0x40) { /* SIA (CSR13-15) setup values are provided. */
csr13val = setup[0];
csr14val = setup[1];
csr15dir = (setup[3]<<16) | setup[2];
csr15val = (setup[4]<<16) | setup[2];
outl(0, ioaddr + CSR13);
outl(csr14val, ioaddr + CSR14);
outl(csr15dir, ioaddr + CSR15); /* Direction */
outl(csr15val, ioaddr + CSR15); /* Data */
outl(csr13val, ioaddr + CSR13);
} else {
csr13val = 1;
csr14val = 0x0003FF7F;
^^
csr15dir = (setup[0]<<16) | 0x0008;
csr15val = (setup[1]<<16) | 0x0008;
In the value underscored above, autonegotiation of the media is turned
off if the eeprom doesn't provide the CSR14 value. This doesn't seem
right (and doesn't work on our Ramix cards), it seems like you would
want to leave autonegotiation on here (the value would be 0x0003).
Without this, our Ramix cards will not autonegotiate. With the
change, they work great.
Corey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Make CPU usage and virtual itimers accurate
I have patches written to improve the accuracy of CPU measurement (getrusage()) and virtual itimers. This lets the amount of CPU used by a process be measured fairly accurately. The patches are on my web page: http://members.home.com/minyard This is if anyone is interested. I don't know if anyone has any interest in putting these into the main kernel. The patches are only turned on if the configuration option is set on, so it shouldn't affect anything otherwise. Patches are against 2.4.0-test10 and 2.2.16. I have not tested the 2.2.16 patch yet. Some text from the web page: The current method of measure CPU usage of a process in Linux is crude at best. For most applications, that is fine, but for some it is critical to know how much CPU you have used. Also, using setitimer's viritual timers have a rather crude measurement mechanism, if the timer tick hits when the task is running then an entire tick is added to the processes time. It's actually possible to use no CPU and still time out. CPU measurement is done the same way. The following patch fixes all that. They add a kernel configuration parameter that allows high-resolution (microsecond) timing of task CPU usage. There are a few caveats: * It only works on PowerPC right now. If someone wants to do an Intel port (or another platform port), feel free to send me the patches. * It costs extra time in interrupts, exceptions, and system calls. There is no real way around this, to accurately count system time verses user time you have to know when the kernel is running verses the user process. (If you don't enable the configuration option, there is no affect from the patch) * There is some wierd interaction with gettimeofday. If I have a loop calling gettimeofday, the system doesn't seem to keep time properly. I haven't figured this out yet. It may not be this patch, either. As long as you don't use gettimeofday a few thousand times a second, though, you should be ok. * The patches are new and, of course, experimental. They may crash your system and destroy all your data. * I have not tested the 2.2.16 patch at all, I have just gotten it to compile. Corey - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Remove redundancy from i2c-core.c
Call i2c_transfer() from i2c_master_send() and i2c_master_recv()
to avoid the redundant code that was in all three functions. It
also removes unnecessary debug statements as suggested by Jean
Delvare.
This is important for the non-blocking interfaces because they
will have to handle a non-blocking interface in this area. Having
it in one place greatly simplifies the changes.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.12-rc1/drivers/i2c/i2c-core.c
===
--- linux-2.6.12-rc1.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.12-rc1/drivers/i2c/i2c-core.c
@@ -612,27 +612,17 @@
struct i2c_adapter *adap=client->adapter;
struct i2c_msg msg;
- if (client->adapter->algo->master_xfer) {
- msg.addr = client->addr;
- msg.flags = client->flags & I2C_M_TEN;
- msg.len = count;
- msg.buf = (char *)buf;
+ msg.addr = client->addr;
+ msg.flags = client->flags & I2C_M_TEN;
+ msg.len = count;
+ msg.buf = (char *)buf;
- dev_dbg(&client->adapter->dev, "master_send: writing %d bytes.\n",
- count);
-
- down(&adap->bus_lock);
- ret = adap->algo->master_xfer(adap,&msg,1);
- up(&adap->bus_lock);
-
- /* if everything went ok (i.e. 1 msg transmitted), return #bytes
- * transmitted, else error code.
- */
- return (ret == 1 )? count : ret;
- } else {
- dev_err(&client->adapter->dev, "I2C level transfers not supported\n");
- return -ENOSYS;
- }
+ ret = i2c_transfer(adap, &msg, 1);
+
+ /* if everything went ok (i.e. 1 msg transmitted), return #bytes
+ * transmitted, else error code.
+ */
+ return (ret == 1 )? count : ret;
}
int i2c_master_recv(struct i2c_client *client, char *buf ,int count)
@@ -640,31 +630,19 @@
struct i2c_adapter *adap=client->adapter;
struct i2c_msg msg;
int ret;
- if (client->adapter->algo->master_xfer) {
- msg.addr = client->addr;
- msg.flags = client->flags & I2C_M_TEN;
- msg.flags |= I2C_M_RD;
- msg.len = count;
- msg.buf = buf;
- dev_dbg(&client->adapter->dev, "master_recv: reading %d bytes.\n",
- count);
-
- down(&adap->bus_lock);
- ret = adap->algo->master_xfer(adap,&msg,1);
- up(&adap->bus_lock);
-
- dev_dbg(&client->adapter->dev, "master_recv: return:%d (count:%d, addr:0x%02x)\n",
- ret, count, client->addr);
-
- /* if everything went ok (i.e. 1 msg transmitted), return #bytes
- * transmitted, else error code.
- */
- return (ret == 1 )? count : ret;
- } else {
- dev_err(&client->adapter->dev, "I2C level transfers not supported\n");
- return -ENOSYS;
- }
+ msg.addr = client->addr;
+ msg.flags = client->flags & I2C_M_TEN;
+ msg.flags |= I2C_M_RD;
+ msg.len = count;
+ msg.buf = buf;
+
+ ret = i2c_transfer(adap, &msg, 1);
+
+ /* if everything went ok (i.e. 1 msg transmitted), return #bytes
+ * transmitted, else error code.
+ */
+ return (ret == 1 )? count : ret;
}
Re: [PATCH 2.6.11.6] Add power cycle to ipmi_poweroff module
[EMAIL PROTECTED] wrote:
Below is a patch to add "power cycle" functionality to the IPMI power
off module ipmi_poweroff.
A new module param is added to support this:
parmtype: do_power_cycle:int
parm: do_power_cycle: Set to 1 to enable power cycle instead
of power down. Power cycle is contingent on hardware support, otherwise
it defaults back to power down.
This parameter can also be dynamically modified through the proc
filesystem:
/proc/ipmi//poweroff
This should probably be /proc/sys/dev/ipmi/power_cycle_on_halt. Most
things to control a system go there. The /proc/sys/dev/ipmi directory
should probably be created by the base IPMI file, too.
Thinking about it a little more, this should really be an option for
reset, not for power off (thus making the name power_cycle_on_reset).
I'm not sure how easy that will be to tie into. It doesn't look easy;
there's not something like pm_power_off for reset.
All the proc fs stuff should be ifdef-ed appropriately so it will
compile with procfs turned off.
-Corey
The power cycle action is considered an optional chassis control in the
IPMI specification. However, it is definitely useful when the hardware
supports it. A power cycle is usually required in order to reset a
firmware in a bad state. This action is critical to allow remote
management of servers.
The implementation adds power cycle as optional to the ipmi_poweroff
module. It can be modified dynamically through the proc entry mentioned
above. During a power down and enabled, the power cycle command is sent
to the BMC firmware. If it fails either due to non-support or some
error, it will retry to send the command as power off.
Signed-off-by: Christopher A. Poblete <[EMAIL PROTECTED]>
--
Chris Poblete
Software Engineer
Dell OpenManage Instrumentation
= drivers/char/ipmi/ipmi_poweroff.c linux-2.6.11.6 vi edited =
--- linux-2.6.11.6/drivers/char/ipmi/ipmi_poweroff.c.orig
2005-03-25 21:28:22.0 -0600
+++ linux-2.6.11.6/drivers/char/ipmi/ipmi_poweroff.c2005-04-07
18:36:10.656537656 -0500
@@ -34,6 +34,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -41,6 +43,13 @@
#define PFX "IPMI poweroff: "
#define IPMI_POWEROFF_VERSION "v33"
+/* container to flag power cycle instead of power down */
+static int do_power_cycle = 0;
+
+/* parameter definition to allow user to flag power cycle */
+module_param(do_power_cycle, int, 0);
+MODULE_PARM_DESC(do_power_cycle, " Set to 1 to enable power cycle
instead of power down. Power cycle is contingent on hardware support,
otherwise it defaults back to power down.");
+
/* Where to we insert our poweroff function? */
extern void (*pm_power_off)(void);
@@ -349,23 +358,37 @@ static void ipmi_poweroff_chassis (ipmi_
smi_addr.channel = IPMI_BMC_CHANNEL;
smi_addr.lun = 0;
- printk(KERN_INFO PFX "Powering down via IPMI chassis control
command\n");
+ powercyclefailed:
+ printk(KERN_INFO PFX "Powering %s via IPMI chassis control
command\n",
+ ((do_power_cycle != 1) ? "down" : "cycle"));
/*
* Power down
*/
send_msg.netfn = IPMI_NETFN_CHASSIS_REQUEST;
send_msg.cmd = IPMI_CHASSIS_CONTROL_CMD;
- data[0] = 0; /* Power down */
+ if (do_power_cycle != 1) {
+ data[0] = 0; /* Power down */
+ } else {
+ data[0] = 2; /* Power cycle */
+ }
send_msg.data = data;
send_msg.data_len = sizeof(data);
rv = ipmi_request_in_rc_mode(user,
(struct ipmi_addr *) &smi_addr,
&send_msg);
if (rv) {
- printk(KERN_ERR PFX "Unable to send chassis powerdown
message,"
- " IPMI error 0x%x\n", rv);
- goto out;
+ if (do_power_cycle != 1) {
+ printk(KERN_ERR PFX "Unable to send chassis
power " \
+ "down message, IPMI error 0x%x\n", rv);
+ goto out;
+} else {
+ /* power cycle failed, default to power down */
+ printk(KERN_ERR PFX "Unable to send chassis
power " \
+ "cycle message, IPMI error 0x%x\n", rv);
+ do_power_cycle = 0;
+ goto powercyclefailed;
+}
}
out:
@@ -418,6 +441,110 @@ static void ipmi_poweroff_function (void
ipmi_user_set_run_to_completion(ipmi_user, 0);
}
+/* procfs global memory */
+static struct proc_dir_entry *proc_ipo_root = NULL;
+static struct proc_dir_entry *proc_ipo_dir = NULL;
+static char ipo_dirname[4];
+
+/* displays properties to proc */
+static int proc_read_do_power_cycle(char *page, char **start,
+ off_t off, int count,
+ int *eof, void *data)
+{
+ /* sanity check */
+ if (data != NULL) {
+ return spri
Re: [PATCH 2.6.11.6] Add power cycle to ipmi_poweroff module
[EMAIL PROTECTED] wrote:
The message handler and si are already using /proc/ipmi/. The
patch code simply reuse it. By the way, shouldn't we be using sysfs?
Yes, we should. There's not much sysfs support in the ipmi driver yet;
a patch to add class stuff is in the mm kernels, but nothing for the
device stuff.
As for separating from power_off, it just seem so simple to integrate
the power cycle command into the power_off code. It could definitely be
a separate module.
It's probably ok to leave it like it is for now. I really wasn't
talking about separating, I was talking about what user operation causes
the power cycle. It would be nice if you could tie this to reset
instead of poweroff, but that's not a huge deal. It would take a while
to get that infrastructure into the kernel.
-Corey
Thanks,
-Chris Poblete
-Original Message-
From: Corey Minyard [mailto:[EMAIL PROTECTED]
Sent: Friday, April 08, 2005 10:35 AM
To: Poblete, Chris
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]
Subject: Re: [PATCH 2.6.11.6] Add power cycle to ipmi_poweroff module
[EMAIL PROTECTED] wrote:
Below is a patch to add "power cycle" functionality to the IPMI power
off module ipmi_poweroff.
A new module param is added to support this:
parmtype: do_power_cycle:int
parm: do_power_cycle: Set to 1 to enable power cycle instead
of power down. Power cycle is contingent on hardware support, otherwise
it defaults back to power down.
This parameter can also be dynamically modified through the proc
filesystem:
/proc/ipmi//poweroff
This should probably be /proc/sys/dev/ipmi/power_cycle_on_halt. Most
things to control a system go there. The /proc/sys/dev/ipmi directory
should probably be created by the base IPMI file, too.
Thinking about it a little more, this should really be an option for
reset, not for power off (thus making the name power_cycle_on_reset).
I'm not sure how easy that will be to tie into. It doesn't look easy;
there's not something like pm_power_off for reset.
All the proc fs stuff should be ifdef-ed appropriately so it will
compile with procfs turned off.
-Corey
The power cycle action is considered an optional chassis control in the
IPMI specification. However, it is definitely useful when the hardware
supports it. A power cycle is usually required in order to reset a
firmware in a bad state. This action is critical to allow remote
management of servers.
The implementation adds power cycle as optional to the ipmi_poweroff
module. It can be modified dynamically through the proc entry mentioned
above. During a power down and enabled, the power cycle command is sent
to the BMC firmware. If it fails either due to non-support or some
error, it will retry to send the command as power off.
Signed-off-by: Christopher A. Poblete <[EMAIL PROTECTED]>
--
Chris Poblete
Software Engineer
Dell OpenManage Instrumentation
= drivers/char/ipmi/ipmi_poweroff.c linux-2.6.11.6 vi edited =
--- linux-2.6.11.6/drivers/char/ipmi/ipmi_poweroff.c.orig
2005-03-25 21:28:22.0 -0600
+++ linux-2.6.11.6/drivers/char/ipmi/ipmi_poweroff.c2005-04-07
18:36:10.656537656 -0500
@@ -34,6 +34,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -41,6 +43,13 @@
#define PFX "IPMI poweroff: "
#define IPMI_POWEROFF_VERSION "v33"
+/* container to flag power cycle instead of power down */ static int
+do_power_cycle = 0;
+
+/* parameter definition to allow user to flag power cycle */
+module_param(do_power_cycle, int, 0); MODULE_PARM_DESC(do_power_cycle,
+" Set to 1 to enable power cycle
instead of power down. Power cycle is contingent on hardware support,
otherwise it defaults back to power down.");
+
/* Where to we insert our poweroff function? */ extern void
(*pm_power_off)(void);
@@ -349,23 +358,37 @@ static void ipmi_poweroff_chassis (ipmi_
smi_addr.channel = IPMI_BMC_CHANNEL;
smi_addr.lun = 0;
- printk(KERN_INFO PFX "Powering down via IPMI chassis control
command\n");
+ powercyclefailed:
+ printk(KERN_INFO PFX "Powering %s via IPMI chassis control
command\n",
+ ((do_power_cycle != 1) ? "down" : "cycle"));
/*
* Power down
*/
send_msg.netfn = IPMI_NETFN_CHASSIS_REQUEST;
send_msg.cmd = IPMI_CHASSIS_CONTROL_CMD;
- data[0] = 0; /* Power down */
+ if (do_power_cycle != 1) {
+ data[0] = 0; /* Power down */
+ } else {
+ data[0] = 2; /* Power cycle */
+ }
send_msg.data = data;
send_msg.data_len = sizeof(data);
rv = ipmi_request_in_rc_mode(user,
(struct ipmi_addr *) &smi_addr,
&send_msg);
if (rv) {
- prin
Re: dmi_table counting in ipmi_si_intf.c
I have no idea why that value was a "1". However, looking at this, the "0" does seem correct; this is a valid patch. -Corey Vernon Mauery wrote: I am working on getting one of the IBM blades to use ipmi and have run into a problem. The driver doesn't load because it says it can't find the device. dmidecode shows that there are 39 entries and that the last one is the BMC. I looked into dmi_table and noticed that it parses the table by length and by number of entries. But I found that it goes from i=1 to i --- diff -uar a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c --- a/drivers/char/ipmi/ipmi_si_intf.c 2005-08-09 08:11:41.0 -0700 +++ b/drivers/char/ipmi/ipmi_si_intf.c 2005-08-09 08:12:51.0 -0700 @@ -1690,7 +1690,7 @@ static int dmi_table(u32 base, int len, u8 __iomem *buf; struct dmi_header __iomem *dm; u8__iomem *data; - int i=1; + int i=0; int status=-1; int intf_num = 0; - 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/
Re: [PATCH 4/5] 2.6.13-rc5-mm1, IPMI, use dmi_find_device()
Andrew, this patch (along with the patch 3/5) works fine for me and is
an obvious improvement to the IPMI driver. You will need to remove the
patch named dmi_table-counting-in-ipmi_si_intfc.patch first.
Thanks, Andrey.
-Corey
On Wed, 2005-08-10 at 14:32 +0400, Andrey Panin wrote:
> This patch replaces homebrew DMI scanning code in IPMI System Interface driver
> with dmi_find_device() call.
>
> Signed-off-by: Andrey Panin <[EMAIL PROTECTED]>
>
> drivers/char/ipmi/ipmi_si_intf.c | 105
> ++-
> 1 files changed, 17 insertions(+), 88 deletions(-)
>
> diff -urdpNX /usr/share/dontdiff
> linux-2.6.13-rc5-mm1.vanilla/drivers/char/ipmi/ipmi_si_intf.c
> linux-2.6.13-rc5-mm1/drivers/char/ipmi/ipmi_si_intf.c
> --- linux-2.6.13-rc5-mm1.vanilla/drivers/char/ipmi/ipmi_si_intf.c
> 2005-08-08 14:32:07.0 +0400
> +++ linux-2.6.13-rc5-mm1/drivers/char/ipmi/ipmi_si_intf.c 2005-08-08
> 11:39:00.0 +0400
> @@ -75,6 +75,7 @@ static inline void add_usec_to_timer(str
> #include
> #include "ipmi_si_sm.h"
> #include
> +#include
>
> /* Measure times between events in the driver. */
> #undef DEBUG_TIMING
> @@ -1642,22 +1643,15 @@ typedef struct dmi_ipmi_data
> static dmi_ipmi_data_t dmi_data[SI_MAX_DRIVERS];
> static int dmi_data_entries;
>
> -typedef struct dmi_header
> -{
> - u8 type;
> - u8 length;
> - u16 handle;
> -} dmi_header_t;
> -
> -static int decode_dmi(dmi_header_t __iomem *dm, int intf_num)
> +static int __init decode_dmi(struct dmi_header *dm, int intf_num)
> {
> - u8 __iomem *data = (u8 __iomem *)dm;
> + u8 *data = (u8 *)dm;
> unsigned long base_addr;
> u8 reg_spacing;
> - u8 len = readb(&dm->length);
> + u8 len = dm->length;
> dmi_ipmi_data_t *ipmi_data = dmi_data+intf_num;
>
> - ipmi_data->type = readb(&data[4]);
> + ipmi_data->type = data[4];
>
> memcpy(&base_addr, data+8, sizeof(unsigned long));
> if (len >= 0x11) {
> @@ -1672,12 +1666,12 @@ static int decode_dmi(dmi_header_t __iom
> }
> /* If bit 4 of byte 0x10 is set, then the lsb for the address
> is odd. */
> - ipmi_data->base_addr = base_addr | ((readb(&data[0x10]) & 0x10)
> >> 4);
> + ipmi_data->base_addr = base_addr | ((data[0x10] & 0x10) >> 4);
>
> - ipmi_data->irq = readb(&data[0x11]);
> + ipmi_data->irq = data[0x11];
>
> /* The top two bits of byte 0x10 hold the register spacing. */
> - reg_spacing = (readb(&data[0x10]) & 0xC0) >> 6;
> + reg_spacing = (data[0x10] & 0xC0) >> 6;
> switch(reg_spacing){
> case 0x00: /* Byte boundaries */
> ipmi_data->offset = 1;
> @@ -1705,7 +1699,7 @@ static int decode_dmi(dmi_header_t __iom
> ipmi_data->offset = 1;
> }
>
> - ipmi_data->slave_addr = readb(&data[6]);
> + ipmi_data->slave_addr = data[6];
>
> if (is_new_interface(-1, ipmi_data->addr_space,ipmi_data->base_addr)) {
> dmi_data_entries++;
> @@ -1717,82 +1711,17 @@ static int decode_dmi(dmi_header_t __iom
> return -1;
> }
>
> -static int dmi_table(u32 base, int len, int num)
> -{
> - u8__iomem *buf;
> - struct dmi_header __iomem *dm;
> - u8__iomem *data;
> - int i=1;
> - int status=-1;
> - int intf_num = 0;
> -
> - buf = ioremap(base, len);
> - if(buf==NULL)
> - return -1;
> -
> - data = buf;
> -
> - while(i - {
> - dm=(dmi_header_t __iomem *)data;
> -
> - if((data-buf+readb(&dm->length)) >= len)
> - break;
> -
> - if (readb(&dm->type) == 38) {
> - if (decode_dmi(dm, intf_num) == 0) {
> - intf_num++;
> - if (intf_num >= SI_MAX_DRIVERS)
> - break;
> - }
> - }
> -
> - data+=readb(&dm->length);
> - while((data-buf) < len && (readb(data)||readb(data+1)))
> - data++;
> - data+=2;
> - i++;
> - }
> - iounmap(buf);
> -
> - return status;
> -}
> -
> -static inline int dmi_checksum(u8 *buf)
> -{
> - u8 sum=0;
> - int a;
> -
> - for(a=0; a<15; a++)
> - sum+=buf[a];
> - return (sum==0);
> -}
> -
> -static int dmi_decode(void)
> +static void __init dmi_find_bmc(void)
> {
> - u8 buf[15];
> - u32 fp=0xF;
> -
> -#ifdef CONFIG_SIMNOW
> - return -1;
> -#endif
> + struct dmi_device *dev = NULL;
> + int intf_num = 0;
>
> - while(fp < 0xF)
> - {
> - isa_memcpy_fromio(buf, fp, 15);
> - if(memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(bu
[PATCH] Fix panic in the IPMI driver
This panic would only occur in saving panic information to the
system log in another panic, and only when sending the panic
information to a remote management controller, so it's not a huge
deal, but needs to be fixed. Hopefully pasting the message inline
will work ok (testing showed it to be fine, though mozilla did wierd
things with inline text), as I can't get thunderbird to attach a patch
with a valid mimetype (and inline makes everyone happier, I
guess).
The "null message handler" in the IPMI driver is used in startup
and panic situations to handle messages. It was only designed to
work with messages from the local management controller, but in
some cases it was used to get messages from remote management
controllers, and the system would then panic. This patch makes
the "null message handler" in the IPMI driver more general so
it works with any kind of message.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
drivers/char/ipmi/ipmi_msghandler.c | 107
++--
include/linux/ipmi.h|3 -
2 files changed, 69 insertions(+), 41 deletions(-)
Index: linux-2.6.13-rc5/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.13-rc5.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.13-rc5/drivers/char/ipmi/ipmi_msghandler.c
@@ -219,7 +219,7 @@
interface comes in with a NULL user, call this routine with
it. Note that the message will still be freed by the
caller. This only works on the system interface. */
-void (*null_user_handler)(ipmi_smi_t intf, struct ipmi_smi_msg *msg);
+void (*null_user_handler)(ipmi_smi_t intf, struct ipmi_recv_msg *msg);
/* When we are scanning the channels for an SMI, this will
tell which channel we are scanning. */
@@ -459,7 +459,27 @@
static void deliver_response(struct ipmi_recv_msg *msg)
{
-msg->user->handler->ipmi_recv_hndl(msg, msg->user->handler_data);
+if (! msg->user) {
+ipmi_smi_tintf = msg->user_msg_data;
+unsigned long flags;
+
+/* Special handling for NULL users. */
+if (intf->null_user_handler) {
+intf->null_user_handler(intf, msg);
+spin_lock_irqsave(&intf->counter_lock, flags);
+intf->handled_local_responses++;
+spin_unlock_irqrestore(&intf->counter_lock, flags);
+} else {
+/* No handler, so give up. */
+spin_lock_irqsave(&intf->counter_lock, flags);
+intf->unhandled_local_responses++;
+spin_unlock_irqrestore(&intf->counter_lock, flags);
+}
+ipmi_free_recv_msg(msg);
+} else {
+msg->user->handler->ipmi_recv_hndl(msg,
+ msg->user->handler_data);
+}
}
/* Find the next sequence number not being used and add the given
@@ -1389,6 +1409,8 @@
unsigned char saddr, lun;
int rv;
+if (! user)
+return -EINVAL;
rv = check_addr(user->intf, addr, &saddr, &lun);
if (rv)
return rv;
@@ -1418,6 +1440,8 @@
unsigned char saddr, lun;
int rv;
+if (! user)
+return -EINVAL;
rv = check_addr(user->intf, addr, &saddr, &lun);
if (rv)
return rv;
@@ -1638,7 +1662,7 @@
(struct ipmi_addr *) &si,
0,
&msg,
- NULL,
+ intf,
NULL,
NULL,
0,
@@ -1648,19 +1672,20 @@
}
static void
-channel_handler(ipmi_smi_t intf, struct ipmi_smi_msg *msg)
+channel_handler(ipmi_smi_t intf, struct ipmi_recv_msg *msg)
{
int rv = 0;
int chan;
-if ((msg->rsp[0] == (IPMI_NETFN_APP_RESPONSE << 2))
-&& (msg->rsp[1] == IPMI_GET_CHANNEL_INFO_CMD))
+if ((msg->addr.addr_type == IPMI_SYSTEM_INTERFACE_ADDR_TYPE)
+&& (msg->msg.netfn == IPMI_NETFN_APP_RESPONSE)
+&& (msg->msg.cmd == IPMI_GET_CHANNEL_INFO_CMD))
{
/* It's the one we want */
-if (msg->rsp[2] != 0) {
+if (msg->msg.data[0] != 0) {
/* Got an error from the channel, just go on. */
-if (msg->rsp[2] == IPMI_INVALID_COMMAND_ERR) {
+if (msg->msg.data[0] == IPMI_INVALID_COMMAND_ERR) {
/* If the MC does not support this
command, that is legal. We just
assume it has one IPMB at channel
@@ -1677,13 +1702,13 @@
}
goto next_channel;
}
-if (msg->rsp_size < 6) {
+if (msg->msg.data_len < 4) {
/* Message not big enough, just go on. */
goto next_channel;
}
chan = intf->curr_channel;
-intf->channels[chan].medium = msg-&g
Re: [PATCH 2.6.12.4] ACPI oops during ipmi_si driver init
Peter Martuccelli wrote: On Mon, 2005-08-15 at 18:13, Bjorn Helgaas wrote: On Friday 12 August 2005 1:44 pm, Peter Martuccelli wrote: Stumbled into this problem working on the ipmi_si driver. When the ipmi_si driver initialization fails the acpi_tb_get_table call, after rsdt_info has been allocated, acpi_get_firmware_table() will oops trying to reference off rsdt_info->pointer in the cleanup code. I don't know whether the ACPI patch is correct or desirable, but I think the ipmi_si ACPI discovery is bogus (it was probably written before the current ACPI and PNPACPI driver registration interfaces were stable). Currently, ipmi_si uses the static SPMI table to locate the device. But the static table should only be used if we need the device very early, before the ACPI namespace is available. I don't think we use the device early, so we should use pnp_register_driver() to claim the appropriate PNP IDs. Or we might have to use acpi_bus_register_driver() since it looks like it uses ACPI-specific features like GPEs. Adding in Corey to the discussion regarding ipmi_si initialization, waiting on Len to decide on the ACPI fix. I couldn't find any documentation on how the ACPI interfaces work, so I'm kind of in the dark. Basically, the IPMI system interface needs information from a specific IPMI table to know how to configure itself. Those tables can reference GPEs, so the driver can use those (though AFAIK it has never been tested). From spending 30 minutes searching and looking at things, I have no idea how to tie this in. Can you point me to some docs? Or do I have to spend hours digging? -Corey - 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/
[PATCH] IPMI driver update part 1, add per-channel IPMB addresses
ipmi-per-channel-slave-address.patch Description: unknown/unknown
[PATCH] IPMI driver update part 3, watchdog/NMI interaction fixes
ipmi_wdog_nmi_fixes.patch Description: unknown/unknown
[PATCH] IPMI driver update part 4, allow userland to include ipmi.h
ipmi_compiler_h_include.patch Description: unknown/unknown
[PATCH] IPMI driver update part 2, high-res timer support fixes
ipmi_hrt_fixes.diff Description: unknown/unknown
[PATCH] IPMI driver update part 6, clean up versioning of the IPMI driver
ipmi-add-module-info-tags.patch Description: unknown/unknown
[PATCH] IPMI driver update part 5, OEM flag handling and hacks for some Dell machines
ipmi-add-per-OEM-data-available-handlers.patch Description: unknown/unknown
Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses
Andrew Morton wrote:
Corey Minyard <[EMAIL PROTECTED]> wrote:
ipmi-per-channel-slave-address.patch unknown/unknown (13533 bytes)]
Could you fix up the mimetype, please? It makes it hard for various email
clients.
Dang, you switch to a new mail client and everything is screwed up. Sorry.
IPMI allows multiple IPMB channels on a single interface, and
each channel might have a different IPMB address. However, the
driver has only one IPMB address that it uses for everything.
This patch adds new IOCTLS and a new internal interface for
setting per-channel IPMB addresses and LUNs. New systems are
coming out with support for multiple IPMB channels, and they
are broken without this patch.
...
+ for (i=0; i
Preferred coding style is actually
for (i = 0; i < IPMI_MAX_CHANNELS; i++)
but we've kinda lost that fight in drivers :(
Ok, I'll see what I can do. It's the wrong way all over the driver
right now.
+#define IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 24, struct
ipmi_channel_lun_address_set)
+#define IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 25, struct
ipmi_channel_lun_address_set)
+#define IPMICTL_SET_MY_CHANNEL_LUN_CMD_IOR(IPMI_IOC_MAGIC, 26, struct
ipmi_channel_lun_address_set)
+#define IPMICTL_GET_MY_CHANNEL_LUN_CMD_IOR(IPMI_IOC_MAGIC, 27, struct
ipmi_channel_lun_address_set)
Are these all OK wrt compat handling?
Yes, it is a structure of an unsigned short and an unsigned char, so it
should be ok.
case IPMICTL_SET_MY_ADDRESS_CMD:
{
unsigned int val;
...
case IPMICTL_GET_MY_ADDRESS_CMD:
{
- unsigned int val;
+ unsigned int val;
+ unsigned char rval;
...
case IPMICTL_GET_MY_LUN_CMD:
{
- unsigned int val;
+ unsigned int val;
+ unsigned char rval;
+
...
+ case IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD:
+ {
+ struct ipmi_channel_lun_address_set val;
...
+ case IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD:
+ {
+ struct ipmi_channel_lun_address_set val;
...
+ case IPMICTL_SET_MY_CHANNEL_LUN_CMD:
+ {
+ struct ipmi_channel_lun_address_set val;
...
+ case IPMICTL_GET_MY_CHANNEL_LUN_CMD:
+ {
+ struct ipmi_channel_lun_address_set val;
...
case IPMICTL_SET_TIMING_PARMS_CMD:
{
struct ipmi_timing_parms parms;
Be aware that this function will use more stack space than it needs to: gcc
will create a separate stack slot for all the above locals.
Hence it would be better to declare them all at the start of the function.
Faster, too - less dcache footprint.
Maybe not as nice from a purist point of view, but it does allow you to
lose those braces in the switch statement...
Hmm, I assumed that gcc would optimize and allocate the stack as it
needed it without waste. Ok, easy enough to fix.
Thanks,
-Corey
-
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/
Re: [PATCH] IPMI: driver model and sysfs support
This is very good. I believe the structure is correct, but I'm not a
sysfs expert.
There are a few things we need to deal with, though.
* There are some significant changes to versioning in the
driver that are in the mm tree right now (you can pull them from
http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.13-rc6/2.6.13-rc6-mm2/broken-out)
* There are some coding style problems. You have places like:
+static void ipmi_bmc_unregister(struct ipmi_si_device *si,
+ struct ipmi_bmc_device *bmc){
The '{' for functions and structures needs to be on it's own line.
Also, several:
+ if(bmc->guid_present){
that need spaces after the 'if' and before the '{'.
The patch also adds some trailing spaces to empty lines, the
following is a telltale sign:
-
+
There was also one place where you added unneeded braces to
a single statement and another where you deleted the empty line
between two functions.
These are all standard kernel coding style rules.
* I'd prefer to store the product id, device id, and manufacturer id
decoded. This makes it easier to handle (no need to use "memcmp"
to compare) and print. The printing of the product id, for instance,
will be rather unnatural in product_id_show().
* guids are not printable strings (see guid_show()).
* The printing of the guid in ipmi_bmc_register is, well, kind of ugly.
It should probably be moved to its own function that lets you pass in
the rest of the string.
-Corey
Yani Ioannou wrote:
Hi Corey,
Here is a patch against 2.6.13 I've been working on (and mentioned to
you at OLS), that adds support for the 2.6 driver model, and sysfs to
the ipmi subsystem. It is loosely inspired by the USB driver model
system. There is a driver struct added for the ipmi_msghandler, and
one for each system interface 'driver' (for each state machine in
ipmi_si). A device struct is created for each system interface in the
system, and for each unique BMC on the system, with a symlink between
each system interface and the bmc it interfaces to. Each bmc has a set
of fundamental device attributes, and each of the drivers has a
version attribute reflecting the driver/state machine versions.
Furthermore each of these devices is registered as a class_device with
the ipmi class (moved to ipmi_msghandler) as bmcx and smix.
This in effect creates the following sysfs interface on a machine with
a single BMC and two interfaces:
/sys/
|-- block
|-- bus
| |-- platform
| | |-- devices
| | | |-- ipmi_bmc.0 -> ../../../devices/platform/ipmi_bmc.0
| | | |-- ipmi_si.0 -> ../../../devices/platform/ipmi_si.0
| | | |-- ipmi_si.1 -> ../../../devices/platform/ipmi_si.1
| | `-- drivers
| | |-- ipmi_bt_sm
| | | |-- bind
| | | |-- ipmi_si.1 -> ../../../../devices/platform/ipmi_si.1
| | | |-- unbind
| | | `-- version
| | |-- ipmi_kcs_sm
| | | |-- bind
| | | |-- ipmi_si.0 -> ../../../../devices/platform/ipmi_si.0
| | | |-- unbind
| | | `-- version
| | |-- ipmi_msghandler
| | | |-- bind
| | | |-- ipmi_bmc.0 -> ../../../../devices/platform/ipmi_bmc.0
| | | |-- unbind
| | | `-- version
|-- class
| |-- ipmi
| | |-- bmc0
| | | `-- device -> ../../../devices/platform/ipmi_bmc.0
| | |-- ipmi0
| | | `-- dev
| | |-- ipmi1
| | | `-- dev
| | |-- smi0
| | | `-- device -> ../../../devices/platform/ipmi_si.0
| | `-- smi1
| | `-- device -> ../../../devices/platform/ipmi_si.1
|-- devices
| |-- platform
| | |-- ipmi_bmc.0
| | | |-- bus -> ../../../bus/platform
| | | |-- device_id
| | | |-- driver -> ../../../bus/platform/drivers/ipmi_msghandler
| | | |-- firmware_revision
| | | |-- ipmi_version
| | | |-- power
| | | | `-- state
| | | |-- product_id
| | | `-- revision
| | |-- ipmi_si.0
| | | |-- bmc -> ../../../devices/platform/ipmi_bmc.0
| | | |-- bus -> ../../../bus/platform
| | | |-- driver -> ../../../bus/platform/drivers/ipmi_kcs_sm
| | | `-- power
| | | `-- state
| | |-- ipmi_si.1
| | | |-- bmc -> ../../../devices/platform/ipmi_bmc.0
| | | |-- bus -> ../../../bus/platform
| | | |-- driver -> ../../../bus/platform/drivers/ipmi_bt_sm
| | | `-- power
| | | `-- state
|-- kernel
|-- module
| |-- ipmi_devintf
| | |-- refcnt
| | `-- sections
| | `-- __param
| |-- ipmi_msghandler
| | |-- refcnt
| | `-- sections
| | |-- __ksymtab
| | `-- __ksymtab_strings
| |-- ipmi_si
| | |-- refcnt
| | `-- sections
| | `-- __param
`-- power
My motiviation for the patch is to have a device struct (bmc) to
register with the new hwmon class for my in-progress hwmon driver
ipmi_sensors. With this patch the driver can create device attributes
under bmc devices representing sensor reading
Re: [RFC][CFLART] ipmi procfs bogosity
Indeed, this function is badly written. In rewriting, I couldn't find a
nice function for reading integers from userspace, and the proc_dointvec
stuff didn't seem terribly suitable. So I wrote my own general
function, which I can move someplace else if someone likes. Patch is
attached. It should not affect correct usage of this file.
Thank you for pointing this out.
Andrew, can you include this? Matt, can you test?
-Corey
On Thu, 2005-09-01 at 07:43 +0100, [EMAIL PROTECTED] wrote:
> drivers/char/ipmi/ipmi_poweroff.c::proc_write_chassctrl()
> a) does sscanf on userland pointer
> b) does sscanf on array that is not guaranteed to have NUL in it
> c) interprets input in incredibly cretinous way:
> if strings doesn't start with a decimal number => as if it was "0".
> if it starts with decimal number equal to 0 (e.g. is "-splat") - as if
> it was "0".
> if it starts with decimal number equal to 2 (e.g. is "2FOAD") - as if
> it was "2".
> otherwise - -EINVAL.
> In any case that doesn't end up with -EINVAL, pretend that entire
> buffer had been written.
>
> (a) and (b) are immediate bugs; (c) is a valid reason for immediate severe
> LARTing of the pervert who had done _that_ in a user-visible API.
>
> Note that API _is_ user-visible, so we can't blindly change it - not without
> checking WTF do its users actually write to /proc/ipmi/poweroff_control.
>
> Could somebody comment on the actual uses of that FPOS? My preference would
> be to remove the damn thing completely - it's too ugly to live.
The IPMI power control function proc_write_chassctrl was badly
written, it directly used userspace pointers, it assumed that
strings were NULL terminated, and it used the evil sscanf function.
This adds a new function to read integers from userspace and
uses this function to get the integer in question.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
===
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
@@ -40,6 +40,8 @@
#include
#include
#include
+#include
+#include
#define PFX "IPMI poweroff: "
@@ -545,27 +547,179 @@ static int proc_read_chassctrl(char *pag
poweroff_control);
}
+/*
+ * Convert a character to an integer, returning -1 if the character is
+ * not a valid digit for "base".
+ */
+static int convdigit(char ch, int base)
+{
+ int v;
+ if ((ch >= '0') && (ch <= '9'))
+ v = ch - '0';
+ else if ((ch >= 'A') && (ch <= 'Z'))
+ v = ch - 'A' + 10;
+ else if ((ch >= 'a') && (ch <= 'z'))
+ v = ch - 'a' + 10;
+ else
+ return -1;
+ if (v >= base)
+ return -1;
+ return v;
+}
+
+/*
+ * Get a unsigned long from userspace, much like strtoul, but getting
+ * the data from userspace. This is a little complicated to use, but
+ * is quite flexible.
+ *
+ * @buffer - is a pointer to the start of the user buffer. Basically,
+ * this will read from buffer+*pos up to buffer+count-1.
+ * @pos - points to the start offset in buffer to start reading at.
+ * pos will be updated to the current location after the scan upon return.
+ * May be NULL, where it is assumed to be zero.
+ * @count - the total buffer length.
+ * @fbase - forces the base. If this value is zero 0, the base will be
+ * hex if the string starts with 0x, octal if it starts with 0, and
+ * decimal otherwise.
+ * @stop_at_inval - a flag that, if true, causes the function to
+ * return if it already has a valid number and has reached an invalid
+ * character. "pos" will be set to the offset of the invalid
+ * character upon return. If false, the entire buffer is expected to
+ * be a single integer value surrounded by optional space characters.
+ *
+ * Unlike strtoul, the value is returned in the "val" parameter. The
+ * return value of this function is a errno if negative or the number
+ * of characters processed if positive.
+ */
+static ssize_t user_strtoul(const char __user *buffer, loff_t *ppos,
+ size_t count, int fbase, int stop_at_inval,
+ unsigned long *val)
+{
+ unsigned long newval = 0;
+ char buf[20];
+ unsigned int rc;
+ unsigned int i;
+ enum { SC_ST, SC_ST2, SC_AFTPRE, SC_DAT, SC_END } state = SC_ST;
+ int base = fbase;
+ loff_tstart;
+ loff_tpos;
+
+ if (ppos)
+ pos = *ppos;
+ else
+ pos = 0;
+ start = pos;
+ count -= pos;
+ if (base == 0)
+ base = 10;
+ while (count > 0) {
+ rc = min(count, (size_t) sizeof(buf));
+ if (copy_from_user(buf, buffer + pos, rc))
+ return -EFAULT;
+ for (i = 0; i < rc; i++) {
+ char ch = buf[i];
[PATCH] Add hacks for IPMI chassis poweroff for certain Dell servers
This patch allows Dell servers with IPMI controllers that predate IPMI
1.5 to use the standard poweroff or powercycle commands. These
systems firmware don't set the chassis capability bit in the Get
Device ID, but they do implement the standard poweroff and powercycle
commands.
Tested on RHEL3 kernel 2.4.21-20.ELsmp on a PowerEdge 2600. The
standard ipmi_poweroff driver cannot drive these systems. With this
patch, they power off or powercycle as expected.
drivers/char/ipmi/ipmi_poweroff.c | 24
1 files changed, 24 insertions(+)
Signed-off-by: Matt Domsch <[EMAIL PROTECTED]>
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
===
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
@@ -64,6 +64,7 @@ MODULE_PARM_DESC(poweroff_control, " Set
static unsigned int mfg_id;
static unsigned int prod_id;
static unsigned char capabilities;
+static unsigned char ipmi_version;
/* We use our own messages for this operation, we don't let the system
allocate them, since we may be in a panic situation. The whole
@@ -339,6 +340,25 @@ static void ipmi_poweroff_cpi1 (ipmi_use
}
/*
+ * ipmi_dell_chassis_detect()
+ * Dell systems with IPMI < 1.5 don't set the chassis capability bit
+ * but they can handle a chassis poweroff or powercycle command.
+ */
+
+#define DELL_IANA_MFR_ID {0xA2, 0x02, 0x00}
+static int ipmi_dell_chassis_detect (ipmi_user_t user)
+{
+ const char ipmi_version_major = ipmi_version & 0xF;
+ const char ipmi_version_minor = (ipmi_version >> 4) & 0xF;
+ const char mfr[3]=DELL_IANA_MFR_ID;
+ if (!memcmp(mfr, &mfg_id, sizeof(mfr)) &&
+ ipmi_version_major <= 1 &&
+ ipmi_version_minor < 5)
+ return 1;
+ return 0;
+}
+
+/*
* Standard chassis support
*/
@@ -415,6 +435,9 @@ static struct poweroff_function poweroff
{ .platform_type = "CPI1",
.detect = ipmi_cpi1_detect,
.poweroff_func = ipmi_poweroff_cpi1 },
+ { .platform_type = "chassis",
+ .detect = ipmi_dell_chassis_detect,
+ .poweroff_func = ipmi_poweroff_chassis },
/* Chassis should generally be last, other things should override
it. */
{ .platform_type = "chassis",
@@ -500,6 +523,7 @@ static void ipmi_po_new_smi(int if_num)
prod_id = (halt_recv_msg.msg.data[10]
| (halt_recv_msg.msg.data[11] << 8));
capabilities = halt_recv_msg.msg.data[6];
+ ipmi_version = halt_recv_msg.msg.data[5];
/* Scan for a poweroff method */
Re: [RFC][CFLART] ipmi procfs bogosity
[EMAIL PROTECTED] wrote: On Thu, Sep 01, 2005 at 11:41:42AM -0500, Corey Minyard wrote: Indeed, this function is badly written. In rewriting, I couldn't find a nice function for reading integers from userspace, and the proc_dointvec stuff didn't seem terribly suitable. So I wrote my own general function, which I can move someplace else if someone likes. Patch is attached. It should not affect correct usage of this file. Eeeek... Much, _much_ simpler approach would be to have char buf[10]; if (count > 9) return -EINVAL; if (copy_from_user(buf, buffer, count)) return -EFAULT; buf[count] = '\0'; /* use sscanf() or anything normal */ Would that change behaviour in any cases you care about? Because then, for a general solution that avoids integer perversion, you need something like: char buf[10]; char *end; if (count > (sizeof(buf) - 1)) return -EINVAL; if (copy_from_user(buf, buffer, count)) return -EFAULT; buf[count] = '\0'; newval = simple_strtoul(buf, &end, 0); if (buf == end) /* Empty string or first char not a number */ return -EINVAL; if (*end && ! isspace(*end)) /* Bogus number. */ return -EINVAL; To me, It's a lot nicer to do: rv = user_strtoul(); if (rv < 0) return rv; Plus the scanning function I wrote handles arbitrary leading and trailing space, etc. Not a big deal, but a little nicer. -Corey - 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/
[PATCH] part 2 - remove unused fields from the IPMI driver
This removes the unused "all_cmd_rcvr" variable from the
IPMI driver.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.13/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_msghandler.c
@@ -202,12 +202,6 @@ struct ipmi_smi
struct list_head waiting_events;
unsigned int waiting_events_count; /* How many events in queue? */
- /* This will be non-null if someone registers to receive all
- IPMI commands (this is for interface emulation). There
- may not be any things in the cmd_rcvrs list above when
- this is registered. */
- ipmi_user_t all_cmd_rcvr;
-
/* The event receiver for my BMC, only really used at panic
shutdown as a place to store this. */
unsigned char event_receiver;
@@ -867,11 +861,6 @@ int ipmi_register_for_cmd(ipmi_user_t
read_lock(&(user->intf->users_lock));
write_lock_irqsave(&(user->intf->cmd_rcvr_lock), flags);
- if (user->intf->all_cmd_rcvr != NULL) {
- rv = -EBUSY;
- goto out_unlock;
- }
-
/* Make sure the command/netfn is not already registered. */
list_for_each_entry(cmp, &(user->intf->cmd_rcvrs), link) {
if ((cmp->netfn == netfn) && (cmp->cmd == cmd)) {
@@ -886,7 +875,7 @@ int ipmi_register_for_cmd(ipmi_user_t
rcvr->user = user;
list_add_tail(&(rcvr->link), &(user->intf->cmd_rcvrs));
}
- out_unlock:
+
write_unlock_irqrestore(&(user->intf->cmd_rcvr_lock), flags);
read_unlock(&(user->intf->users_lock));
@@ -1799,7 +1788,6 @@ int ipmi_register_smi(struct ipmi_smi_ha
rwlock_init(&(new_intf->cmd_rcvr_lock));
init_waitqueue_head(&new_intf->waitq);
INIT_LIST_HEAD(&(new_intf->cmd_rcvrs));
- new_intf->all_cmd_rcvr = NULL;
spin_lock_init(&(new_intf->counter_lock));
@@ -2037,15 +2025,11 @@ static int handle_ipmb_get_msg_cmd(ipmi_
read_lock(&(intf->cmd_rcvr_lock));
- if (intf->all_cmd_rcvr) {
- user = intf->all_cmd_rcvr;
- } else {
- /* Find the command/netfn. */
- list_for_each_entry(rcvr, &(intf->cmd_rcvrs), link) {
- if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)) {
-user = rcvr->user;
-break;
- }
+ /* Find the command/netfn. */
+ list_for_each_entry(rcvr, &(intf->cmd_rcvrs), link) {
+ if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)) {
+ user = rcvr->user;
+ break;
}
}
read_unlock(&(intf->cmd_rcvr_lock));
@@ -,15 +2206,11 @@ static int handle_lan_get_msg_cmd(ipmi_s
read_lock(&(intf->cmd_rcvr_lock));
- if (intf->all_cmd_rcvr) {
- user = intf->all_cmd_rcvr;
- } else {
- /* Find the command/netfn. */
- list_for_each_entry(rcvr, &(intf->cmd_rcvrs), link) {
- if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)) {
-user = rcvr->user;
-break;
- }
+ /* Find the command/netfn. */
+ list_for_each_entry(rcvr, &(intf->cmd_rcvrs), link) {
+ if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)) {
+ user = rcvr->user;
+ break;
}
}
read_unlock(&(intf->cmd_rcvr_lock));
[PATCH] part 3 - Convert IPMI driver over to use refcounts
The IPMI driver uses read/write locks to ensure that things
exist while they are in use. This is bad from a number of
points of view. This patch removes the rwlocks and uses
refcounts and a special synced list (the entries can be
refcounted and removal is blocked while an entry is in
use).
drivers/char/ipmi/ipmi_msghandler.c | 1153 +---
1 files changed, 692 insertions(+), 461 deletions(-)
Index: linux-2.6.13/drivers/char/ipmi/ipmi_msghandler.c
===
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_msghandler.c
@@ -38,7 +38,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -65,9 +64,210 @@ struct proc_dir_entry *proc_ipmi_root =
the max message timer. This is in milliseconds. */
#define MAX_MSG_TIMEOUT 6
-struct ipmi_user
+
+/*
+ * The following is an implementation of a list that allows traversals
+ * and additions/deletions at the same time. If a list item is in use
+ * while the deletion is occurring, then the deletion will block until
+ * the list item is no longer in use.
+ */
+#include
+#include
+struct synced_list
+{
+ struct list_head head;
+ spinlock_t lock;
+};
+
+struct synced_list_entry
+{
+ struct list_head link;
+ atomic_t usecount;
+
+ struct list_head task_list;
+};
+
+/* Return values for match functions. */
+#define SYNCED_LIST_NO_MATCH 0
+#define SYNCED_LIST_MATCH_STOP 1
+#define SYNCED_LIST_MATCH_CONTINUE -1
+
+/* Can be used for synced list find and clear operations for finding
+ and deleting a specific entry. */
+static int match_entry(struct synced_list_entry *e, void *match_data)
+{
+ if (e == match_data)
+ return SYNCED_LIST_MATCH_STOP;
+ else
+ return SYNCED_LIST_NO_MATCH;
+}
+
+struct synced_list_entry_task_q
{
struct list_head link;
+ task_t *process;
+};
+
+static inline void init_synced_list(struct synced_list *list)
+{
+ INIT_LIST_HEAD(&list->head);
+ spin_lock_init(&list->lock);
+}
+
+/* Called with the list head lock held */
+static void synced_list_wake(struct synced_list_entry *entry)
+{
+ struct synced_list_entry_task_q *e;
+
+ if (!atomic_dec_and_test(&entry->usecount))
+ /* Another thread is using the entry, too. */
+ return;
+
+ list_for_each_entry(e, &entry->task_list, link)
+ wake_up_process(e->process);
+}
+
+/* Can only be called on entries that have been "gotten". */
+#define synced_list_put_entry(pos, head) \
+ synced_list_before_exit(pos, head)
+/* Must be called with head->lock already held. */
+#define synced_list_put_entry_nolock(pos, head) \
+ synced_list_wake(pos);
+
+#define synced_list_for_each_entry(pos, l, entry, flags) \
+ for ((spin_lock_irqsave(&(l)->lock, flags), \
+ pos = container_of((l)->head.next, typeof(*(pos)),entry.link)); \
+ (prefetch((pos)->entry.link.next), \
+ &(pos)->entry.link != (&(l)->head) \
+ ? (atomic_inc(&(pos)->entry.usecount), \
+ spin_unlock_irqrestore(&(l)->lock, flags), 1) \
+ : (spin_unlock_irqrestore(&(l)->lock, flags), 0)); \
+ (spin_lock_irqsave(&(l)->lock, flags), \
+ synced_list_wake(&(pos)->entry), \
+ pos = container_of((pos)->entry.link.next, typeof(*(pos)), \
+ entry.link)))
+
+/* If you must exit a synced_list_for_each_entry loop abnormally (with
+ a break, return, goto) then you *must* call this first, with the
+ current entry. Otherwise, the entry will be left locked. */
+static void synced_list_before_exit(struct synced_list_entry *entry,
+struct synced_list *head)
+{
+ unsigned long flags;
+ spin_lock_irqsave(&head->lock, flags);
+ synced_list_wake(entry);
+ spin_unlock_irqrestore(&head->lock, flags);
+}
+
+/* Can only be called in a synced list loop. This will preserve the
+ entry at least until synced_list_put_entry() is called. */
+#define synced_list_get_entry(pos) atomic_inc(&(pos)->usecount)
+
+/* Must be called with head->lock already held. */
+static void synced_list_add_nolock(struct synced_list *head,
+ struct synced_list_entry *entry)
+{
+ atomic_set(&entry->usecount, 0);
+ INIT_LIST_HEAD(&entry->task_list);
+ list_add(&entry->link, &head->head);
+}
+
+static void synced_list_add(struct synced_list *head,
+ struct synced_list_entry *entry)
+{
+ spin_lock_irq(&head->lock);
+ synced_list_add_nolock(head, entry);
+ spin_unlock_irq(&head->lock);
+}
+
+/*
+ * See the SYNCED_LIST_MATCH... defines for the return values from the
+ * "match" function. If the free function is non-NULL, it will be
+ * called with the entry after it is removed from the list. This must
+ * be called with the head->lock already held.
+ */
+static int synced_list_clear(struct synced_list *head,
+ int (*match)(struct synced_list_entry *,
+ void *),
+ void (*free)(struct synced_list_entry *),
+ v
[PATCH] part 1 - IPMI style cleanups
Clean up various style issues in the IPMI driver. Should be
no functional changes.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
===
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
@@ -527,7 +527,7 @@ static void ipmi_po_new_smi(int if_num)
/* Scan for a poweroff method */
- for (i=0; i> 26) & 0x3f);\
seqid = (msgid & 0x3f);\
-} while(0)
+} while (0)
#define NEXT_SEQID(seqid) (((seqid) + 1) & 0x3f)
@@ -326,7 +326,7 @@ int ipmi_smi_watcher_register(struct ipm
down_read(&interfaces_sem);
down_write(&smi_watchers_sem);
list_add(&(watcher->link), &smi_watchers);
- for (i=0; inew_smi(i);
}
@@ -496,9 +496,9 @@ static int intf_next_seq(ipmi_smi_t
int rv = 0;
unsigned int i;
- for (i=intf->curr_seq;
+ for (i = intf->curr_seq;
(i+1)%IPMI_IPMB_NUM_SEQ != intf->curr_seq;
- i=(i+1)%IPMI_IPMB_NUM_SEQ)
+ i = (i+1)%IPMI_IPMB_NUM_SEQ)
{
if (! intf->seq_table[i].inuse)
break;
@@ -733,7 +733,7 @@ static int ipmi_destroy_user_nolock(ipmi
/* Remove the user from the interfaces sequence table. */
spin_lock_irqsave(&(user->intf->seq_lock), flags);
- for (i=0; iintf->seq_table[i].inuse
&& (user->intf->seq_table[i].recv_msg->user == user))
{
@@ -1370,7 +1370,7 @@ static inline int i_ipmi_request(ipmi_us
#ifdef DEBUG_MSGING
{
int m;
- for (m=0; mdata_size; m++)
+ for (m = 0; m < smi_msg->data_size; m++)
printk(" %2.2x", smi_msg->data[m]);
printk("\n");
}
@@ -1467,7 +1467,7 @@ static int ipmb_file_read_proc(char *pag
inti;
intrv= 0;
- for (i=0; ichannels[i].address);
out[rv-1] = '\n'; /* Replace the final space with a newline */
out[rv] = '\0';
@@ -1766,12 +1766,12 @@ int ipmi_register_smi(struct ipmi_smi_ha
rv = -ENOMEM;
down_write(&interfaces_sem);
- for (i=0; iintf_num = i;
new_intf->version_major = version_major;
new_intf->version_minor = version_minor;
- for (j=0; jchannels[j].address
= IPMI_BMC_SLAVE_ADDR;
new_intf->channels[j].lun = 2;
@@ -1783,7 +1783,7 @@ int ipmi_register_smi(struct ipmi_smi_ha
new_intf->handlers = handlers;
new_intf->send_info = send_info;
spin_lock_init(&(new_intf->seq_lock));
- for (j=0; jseq_table[j].inuse = 0;
new_intf->seq_table[j].seqid = 0;
}
@@ -1891,7 +1891,7 @@ static void clean_up_interface_data(ipmi
free_recv_msg_list(&(intf->waiting_events));
free_cmd_rcvr_list(&(intf->cmd_rcvrs));
- for (i=0; iseq_table[i].inuse)
&& (intf->seq_table[i].recv_msg))
{
@@ -1910,7 +1910,7 @@ int ipmi_unregister_smi(ipmi_smi_t intf)
down_write(&interfaces_sem);
if (list_empty(&(intf->users)))
{
- for (i=0; idata_size; m++)
+ for (m = 0; m < msg->data_size; m++)
printk(" %2.2x", msg->data[m]);
printk("\n");
}
@@ -2469,7 +2469,7 @@ static int handle_new_recv_msg(ipmi_smi_
#ifdef DEBUG_MSGING
int m;
printk("Recv:");
- for (m=0; mrsp_size; m++)
+ for (m = 0; m < msg->rsp_size; m++)
printk(" %2.2x", msg->rsp[m]);
printk("\n");
#endif
@@ -2703,7 +2703,7 @@ smi_from_recv_msg(ipmi_smi_t intf, struc
{
int m;
printk("Resend: ");
- for (m=0; mdata_size; m++)
+ for (m = 0; m < smi_msg->data_size; m++)
printk(" %2.2x", smi_msg->data[m]);
printk("\n");
}
@@ -2724,7 +2724,7 @@ ipmi_timeout_handler(long timeout_period
INIT_LIST_HEAD(&timeouts);
spin_lock(&interfaces_lock);
- for (i=0; iseq_lock), flags);
- for (j=0; jseq_table[j]);
if (!ent->inuse)
continue;
@@ -2789,7 +2789,7 @@ ipmi_timeout_handler(long timeout_period
spin_unlock(&intf->counter_lock);
smi_msg = smi_from_recv_msg(intf,
ent->recv_msg, j, ent->seqid);
-if(!smi_msg)
+if (! smi_msg)
continue;
spin_unlock_irqrestore(&(intf->seq_lock),flags);
@@ -2820,7 +2820,7 @@ static void ipmi_request_event(void)
inti;
spin_lock(&interfaces_lock);
- for (i=0; ixmit_msgs.next;
}
- if (!entry) {
+ if (! entry) {
smi_info->curr_msg = NULL;
rv = SI_SM_IDLE;
} else {
@@ -328,7 +328,7 @@ static void start_clear_flags(struct smi
memory, we will re-enable the interrupt. */
static inline void disable_si_irq(struct smi_info *smi_info)
{
- if ((smi_info->irq) && (!smi_info->interrupt_disabled)) {
+ if ((smi_info->irq) && (! smi_info->interrupt_disabled)) {
disable_irq_nosync(smi_info->irq);
smi_info->interrupt_disabled = 1;
}
@@ -359,7 +359,7 @@ static void handle_flags(stru
Re: [RFC][CFLART] ipmi procfs bogosity
Andrew Morton wrote: Corey Minyard <[EMAIL PROTECTED]> wrote: Indeed, this function is badly written. In rewriting, I couldn't find a nice function for reading integers from userspace, and the proc_dointvec stuff didn't seem terribly suitable. We write numbers into profs files all the time. Is there something different about the IPMI requirement which makes the approach used by, say, dirty_writeback_centisecs_handler() inappropriate? Ok, that's probably better, and this probably belongs in /proc/sys/dev/ipmi. This is new enough that it doesn't matter, I don't think any one is using it yet. Patch is attached. -Corey ipmi-poweroff-fix-chassis-ctrl.patch Description: unknown/unknown
Re: [PATCH] part 3 - Convert IPMI driver over to use refcounts
Andrew Morton wrote:
The IPMI driver uses read/write locks to ensure that things
exist while they are in use. This is bad from a number of
points of view. This patch removes the rwlocks and uses
refcounts and a special synced list (the entries can be
refcounted and removal is blocked while an entry is in
use).
This:
+
+struct synced_list_entry_task_q
{
struct list_head link;
+ task_t *process;
+};
+
+#define synced_list_for_each_entry(pos, l, entry, flags) \
+ for ((spin_lock_irqsave(&(l)->lock, flags),\
+ pos = container_of((l)->head.next, typeof(*(pos)),entry.link)); \
+(prefetch((pos)->entry.link.next),
\
+ &(pos)->entry.link != (&(l)->head)\
+ ? (atomic_inc(&(pos)->entry.usecount), \
+ spin_unlock_irqrestore(&(l)->lock, flags), 1) \
+ : (spin_unlock_irqrestore(&(l)->lock, flags), 0)); \
+(spin_lock_irqsave(&(l)->lock, flags),\
+ synced_list_wake(&(pos)->entry), \
+ pos = container_of((pos)->entry.link.next, typeof(*(pos)), \
+entry.link)))
(gad)
Yes, I was trying to preserve list semantics and it got out of hand. It
is pretty ugly.
And this:
+static int synced_list_clear(struct synced_list *head,
+int (*match)(struct synced_list_entry *,
+ void *),
+void (*free)(struct synced_list_entry *),
+void *match_data)
+{
+ struct synced_list_entry *ent, *ent2;
+ int rv = -ENODEV;
+ int mrv = SYNCED_LIST_MATCH_CONTINUE;
+
+ spin_lock_irq(&head->lock);
+ restart:
+ list_for_each_entry_safe(ent, ent2, &head->head, link) {
+ if (match) {
+ mrv = match(ent, match_data);
+ if (mrv == SYNCED_LIST_NO_MATCH)
+ continue;
+ }
+ if (atomic_read(&ent->usecount)) {
+ struct synced_list_entry_task_q e;
+ e.process = current;
+ list_add(&e.link, &ent->task_list);
+ __set_current_state(TASK_UNINTERRUPTIBLE);
+ spin_unlock_irq(&head->lock);
+ schedule();
+ spin_lock_irq(&head->lock);
+ list_del(&e.link);
+ goto restart;
+ }
+ list_del(&ent->link);
+ rv = 0;
+ if (free)
+ free(ent);
+ if (mrv == SYNCED_LIST_MATCH_STOP)
+ break;
+ }
+ spin_unlock_irq(&head->lock);
+ return rv;
+}
Look awfully similar to wait_queue_head_t. Are you sure existing
infrastructure cannot be used?
I already had a lock and it seemed wasteful to have two locks. And it
didn't really save much code and it didn't quite fit.
Basically, I need a notifier list that avoids traversal/removal race
conditions. An asynchronous event or command comes in and you need to
inform all the users who are waiting for events or commands. I don't
think anything like that exists currently (the current notifier list has
no lock on traversal). I don't want a lock around the whole traversal
because doing callbacks with locks held is not a good idea.
The list I created simply blocks a deleter while the item is in use.
There may be a better way to handle this, I'll think about it (and take
any suggestions :). I read some stuff about RCU when looking at this,
and it didn't seem suitable at the time, but I just reread it and it
seems like it would be fine. I'll remove all this garbage and replace it
with an RCU list.
1 files changed, 692 insertions(+), 461 deletions(-)
Ow. Why is it worthwhile?
The change is definately needed. The driver, as it currently is, holds
read locks for very long periods of time. It does this to make sure
various objects don't get deleted. Holding read locks for this long is
bad, and this is the job of refcounts. It's hard to break up changing
fundamental underpinnings like this.
The patch is big, and from the above it looks like it is not quite
ready. I'll continue to look at it.
Thank you,
-Corey
-
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/
[PATCH] ARM: Ignore memory tags with invalid data
From: Corey Minyard <[EMAIL PROTECTED]>
The DNS-323 system has several bogus memory entries in the tag table,
and it caused the system to crash at startup. Ignore tag entries that
are obviously bogus.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
---
arch/arm/kernel/setup.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index bf56eb3..dfdb469 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -630,7 +630,12 @@ __tagtable(ATAG_CORE, parse_tag_core);
static int __init parse_tag_mem32(const struct tag *tag)
{
- if (meminfo.nr_banks >= NR_BANKS) {
+ /*
+* Make sure that the memory size is non-zero, page aligned,
+* and that it doesn't overflow the meminfo table.
+*/
+ if (meminfo.nr_banks >= NR_BANKS || tag->u.mem.size & ~PAGE_MASK ||
+ tag->u.mem.size == 0 || tag->u.mem.start & ~PAGE_MASK) {
printk(KERN_WARNING
"Ignoring memory bank 0x%08x size %dKB\n",
tag->u.mem.start, tag->u.mem.size / 1024);
--
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/
Re: [PATCH] ARM: Ignore memory tags with invalid data
Ben Dooks wrote:
On Mon, Jan 21, 2008 at 10:05:56PM -0600, Corey Minyard wrote:
From: Corey Minyard <[EMAIL PROTECTED]>
The DNS-323 system has several bogus memory entries in the tag table,
and it caused the system to crash at startup. Ignore tag entries that
are obviously bogus.
This should have gone to the linux-arm-kernel mailing list as well,
so that all the people interested in ARM can see it as soon as
possible. See [EMAIL PROTECTED]
You have to be a member to post, and I'm not a member. I guess I'll
join, though it seems somewhat pointless for a single patch.
The only suggestion I could think of would to be change to having
an check_tag_mem32() function, or put each check on its own line
to make it easier to read what each check is doing.
ie:
if (meminfo.nr_banks >= NR_BANKS ||
tag->u.mem.size & ~PAGE_MASK || /* size is unaligned */
tag->u.mem.size == 0 || /* no memory here */
tag->u.mem.start & ~PAGE_MASK) /* start is unaligned */
{
(even without the comments it makes it easier to see what each test
is.
That's fine, it is easier to read.
Thanks,
-corey
--
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/
Re: [Openipmi-developer] modprobing ipmi_si on Dell Power Edge 2600 make the terminal hang kernel ver. >= 2.6.20
Matt Domsch wrote: On Tue, Jan 22, 2008 at 06:50:31AM -0600, Matt Domsch wrote: On Mon, Jan 21, 2008 at 09:40:59AM +0800, william cheng wrote: Dear all, We got some problem on modprobing the ipmi_si module on Dell Power Edge 2600. I've asked someone to look into this. and I've filed this as http://bugzilla.kernel.org/show_bug.cgi?id=9793 I've posted a response to that bugzilla entry. -corey -- 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/
Re: [PATCH 3/4] docs: convert kref semaphore to mutex
Yes, a good idea.
Acked-by: Corey Minyard <[EMAIL PROTECTED]>
Daniel Walker wrote:
Just converting this documentation semaphore reference, since we don't
want to promote semaphore usage.
Signed-off-by: Daniel Walker <[EMAIL PROTECTED]>
---
Documentation/kref.txt | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
Index: linux-2.6.23/Documentation/kref.txt
===
--- linux-2.6.23.orig/Documentation/kref.txt
+++ linux-2.6.23/Documentation/kref.txt
@@ -141,10 +141,10 @@ The last rule (rule 3) is the nastiest o
instance, you have a list of items that are each kref-ed, and you wish
to get the first one. You can't just pull the first item off the list
and kref_get() it. That violates rule 3 because you are not already
-holding a valid pointer. You must add locks or semaphores. For
-instance:
+holding a valid pointer. You must add a mutex (or some other lock).
+For instance:
-static DECLARE_MUTEX(sem);
+static DEFINE_MUTEX(mutex);
static LIST_HEAD(q);
struct my_data
{
@@ -155,12 +155,12 @@ struct my_data
static struct my_data *get_entry()
{
struct my_data *entry = NULL;
- down(&sem);
+ mutex_lock(&mutex);
if (!list_empty(&q)) {
entry = container_of(q.next, struct my_q_entry, link);
kref_get(&entry->refcount);
}
- up(&sem);
+ mutex_unlock(&mutex);
return entry;
}
@@ -174,9 +174,9 @@ static void release_entry(struct kref *r
static void put_entry(struct my_data *entry)
{
- down(&sem);
+ mutex_lock(&mutex);
kref_put(&entry->refcount, release_entry);
- up(&sem);
+ mutex_unlock(&mutex);
}
The kref_put() return value is useful if you do not want to hold the
@@ -191,13 +191,13 @@ static void release_entry(struct kref *r
static void put_entry(struct my_data *entry)
{
- down(&sem);
+ mutex_lock(&mutex);
if (kref_put(&entry->refcount, release_entry)) {
list_del(&entry->link);
- up(&sem);
+ mutex_unlock(&mutex);
kfree(entry);
} else
- up(&sem);
+ mutex_unlock(&mutex);
}
This is really more useful if you have to call other routines as part
--
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/
[PATCH] ipmi: add the standard watchdog timeout ioctls
From: Corey Minyard <[EMAIL PROTECTED]> Add the standard IOCTLs to the IPMI driver for setting and getting the pretimeout. Tested by Benoît Guillon. Signed off by: Corey Minyard <[EMAIL PROTECTED]> Cc: Benoît Guillon <[EMAIL PROTECTED]> --- Index: linux-2.6.23/drivers/char/ipmi/ipmi_watchdog.c === --- linux-2.6.23.orig/drivers/char/ipmi/ipmi_watchdog.c +++ linux-2.6.23/drivers/char/ipmi/ipmi_watchdog.c @@ -683,6 +683,7 @@ static int ipmi_ioctl(struct inode *inod return 0; case WDIOC_SET_PRETIMEOUT: + case WDIOC_SETPRETIMEOUT: i = copy_from_user(&val, argp, sizeof(int)); if (i) return -EFAULT; @@ -690,6 +691,7 @@ static int ipmi_ioctl(struct inode *inod return ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY); case WDIOC_GET_PRETIMEOUT: + case WDIOC_GETPRETIMEOUT: i = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); if (i) return -EFAULT; - 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/
Re: ipmi_watchdog can not reset the kernel panic machine
The watchdog is "off" by default, meaning that you have to have
something actually start resetting the watchdog before it will start
running. That's why you are seeing this behavior.
There is a start_now option that will start the watchdog when it is
loaded, but then it will reset the system unless something resets the
watchdog periodically, and you have a limited time to start this operation.
On a panic, the IPMI driver attempts to preserve the state of the
watchdog and (if running) increase the timeout time to allow a kdump or
something like that to occur. That's the purpose of the code you
reference. It is not to start a reset operation on any panic. It used
to start a reset on every panic, but that cause problems for many users.
-corey
Andrew Morton wrote:
(cc's added)
On Fri, 23 Nov 2007 20:28:41 -0800 (PST) [EMAIL PROTECTED] wrote:
Build kernel-2.6.24-rc3. pmi_watchdog can not reset the kernel panic
machine. The watchdog can never to record panic information to IPMI SEL.
1. I disable auto reset when kernel panic by echo "0" >
/proc/sys/kernel/panic
2. modprobe ipmi_watchdog timeout=120 action=reset
3. Load a driver, the driver will call panic() when ioctl to call into
the driver.
4. By ioctl call into the driver, panic the system.
in wdog_panic_handler, I printk "ipmi_watchdog_state=WDOG_TIMEOUT_NONE"
so, the watchdog can never to record panic information to IPMI SEL.
static int wdog_panic_handler(struct notifier_block *this,
unsigned long event,
void *unused)
{
static int panic_event_handled = 0;
/* On a panic, if we have a panic timeout, make sure to extend
the watchdog timer to a reasonable value to complete the
panic, if the watchdog timer is running. Plus the
pretimeout is meaningless at panic time. */
if (watchdog_user && !panic_event_handled &&
ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
/* Make sure we do this only once. */
panic_event_handled = 1;
timeout = 255;
pretimeout = 0;
panic_halt_ipmi_set_timeout();
}
return NOTIFY_OK;
}
-
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/
[PATCH] Add sysfs support to the IPMI driver
The IPMI driver has long needed to tie into the device model (and I've
long been hoping someone else would do it). I finally gave up and spent
the time to learn how to do it. I think this is right, it seems to work
on on my system.
-Corey
Add support for sysfs to the IPMI device interface.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-mm1/drivers/char/ipmi/ipmi_devintf.c
===
--- linux-2.6.11-mm1.orig/drivers/char/ipmi/ipmi_devintf.c
+++ linux-2.6.11-mm1/drivers/char/ipmi/ipmi_devintf.c
@@ -44,6 +44,7 @@
#include
#include
#include
+#include
#define IPMI_DEVINTF_VERSION "v33"
@@ -519,15 +520,24 @@
" interface. Other values will set the major device number"
" to that value.");
+static struct class_simple *ipmi_class;
+
static void ipmi_new_smi(int if_num)
{
+ char name[10];
+ dev_t dev = MKDEV(ipmi_major, if_num);
+
devfs_mk_cdev(MKDEV(ipmi_major, if_num),
S_IFCHR | S_IRUSR | S_IWUSR,
"ipmidev/%d", if_num);
+
+ snprintf(name, sizeof(name), "ipmi%d", if_num);
+ class_simple_device_add(ipmi_class, dev, NULL, name);
}
static void ipmi_smi_gone(int if_num)
{
+ class_simple_device_remove(MKDEV(ipmi_major, if_num));
devfs_remove("ipmidev/%d", if_num);
}
@@ -548,8 +558,15 @@
printk(KERN_INFO "ipmi device interface version "
IPMI_DEVINTF_VERSION "\n");
+ ipmi_class = class_simple_create(THIS_MODULE, "ipmi");
+ if (IS_ERR(ipmi_class)) {
+ printk(KERN_ERR "ipmi: can't register device class\n");
+ return PTR_ERR(ipmi_class);
+ }
+
rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops);
if (rv < 0) {
+ class_simple_destroy(ipmi_class);
printk(KERN_ERR "ipmi: can't get major %d\n", ipmi_major);
return rv;
}
@@ -563,6 +580,7 @@
rv = ipmi_smi_watcher_register(&smi_watcher);
if (rv) {
unregister_chrdev(ipmi_major, DEVICE_NAME);
+ class_simple_destroy(ipmi_class);
printk(KERN_WARNING "ipmi: can't register smi watcher\n");
return rv;
}
@@ -573,6 +591,7 @@
static __exit void cleanup_ipmi(void)
{
+ class_simple_destroy(ipmi_class);
ipmi_smi_watcher_unregister(&smi_watcher);
devfs_remove(DEVICE_NAME);
unregister_chrdev(ipmi_major, DEVICE_NAME);
Re: [PATCH] Add sysfs support to the IPMI driver
Greg KH wrote: On Sat, Mar 12, 2005 at 10:57:24PM -0600, Corey Minyard wrote: The IPMI driver has long needed to tie into the device model (and I've long been hoping someone else would do it). I finally gave up and spent the time to learn how to do it. I think this is right, it seems to work on on my system. Looks good. One minor question: + + snprintf(name, sizeof(name), "ipmi%d", if_num); + class_simple_device_add(ipmi_class, dev, NULL, name); What do ipmi class devices live on? pci devices? i2c devices? platform devices? Or are they purely virtual things? Good question. I struggled with this for a little while and decided the class interface was important to have in first and I'd figure out the rest later. They live in different places depending on the particular low-level interface. Some live on the I2C bus (and will show up there in sysfs with the I2C driver). Some live on the ISA bus, some are memory-mapped, some are on the PCI bus (though there is not a driver for PCI support yet), and some sit on the end of a serial port (driver is in the works). I know, it's a mess, but there's not much I can do about these crazy hardware manufacturers. I wasn't sure where to handle all this. The I2C and PCI bus side of things should be handled. However, the others probably need to sit someplace on a bus, right? That should probably be handled in the low-level code that actually knows where the hardware sits. Thanks, -Corey - 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/
Re: [PATCH] Add a non-blocking interface to the I2C code, part 1
Greg KH wrote: On Wed, Mar 02, 2005 at 01:58:51PM -0600, Corey Minyard wrote: This patch reorganizes the I2C SMBus formatting code to make it more suitable for the upcoming non-blocking changes. You are changing too much stuff here to claim it's just a reorganization: - variable name changes for no reason Well, yes. Both "adap" and "adapter" are used to refer to the same thing in the file; "adap" seemed to be the most common usage so I chose that for the new functions I added. In one place I changed adapter to adap. I also changed "res" to "result". I can fix those. Or I can submit a patch first that renames the adap and adapter to make the usage consistent (I would prefer adapter). - coding style changes (improper ones at that) I don't see that. The ugliest thing about this is the functions that take the massive numbers of parameters, but that goes away in the next patch which puts all the data into a single data structure and passes it around. The code here was also very inconsistent about use of spaces, like x(a,b,c) vs x(a, b, c), "struct a *b" vs "struct a * b", "a=b" vs "a = b". It's hard to know what was right. The changes I made in these respects was to try to make it use the usage most common in the file. If you like, I can do a pass and make everything consistent in the file as part of the previous patch I talked about. - logic changes. I tried very hard not to make logic changes. Now I see there were two places where the function checked client->adapter->algo->master_xfer then called i2c_transfer(), which did the same check and returned the same error if it was NULL. I removed the redundant check. That belongs in a separate patch. I couldn't find any others. What exactly are you doing with this patch, and why? The i2c main functions do the following: Format the data for transmission Send the data to the next layer down for handling Clean up the results The original code did all this in single big functions. This patch breaks the formatting and cleanup operations into separate functions. Beyond one big function being ugly, the non-blocking code needs this because it needs to perform these separately. When you start the operation, the non-blocking code needs to do the format then return. Later on, when the operation is complete, the thread of execution handling the completion will do the cleanup. -Corey - 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/
Re: [PATCH][alpha] "pm_power_off" [drivers/char/ipmi/ipmi_poweroff.ko] undefined!
This is not the right fix. I know of IPMI hardware on ppc and xscale systems. There should be nothing general in the driver that limits it to x86/ia64. pm_power_off is defined in linux/pm.h. Shouldn't it be available everywhere? -Corey Ivan Kokshaysky wrote: On Tue, Mar 22, 2005 at 04:53:12PM -0500, Jeff Garzik wrote: Although I suppose its possible that some alpha machines have SMI hardware, I don't think I've ever seen ACPI or IPMI on any alpha. Yes, this stuff doesn't exist. I think it would be correct to add the following to drivers/char/ipmi/Kconfig, like it's done for ACPI: menu "IPMI" + depends on IA64 || X86 config IPMI_HANDLER tristate 'IPMI top-level message handler' + depends on IA64 || X86 Ivan. - 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/
[PATCH] I2C Part 1 - Remove some redundancy from the i2c-core.c file
This is a series of patches to add a non-blocking interface to the I2C
driver. Hopefully it is broken into small enough functional changes.
I'm not posting the whole series right now, just the first few.
Call i2c_transfer() from i2c_master_send() and i2c_master_recv()
to avoid the redundant code that was in all three functions.
This is important for the non-blocking interfaces because they
will have to handle a non-blocking interface in this area. Having
it in one place greatly simplifies the changes.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc4/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-rc4.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.11-rc4/drivers/i2c/i2c-core.c
@@ -606,27 +606,20 @@
struct i2c_adapter *adap=client->adapter;
struct i2c_msg msg;
- if (client->adapter->algo->master_xfer) {
- msg.addr = client->addr;
- msg.flags = client->flags & I2C_M_TEN;
- msg.len = count;
- msg.buf = (char *)buf;
+ msg.addr = client->addr;
+ msg.flags = client->flags & I2C_M_TEN;
+ msg.len = count;
+ msg.buf = (char *)buf;
- dev_dbg(&client->adapter->dev, "master_send: writing %d bytes.\n",
- count);
-
- down(&adap->bus_lock);
- ret = adap->algo->master_xfer(adap,&msg,1);
- up(&adap->bus_lock);
+ dev_dbg(&client->adapter->dev, "master_send: writing %d bytes.\n",
+ count);
- /* if everything went ok (i.e. 1 msg transmitted), return #bytes
- * transmitted, else error code.
- */
- return (ret == 1 )? count : ret;
- } else {
- dev_err(&client->adapter->dev, "I2C level transfers not supported\n");
- return -ENOSYS;
- }
+ ret = i2c_transfer(adap, &msg, 1);
+
+ /* if everything went ok (i.e. 1 msg transmitted), return #bytes
+ * transmitted, else error code.
+ */
+ return (ret == 1 )? count : ret;
}
int i2c_master_recv(struct i2c_client *client, char *buf ,int count)
@@ -634,31 +627,25 @@
struct i2c_adapter *adap=client->adapter;
struct i2c_msg msg;
int ret;
- if (client->adapter->algo->master_xfer) {
- msg.addr = client->addr;
- msg.flags = client->flags & I2C_M_TEN;
- msg.flags |= I2C_M_RD;
- msg.len = count;
- msg.buf = buf;
- dev_dbg(&client->adapter->dev, "master_recv: reading %d bytes.\n",
- count);
-
- down(&adap->bus_lock);
- ret = adap->algo->master_xfer(adap,&msg,1);
- up(&adap->bus_lock);
+ msg.addr = client->addr;
+ msg.flags = client->flags & I2C_M_TEN;
+ msg.flags |= I2C_M_RD;
+ msg.len = count;
+ msg.buf = buf;
+
+ dev_dbg(&client->adapter->dev, "master_recv: reading %d bytes.\n",
+ count);
- dev_dbg(&client->adapter->dev, "master_recv: return:%d (count:%d, addr:0x%02x)\n",
- ret, count, client->addr);
+ ret = i2c_transfer(adap, &msg, 1);
+
+ dev_dbg(&client->adapter->dev, "master_recv: return:%d (count:%d, addr:0x%02x)\n",
+ ret, count, client->addr);
- /* if everything went ok (i.e. 1 msg transmitted), return #bytes
- * transmitted, else error code.
- */
- return (ret == 1 )? count : ret;
- } else {
- dev_err(&client->adapter->dev, "I2C level transfers not supported\n");
- return -ENOSYS;
- }
+ /* if everything went ok (i.e. 1 msg transmitted), return #bytes
+ * transmitted, else error code.
+ */
+ return (ret == 1 )? count : ret;
}
[PATCH] I2C Part 2 - Break up the formatting code into individual functions
This patch reorganizes the I2C SMBus formatting code to make it more
suitable for the upcoming non-blocking changes.
The I2C main functions do the following:
Format the data for transmission
Send the data to the next layer down for handling
Clean up the results
The original code did all this in single big function. This patch
breaks the formatting and cleanup operations into separate functions.
Beyond one big function being ugly, the non-blocking code needs this
because it needs to perform these separately. When you start the
operation, the non-blocking code needs to do the format then return.
Later on, when the operation is complete, the thread of execution
handling the completion will do the cleanup.
This patch does create some functions with lots of parameters. That
goes away in a future patch that consolidates the data for an I2C
operation into a single data structure.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-mm1/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-mm1.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.11-mm1/drivers/i2c/i2c-core.c
@@ -1038,25 +1038,127 @@
}
}
-/* Simulate a SMBus command using the i2c protocol
- No checking of parameters is done! */
-static s32 i2c_smbus_xfer_emulated(struct i2c_adapter * adapter, u16 addr,
- unsigned short flags,
- char read_write, u8 command, int size,
- union i2c_smbus_data * data)
-{
- /* So we need to generate a series of msgs. In the case of writing, we
- need to use only one message; when reading, we need two. We initialize
- most things with sane defaults, to keep the code below somewhat
- simpler. */
- unsigned char msgbuf0[34];
- unsigned char msgbuf1[34];
+static int i2c_smbus_complete_entry(struct i2c_adapter * adapter, u16 addr,
+unsigned short flags, char read_write,
+u8 command, int size,
+union i2c_smbus_data * data,
+int swpec, u8 partial,
+int result)
+{
+ if (result < 0)
+ return result;
+
+ if(swpec &&
+ size != I2C_SMBUS_QUICK &&
+ size != I2C_SMBUS_I2C_BLOCK_DATA &&
+ (read_write == I2C_SMBUS_READ ||
+ size == I2C_SMBUS_PROC_CALL_PEC ||
+ size == I2C_SMBUS_BLOCK_PROC_CALL_PEC)) {
+ if(i2c_smbus_check_pec(addr,
+ command,
+ size,
+ partial,
+ data))
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void i2c_smbus_format_entry(struct i2c_adapter * adapter, u16 addr,
+ unsigned short *flags, char read_write,
+ u8 command, int *size,
+ union i2c_smbus_data * data,
+ int *swpec, u8 *partial)
+{
+ *swpec = 0;
+ *partial = 0;
+ *flags &= I2C_M_TEN | I2C_CLIENT_PEC;
+ if((*flags & I2C_CLIENT_PEC) &&
+ !(i2c_check_functionality(adapter, I2C_FUNC_SMBUS_HWPEC_CALC))) {
+ *swpec = 1;
+ if(read_write == I2C_SMBUS_READ &&
+ *size == I2C_SMBUS_BLOCK_DATA)
+ *size = I2C_SMBUS_BLOCK_DATA_PEC;
+ else if(*size == I2C_SMBUS_PROC_CALL)
+ *size = I2C_SMBUS_PROC_CALL_PEC;
+ else if(*size == I2C_SMBUS_BLOCK_PROC_CALL) {
+ unsigned char *sdata = data->block;
+ i2c_smbus_add_pec(addr, command, I2C_SMBUS_BLOCK_DATA,
+ data);
+ *partial = sdata[sdata[0] + 1];
+ *size = I2C_SMBUS_BLOCK_PROC_CALL_PEC;
+ } else if(read_write == I2C_SMBUS_WRITE &&
+ *size != I2C_SMBUS_QUICK &&
+ *size != I2C_SMBUS_I2C_BLOCK_DATA)
+ *size = i2c_smbus_add_pec(addr, command, *size, data);
+ }
+}
+
+static int i2c_smbus_emu_complete(struct i2c_adapter * adapter, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size,
+ union i2c_smbus_data * data,
+ struct i2c_msg *msg,
+ int swpec, u8 partial,
+ int result)
+{
+ unsigned char *msgbuf0 = msg[0].buf;
+ unsigned char *msgbuf1 = msg[1].buf;
+ int i;
+
+
+ if (result < 0)
+ return result;
+
+ if (read_write != I2C_SMBUS_READ)
+ return result;
+
+ switch(size) {
+ case I2C_SMBUS_BYTE:
+ data->byte = msgbuf0[0];
+ break;
+ case I2C_SMBUS_BYTE_DATA:
+ data->byte = msgbuf1[0];
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ case I2C_SMBUS_PROC_CALL:
+ data->word = msgbuf1[0]|(msgbuf1[1] << 8);
+ break;
+ case I2C_SMBUS_I2C_BLOCK_DATA:
+ /* fixed at 32 for now */
+ data->block[0] = I2C_SMBUS_I2C_BLOCK_MAX;
+ for (i = 0; i < I2C_SMBUS_I2C_BLOCK_MAX; i++)
+ data->block[i+1] = msgbuf1[i];
+ break;
+ }
+
+ return i2c_smbus_complete_entry(adapter, addr, flags,
+ read_write, command,
+ size, data, swpec, partial, result);
+}
+
+static int i2c_smbus_emu_format(struct i2c_adapter *adapter, u16 addr,
+unsigned short flags, char read_write,
+u8 command, int size,
+union i2c_smbus_data * data,
+struct i2c_msg *msg)
+{
+ /* So we need to generate a series of msgs. In the case of
+ writin
[PATCH] I2C Part 3 - Add an operation queue data structure
This patch adds an operation queue structure and converts over the
passing around of data to use the operation queue. The op queue
entry will be used for queueing in the non-blocking case.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-mm1/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-mm1.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.11-mm1/drivers/i2c/i2c-core.c
@@ -582,24 +582,45 @@
*
*/
-int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
+static void i2c_transfer_entry(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry)
{
- int ret;
-
+ entry->xfer_type = I2C_OP_I2C;
+ entry->complete = NULL;
if (adap->algo->master_xfer) {
- dev_dbg(&adap->dev, "master_xfer: with %d msgs.\n", num);
+ dev_dbg(&adap->dev, "master_xfer: with %d msgs.\n",
+ entry->i2c.num);
down(&adap->bus_lock);
- ret = adap->algo->master_xfer(adap,msgs,num);
+ entry->result = adap->algo->master_xfer(adap, entry->i2c.msgs,
+ entry->i2c.num);
up(&adap->bus_lock);
-
- return ret;
+ if (entry->complete)
+ entry->complete(adap, entry);
} else {
dev_dbg(&adap->dev, "I2C level transfers not supported\n");
- return -ENOSYS;
+ entry->result = -ENOSYS;
}
}
+int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
+{
+ struct i2c_op_q_entry *entry;
+ int rv;
+
+ entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ entry->i2c.msgs = msgs;
+ entry->i2c.num = num;
+
+ i2c_transfer_entry(adap, entry);
+ rv = entry->result;
+ kfree(entry);
+ return rv;
+}
+
int i2c_master_send(struct i2c_client *client,const char *buf ,int count)
{
int ret;
@@ -1038,185 +1059,181 @@
}
}
-static int i2c_smbus_complete_entry(struct i2c_adapter * adapter, u16 addr,
-unsigned short flags, char read_write,
-u8 command, int size,
-union i2c_smbus_data * data,
-int swpec, u8 partial,
-int result)
-{
- if (result < 0)
- return result;
-
- if(swpec &&
- size != I2C_SMBUS_QUICK &&
- size != I2C_SMBUS_I2C_BLOCK_DATA &&
- (read_write == I2C_SMBUS_READ ||
- size == I2C_SMBUS_PROC_CALL_PEC ||
- size == I2C_SMBUS_BLOCK_PROC_CALL_PEC)) {
- if(i2c_smbus_check_pec(addr,
- command,
- size,
- partial,
- data))
- return -EINVAL;
- }
+static void i2c_smbus_complete_entry(struct i2c_adapter * adapter,
+ struct i2c_op_q_entry * entry)
+{
+ if (entry->result < 0)
+ return;
- return 0;
+ if(entry->swpec &&
+ entry->smbus.size != I2C_SMBUS_QUICK &&
+ entry->smbus.size != I2C_SMBUS_I2C_BLOCK_DATA &&
+ (entry->smbus.read_write == I2C_SMBUS_READ ||
+ entry->smbus.size == I2C_SMBUS_PROC_CALL_PEC ||
+ entry->smbus.size == I2C_SMBUS_BLOCK_PROC_CALL_PEC)) {
+ if(i2c_smbus_check_pec(entry->smbus.addr,
+ entry->smbus.command,
+ entry->smbus.size,
+ entry->partial,
+ entry->smbus.data))
+ entry->result = -EINVAL;
+ }
}
-static void i2c_smbus_format_entry(struct i2c_adapter * adapter, u16 addr,
- unsigned short *flags, char read_write,
- u8 command, int *size,
- union i2c_smbus_data * data,
- int *swpec, u8 *partial)
-{
- *swpec = 0;
- *partial = 0;
- *flags &= I2C_M_TEN | I2C_CLIENT_PEC;
- if((*flags & I2C_CLIENT_PEC) &&
+static void i2c_smbus_format_entry(struct i2c_adapter * adapter,
+ struct i2c_op_q_entry * entry)
+{
+ entry->swpec = 0;
+ entry->partial = 0;
+ entry->smbus.flags &= I2C_M_TEN | I2C_CLIENT_PEC;
+ if((entry->smbus.flags & I2C_CLIENT_PEC) &&
!(i2c_check_functionality(adapter, I2C_FUNC_SMBUS_HWPEC_CALC))) {
- *swpec = 1;
- if(read_write == I2C_SMBUS_READ &&
- *size == I2C_SMBUS_BLOCK_DATA)
- *size = I2C_SMBUS_BLOCK_DATA_PEC;
- else if(*size == I2C_SMBUS_PROC_CALL)
- *size = I2C_SMBUS_PROC_CALL_PEC;
- else if(*size == I2C_SMBUS_BLOCK_PROC_CALL) {
- unsigned char *sdata = data->block;
- i2c_smbus_add_pec(addr, command, I2C_SMBUS_BLOCK_DATA,
- data);
- *partial = sdata[sdata[0] + 1];
- *size = I2C_SMBUS_BLOCK_PROC_CALL_PEC;
- } else if(read_write == I2C_SMBUS_WRITE &&
- *size != I2C_SMBUS_QUICK &&
- *size != I2C_SMBUS_I2C_BLOCK_DATA)
- *size = i2c_smbus_add_pec(addr, command, *size, data);
+ entry->swpec = 1;
+ if(entry->smbus.read_write == I2C_SMBUS_READ &&
+ entry->smbus.size == I2C_SMBUS_BLOCK_DATA)
+ entry->smbus.size = I2C_SMBUS_BLOCK_DATA_PEC;
+ else if(entry->smbus.size == I2C_SMBUS_PROC_CALL)
+ entry->smbu
[PATCH] Update to IPMI driver to support old DMI spec
BTW, I'm also working with the person who had the trouble with the I2C
non-blocking driver updates, but we haven't figured it out yet.
Hopefully soon. (Though that has nothing to do with this patch.)
Thanks,
-Corey
The 1999 version of the DMI spec had a different configuration
than the newer versions for the IPMI configuration information.
This patch handles the differences between the two.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc3/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.11-rc3.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.11-rc3/drivers/char/ipmi/ipmi_si_intf.c
@@ -1578,46 +1578,53 @@
u8 *data = (u8 *)dm;
unsigned long base_addr;
u8 reg_spacing;
+ u8 len = dm->length;
dmi_ipmi_data_t *ipmi_data = dmi_data+intf_num;
ipmi_data->type = data[4];
memcpy(&base_addr, data+8, sizeof(unsigned long));
- if (base_addr & 1) {
- /* I/O */
- base_addr &= 0xFFFE;
+ if (len >= 0x11) {
+ if (base_addr & 1) {
+ /* I/O */
+ base_addr &= 0xFFFE;
+ ipmi_data->addr_space = IPMI_IO_ADDR_SPACE;
+ }
+ else {
+ /* Memory */
+ ipmi_data->addr_space = IPMI_MEM_ADDR_SPACE;
+ }
+ /* If bit 4 of byte 0x10 is set, then the lsb for the address
+ is odd. */
+ ipmi_data->base_addr = base_addr | ((data[0x10] & 0x10) >> 4);
+
+ ipmi_data->irq = data[0x11];
+
+ /* The top two bits of byte 0x10 hold the register spacing. */
+ reg_spacing = (data[0x10] & 0xC0) >> 6;
+ switch(reg_spacing){
+ case 0x00: /* Byte boundaries */
+ ipmi_data->offset = 1;
+ break;
+ case 0x01: /* 32-bit boundaries */
+ ipmi_data->offset = 4;
+ break;
+ case 0x02: /* 16-byte boundaries */
+ ipmi_data->offset = 16;
+ break;
+ default:
+ /* Some other interface, just ignore it. */
+ return -EIO;
+ }
+ } else {
+ /* Old DMI spec. */
+ ipmi_data->base_addr = base_addr;
ipmi_data->addr_space = IPMI_IO_ADDR_SPACE;
- }
- else {
- /* Memory */
- ipmi_data->addr_space = IPMI_MEM_ADDR_SPACE;
- }
-
- /* The top two bits of byte 0x10 hold the register spacing. */
- reg_spacing = (data[0x10] & 0xC0) >> 6;
- switch(reg_spacing){
- case 0x00: /* Byte boundaries */
ipmi_data->offset = 1;
- break;
- case 0x01: /* 32-bit boundaries */
- ipmi_data->offset = 4;
- break;
- case 0x02: /* 16-byte boundaries */
- ipmi_data->offset = 16;
- break;
- default:
- /* Some other interface, just ignore it. */
- return -EIO;
}
ipmi_data->slave_addr = data[6];
- /* If bit 4 of byte 0x10 is set, then the lsb for the address
- is odd. */
- ipmi_data->base_addr = base_addr | ((data[0x10] & 0x10) >> 4);
-
- ipmi_data->irq = data[0x11];
-
if (is_new_interface(-1, ipmi_data->addr_space,ipmi_data->base_addr)) {
dmi_data_entries++;
return 0;
[PATCH] Fix IPMI SMBus driver to select the I2C layer instead of depend on it
This is on top of the IPMI SMBus driver in the current mm kernel. Thanks, -Corey Make the IPMI SMBus select I2C, not depend on it, so I2C gets enabled properly when the IPMI SMBus is enabled. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Index: linux-2.6.11-rc3/drivers/char/ipmi/Kconfig === --- linux-2.6.11-rc3.orig/drivers/char/ipmi/Kconfig +++ linux-2.6.11-rc3/drivers/char/ipmi/Kconfig @@ -53,7 +53,8 @@ config IPMI_SMB tristate 'IPMI SMBus handler' - depends on IPMI_HANDLER && I2C + depends on IPMI_HANDLER + select I2C help Provides a driver for a SMBus interface to a BMC, meaning that you have a driver that must be accessed over an I2C bus instead of a
Re: [PATCH] Update to IPMI driver to support old DMI spec
Bukie Mabayoje wrote: Corey Minyard wrote: BTW, I'm also working with the person who had the trouble with the I2C non-blocking driver updates, but we haven't figured it out yet. Hopefully soon. (Though that has nothing to do with this patch.) Thanks, -Corey --- The 1999 version of the DMI spec had a different configuration than the newer versions for the IPMI configuration information. Are you referring to the System Management BIOS Reference Specification version 2.3.1 16 March 1999? Yes, that is correct. -Corey - 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/
Re: Fw: [Bug 4171] New: bttv seems broken here
JJ Luza and I found the problem. A patch to the I2C non-blocking
changes is attached. JJ was a tremendous help on this.
-Corey
Andrew Morton wrote:
Not sure who to blame here ;)
Corey made some i2c changes...
Begin forwarded message:
Date: Sat, 5 Feb 2005 13:36:53 -0800
From: [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
Subject: [Bug 4171] New: bttv seems broken here
http://bugme.osdl.org/show_bug.cgi?id=4171
Summary: bttv seems broken here
Kernel Version: 2.6.11-rc3-mm1
Status: NEW
Severity: normal
Distribution: Debian Sid
Hardware Environment: Nforce2 + gforce4 + leadtek winfast TV 2000 (Bt878)
Software Environment: kernel module is snd_bt87x (and tuner and bttv), and tv
app is xdtv.
Problem Description: I can't watch tv with this kernel (it worked with
2.6.10). There is no image or sound with xdtv, and I get these errors sent by
kernel when I try to access the bttv device :
The I2C non-blocking patch would incorrectly return success in some
cases because it wasn't updating the proper result variable. This
patch fixes the problem. The bttv driver now works properly with
the I2C non-blocking patch.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc3/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-rc3.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.11-rc3/drivers/i2c/i2c-core.c
@@ -899,9 +899,11 @@
entry->i2c.num);
up(&adap->bus_lock);
+ entry->result = ret;
return ret;
} else {
dev_dbg(&adap->dev, "I2C level transfers not supported\n");
+ entry->result = -ENOSYS;
return -ENOSYS;
}
}
@@ -1491,7 +1493,7 @@
msg[0].len = 1;
msg[1].addr = entry->smbus.addr;
msg[1].flags = entry->smbus.flags | I2C_M_RD;
- msg[1].len = 1;
+ msg[1].len = 0;
msgbuf0[0] = entry->smbus.command;
switch(entry->smbus.size) {
@@ -1593,17 +1595,17 @@
/* Simulate a SMBus command using the i2c protocol
No checking of parameters is done! */
-static s32 i2c_smbus_xfer_emulated(struct i2c_adapter * adap,
- struct i2c_op_q_entry * entry)
+static void i2c_smbus_xfer_emulated(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry)
{
- if (i2c_smbus_emu_format(adap, entry))
- return -EINVAL;
-
- if (i2c_transfer_entry(adap, entry) < 0)
- return -EINVAL;
+ if (i2c_smbus_emu_format(adap, entry)) {
+ entry->result = -EINVAL;
+ return;
+ }
- return entry->result;
+ /* Return value will be put into entry->result. */
+ i2c_transfer_entry(adap, entry);
}
s32 i2c_smbus_xfer(struct i2c_adapter * adap, u16 addr, unsigned short flags,
Re: 2.6.11-rc3-mm2
Andrew Morton wrote: ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11-rc3/2.6.11-rc3-mm2/ - Added the mlock and !SCHED_OTHER Linux Security Module for the audio guys. It seems that nothing else is going to come along and this is completely encapsulated. - Various other stuff. If anyone has a patch in here which they think should be in 2.6.11, please let me know. I'm intending to merge the following into 2.6.11: alpha-add-missing-dma_mapping_error.patch fix-compat-shmget-overflow.patch fix-shmget-for-ppc64-s390-64-sparc64.patch binfmt_elf-clearing-bss-may-fail.patch qlogic-warning-fixes.patch oprofile-exittext-referenced-in-inittext.patch force-read-implies-exec-for-all-32bit-processes-in-x86-64.patch oprofile-arm-xscale1-pmu-support-fix.patch The following one should probably go in: +update-to-ipmi-driver-to-support-old-dmi-spec.patch Systems with old data will not work correctly without it. There seems to be a few of them out there. -Corey - 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/
[PATCH] Fix IPMI LAN bridging
The size of LAN bridged messages was not being returned properly from the function that calculated address sizes. This fixes the problem. Signed-off-by: Corey Minyard <[EMAIL PROTECTED]> Index: linux-2.6.11-rc3/drivers/char/ipmi/ipmi_msghandler.c === --- linux-2.6.11-rc3.orig/drivers/char/ipmi/ipmi_msghandler.c +++ linux-2.6.11-rc3/drivers/char/ipmi/ipmi_msghandler.c @@ -480,6 +480,9 @@ return sizeof(struct ipmi_ipmb_addr); } + if (addr_type == IPMI_LAN_ADDR_TYPE) + return sizeof(struct ipmi_lan_addr); + return 0; }
Re: Adding an async I2C interface
Mark Studebaker wrote: is there a way to do this solely in i2c-core without having to add support to all the drivers? Yes and no. In order to support this async operation, the driver cannot block and do things like msleep() or schedule(). It has to start the operation, return, and either let polling or an interrupt drive the continued operation. Thus for async operations the driver has to be modified. However, if async operation is not required, the driver can stay as is. I've been working on this and will probably have a patch tomorrow. I've modified the piix4 and the i801 drivers, I probably won't do any more myself unless the need arises, since I can't test any others. Note that this still supports the old driver interface, so no drivers need to be rewritten. That way, they only need to be modified if something needs the async interface. So drivers that have an RTC on them or that support IPMI BMCs could be rewritten, but nothing else needs to be done. I've also noticed a somewhat cavalier attitude in this code with respect to return values. I've cleaned some of that up so return values are not just -1 on error, but are proper errno values. However, I've only fixed the core code and the drivers I've worked on. Thanks, -Corey Corey Minyard wrote: I have an IPMI interface driver that sits on top of the I2C code. I'd like to get it into the mainstream kernel, but I have a few problems to solve first before I can do that. The I2C code is synchronous and must run from a task context. The IPMI driver has certain operations that occur at panic time, including: * Storing panic information in IPMI's system event log * Extending the watchdog timer so it doesn't go off during panic operations (like kernel coredumps). * Powering the system off I can't really put the IPMI SMB interface into the kernel until I can do those operations. Also, I understand that some vendors put RTC chips onto the I2C bus and this must be accessed outside task context, too. I would really like add asynchronous interface to the I2C bus drivers. I propose: * Adding an async send interface to the busses that does a callback when the operation is complete. * Adding a poll interface to the busses. The I2C core code could call this if a synchronous call is made from task context (much like all the current drivers do right now). For asyncronous operation, the I2C core code would call it from a timer interrupt. If the driver supported interrupts, polling from the timer interrupt would not be necessary. * Add async operations for the user to call, including access to the polling code. * If the driver didn't support an async send, it would work as it does today and the async calls would return ENOSYS. This way, the bus drivers on I2C could be converted on a driver-by-driver basis. The IPMI code could query to see if the driver supported async operations. And the RTC code could use it, too. Is this ok with the I2C community? I would do the base work and convert over a few drivers. Thanks, -Corey - 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/
Re: Adding an async I2C interface
Bukie Mabayoje wrote: I will be glad to work with on this, I have some exposure to the BMC. See text below in blue. bukie Corey Minyard wrote: Mark Studebaker wrote: > is there a way to do this solely in i2c-core without having to > add support to all the drivers? Yes and no. In order to support this async operation, the driver cannot block and do things like msleep() or schedule(). The Interface driver must be a user space driver. That is precisely the problem. What happens if you need to get to the i2c bus but you are not running from a task context? It has to start the operation, return, and either let polling or an interrupt drive the continued operation. Thus for async operations the driver has to be modified. However, if async operation is not required, the driver can stay as is. I've been working on this and will probably have a patch tomorrow. I've modified the piix4 and the i801 drivers, I probably won't do any more myself unless the need arises, since I can't test any others. Note that this still supports the old driver interface, so no drivers need to be rewritten. That way, they only need to be modified if something needs the async interface. So drivers that have an RTC on them or that support IPMI BMCs could be rewritten, but nothing else needs to be done. I've also noticed a somewhat cavalier attitude in this code with respect to return values. I've cleaned some of that up so return values are not just -1 on error, but are proper errno values. However, I've only fixed the core code and the drivers I've worked on. Thanks, -Corey > > Corey Minyard wrote: > >> I have an IPMI interface driver that sits on top of the I2C code. I'd >> like to get it into the mainstream kernel, but I have a few problems >> to solve first before I can do that. The I2C code is synchronous and >> must run from a task context. I am not sure what you mean that the I2C code is synchronous. I2C bus is Asynchronous which means that the data clock is not included in the data. The Sender and Receiver agrees on the timing parameters prior to the bus transaction. I mean the driver, not the hardware. By the I2C driver being synchronous, I mean that you call a function, the whole operation occurs in the function, and then the function returns the result. By asynchronous, I mean that you tell the driver to start an operation and the driver starts it and immediately returns. It then uses interrupts, timers, or polling calls to drive the operation of the interface. When the operation is done, the results are reported back through a callback function provided when the driver started. Plus there are SMB alerts which the hardware can generate asynchronously. The I2C driver has no concept of that right now. The IPMI driver has certain >> operations that occur at panic time, including: >> >> * Storing panic information in IPMI's system event log >> * Extending the watchdog timer so it doesn't go off during panic >> operations (like kernel coredumps). >> * Powering the system off >> Is this driver compliant with the IPMI spec? Because the above should be a sensor that must be enable or disable. A driver should not make sure a decision by itself. There are two parts here. There is the I2C driver and the IPMI driver that sits on top of it. I'm really only taking about the I2C driver here, I need asynchronous operation from the I2C driver to do those things in the IPMI driver. >> I can't really put the IPMI SMB interface into the kernel until I can >> do those operations. Also, I understand that some vendors put RTC >> chips onto the I2C bus and this must be accessed outside task context, >> too. What the vendor put on the board doesn't matter with respect to IPMI. What matter is that you have access to the Master where the slave you talking to is connect on the I2C bus. I would really like add asynchronous interface to the I2C bus >> drivers. Do you mean a blocking and non blocking I/O? Yes, that is a better term. I'll switch to using that. -Corey - 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/
[PATCH] Add a non-blocking I2C interface
Here's the code that I have so far for adding a non-blocking interface
to the I2C interface. I've debated whether to do this as a patch or
just post the files, because the patch is about half the size of the
files. I've decided on the diff for now, it seems to be fairly
readable. This is relative to 2.6.11-rc2.
I have not extensively tested this patch. I've done eeprom operations
on the piix4 and i801 chips both with and without updating the those
interface for non-blocking operation. I'm going to test the block
transfers with the IPMI driver when I get access to a system (this
weekend). This is posted for comment.
This patch doesn't actually change much. It adds a queue entry that is
used to pass around the message data (and is obviously used for queuing
:). But this mostly is using the queue entry for data, breaking the
operations into smaller functions, and doing the polling in the i2c-core
code instead of the drivers themselves (for non-blocking capable
drivers). Function for unchanged drivers should be unchanged.
This does not implement SMB Alert, which should improve the performace
of the IPMI SMB driver. I'll do that in the future and just poll the
interface for now.
I'll post the changes to the i801 driver next.
This patch requires the fixes to the completion code that Mike Waychison
posted a few days ago
(http://marc.theaimsgroup.com/?l=linux-kernel&m=110669761400454&w=2).
Otherwise the wait_for_completion_interruptible() and
wait_for_completion_timeout() are broken.
In case you missed the previous discussion, I need a non-blocking
interface in the I2C code for use by the IPMI SMB driver. The driver
needs to be able to do things like store panic information in the SEL,
ping the watchdog timer while in a panic, and power the system off. The
current I2C driver requires a task context, but you can't exactly do
things at panic time with a task context.
-Corey
Index: linux-2.6.11-rc2/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-rc2.orig/drivers/i2c/i2c-core.c2005-01-26
15:59:53.0 -0600
+++ linux-2.6.11-rc2/drivers/i2c/i2c-core.c 2005-01-28 08:54:00.0
-0600
@@ -30,8 +30,14 @@
#include
#include
#include
+#include
#include
+static int i2c_stop_timer(struct i2c_adapter * adap);
+static void i2c_start_timer(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry);
+
+#define USEC_PER_JIFFIE (100 / HZ)
static LIST_HEAD(adapters);
static LIST_HEAD(drivers);
@@ -134,11 +140,26 @@
}
adap->nr = id & MAX_ID_MASK;
+ spin_lock_init(&adap->q_lock);
+ INIT_LIST_HEAD(&adap->q);
init_MUTEX(&adap->bus_lock);
init_MUTEX(&adap->clist_lock);
list_add_tail(&adap->list,&adapters);
INIT_LIST_HEAD(&adap->clients);
+ adap->timer = kmalloc(sizeof(*adap->timer), GFP_KERNEL);
+ if (!adap->timer) {
+ res = -ENOMEM;
+ goto out_unlock;
+ }
+
+ init_timer(&adap->timer->timer);
+ spin_lock_init(&adap->timer->lock);
+ adap->timer->deleted = 0;
+ adap->timer->running = 0;
+ adap->timer->next_call_time = 0;
+ adap->timer->adapter = adap;
+
/* Add the adapter to the driver core.
* If the parent pointer is not set up,
* we add this adapter to the host bus.
@@ -181,6 +202,7 @@
struct i2c_driver *driver;
struct i2c_client *client;
int res = 0;
+ unsigned long flags;
down(&core_lists);
@@ -233,6 +255,17 @@
device_unregister(&adap->dev);
list_del(&adap->list);
+ /* Stop the timer and free its memory */
+ spin_lock_irqsave(&adap->timer->lock, flags);
+ if (i2c_stop_timer(adap)) {
+ spin_unlock_irqrestore(&adap->timer->lock, flags);
+ kfree(adap->timer);
+ } else {
+ adap->timer->deleted = 1;
+ spin_unlock_irqrestore(&adap->timer->lock, flags);
+ }
+ adap->timer = NULL;
+
/* wait for sysfs to drop all references */
wait_for_completion(&adap->dev_released);
wait_for_completion(&adap->class_dev_released);
@@ -583,15 +616,283 @@
*
*/
-int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg msgs[],int num)
+/* Must be called with the q_lock held. */
+static void i2c_start_entry(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry)
+{
+ entry->started = 1;
+ switch (entry->xfer_type) {
+ case I2C_OP_I2C:
+ adap->algo->master_start(adap, entry);
+ break;
+ case I2C_OP_SMBUS:
+ adap->algo->smbus_start(adap, entry);
+ break;
+ default:
+ entry->result = -EINVAL;
+ i2c_op_done(adap, entry);
+ }
+
+ if
[PATCH] Updates for the i801 driver to support the I2C non-blocking interface
Here's the changes required for the i801 driver. See the previous post
for the patch to add a non-blocking interface to the I2C driver.
Like the core I2C changes, this is mostly breaking the functions into
smaller pieces and calling them from the appropriate places.
-Corey
Index: linux-2.6.11-rc2/drivers/i2c/busses/i2c-i801.c
===
--- linux-2.6.11-rc2.orig/drivers/i2c/busses/i2c-i801.c 2005-01-27
13:09:12.0 -0600
+++ linux-2.6.11-rc2/drivers/i2c/busses/i2c-i801.c 2005-01-27
13:46:23.0 -0600
@@ -79,7 +79,8 @@
#define SMBHSTCFG_I2C_EN 4
/* Other settings */
-#define MAX_TIMEOUT100
+#define MAX_TIMEOUT_US 10
+#define RETRY_TIME_US 500 /* Retry minimum is 500us */
#define ENABLE_INT90 /* set to 0x01 to enable - untested */
/* I801 command constants */
@@ -105,21 +106,31 @@
"Forcibly enable the I801 at the given address. "
"EXTREMELY DANGEROUS!");
-static int i801_transaction(void);
-static int i801_block_transaction(union i2c_smbus_data *data,
- char read_write, int command);
-
static unsigned short i801_smba;
static struct pci_dev *I801_dev;
static int isich4;
+struct i801_i2c_data
+{
+ int i;
+ int len;
+ unsigned char hostc;
+ int block;
+ int hwpec;
+ int xact;
+ int hststs;
+ int wait_intr;
+ int finished;
+};
+struct i801_i2c_data i801_data;
+
static int i801_setup(struct pci_dev *dev)
{
int error_return = 0;
unsigned char temp;
/* Note: we keep on searching until we have found 'function 3' */
- if(PCI_FUNC(dev->devfn) != 3)
+ if (PCI_FUNC(dev->devfn) != 3)
return -ENODEV;
I801_dev = dev;
@@ -136,7 +147,7 @@
} else {
pci_read_config_word(I801_dev, SMBBA, &i801_smba);
i801_smba &= 0xfff0;
- if(i801_smba == 0) {
+ if (i801_smba == 0) {
dev_err(&dev->dev, "SMB base address uninitialized"
"- upgrade BIOS or use force_addr=0xaddr\n");
return -ENODEV;
@@ -180,12 +191,93 @@
return error_return;
}
-static int i801_transaction(void)
+static void i801_check_hststs(struct i2c_adapter *adap,
+ struct i2c_op_q_entry *entry,
+ struct i801_i2c_data *d)
+{
+ if (d->hststs & 0x10) {
+ entry->result = -EIO;
+ dev_dbg(&I801_dev->dev,
+ "Error: Failed bus transaction\n");
+ } else if (d->hststs & 0x08) {
+ entry->result = -EIO;
+ dev_err(&I801_dev->dev, "Bus collision!\n");
+ /* Clock stops and slave is stuck in mid-transmission */
+ } else if (d->hststs & 0x04) {
+ entry->result = -EIO;
+ dev_dbg(&I801_dev->dev, "Error: no response!\n");
+ }
+}
+
+static void i801_finish(struct i2c_adapter *adap,
+ struct i2c_op_q_entry *entry,
+ struct i801_i2c_data *d)
+{
+ d->finished = 1;
+
+#ifdef HAVE_PEC
+ if (isich4 && d->hwpec) {
+ if (entry->smbus.size != I2C_SMBUS_QUICK &&
+ entry->smbus.size != I2C_SMBUS_I2C_BLOCK_DATA)
+ outb_p(0, SMBAUXCTL);
+ }
+#endif
+
+ if (d->block || (entry->result < 0) ||
+ ((entry->smbus.read_write == I2C_SMBUS_WRITE)
+ || (d->xact == I801_QUICK)))
+ return;
+
+ switch (d->xact & 0x7f) {
+ case I801_BYTE: /* Result put in SMBHSTDAT0 */
+ case I801_BYTE_DATA:
+ entry->smbus.data->byte = inb_p(SMBHSTDAT0);
+ break;
+ case I801_WORD_DATA:
+ entry->smbus.data->word = inb_p(SMBHSTDAT0)
+ + (inb_p(SMBHSTDAT1) << 8);
+ break;
+ }
+}
+
+static void i801_transaction_final_check(struct i2c_adapter *adap,
+struct i2c_op_q_entry *entry,
+struct i801_i2c_data *d)
+{
+ i801_check_hststs(adap, entry, d);
+
+ if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
+ outb_p(inb(SMBHSTSTS), SMBHSTSTS);
+
+ if ((d->hststs = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
+ dev_dbg(&I801_dev->dev, "Failed reset at end of transaction"
+ "(%02x)\n", d->hststs);
+ }
+ dev_dbg(&I801_dev->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
+ "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
+ inb_p(SMBHSTCMD), inb_p(SMBHSTADD), inb_p(SMBHSTDAT0),
+ inb_p(SMBHSTDAT1));
+}
+
+static void i801_transaction_poll(struct i2c_adapter *adap,
+ struct i2c_op_q_entry *entry,
+
[PATCH] Changes to the I2C driver to support a non-blocking interface
Following is a patch to add a non-blocking interface to I2C. I need
this because I need to be able to do I2C things at panic time in the
IPMI SMB driver. It should also help systems with an RTC on the I2C bus
since they need access to the RTC in situations without a task context.
-Corey
This patch adds a non-blocking interface to the I2C code. This
is needed by some RTC drivers on the I2C bus and is needed by
the IPMI code so it can do things at panic time.
The non-blocking interface requires changes to the driver
below it. The current drivers will work as-is, but will not
be able to use the non-blocking interfaces.
This is been tested with a PIIX4 and i801 driver modified to use
the asynchronous interface. The eeprom code and the IPMI SMB
driver were tested on top of it. The IPMI SMB driver was
modified to use the SMB interface, and I ran it for many
hours without problem under moderate to heavy load.
The changes are rather extensive, but are mostly reorganization
and the addition of the async interface.
This also adds back in the i2c_smbus_read_block_data() function
which is needed by the IPMI SMB driver (coming soon).
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc2/drivers/i2c/i2c-core.c
===
--- linux-2.6.11-rc2.orig/drivers/i2c/i2c-core.c
+++ linux-2.6.11-rc2/drivers/i2c/i2c-core.c
@@ -30,8 +30,14 @@
#include
#include
#include
+#include
#include
+static int i2c_stop_timer(struct i2c_adapter * adap);
+static void i2c_start_timer(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry);
+
+#define USEC_PER_JIFFIE (100 / HZ)
static LIST_HEAD(adapters);
static LIST_HEAD(drivers);
@@ -134,11 +140,26 @@
}
adap->nr = id & MAX_ID_MASK;
+ spin_lock_init(&adap->q_lock);
+ INIT_LIST_HEAD(&adap->q);
init_MUTEX(&adap->bus_lock);
init_MUTEX(&adap->clist_lock);
list_add_tail(&adap->list,&adapters);
INIT_LIST_HEAD(&adap->clients);
+ adap->timer = kmalloc(sizeof(*adap->timer), GFP_KERNEL);
+ if (!adap->timer) {
+ res = -ENOMEM;
+ goto out_unlock;
+ }
+
+ init_timer(&adap->timer->timer);
+ spin_lock_init(&adap->timer->lock);
+ adap->timer->deleted = 0;
+ adap->timer->running = 0;
+ adap->timer->next_call_time = 0;
+ adap->timer->adapter = adap;
+
/* Add the adapter to the driver core.
* If the parent pointer is not set up,
* we add this adapter to the host bus.
@@ -181,6 +202,7 @@
struct i2c_driver *driver;
struct i2c_client *client;
int res = 0;
+ unsigned long flags;
down(&core_lists);
@@ -233,6 +255,17 @@
device_unregister(&adap->dev);
list_del(&adap->list);
+ /* Stop the timer and free its memory */
+ spin_lock_irqsave(&adap->timer->lock, flags);
+ if (i2c_stop_timer(adap)) {
+ spin_unlock_irqrestore(&adap->timer->lock, flags);
+ kfree(adap->timer);
+ } else {
+ adap->timer->deleted = 1;
+ spin_unlock_irqrestore(&adap->timer->lock, flags);
+ }
+ adap->timer = NULL;
+
/* wait for sysfs to drop all references */
wait_for_completion(&adap->dev_released);
wait_for_completion(&adap->class_dev_released);
@@ -583,15 +616,287 @@
*
*/
-int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg msgs[],int num)
+#define entry_completed(e) (atomic_read(&(e)->completed) <= 0)
+
+/* Must be called with the q_lock held. */
+static void i2c_start_entry(struct i2c_adapter * adap,
+ struct i2c_op_q_entry * entry)
+{
+ entry->started = 1;
+ switch (entry->xfer_type) {
+ case I2C_OP_I2C:
+ adap->algo->master_start(adap, entry);
+ break;
+ case I2C_OP_SMBUS:
+ adap->algo->smbus_start(adap, entry);
+ break;
+ default:
+ entry->result = -EINVAL;
+ i2c_op_done(adap, entry);
+ }
+
+ if (!entry_completed(entry) && entry->use_timer)
+ i2c_start_timer(adap, entry);
+}
+
+/* Get the first entry off the head of the queue and lock it there.
+ The entry is guaranteed to remain first in the list and the handler
+ not be called until i2c_entry_put() is called. */
+static struct i2c_op_q_entry *_i2c_entry_get(struct i2c_adapter * adap)
{
- int ret;
+ struct i2c_op_q_entry * entry = NULL;
+
+ if (!list_empty(&adap->q)) {
+ struct list_head * link = adap-&
[PATCH] Minor IPMI enhancements
This patch cleans up the DMI handling so that multiple interfaces
can be reported from the DMI tables and so that the DMI slave
address can be transferred up to the upper layer. It also adds
an option to specify the slave address as an init parm and removes
some unnecessary initializers.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc2/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux-2.6.11-rc2.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.11-rc2/drivers/char/ipmi/ipmi_si_intf.c
@@ -176,6 +176,9 @@
unsigned char ipmi_version_major;
unsigned char ipmi_version_minor;
+ /* Slave address, could be reported from DMI. */
+ unsigned char slave_addr;
+
/* Counters and things for the proc filesystem. */
spinlock_t count_lock;
unsigned long short_timeouts;
@@ -407,7 +410,7 @@
/* Error fetching flags, just give up for
now. */
smi_info->si_state = SI_NORMAL;
- } else if (len < 3) {
+ } else if (len < 4) {
/* Hmm, no flags. That's technically illegal, but
don't use uninitialized data. */
smi_info->si_state = SI_NORMAL;
@@ -897,21 +900,23 @@
#define DEFAULT_REGSPACING 1
static int si_trydefaults = 1;
-static char *si_type[SI_MAX_PARMS] = { NULL, NULL, NULL, NULL };
+static char *si_type[SI_MAX_PARMS];
#define MAX_SI_TYPE_STR 30
static char si_type_str[MAX_SI_TYPE_STR];
-static unsigned long addrs[SI_MAX_PARMS] = { 0, 0, 0, 0 };
-static int num_addrs = 0;
-static unsigned int ports[SI_MAX_PARMS] = { 0, 0, 0, 0 };
-static int num_ports = 0;
-static int irqs[SI_MAX_PARMS] = { 0, 0, 0, 0 };
-static int num_irqs = 0;
-static int regspacings[SI_MAX_PARMS] = { 0, 0, 0, 0 };
+static unsigned long addrs[SI_MAX_PARMS];
+static int num_addrs;
+static unsigned int ports[SI_MAX_PARMS];
+static int num_ports;
+static int irqs[SI_MAX_PARMS];
+static int num_irqs;
+static int regspacings[SI_MAX_PARMS];
static int num_regspacings = 0;
-static int regsizes[SI_MAX_PARMS] = { 0, 0, 0, 0 };
+static int regsizes[SI_MAX_PARMS];
static int num_regsizes = 0;
-static int regshifts[SI_MAX_PARMS] = { 0, 0, 0, 0 };
+static int regshifts[SI_MAX_PARMS];
static int num_regshifts = 0;
+static int slave_addrs[SI_MAX_PARMS];
+static int num_slave_addrs = 0;
module_param_named(trydefaults, si_trydefaults, bool, 0);
@@ -955,6 +960,12 @@
" IPMI register, in bits. For instance, if the data"
" is read from a 32-bit word and the IPMI data is in"
" bit 8-15, then the shift would be 8");
+module_param_array(slave_addrs, int, &num_slave_addrs, 0);
+MODULE_PARM_DESC(slave_addrs, "Set the default IPMB slave address for"
+" the controller. Normally this is 0x20, but can be"
+" overridden by this parm. This is an array indexed"
+" by interface number.");
+
#define IPMI_MEM_ADDR_SPACE 1
#define IPMI_IO_ADDR_SPACE 2
@@ -1542,7 +1553,6 @@
#endif
#ifdef CONFIG_X86
-
typedef struct dmi_ipmi_data
{
u8 type;
@@ -1550,24 +1560,29 @@
unsigned long base_addr;
u8 irq;
u8 offset;
-}dmi_ipmi_data_t;
+ u8 slave_addr;
+} dmi_ipmi_data_t;
+
+static dmi_ipmi_data_t dmi_data[SI_MAX_DRIVERS];
+static int dmi_data_entries;
typedef struct dmi_header
{
u8 type;
u8 length;
u16 handle;
-}dmi_header_t;
+} dmi_header_t;
-static int decode_dmi(dmi_header_t *dm, dmi_ipmi_data_t *ipmi_data)
+static int decode_dmi(dmi_header_t *dm, int intf_num)
{
u8 *data = (u8 *)dm;
unsigned long base_addr;
u8 reg_spacing;
+ dmi_ipmi_data_t *ipmi_data = dmi_data+intf_num;
- ipmi_data->type = data[0x04];
+ ipmi_data->type = data[4];
- memcpy(&base_addr,&data[0x08],sizeof(unsigned long));
+ memcpy(&base_addr, data+8, sizeof(unsigned long));
if (base_addr & 1) {
/* I/O */
base_addr &= 0xFFFE;
@@ -1591,33 +1606,36 @@
ipmi_data->offset = 16;
break;
default:
- printk("ipmi_si: Unknown SMBIOS IPMI Base Addr"
- " Modifier: 0x%x\n", reg_spacing);
+ /* Some other interface, just ignore it. */
return -EIO;
}
+ ipmi_data->slave_addr = data[6];
+
/* If bit 4 of byte 0x10 is set, then the lsb for the address
[PATCH] Modify the i801 I2C driver to use the non-blocking interface.
I just posted my proposed non-blocking changes to the i2c driver. This
is the changes for the i801 driver.
This patch modifies the I801 SMBus driver to use the non-blocking
interface.
Signed-off-by: Corey Minyard <[EMAIL PROTECTED]>
Index: linux-2.6.11-rc2/drivers/i2c/busses/i2c-i801.c
===
--- linux-2.6.11-rc2.orig/drivers/i2c/busses/i2c-i801.c
+++ linux-2.6.11-rc2/drivers/i2c/busses/i2c-i801.c
@@ -40,6 +40,14 @@
/* Note: we assume there can only be one I801, with one SMBus interface */
+/* Another note: This interface is extremely sensitive to timing and
+ failure handling. If you don't wait at least one jiffie after
+ starting the transaction before checking things, you will screw it
+ up. If you don't wait a jiffie after the final check, you will
+ screw it up. If you screw it up by these manners or by abandoning
+ an operation in progress, the I2C bus is likely stuck and won't
+ work any more. Gotta love this hardware. */
+
#include
#include
#include
@@ -79,7 +87,8 @@
#define SMBHSTCFG_I2C_EN 4
/* Other settings */
-#define MAX_TIMEOUT100
+#define MAX_TIMEOUT_US 10
+#define RETRY_TIME_US 500 /* Retry minimum is 500us */
#define ENABLE_INT90 /* set to 0x01 to enable - untested */
/* I801 command constants */
@@ -105,21 +114,35 @@
"Forcibly enable the I801 at the given address. "
"EXTREMELY DANGEROUS!");
-static int i801_transaction(void);
-static int i801_block_transaction(union i2c_smbus_data *data,
- char read_write, int command);
-
static unsigned short i801_smba;
static struct pci_dev *I801_dev;
static int isich4;
+struct i801_i2c_data
+{
+ int i;
+ int len;
+ unsigned char hostc;
+ int block;
+ int hwpec;
+ int xact;
+ int hststs;
+ int wait_intr;
+ int finished;
+
+ /* Used to handle removal race conditions. */
+ int in_removal;
+ int in_use;
+};
+struct i801_i2c_data i801_data;
+
static int i801_setup(struct pci_dev *dev)
{
int error_return = 0;
unsigned char temp;
/* Note: we keep on searching until we have found 'function 3' */
- if(PCI_FUNC(dev->devfn) != 3)
+ if (PCI_FUNC(dev->devfn) != 3)
return -ENODEV;
I801_dev = dev;
@@ -136,7 +159,7 @@
} else {
pci_read_config_word(I801_dev, SMBBA, &i801_smba);
i801_smba &= 0xfff0;
- if(i801_smba == 0) {
+ if (i801_smba == 0) {
dev_err(&dev->dev, "SMB base address uninitialized"
"- upgrade BIOS or use force_addr=0xaddr\n");
return -ENODEV;
@@ -180,12 +203,93 @@
return error_return;
}
-static int i801_transaction(void)
+static void i801_check_hststs(struct i2c_adapter *adap,
+ struct i2c_op_q_entry *entry,
+ struct i801_i2c_data *d)
+{
+ if (d->hststs & 0x10) {
+ entry->result = -EIO;
+ dev_dbg(&I801_dev->dev,
+ "Error: Failed bus transaction\n");
+ } else if (d->hststs & 0x08) {
+ entry->result = -EIO;
+ dev_err(&I801_dev->dev, "Bus collision!\n");
+ /* Clock stops and slave is stuck in mid-transmission */
+ } else if (d->hststs & 0x04) {
+ entry->result = -EIO;
+ dev_dbg(&I801_dev->dev, "Error: no response!\n");
+ }
+}
+
+static void i801_finish(struct i2c_adapter *adap,
+ struct i2c_op_q_entry *entry,
+ struct i801_i2c_data *d)
+{
+ d->finished = 1;
+
+#ifdef HAVE_PEC
+ if (isich4 && d->hwpec) {
+ if (entry->smbus.size != I2C_SMBUS_QUICK &&
+ entry->smbus.size != I2C_SMBUS_I2C_BLOCK_DATA)
+ outb_p(0, SMBAUXCTL);
+ }
+#endif
+
+ if (d->block || (entry->result < 0) ||
+ ((entry->smbus.read_write == I2C_SMBUS_WRITE)
+ || (d->xact == I801_QUICK)))
+ return;
+
+ switch (d->xact & 0x7f) {
+ case I801_BYTE: /* Result put in SMBHSTDAT0 */
+ case I801_BYTE_DATA:
+ entry->smbus.data->byte = inb_p(SMBHSTDAT0);
+ break;
+ case I801_WORD_DATA:
+ entry->smbus.data->word = inb_p(SMBHSTDAT0)
+ + (inb_p(SMBHSTDAT1) << 8);
+ break;
+ }
+}
+
+static void i801_transaction_final_check(struct i2c_adapter *adap,
+struct i2c_op_q
