From 9e91f8a6c8688e27f4ccce7db457da87d6458836 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 18 Mar 2025 19:57:42 -0500 Subject: [PATCH 01/16] ipmi:msghandler: Remove srcu for the ipmi_interfaces list With reworks srcu is no longer necessary, this simplifies locking a lot. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 194 +++++++++++++++------------- 1 file changed, 102 insertions(+), 92 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 0dc32319f7ba..c7cc885e12fa 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -628,7 +628,6 @@ static DEFINE_MUTEX(ipmidriver_mutex); static LIST_HEAD(ipmi_interfaces); static DEFINE_MUTEX(ipmi_interfaces_mutex); -static struct srcu_struct ipmi_interfaces_srcu; /* * List of watchers that want to know when smi's are added and deleted. @@ -694,28 +693,20 @@ static void free_smi_msg_list(struct list_head *q) } } -static void clean_up_interface_data(struct ipmi_smi *intf) +static void intf_free(struct kref *ref) { + struct ipmi_smi *intf = container_of(ref, struct ipmi_smi, refcount); int i; struct cmd_rcvr *rcvr, *rcvr2; - struct list_head list; - - cancel_work_sync(&intf->smi_work); - /* smi_work() can no longer be in progress after this. */ free_smi_msg_list(&intf->waiting_rcv_msgs); free_recv_msg_list(&intf->waiting_events); /* * Wholesale remove all the entries from the list in the - * interface. + * interface. No need for locks, this is single-threaded. */ - mutex_lock(&intf->cmd_rcvrs_mutex); - INIT_LIST_HEAD(&list); - list_splice_init_rcu(&intf->cmd_rcvrs, &list, synchronize_rcu); - mutex_unlock(&intf->cmd_rcvrs_mutex); - - list_for_each_entry_safe(rcvr, rcvr2, &list, link) + list_for_each_entry_safe(rcvr, rcvr2, &intf->cmd_rcvrs, link) kfree(rcvr); for (i = 0; i < IPMI_IPMB_NUM_SEQ; i++) { @@ -723,20 +714,17 @@ static void clean_up_interface_data(struct ipmi_smi *intf) && (intf->seq_table[i].recv_msg)) ipmi_free_recv_msg(intf->seq_table[i].recv_msg); } -} -static void intf_free(struct kref *ref) -{ - struct ipmi_smi *intf = container_of(ref, struct ipmi_smi, refcount); - - clean_up_interface_data(intf); kfree(intf); } int ipmi_smi_watcher_register(struct ipmi_smi_watcher *watcher) { struct ipmi_smi *intf; - int index, rv; + unsigned int count = 0, i; + int *interfaces = NULL; + struct device **devices = NULL; + int rv = 0; /* * Make sure the driver is actually initialized, this handles @@ -750,20 +738,53 @@ int ipmi_smi_watcher_register(struct ipmi_smi_watcher *watcher) list_add(&watcher->link, &smi_watchers); - index = srcu_read_lock(&ipmi_interfaces_srcu); - list_for_each_entry_rcu(intf, &ipmi_interfaces, link, - lockdep_is_held(&smi_watchers_mutex)) { - int intf_num = READ_ONCE(intf->intf_num); + /* + * Build an array of ipmi interfaces and fill it in, and + * another array of the devices. We can't call the callback + * with ipmi_interfaces_mutex held. smi_watchers_mutex will + * keep things in order for the user. + */ + mutex_lock(&ipmi_interfaces_mutex); + list_for_each_entry(intf, &ipmi_interfaces, link) + count++; + if (count > 0) { + interfaces = kmalloc_array(count, sizeof(*interfaces), + GFP_KERNEL); + if (!interfaces) { + rv = -ENOMEM; + } else { + devices = kmalloc_array(count, sizeof(*devices), + GFP_KERNEL); + if (!devices) { + kfree(interfaces); + interfaces = NULL; + rv = -ENOMEM; + } + } + count = 0; + } + if (interfaces) { + list_for_each_entry(intf, &ipmi_interfaces, link) { + int intf_num = READ_ONCE(intf->intf_num); - if (intf_num == -1) - continue; - watcher->new_smi(intf_num, intf->si_dev); + if (intf_num == -1) + continue; + devices[count] = intf->si_dev; + interfaces[count++] = intf_num; + } + } + mutex_unlock(&ipmi_interfaces_mutex); + + if (interfaces) { + for (i = 0; i < count; i++) + watcher->new_smi(interfaces[i], devices[i]); + kfree(interfaces); + kfree(devices); } - srcu_read_unlock(&ipmi_interfaces_srcu, index); mutex_unlock(&smi_watchers_mutex); - return 0; + return rv; } EXPORT_SYMBOL(ipmi_smi_watcher_register); @@ -781,14 +802,12 @@ call_smi_watchers(int i, struct device *dev) { struct ipmi_smi_watcher *w; - mutex_lock(&smi_watchers_mutex); list_for_each_entry(w, &smi_watchers, link) { if (try_module_get(w->owner)) { w->new_smi(i, dev); module_put(w->owner); } } - mutex_unlock(&smi_watchers_mutex); } static int @@ -1195,7 +1214,7 @@ int ipmi_create_user(unsigned int if_num, { unsigned long flags; struct ipmi_user *new_user = NULL; - int rv, index; + int rv = 0; struct ipmi_smi *intf; /* @@ -1217,8 +1236,8 @@ int ipmi_create_user(unsigned int if_num, if (rv) return rv; - index = srcu_read_lock(&ipmi_interfaces_srcu); - list_for_each_entry_rcu(intf, &ipmi_interfaces, link) { + mutex_lock(&ipmi_interfaces_mutex); + list_for_each_entry(intf, &ipmi_interfaces, link) { if (intf->intf_num == if_num) goto found; } @@ -1260,45 +1279,44 @@ int ipmi_create_user(unsigned int if_num, new_user->intf = intf; new_user->gets_events = false; + mutex_lock(&intf->users_mutex); spin_lock_irqsave(&intf->seq_lock, flags); list_add(&new_user->link, &intf->users); spin_unlock_irqrestore(&intf->seq_lock, flags); + mutex_unlock(&intf->users_mutex); + if (handler->ipmi_watchdog_pretimeout) /* User wants pretimeouts, so make sure to watch for them. */ smi_add_watch(intf, IPMI_WATCH_MASK_CHECK_WATCHDOG); - srcu_read_unlock(&ipmi_interfaces_srcu, index); - *user = new_user; - return 0; out_kfree: - atomic_dec(&intf->nr_users); - srcu_read_unlock(&ipmi_interfaces_srcu, index); - vfree(new_user); + if (rv) { + atomic_dec(&intf->nr_users); + vfree(new_user); + } else { + *user = new_user; + } + mutex_unlock(&ipmi_interfaces_mutex); return rv; } EXPORT_SYMBOL(ipmi_create_user); int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data) { - int rv, index; + int rv = -EINVAL; struct ipmi_smi *intf; - index = srcu_read_lock(&ipmi_interfaces_srcu); - list_for_each_entry_rcu(intf, &ipmi_interfaces, link) { - if (intf->intf_num == if_num) - goto found; + mutex_lock(&ipmi_interfaces_mutex); + list_for_each_entry(intf, &ipmi_interfaces, link) { + if (intf->intf_num == if_num) { + if (!intf->handlers->get_smi_info) + rv = -ENOTTY; + else + rv = intf->handlers->get_smi_info(intf->send_info, data); + break; + } } - srcu_read_unlock(&ipmi_interfaces_srcu, index); - - /* Not found, return an error */ - return -EINVAL; - -found: - if (!intf->handlers->get_smi_info) - rv = -ENOTTY; - else - rv = intf->handlers->get_smi_info(intf->send_info, data); - srcu_read_unlock(&ipmi_interfaces_srcu, index); + mutex_unlock(&ipmi_interfaces_mutex); return rv; } @@ -1345,7 +1363,7 @@ static void _ipmi_destroy_user(struct ipmi_user *user) * Remove the user from the command receiver's table. First * we build a list of everything (not using the standard link, * since other things may be using it till we do - * synchronize_srcu()) then free everything in that list. + * synchronize_rcu()) then free everything in that list. */ mutex_lock(&intf->cmd_rcvrs_mutex); list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, @@ -1357,7 +1375,6 @@ static void _ipmi_destroy_user(struct ipmi_user *user) } } mutex_unlock(&intf->cmd_rcvrs_mutex); - synchronize_rcu(); while (rcvrs) { rcvr = rcvrs; rcvrs = rcvr->next; @@ -2298,7 +2315,7 @@ static int i_ipmi_request(struct ipmi_user *user, } } - rcu_read_lock(); + mutex_lock(&ipmi_interfaces_mutex); if (intf->in_shutdown) { rv = -ENODEV; goto out_err; @@ -2344,7 +2361,7 @@ out_err: smi_send(intf, intf->handlers, smi_msg, priority); } - rcu_read_unlock(); + mutex_unlock(&ipmi_interfaces_mutex); out: if (rv && user) @@ -3578,12 +3595,16 @@ int ipmi_add_smi(struct module *owner, for (i = 0; i < IPMI_NUM_STATS; i++) atomic_set(&intf->stats[i], 0); + /* + * Grab the watchers mutex so we can deliver the new interface + * without races. + */ + mutex_lock(&smi_watchers_mutex); mutex_lock(&ipmi_interfaces_mutex); /* Look for a hole in the numbers. */ i = 0; link = &ipmi_interfaces; - list_for_each_entry_rcu(tintf, &ipmi_interfaces, link, - ipmi_interfaces_mutex_held()) { + list_for_each_entry(tintf, &ipmi_interfaces, link) { if (tintf->intf_num != i) { link = &tintf->link; break; @@ -3592,9 +3613,9 @@ int ipmi_add_smi(struct module *owner, } /* Add the new interface in numeric order. */ if (i == 0) - list_add_rcu(&intf->link, &ipmi_interfaces); + list_add(&intf->link, &ipmi_interfaces); else - list_add_tail_rcu(&intf->link, link); + list_add_tail(&intf->link, link); rv = handlers->start_processing(send_info, intf); if (rv) @@ -3626,18 +3647,14 @@ int ipmi_add_smi(struct module *owner, goto out_err_bmc_reg; } - /* - * Keep memory order straight for RCU readers. Make - * sure everything else is committed to memory before - * setting intf_num to mark the interface valid. - */ - smp_wmb(); intf->intf_num = i; mutex_unlock(&ipmi_interfaces_mutex); /* After this point the interface is legal to use. */ call_smi_watchers(i, intf->si_dev); + mutex_unlock(&smi_watchers_mutex); + return 0; out_err_bmc_reg: @@ -3646,9 +3663,9 @@ int ipmi_add_smi(struct module *owner, if (intf->handlers->shutdown) intf->handlers->shutdown(intf->send_info); out_err: - list_del_rcu(&intf->link); + list_del(&intf->link); mutex_unlock(&ipmi_interfaces_mutex); - synchronize_srcu(&ipmi_interfaces_srcu); + mutex_unlock(&smi_watchers_mutex); kref_put(&intf->refcount, intf_free); return rv; @@ -3718,13 +3735,16 @@ void ipmi_unregister_smi(struct ipmi_smi *intf) if (!intf) return; + intf_num = intf->intf_num; mutex_lock(&ipmi_interfaces_mutex); + cancel_work_sync(&intf->smi_work); + /* smi_work() can no longer be in progress after this. */ + intf->intf_num = -1; intf->in_shutdown = true; - list_del_rcu(&intf->link); + list_del(&intf->link); mutex_unlock(&ipmi_interfaces_mutex); - synchronize_srcu(&ipmi_interfaces_srcu); /* At this point no users can be added to the interface. */ @@ -3880,7 +3900,7 @@ static int handle_ipmb_get_msg_cmd(struct ipmi_smi *intf, dev_dbg(intf->si_dev, "Invalid command: %*ph\n", msg->data_size, msg->data); - rcu_read_lock(); + mutex_lock(&ipmi_interfaces_mutex); if (!intf->in_shutdown) { smi_send(intf, intf->handlers, msg, 0); /* @@ -3890,7 +3910,7 @@ static int handle_ipmb_get_msg_cmd(struct ipmi_smi *intf, */ rv = -1; } - rcu_read_unlock(); + mutex_unlock(&ipmi_interfaces_mutex); } else { recv_msg = ipmi_alloc_recv_msg(); if (!recv_msg) { @@ -3971,7 +3991,7 @@ static int handle_ipmb_direct_rcv_cmd(struct ipmi_smi *intf, msg->data[4] = IPMI_INVALID_CMD_COMPLETION_CODE; msg->data_size = 5; - rcu_read_lock(); + mutex_lock(&ipmi_interfaces_mutex); if (!intf->in_shutdown) { smi_send(intf, intf->handlers, msg, 0); /* @@ -3981,7 +4001,7 @@ static int handle_ipmb_direct_rcv_cmd(struct ipmi_smi *intf, */ rv = -1; } - rcu_read_unlock(); + mutex_unlock(&ipmi_interfaces_mutex); } else { recv_msg = ipmi_alloc_recv_msg(); if (!recv_msg) { @@ -5053,13 +5073,12 @@ static void ipmi_timeout_work(struct work_struct *work) struct ipmi_smi *intf; bool need_timer = false; - int index; if (atomic_read(&stop_operation)) return; - index = srcu_read_lock(&ipmi_interfaces_srcu); - list_for_each_entry_rcu(intf, &ipmi_interfaces, link) { + mutex_lock(&ipmi_interfaces_mutex); + list_for_each_entry(intf, &ipmi_interfaces, link) { if (atomic_read(&intf->event_waiters)) { intf->ticks_to_req_ev--; if (intf->ticks_to_req_ev == 0) { @@ -5071,7 +5090,7 @@ static void ipmi_timeout_work(struct work_struct *work) need_timer |= ipmi_timeout_handler(intf, IPMI_TIMEOUT_TIME); } - srcu_read_unlock(&ipmi_interfaces_srcu, index); + mutex_unlock(&ipmi_interfaces_mutex); if (need_timer) mod_timer(&ipmi_timer, jiffies + IPMI_TIMEOUT_JIFFIES); @@ -5453,15 +5472,11 @@ static int ipmi_init_msghandler(void) if (initialized) goto out; - rv = init_srcu_struct(&ipmi_interfaces_srcu); - if (rv) - goto out; - bmc_remove_work_wq = create_singlethread_workqueue("ipmi-msghandler-remove-wq"); if (!bmc_remove_work_wq) { pr_err("unable to create ipmi-msghandler-remove-wq workqueue"); rv = -ENOMEM; - goto out_wq; + goto out; } timer_setup(&ipmi_timer, ipmi_timeout, 0); @@ -5471,9 +5486,6 @@ static int ipmi_init_msghandler(void) initialized = true; -out_wq: - if (rv) - cleanup_srcu_struct(&ipmi_interfaces_srcu); out: mutex_unlock(&ipmi_interfaces_mutex); return rv; @@ -5525,8 +5537,6 @@ static void __exit cleanup_ipmi(void) count = atomic_read(&recv_msg_inuse_count); if (count != 0) pr_warn("recv message count %d at exit\n", count); - - cleanup_srcu_struct(&ipmi_interfaces_srcu); } if (drvregistered) driver_unregister(&ipmidriver.driver); -- 2.51.0 From b2b69ee34b07579fc7c33e2c62a88f19514b052f Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 18 Mar 2025 20:13:34 -0500 Subject: [PATCH 02/16] ipmi:watchdog: Change lock to mutex Now that the msghandler does all callbacks in user threads, there is no need to have a lock any more, a mutex will work fine. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_watchdog.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index f1875b2bebbc..01c10bd5f099 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -150,7 +150,7 @@ static char preaction[16] = "pre_none"; static unsigned char preop_val = WDOG_PREOP_NONE; static char preop[16] = "preop_none"; -static DEFINE_SPINLOCK(ipmi_read_lock); +static DEFINE_MUTEX(ipmi_read_mutex); static char data_to_read; static DECLARE_WAIT_QUEUE_HEAD(read_q); static struct fasync_struct *fasync_q; @@ -793,7 +793,7 @@ static ssize_t ipmi_read(struct file *file, * Reading returns if the pretimeout has gone off, and it only does * it once per pretimeout. */ - spin_lock_irq(&ipmi_read_lock); + mutex_lock(&ipmi_read_mutex); if (!data_to_read) { if (file->f_flags & O_NONBLOCK) { rv = -EAGAIN; @@ -804,9 +804,9 @@ static ssize_t ipmi_read(struct file *file, add_wait_queue(&read_q, &wait); while (!data_to_read && !signal_pending(current)) { set_current_state(TASK_INTERRUPTIBLE); - spin_unlock_irq(&ipmi_read_lock); + mutex_unlock(&ipmi_read_mutex); schedule(); - spin_lock_irq(&ipmi_read_lock); + mutex_lock(&ipmi_read_mutex); } remove_wait_queue(&read_q, &wait); @@ -818,7 +818,7 @@ static ssize_t ipmi_read(struct file *file, data_to_read = 0; out: - spin_unlock_irq(&ipmi_read_lock); + mutex_unlock(&ipmi_read_mutex); if (rv == 0) { if (copy_to_user(buf, &data_to_read, 1)) @@ -856,10 +856,10 @@ static __poll_t ipmi_poll(struct file *file, poll_table *wait) poll_wait(file, &read_q, wait); - spin_lock_irq(&ipmi_read_lock); + mutex_lock(&ipmi_read_mutex); if (data_to_read) mask |= (EPOLLIN | EPOLLRDNORM); - spin_unlock_irq(&ipmi_read_lock); + mutex_unlock(&ipmi_read_mutex); return mask; } @@ -932,13 +932,11 @@ static void ipmi_wdog_pretimeout_handler(void *handler_data) if (atomic_inc_and_test(&preop_panic_excl)) panic("Watchdog pre-timeout"); } else if (preop_val == WDOG_PREOP_GIVE_DATA) { - unsigned long flags; - - spin_lock_irqsave(&ipmi_read_lock, flags); + mutex_lock(&ipmi_read_mutex); data_to_read = 1; wake_up_interruptible(&read_q); kill_fasync(&fasync_q, SIGIO, POLL_IN); - spin_unlock_irqrestore(&ipmi_read_lock, flags); + mutex_unlock(&ipmi_read_mutex); } } -- 2.51.0 From 5017b1b02640234b08ad38a046043f143670dea2 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 18 Mar 2025 20:41:42 -0500 Subject: [PATCH 03/16] ipmi: Add a note about the pretimeout callback You can't do IPMI calls from the callback, it's called with locks held. Signed-off-by: Corey Minyard --- include/linux/ipmi.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h index 2f74dd90c271..27cd5980bb27 100644 --- a/include/linux/ipmi.h +++ b/include/linux/ipmi.h @@ -93,7 +93,8 @@ struct ipmi_user_hndl { /* * Called when the interface detects a watchdog pre-timeout. If - * this is NULL, it will be ignored for the user. + * this is NULL, it will be ignored for the user. Note that you + * can't do any IPMI calls from here, it's called with locks held. */ void (*ipmi_watchdog_pretimeout)(void *handler_data); -- 2.51.0 From 83d19f03f3e5e1421d7cda78d0bec80e1769e8aa Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 18 Mar 2025 21:46:05 -0500 Subject: [PATCH 04/16] ipmi:msghandler: Remove some user level processing in panic mode When run to completion is set, don't call things that will claim mutexes or call user callbacks. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index c7cc885e12fa..f40f281b46b3 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -4808,6 +4808,10 @@ static void smi_work(struct work_struct *t) handle_new_recv_msgs(intf); + /* Nothing below applies during panic time. */ + if (run_to_completion) + return; + /* * If the pretimout count is non-zero, decrement one from it and * deliver pretimeouts to all the users. -- 2.51.0 From 84fe1ebcc92cf3880130bfe51040769d7931423a Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 21 Mar 2025 14:46:46 -0500 Subject: [PATCH 05/16] ipmi:msghandler: Fix locking around users and interfaces Now that SRCU is gone from IPMI, it can no longer be sloppy about locking. Use the users mutex now when sending a message, not the big ipmi_interfaces mutex, because it can result in a recursive lock. The users mutex will work because the interface destroy code claims it after setting the interface in shutdown mode. Also, due to the same changes, rework the refcounting on users and interfaces. Remove the refcount to an interface when the user is freed, not when it is destroyed. If the interface is destroyed while the user still exists, the user will still point to the interface to test that it is valid if the user tries to do anything but delete the user. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 51 ++++++++++++++--------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index f40f281b46b3..c7533a2846a6 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -46,6 +46,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf); static void need_waiter(struct ipmi_smi *intf); static int handle_one_recv_msg(struct ipmi_smi *intf, struct ipmi_smi_msg *msg); +static void intf_free(struct kref *ref); static bool initialized; static bool drvregistered; @@ -196,25 +197,6 @@ struct ipmi_user { atomic_t nr_msgs; }; -static void free_ipmi_user(struct kref *ref) -{ - struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); - - vfree(user); -} - -static void release_ipmi_user(struct ipmi_user *user) -{ - kref_put(&user->refcount, free_ipmi_user); -} - -static struct ipmi_user *acquire_ipmi_user(struct ipmi_user *user) -{ - if (!kref_get_unless_zero(&user->refcount)) - return NULL; - return user; -} - struct cmd_rcvr { struct list_head link; @@ -611,6 +593,28 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf, bool guid_set, guid_t *guid, int intf_num); static int __scan_channels(struct ipmi_smi *intf, struct ipmi_device_id *id); +static void free_ipmi_user(struct kref *ref) +{ + struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); + struct module *owner; + + owner = user->intf->owner; + kref_put(&user->intf->refcount, intf_free); + module_put(owner); + vfree(user); +} + +static void release_ipmi_user(struct ipmi_user *user) +{ + kref_put(&user->refcount, free_ipmi_user); +} + +static struct ipmi_user *acquire_ipmi_user(struct ipmi_user *user) +{ + if (!kref_get_unless_zero(&user->refcount)) + return NULL; + return user; +} /* * The driver model view of the IPMI messaging driver. @@ -1330,7 +1334,6 @@ static void _ipmi_destroy_user(struct ipmi_user *user) unsigned long flags; struct cmd_rcvr *rcvr; struct cmd_rcvr *rcvrs = NULL; - struct module *owner; if (!refcount_dec_if_one(&user->destroyed)) return; @@ -1382,10 +1385,6 @@ static void _ipmi_destroy_user(struct ipmi_user *user) } release_ipmi_user(user); - - owner = intf->owner; - kref_put(&intf->refcount, intf_free); - module_put(owner); } void ipmi_destroy_user(struct ipmi_user *user) @@ -2315,7 +2314,7 @@ static int i_ipmi_request(struct ipmi_user *user, } } - mutex_lock(&ipmi_interfaces_mutex); + mutex_lock(&intf->users_mutex); if (intf->in_shutdown) { rv = -ENODEV; goto out_err; @@ -2361,7 +2360,7 @@ out_err: smi_send(intf, intf->handlers, smi_msg, priority); } - mutex_unlock(&ipmi_interfaces_mutex); + mutex_unlock(&intf->users_mutex); out: if (rv && user) -- 2.51.0 From ff2d2bc9f29d26f5eff9b8fdde225fd705f5555f Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 21 Mar 2025 15:12:06 -0500 Subject: [PATCH 06/16] ipmi:msghandler: Don't acquire a user refcount for queued messages Messages already have a refcount for the user, so there's no need to account for a new one. As part of this, grab a refcount to the interface when processing received messages. The messages can be freed there, cause the user then the interface to be freed. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 32 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index c7533a2846a6..6c9773b3c857 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -958,20 +958,14 @@ static int deliver_response(struct ipmi_smi *intf, struct ipmi_recv_msg *msg) ipmi_free_recv_msg(msg); atomic_dec(&msg->user->nr_msgs); } else { - struct ipmi_user *user = acquire_ipmi_user(msg->user); - - if (user) { - /* Deliver it in smi_work. */ - mutex_lock(&intf->user_msgs_mutex); - list_add_tail(&msg->link, &intf->user_msgs); - mutex_unlock(&intf->user_msgs_mutex); - queue_work(system_wq, &intf->smi_work); - /* User release will happen in the work queue. */ - } else { - /* User went away, give up. */ - ipmi_free_recv_msg(msg); - rv = -EINVAL; - } + /* + * Deliver it in smi_work. The message will hold a + * refcount to the user. + */ + mutex_lock(&intf->user_msgs_mutex); + list_add_tail(&msg->link, &intf->user_msgs); + mutex_unlock(&intf->user_msgs_mutex); + queue_work(system_wq, &intf->smi_work); } return rv; @@ -4827,6 +4821,13 @@ static void smi_work(struct work_struct *t) mutex_unlock(&intf->users_mutex); } + /* + * Freeing the message can cause a user to be released, which + * can then cause the interface to be freed. Make sure that + * doesn't happen until we are ready. + */ + kref_get(&intf->refcount); + mutex_lock(&intf->user_msgs_mutex); list_for_each_entry_safe(msg, msg2, &intf->user_msgs, link) { struct ipmi_user *user = msg->user; @@ -4834,9 +4835,10 @@ static void smi_work(struct work_struct *t) list_del(&msg->link); atomic_dec(&user->nr_msgs); user->handler->ipmi_recv_hndl(msg, user->handler_data); - release_ipmi_user(user); } mutex_unlock(&intf->user_msgs_mutex); + + kref_put(&intf->refcount, intf_free); } /* Handle a new message from the lower layer. */ -- 2.51.0 From f2a31163d6a32143f9e97418fbb30b398c1ebeb7 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 21 Mar 2025 15:15:05 -0500 Subject: [PATCH 07/16] ipmi:msghandler: Don't check for shutdown when returning responses The lower level interface shouldn't attempt to unregister if it has a callback in the pending queue. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 34 ++++++++++------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 6c9773b3c857..6a680c559ed1 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -3893,17 +3893,12 @@ static int handle_ipmb_get_msg_cmd(struct ipmi_smi *intf, dev_dbg(intf->si_dev, "Invalid command: %*ph\n", msg->data_size, msg->data); - mutex_lock(&ipmi_interfaces_mutex); - if (!intf->in_shutdown) { - smi_send(intf, intf->handlers, msg, 0); - /* - * We used the message, so return the value - * that causes it to not be freed or - * queued. - */ - rv = -1; - } - mutex_unlock(&ipmi_interfaces_mutex); + smi_send(intf, intf->handlers, msg, 0); + /* + * We used the message, so return the value that + * causes it to not be freed or queued. + */ + rv = -1; } else { recv_msg = ipmi_alloc_recv_msg(); if (!recv_msg) { @@ -3984,17 +3979,12 @@ static int handle_ipmb_direct_rcv_cmd(struct ipmi_smi *intf, msg->data[4] = IPMI_INVALID_CMD_COMPLETION_CODE; msg->data_size = 5; - mutex_lock(&ipmi_interfaces_mutex); - if (!intf->in_shutdown) { - smi_send(intf, intf->handlers, msg, 0); - /* - * We used the message, so return the value - * that causes it to not be freed or - * queued. - */ - rv = -1; - } - mutex_unlock(&ipmi_interfaces_mutex); + smi_send(intf, intf->handlers, msg, 0); + /* + * We used the message, so return the value that + * causes it to not be freed or queued. + */ + rv = -1; } else { recv_msg = ipmi_alloc_recv_msg(); if (!recv_msg) { -- 2.51.0 From 60afcc429c5bc8ca364b6cdf057b69b66b8ebd76 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 21 Mar 2025 15:40:07 -0500 Subject: [PATCH 08/16] ipmi:msghandler: Remove proc_fs.h It's no longer used. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 6a680c559ed1..75f124c273d9 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -27,7 +27,6 @@ #include #include #include -#include #include #include #include -- 2.51.0 From 8871e77ec73b0b6da6d80677ea49e3f462d03fda Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 21 Mar 2025 16:46:02 -0500 Subject: [PATCH 09/16] ipmi:msghandler: Shut down lower layer first at unregister This makes sure any outstanding messages are returned to the user before the interface is cleaned up. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 75f124c273d9..d6f3fc1f39e0 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -3738,7 +3738,13 @@ void ipmi_unregister_smi(struct ipmi_smi *intf) list_del(&intf->link); mutex_unlock(&ipmi_interfaces_mutex); - /* At this point no users can be added to the interface. */ + /* + * At this point no users can be added to the interface and no + * new messages can be sent. + */ + + if (intf->handlers->shutdown) + intf->handlers->shutdown(intf->send_info); device_remove_file(intf->si_dev, &intf->nr_msgs_devattr); device_remove_file(intf->si_dev, &intf->nr_users_devattr); @@ -3761,9 +3767,6 @@ void ipmi_unregister_smi(struct ipmi_smi *intf) } mutex_unlock(&intf->users_mutex); - if (intf->handlers->shutdown) - intf->handlers->shutdown(intf->send_info); - cleanup_smi_msgs(intf); ipmi_bmc_unregister(intf); -- 2.51.0 From ed59cd28aff9259ea09f57d4e3304abed219179f Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Thu, 27 Mar 2025 13:17:51 -0500 Subject: [PATCH 10/16] ipmi:msghandler: Add a error return from unhandle LAN cmds If we get a command from a LAN channel, return an error instead of just throwing it away. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index d6f3fc1f39e0..29aa52999a96 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -4165,14 +4165,33 @@ static int handle_lan_get_msg_cmd(struct ipmi_smi *intf, rcu_read_unlock(); if (user == NULL) { - /* We didn't find a user, just give up. */ + /* We didn't find a user, just give up and return an error. */ ipmi_inc_stat(intf, unhandled_commands); + msg->data[0] = (IPMI_NETFN_APP_REQUEST << 2); + msg->data[1] = IPMI_SEND_MSG_CMD; + msg->data[2] = chan; + msg->data[3] = msg->rsp[4]; /* handle */ + msg->data[4] = msg->rsp[8]; /* rsSWID */ + msg->data[5] = ((netfn + 1) << 2) | (msg->rsp[9] & 0x3); + msg->data[6] = ipmb_checksum(&msg->data[3], 3); + msg->data[7] = msg->rsp[5]; /* rqSWID */ + /* rqseq/lun */ + msg->data[8] = (msg->rsp[9] & 0xfc) | (msg->rsp[6] & 0x3); + msg->data[9] = cmd; + msg->data[10] = IPMI_INVALID_CMD_COMPLETION_CODE; + msg->data[11] = ipmb_checksum(&msg->data[7], 4); + msg->data_size = 12; + + dev_dbg(intf->si_dev, "Invalid command: %*ph\n", + msg->data_size, msg->data); + + smi_send(intf, intf->handlers, msg, 0); /* - * Don't do anything with these messages, just allow - * them to be freed. + * We used the message, so return the value that + * causes it to not be freed or queued. */ - rv = 0; + rv = -1; } else { recv_msg = ipmi_alloc_recv_msg(); if (!recv_msg) { -- 2.51.0 From ada2abaddad9c2c6557353b866c39e3aec9c8443 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 4 Apr 2025 16:08:09 -0500 Subject: [PATCH 11/16] ipmi:si: Rework startup of IPMI devices It is possible in some situations that IPMI devices won't get started up properly. This change makes it so all non-duplicate devices will get started up. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 82 ++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 20 deletions(-) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 65881878d42c..7fe891783a37 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2095,9 +2095,18 @@ static int try_smi_init(struct smi_info *new_smi) return rv; } +/* + * Devices in the same address space at the same address are the same. + */ +static bool __init ipmi_smi_info_same(struct smi_info *e1, struct smi_info *e2) +{ + return (e1->io.addr_space == e2->io.addr_space && + e1->io.addr_data == e2->io.addr_data); +} + static int __init init_ipmi_si(void) { - struct smi_info *e; + struct smi_info *e, *e2; enum ipmi_addr_src type = SI_INVALID; if (initialized) @@ -2113,37 +2122,70 @@ static int __init init_ipmi_si(void) ipmi_si_parisc_init(); - /* We prefer devices with interrupts, but in the case of a machine - with multiple BMCs we assume that there will be several instances - of a given type so if we succeed in registering a type then also - try to register everything else of the same type */ mutex_lock(&smi_infos_lock); + + /* + * Scan through all the devices. We prefer devices with + * interrupts, so go through those first in case there are any + * duplicates that don't have the interrupt set. + */ list_for_each_entry(e, &smi_infos, link) { - /* Try to register a device if it has an IRQ and we either - haven't successfully registered a device yet or this - device has the same type as one we successfully registered */ - if (e->io.irq && (!type || e->io.addr_source == type)) { - if (!try_smi_init(e)) { - type = e->io.addr_source; + bool dup = false; + + /* Register ones with interrupts first. */ + if (!e->io.irq) + continue; + + /* + * Go through the ones we have already seen to see if this + * is a dup. + */ + list_for_each_entry(e2, &smi_infos, link) { + if (e2 == e) + break; + if (e2->io.irq && ipmi_smi_info_same(e, e2)) { + dup = true; + break; } } + if (!dup) + try_smi_init(e); } - /* type will only have been set if we successfully registered an si */ - if (type) - goto skip_fallback_noirq; + /* + * Now try devices without interrupts. + */ + list_for_each_entry(e, &smi_infos, link) { + bool dup = false; - /* Fall back to the preferred device */ + if (e->io.irq) + continue; - list_for_each_entry(e, &smi_infos, link) { - if (!e->io.irq && (!type || e->io.addr_source == type)) { - if (!try_smi_init(e)) { - type = e->io.addr_source; + /* + * Go through the ones we have already seen to see if + * this is a dup. We have already looked at the ones + * with interrupts. + */ + list_for_each_entry(e2, &smi_infos, link) { + if (!e2->io.irq) + continue; + if (ipmi_smi_info_same(e, e2)) { + dup = true; + break; + } + } + list_for_each_entry(e2, &smi_infos, link) { + if (e2 == e) + break; + if (ipmi_smi_info_same(e, e2)) { + dup = true; + break; } } + if (!dup) + try_smi_init(e); } -skip_fallback_noirq: initialized = true; mutex_unlock(&smi_infos_lock); -- 2.51.0 From 87105e07806767b81910d165441607c1461ba2b8 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Thu, 10 Apr 2025 13:54:56 -0500 Subject: [PATCH 12/16] ipmi:msghandler: Don't deliver messages to deleted users Check to see if they have been destroyed before trying to deliver a message. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 29aa52999a96..3cb72f6b12ea 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -1327,6 +1327,7 @@ static void _ipmi_destroy_user(struct ipmi_user *user) unsigned long flags; struct cmd_rcvr *rcvr; struct cmd_rcvr *rcvrs = NULL; + struct ipmi_recv_msg *msg, *msg2; if (!refcount_dec_if_one(&user->destroyed)) return; @@ -1377,6 +1378,15 @@ static void _ipmi_destroy_user(struct ipmi_user *user) kfree(rcvr); } + mutex_lock(&intf->user_msgs_mutex); + list_for_each_entry_safe(msg, msg2, &intf->user_msgs, link) { + if (msg->user != user) + continue; + list_del(&msg->link); + ipmi_free_recv_msg(msg); + } + mutex_unlock(&intf->user_msgs_mutex); + release_ipmi_user(user); } @@ -4844,8 +4854,22 @@ static void smi_work(struct work_struct *t) struct ipmi_user *user = msg->user; list_del(&msg->link); - atomic_dec(&user->nr_msgs); - user->handler->ipmi_recv_hndl(msg, user->handler_data); + + /* + * I would like for this check (and user->destroyed) + * to go away, but it's possible that an interface is + * processing a message that belongs to the user while + * the user is being deleted. When that response + * comes back, it could be queued after the user is + * destroyed. This is simpler than handling it in the + * interface. + */ + if (refcount_read(&user->destroyed) == 0) { + ipmi_free_recv_msg(msg); + } else { + atomic_dec(&user->nr_msgs); + user->handler->ipmi_recv_hndl(msg, user->handler_data); + } } mutex_unlock(&intf->user_msgs_mutex); -- 2.51.0 From 6bd0eb6d759b9a22c5509ea04e19c2e8407ba418 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Thu, 10 Apr 2025 14:44:26 -0500 Subject: [PATCH 13/16] ipmi:ssif: Fix a shutdown race It was possible for the SSIF thread to stop and quit before the kthread_stop() call because ssif->stopping was set before the stop. So only exit the SSIF thread is kthread_should_stop() returns true. There is no need to wake the thread, as the wait will be interrupted by kthread_stop(). Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_ssif.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 0b45b07dec22..5bf038e620c7 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -481,8 +481,6 @@ static int ipmi_ssif_thread(void *data) /* Wait for something to do */ result = wait_for_completion_interruptible( &ssif_info->wake_thread); - if (ssif_info->stopping) - break; if (result == -ERESTARTSYS) continue; init_completion(&ssif_info->wake_thread); @@ -1270,10 +1268,8 @@ static void shutdown_ssif(void *send_info) ssif_info->stopping = true; timer_delete_sync(&ssif_info->watch_timer); timer_delete_sync(&ssif_info->retry_timer); - if (ssif_info->thread) { - complete(&ssif_info->wake_thread); + if (ssif_info->thread) kthread_stop(ssif_info->thread); - } } static void ssif_remove(struct i2c_client *client) -- 2.51.0 From be816bc4f8413f227a48278f14693674d9296fe2 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 15 Apr 2025 19:56:27 -0500 Subject: [PATCH 14/16] Documentation:ipmi: Remove comments about interrupt level Callbacks no longer run at interrupt level or bh, so remove those comments. Signed-off-by: Corey Minyard --- Documentation/driver-api/ipmi.rst | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Documentation/driver-api/ipmi.rst b/Documentation/driver-api/ipmi.rst index dfa021eacd63..3a533cd2ef60 100644 --- a/Documentation/driver-api/ipmi.rst +++ b/Documentation/driver-api/ipmi.rst @@ -280,10 +280,8 @@ Creating the User To use the message handler, you must first create a user using ipmi_create_user. The interface number specifies which SMI you want to connect to, and you must supply callback functions to be called -when data comes in. The callback function can run at interrupt level, -so be careful using the callbacks. This also allows to you pass in a -piece of data, the handler_data, that will be passed back to you on -all calls. +when data comes in. This also allows to you pass in a piece of data, +the handler_data, that will be passed back to you on all calls. Once you are done, call ipmi_destroy_user() to get rid of the user. @@ -303,8 +301,7 @@ use it for anything you like. Responses come back in the function pointed to by the ipmi_recv_hndl field of the "handler" that you passed in to ipmi_create_user(). -Remember again, these may be running at interrupt level. Remember to -look at the receive type, too. +Remember to look at the receive type, too. From userland, you fill out an ipmi_req_t structure and use the IPMICTL_SEND_COMMAND ioctl. For incoming stuff, you can use select() -- 2.51.0 From 6f7f6605c9aeb472b5d4101b263b1fc5dc3cf623 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 22 Apr 2025 12:06:02 -0500 Subject: [PATCH 15/16] ipmi:msghandler: Export and fix panic messaging capability Don't have the other users that do things at panic time (the watchdog) do all this themselves, provide a function to do it. Also, with the new design where most stuff happens at thread context, a few things needed to be fixed to avoid doing locking in a panic context. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 50 ++++++++++++++++++----------- include/linux/ipmi.h | 10 ++++++ 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 3cb72f6b12ea..8b2c3fca9c4c 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -2284,6 +2284,7 @@ static int i_ipmi_request(struct ipmi_user *user, { struct ipmi_smi_msg *smi_msg; struct ipmi_recv_msg *recv_msg; + int run_to_completion = READ_ONCE(intf->run_to_completion); int rv = 0; if (user) { @@ -2317,7 +2318,8 @@ static int i_ipmi_request(struct ipmi_user *user, } } - mutex_lock(&intf->users_mutex); + if (!run_to_completion) + mutex_lock(&intf->users_mutex); if (intf->in_shutdown) { rv = -ENODEV; goto out_err; @@ -2363,7 +2365,8 @@ out_err: smi_send(intf, intf->handlers, smi_msg, priority); } - mutex_unlock(&intf->users_mutex); + if (!run_to_completion) + mutex_unlock(&intf->users_mutex); out: if (rv && user) @@ -4559,7 +4562,7 @@ return_unspecified: && (msg->data[1] == IPMI_SEND_MSG_CMD) && (msg->user_data == NULL)) { - if (intf->in_shutdown) + if (intf->in_shutdown || intf->run_to_completion) goto out; /* @@ -4631,6 +4634,9 @@ return_unspecified: */ struct ipmi_recv_msg *recv_msg; + if (intf->run_to_completion) + goto out; + chan = msg->data[2] & 0x0f; if (chan >= IPMI_MAX_CHANNELS) /* Invalid channel number */ @@ -4653,6 +4659,9 @@ process_response_response: && (msg->rsp[1] == IPMI_GET_MSG_CMD)) { struct ipmi_channel *chans; + if (intf->run_to_completion) + goto out; + /* It's from the receive queue. */ chan = msg->rsp[3] & 0xf; if (chan >= IPMI_MAX_CHANNELS) { @@ -4727,6 +4736,9 @@ process_response_response: } else if ((msg->rsp[0] == ((IPMI_NETFN_APP_REQUEST|1) << 2)) && (msg->rsp[1] == IPMI_READ_EVENT_MSG_BUFFER_CMD)) { /* It's an asynchronous event. */ + if (intf->run_to_completion) + goto out; + requeue = handle_read_event_rsp(intf, msg); } else { /* It's a response from the local BMC. */ @@ -4855,15 +4867,6 @@ static void smi_work(struct work_struct *t) list_del(&msg->link); - /* - * I would like for this check (and user->destroyed) - * to go away, but it's possible that an interface is - * processing a message that belongs to the user while - * the user is being deleted. When that response - * comes back, it could be queued after the user is - * destroyed. This is simpler than handling it in the - * interface. - */ if (refcount_read(&user->destroyed) == 0) { ipmi_free_recv_msg(msg); } else { @@ -5222,9 +5225,9 @@ static void dummy_recv_done_handler(struct ipmi_recv_msg *msg) /* * Inside a panic, send a message and wait for a response. */ -static void ipmi_panic_request_and_wait(struct ipmi_smi *intf, - struct ipmi_addr *addr, - struct kernel_ipmi_msg *msg) +static void _ipmi_panic_request_and_wait(struct ipmi_smi *intf, + struct ipmi_addr *addr, + struct kernel_ipmi_msg *msg) { struct ipmi_smi_msg smi_msg; struct ipmi_recv_msg recv_msg; @@ -5254,6 +5257,15 @@ static void ipmi_panic_request_and_wait(struct ipmi_smi *intf, ipmi_poll(intf); } +void ipmi_panic_request_and_wait(struct ipmi_user *user, + struct ipmi_addr *addr, + struct kernel_ipmi_msg *msg) +{ + user->intf->run_to_completion = 1; + _ipmi_panic_request_and_wait(user->intf, addr, msg); +} +EXPORT_SYMBOL(ipmi_panic_request_and_wait); + static void event_receiver_fetcher(struct ipmi_smi *intf, struct ipmi_recv_msg *msg) { @@ -5322,7 +5334,7 @@ static void send_panic_events(struct ipmi_smi *intf, char *str) } /* Send the event announcing the panic. */ - ipmi_panic_request_and_wait(intf, &addr, &msg); + _ipmi_panic_request_and_wait(intf, &addr, &msg); /* * On every interface, dump a bunch of OEM event holding the @@ -5358,7 +5370,7 @@ static void send_panic_events(struct ipmi_smi *intf, char *str) msg.data = NULL; msg.data_len = 0; intf->null_user_handler = device_id_fetcher; - ipmi_panic_request_and_wait(intf, &addr, &msg); + _ipmi_panic_request_and_wait(intf, &addr, &msg); if (intf->local_event_generator) { /* Request the event receiver from the local MC. */ @@ -5367,7 +5379,7 @@ static void send_panic_events(struct ipmi_smi *intf, char *str) msg.data = NULL; msg.data_len = 0; intf->null_user_handler = event_receiver_fetcher; - ipmi_panic_request_and_wait(intf, &addr, &msg); + _ipmi_panic_request_and_wait(intf, &addr, &msg); } intf->null_user_handler = NULL; @@ -5419,7 +5431,7 @@ static void send_panic_events(struct ipmi_smi *intf, char *str) memcpy_and_pad(data+5, 11, p, size, '\0'); p += size; - ipmi_panic_request_and_wait(intf, &addr, &msg); + _ipmi_panic_request_and_wait(intf, &addr, &msg); } } diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h index 27cd5980bb27..7da6602eab71 100644 --- a/include/linux/ipmi.h +++ b/include/linux/ipmi.h @@ -344,4 +344,14 @@ extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data); /* Helper function for computing the IPMB checksum of some data. */ unsigned char ipmb_checksum(unsigned char *data, int size); +/* + * For things that must send messages at panic time, like the IPMI watchdog + * driver that extends the reset time on a panic, use this to send messages + * from panic context. Note that this puts the driver into a mode that + * only works at panic time, so only use it then. + */ +void ipmi_panic_request_and_wait(struct ipmi_user *user, + struct ipmi_addr *addr, + struct kernel_ipmi_msg *msg); + #endif /* __LINUX_IPMI_H */ -- 2.51.0 From 971a00454d9604493ecfe4ca8fb8de0fad2863e3 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 22 Apr 2025 12:07:28 -0500 Subject: [PATCH 16/16] ipmi:watchdog: Use the new interface for panic messages It's available, remove all the duplicate code. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_watchdog.c | 72 ++++++++----------------------- 1 file changed, 17 insertions(+), 55 deletions(-) diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index 01c10bd5f099..ab759b492fdd 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -363,7 +363,7 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg *smi_msg, { struct kernel_ipmi_msg msg; unsigned char data[6]; - int rv; + int rv = 0; struct ipmi_system_interface_addr addr; int hbnow = 0; @@ -405,14 +405,18 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg *smi_msg, msg.cmd = IPMI_WDOG_SET_TIMER; msg.data = data; msg.data_len = sizeof(data); - rv = ipmi_request_supply_msgs(watchdog_user, - (struct ipmi_addr *) &addr, - 0, - &msg, - NULL, - smi_msg, - recv_msg, - 1); + if (smi_msg) + rv = ipmi_request_supply_msgs(watchdog_user, + (struct ipmi_addr *) &addr, + 0, + &msg, + NULL, + smi_msg, + recv_msg, + 1); + else + ipmi_panic_request_and_wait(watchdog_user, + (struct ipmi_addr *) &addr, &msg); if (rv) pr_warn("set timeout error: %d\n", rv); else if (send_heartbeat_now) @@ -431,9 +435,7 @@ static int _ipmi_set_timeout(int do_heartbeat) atomic_set(&msg_tofree, 2); - rv = __ipmi_set_timeout(&smi_msg, - &recv_msg, - &send_heartbeat_now); + rv = __ipmi_set_timeout(&smi_msg, &recv_msg, &send_heartbeat_now); if (rv) { atomic_set(&msg_tofree, 0); return rv; @@ -460,27 +462,10 @@ static int ipmi_set_timeout(int do_heartbeat) return rv; } -static atomic_t panic_done_count = ATOMIC_INIT(0); - -static void panic_smi_free(struct ipmi_smi_msg *msg) -{ - atomic_dec(&panic_done_count); -} -static void panic_recv_free(struct ipmi_recv_msg *msg) -{ - atomic_dec(&panic_done_count); -} - -static struct ipmi_smi_msg panic_halt_heartbeat_smi_msg = - INIT_IPMI_SMI_MSG(panic_smi_free); -static struct ipmi_recv_msg panic_halt_heartbeat_recv_msg = - INIT_IPMI_RECV_MSG(panic_recv_free); - static void panic_halt_ipmi_heartbeat(void) { struct kernel_ipmi_msg msg; struct ipmi_system_interface_addr addr; - int rv; /* * Don't reset the timer if we have the timer turned off, that @@ -497,24 +482,10 @@ static void panic_halt_ipmi_heartbeat(void) msg.cmd = IPMI_WDOG_RESET_TIMER; msg.data = NULL; msg.data_len = 0; - atomic_add(2, &panic_done_count); - rv = ipmi_request_supply_msgs(watchdog_user, - (struct ipmi_addr *) &addr, - 0, - &msg, - NULL, - &panic_halt_heartbeat_smi_msg, - &panic_halt_heartbeat_recv_msg, - 1); - if (rv) - atomic_sub(2, &panic_done_count); + ipmi_panic_request_and_wait(watchdog_user, (struct ipmi_addr *) &addr, + &msg); } -static struct ipmi_smi_msg panic_halt_smi_msg = - INIT_IPMI_SMI_MSG(panic_smi_free); -static struct ipmi_recv_msg panic_halt_recv_msg = - INIT_IPMI_RECV_MSG(panic_recv_free); - /* * Special call, doesn't claim any locks. This is only to be called * at panic or halt time, in run-to-completion mode, when the caller @@ -526,22 +497,13 @@ static void panic_halt_ipmi_set_timeout(void) int send_heartbeat_now; int rv; - /* Wait for the messages to be free. */ - while (atomic_read(&panic_done_count) != 0) - ipmi_poll_interface(watchdog_user); - atomic_add(2, &panic_done_count); - rv = __ipmi_set_timeout(&panic_halt_smi_msg, - &panic_halt_recv_msg, - &send_heartbeat_now); + rv = __ipmi_set_timeout(NULL, NULL, &send_heartbeat_now); if (rv) { - atomic_sub(2, &panic_done_count); pr_warn("Unable to extend the watchdog timeout\n"); } else { if (send_heartbeat_now) panic_halt_ipmi_heartbeat(); } - while (atomic_read(&panic_done_count) != 0) - ipmi_poll_interface(watchdog_user); } static int __ipmi_heartbeat(void) -- 2.51.0