I finally got some time to think this through and test it.
This patch is in addition to the ipmi-use-refcount-in-message-handler
patch.
Thanks again, Paul, for seeing this.
Use rcu_read_lock for the cmd_rcvrs list, since that was what
what intended, anyway. This means that all the users of the
cmd_rcvrs_lock are tasks, so the irq disables are no longer
required for that lock and it can become a semaphore.
Signed-off-by: Corey Minyard <[email protected]>
Index: linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c
===================================================================
--- linux-2.6.14-rc4.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c
@@ -209,7 +209,7 @@ struct ipmi_smi
/* The list of command receivers that are registered for commands
on this interface. */
- spinlock_t cmd_rcvrs_lock;
+ struct semaphore cmd_rcvrs_lock;
struct list_head cmd_rcvrs;
/* Events that were queues because no one was there to receive
@@ -345,7 +345,6 @@ static void clean_up_interface_data(ipmi
{
int i;
struct cmd_rcvr *rcvr, *rcvr2;
- unsigned long flags;
struct list_head list;
free_recv_msg_list(&intf->waiting_msgs);
@@ -353,10 +352,10 @@ static void clean_up_interface_data(ipmi
/* Wholesale remove all the entries from the list in the
* interface and wait for RCU to know that none are in use. */
- spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
+ down(&intf->cmd_rcvrs_lock);
list_add_rcu(&list, &intf->cmd_rcvrs);
list_del_rcu(&intf->cmd_rcvrs);
- spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
+ up(&intf->cmd_rcvrs_lock);
synchronize_rcu();
list_for_each_entry_safe(rcvr, rcvr2, &list, link)
@@ -812,7 +811,7 @@ int ipmi_destroy_user(ipmi_user_t user)
* since other things may be using it till we do
* synchronize_rcu()) then free everything in that list.
*/
- spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
+ down(&intf->cmd_rcvrs_lock);
list_for_each_safe_rcu(entry1, entry2, &intf->cmd_rcvrs) {
rcvr = list_entry(entry1, struct cmd_rcvr, link);
if (rcvr->user == user) {
@@ -821,7 +820,7 @@ int ipmi_destroy_user(ipmi_user_t user)
rcvrs = rcvr;
}
}
- spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
+ up(&intf->cmd_rcvrs_lock);
synchronize_rcu();
while (rcvrs) {
rcvr = rcvrs;
@@ -950,7 +949,7 @@ int ipmi_register_for_cmd(ipmi_user_t
rcvr->netfn = netfn;
rcvr->user = user;
- spin_lock_irq(&intf->cmd_rcvrs_lock);
+ down(&intf->cmd_rcvrs_lock);
/* Make sure the command/netfn is not already registered. */
entry = find_cmd_rcvr(intf, netfn, cmd);
if (entry) {
@@ -961,7 +960,7 @@ int ipmi_register_for_cmd(ipmi_user_t
list_add_rcu(&rcvr->link, &intf->cmd_rcvrs);
out_unlock:
- spin_unlock_irq(&intf->cmd_rcvrs_lock);
+ up(&intf->cmd_rcvrs_lock);
if (rv)
kfree(rcvr);
@@ -975,17 +974,17 @@ int ipmi_unregister_for_cmd(ipmi_user_t
ipmi_smi_t intf = user->intf;
struct cmd_rcvr *rcvr;
- spin_lock_irq(&intf->cmd_rcvrs_lock);
+ down(&intf->cmd_rcvrs_lock);
/* Make sure the command/netfn is not already registered. */
rcvr = find_cmd_rcvr(intf, netfn, cmd);
if ((rcvr) && (rcvr->user == user)) {
list_del_rcu(&rcvr->link);
- spin_unlock_irq(&intf->cmd_rcvrs_lock);
+ up(&intf->cmd_rcvrs_lock);
synchronize_rcu();
kfree(rcvr);
return 0;
} else {
- spin_unlock_irq(&intf->cmd_rcvrs_lock);
+ up(&intf->cmd_rcvrs_lock);
return -ENOENT;
}
}
@@ -1858,7 +1857,7 @@ int ipmi_register_smi(struct ipmi_smi_ha
spin_lock_init(&intf->events_lock);
INIT_LIST_HEAD(&intf->waiting_events);
intf->waiting_events_count = 0;
- spin_lock_init(&intf->cmd_rcvrs_lock);
+ init_MUTEX(&intf->cmd_rcvrs_lock);
INIT_LIST_HEAD(&intf->cmd_rcvrs);
init_waitqueue_head(&intf->waitq);
@@ -2058,14 +2057,14 @@ static int handle_ipmb_get_msg_cmd(ipmi_
netfn = msg->rsp[4] >> 2;
cmd = msg->rsp[8];
- spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
+ rcu_read_lock();
rcvr = find_cmd_rcvr(intf, netfn, cmd);
if (rcvr) {
user = rcvr->user;
kref_get(&user->refcount);
} else
user = NULL;
- spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
+ rcu_read_unlock();
if (user == NULL) {
/* We didn't find a user, deliver an error response. */
@@ -2238,14 +2237,14 @@ static int handle_lan_get_msg_cmd(ipmi_s
netfn = msg->rsp[6] >> 2;
cmd = msg->rsp[10];
- spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
+ rcu_read_lock();
rcvr = find_cmd_rcvr(intf, netfn, cmd);
if (rcvr) {
user = rcvr->user;
kref_get(&user->refcount);
} else
user = NULL;
- spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
+ rcu_read_unlock();
if (user == NULL) {
/* We didn't find a user, just give up. */
On Fri, Oct 28, 2005 at 08:19:44AM -0500, Corey Minyard wrote:
> I finally got some time to think this through and test it.
> This patch is in addition to the ipmi-use-refcount-in-message-handler
> patch.
> Thanks again, Paul, for seeing this.
>
> Use rcu_read_lock for the cmd_rcvrs list, since that was what
> what intended, anyway. This means that all the users of the
> cmd_rcvrs_lock are tasks, so the irq disables are no longer
> required for that lock and it can become a semaphore.
Looks good from an RCU viewpoint! I presume that find_cmd_rcvr()
still includes an rcu_dereference().
Thanx, Paul
Acked-by: <[email protected]>
> Signed-off-by: Corey Minyard <[email protected]>
>
> Index: linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/drivers/char/ipmi/ipmi_msghandler.c
> +++ linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c
> @@ -209,7 +209,7 @@ struct ipmi_smi
>
> /* The list of command receivers that are registered for commands
> on this interface. */
> - spinlock_t cmd_rcvrs_lock;
> + struct semaphore cmd_rcvrs_lock;
> struct list_head cmd_rcvrs;
>
> /* Events that were queues because no one was there to receive
> @@ -345,7 +345,6 @@ static void clean_up_interface_data(ipmi
> {
> int i;
> struct cmd_rcvr *rcvr, *rcvr2;
> - unsigned long flags;
> struct list_head list;
>
> free_recv_msg_list(&intf->waiting_msgs);
> @@ -353,10 +352,10 @@ static void clean_up_interface_data(ipmi
>
> /* Wholesale remove all the entries from the list in the
> * interface and wait for RCU to know that none are in use. */
> - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
> + down(&intf->cmd_rcvrs_lock);
> list_add_rcu(&list, &intf->cmd_rcvrs);
> list_del_rcu(&intf->cmd_rcvrs);
> - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
> + up(&intf->cmd_rcvrs_lock);
> synchronize_rcu();
>
> list_for_each_entry_safe(rcvr, rcvr2, &list, link)
> @@ -812,7 +811,7 @@ int ipmi_destroy_user(ipmi_user_t user)
> * since other things may be using it till we do
> * synchronize_rcu()) then free everything in that list.
> */
> - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
> + down(&intf->cmd_rcvrs_lock);
> list_for_each_safe_rcu(entry1, entry2, &intf->cmd_rcvrs) {
> rcvr = list_entry(entry1, struct cmd_rcvr, link);
> if (rcvr->user == user) {
> @@ -821,7 +820,7 @@ int ipmi_destroy_user(ipmi_user_t user)
> rcvrs = rcvr;
> }
> }
> - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
> + up(&intf->cmd_rcvrs_lock);
> synchronize_rcu();
> while (rcvrs) {
> rcvr = rcvrs;
> @@ -950,7 +949,7 @@ int ipmi_register_for_cmd(ipmi_user_t
> rcvr->netfn = netfn;
> rcvr->user = user;
>
> - spin_lock_irq(&intf->cmd_rcvrs_lock);
> + down(&intf->cmd_rcvrs_lock);
> /* Make sure the command/netfn is not already registered. */
> entry = find_cmd_rcvr(intf, netfn, cmd);
> if (entry) {
> @@ -961,7 +960,7 @@ int ipmi_register_for_cmd(ipmi_user_t
> list_add_rcu(&rcvr->link, &intf->cmd_rcvrs);
>
> out_unlock:
> - spin_unlock_irq(&intf->cmd_rcvrs_lock);
> + up(&intf->cmd_rcvrs_lock);
> if (rv)
> kfree(rcvr);
>
> @@ -975,17 +974,17 @@ int ipmi_unregister_for_cmd(ipmi_user_t
> ipmi_smi_t intf = user->intf;
> struct cmd_rcvr *rcvr;
>
> - spin_lock_irq(&intf->cmd_rcvrs_lock);
> + down(&intf->cmd_rcvrs_lock);
> /* Make sure the command/netfn is not already registered. */
> rcvr = find_cmd_rcvr(intf, netfn, cmd);
> if ((rcvr) && (rcvr->user == user)) {
> list_del_rcu(&rcvr->link);
> - spin_unlock_irq(&intf->cmd_rcvrs_lock);
> + up(&intf->cmd_rcvrs_lock);
> synchronize_rcu();
> kfree(rcvr);
> return 0;
> } else {
> - spin_unlock_irq(&intf->cmd_rcvrs_lock);
> + up(&intf->cmd_rcvrs_lock);
> return -ENOENT;
> }
> }
> @@ -1858,7 +1857,7 @@ int ipmi_register_smi(struct ipmi_smi_ha
> spin_lock_init(&intf->events_lock);
> INIT_LIST_HEAD(&intf->waiting_events);
> intf->waiting_events_count = 0;
> - spin_lock_init(&intf->cmd_rcvrs_lock);
> + init_MUTEX(&intf->cmd_rcvrs_lock);
> INIT_LIST_HEAD(&intf->cmd_rcvrs);
> init_waitqueue_head(&intf->waitq);
>
> @@ -2058,14 +2057,14 @@ static int handle_ipmb_get_msg_cmd(ipmi_
> netfn = msg->rsp[4] >> 2;
> cmd = msg->rsp[8];
>
> - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
> + rcu_read_lock();
> rcvr = find_cmd_rcvr(intf, netfn, cmd);
> if (rcvr) {
> user = rcvr->user;
> kref_get(&user->refcount);
> } else
> user = NULL;
> - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
> + rcu_read_unlock();
>
> if (user == NULL) {
> /* We didn't find a user, deliver an error response. */
> @@ -2238,14 +2237,14 @@ static int handle_lan_get_msg_cmd(ipmi_s
> netfn = msg->rsp[6] >> 2;
> cmd = msg->rsp[10];
>
> - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags);
> + rcu_read_lock();
> rcvr = find_cmd_rcvr(intf, netfn, cmd);
> if (rcvr) {
> user = rcvr->user;
> kref_get(&user->refcount);
> } else
> user = NULL;
> - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags);
> + rcu_read_unlock();
>
> if (user == NULL) {
> /* We didn't find a user, just give up. */
>