On Thu, Dec 20, 2018 at 05:06:19PM -0600, miny...@acm.org wrote:
> From: Corey Minyard <cminy...@mvista.com>
>
> The IPMI driver was recently modified to use SRCU, but it turns out
> this uses a chunk of percpu memory, even if IPMI is never used.
>
> So modify thing to on initialize on the first use. There was already
> code to sort of handle this for handling init races, so piggy back
> on top of that, and simplify it in the process.
>
> Signed-off-by: Corey Minyard <cminy...@mvista.com>
> Reported-by: Tejun Heo <t...@kernel.org>
> Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com>
The initialization portion looks good, but don't you need a
cleanup_srcu_struct() somewhere in cleanup_ipmi()?
Or is it not possible to repeatedly load and unload this as a module?
Thanx, Paul
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 133 +++++++++++++++-------------
> 1 file changed, 70 insertions(+), 63 deletions(-)
>
> Tejun, here's a patch to fix the issue. Does this look ok?
>
> Maybe it would be best to not use SRCU, but IMHO it's easier to use
> than locking. That's why I used it.
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 9650fe109f67..7490b9ae8508 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -62,7 +62,8 @@ static void ipmi_debug_msg(const char *title, unsigned char
> *data,
> { }
> #endif
>
> -static int initialized;
> +static bool initialized;
> +static bool drvregistered;
>
> enum ipmi_panic_event_op {
> IPMI_SEND_PANIC_EVENT_NONE,
> @@ -630,7 +631,7 @@ static DEFINE_MUTEX(ipmidriver_mutex);
>
> static LIST_HEAD(ipmi_interfaces);
> static DEFINE_MUTEX(ipmi_interfaces_mutex);
> -DEFINE_STATIC_SRCU(ipmi_interfaces_srcu);
> +struct srcu_struct ipmi_interfaces_srcu;
>
> /*
> * List of watchers that want to know when smi's are added and deleted.
> @@ -1154,7 +1155,7 @@ int ipmi_create_user(unsigned int if_num,
> {
> unsigned long flags;
> struct ipmi_user *new_user;
> - int rv = 0, index;
> + int rv, index;
> struct ipmi_smi *intf;
>
> /*
> @@ -1172,18 +1173,9 @@ int ipmi_create_user(unsigned int if_num,
> * Make sure the driver is actually initialized, this handles
> * problems with initialization order.
> */
> - if (!initialized) {
> - rv = ipmi_init_msghandler();
> - if (rv)
> - return rv;
> -
> - /*
> - * The init code doesn't return an error if it was turned
> - * off, but it won't initialize. Check that.
> - */
> - if (!initialized)
> - return -ENODEV;
> - }
> + rv = ipmi_init_msghandler();
> + if (rv)
> + return rv;
>
> new_user = kmalloc(sizeof(*new_user), GFP_KERNEL);
> if (!new_user)
> @@ -3364,17 +3356,9 @@ int ipmi_register_smi(const struct ipmi_smi_handlers
> *handlers,
> * Make sure the driver is actually initialized, this handles
> * problems with initialization order.
> */
> - if (!initialized) {
> - rv = ipmi_init_msghandler();
> - if (rv)
> - return rv;
> - /*
> - * The init code doesn't return an error if it was turned
> - * off, but it won't initialize. Check that.
> - */
> - if (!initialized)
> - return -ENODEV;
> - }
> + rv = ipmi_init_msghandler();
> + if (rv)
> + return rv;
>
> intf = kzalloc(sizeof(*intf), GFP_KERNEL);
> if (!intf)
> @@ -5084,6 +5068,22 @@ static int panic_event(struct notifier_block *this,
> return NOTIFY_DONE;
> }
>
> +/* Must be called with ipmi_interfaces_mutex held. */
> +static int ipmi_register_driver(void)
> +{
> + int rv;
> +
> + if (drvregistered)
> + return 0;
> +
> + rv = driver_register(&ipmidriver.driver);
> + if (rv)
> + pr_err("Could not register IPMI driver\n");
> + else
> + drvregistered = true;
> + return rv;
> +}
> +
> static struct notifier_block panic_block = {
> .notifier_call = panic_event,
> .next = NULL,
> @@ -5094,66 +5094,73 @@ static int ipmi_init_msghandler(void)
> {
> int rv;
>
> + mutex_lock(&ipmi_interfaces_mutex);
> + rv = ipmi_register_driver();
> + if (rv)
> + goto out;
> if (initialized)
> - return 0;
> -
> - rv = driver_register(&ipmidriver.driver);
> - if (rv) {
> - pr_err("Could not register IPMI driver\n");
> - return rv;
> - }
> + goto out;
>
> - pr_info("version " IPMI_DRIVER_VERSION "\n");
> + init_srcu_struct(&ipmi_interfaces_srcu);
>
> timer_setup(&ipmi_timer, ipmi_timeout, 0);
> mod_timer(&ipmi_timer, jiffies + IPMI_TIMEOUT_JIFFIES);
>
> atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
>
> - initialized = 1;
> + initialized = true;
>
> - return 0;
> +out:
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> }
>
> static int __init ipmi_init_msghandler_mod(void)
> {
> - ipmi_init_msghandler();
> - return 0;
> + int rv;
> +
> + pr_info("version " IPMI_DRIVER_VERSION "\n");
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + rv = ipmi_register_driver();
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> }
>
> static void __exit cleanup_ipmi(void)
> {
> int count;
>
> - if (!initialized)
> - return;
> -
> - atomic_notifier_chain_unregister(&panic_notifier_list, &panic_block);
> -
> - /*
> - * This can't be called if any interfaces exist, so no worry
> - * about shutting down the interfaces.
> - */
> + if (initialized) {
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &panic_block);
>
> - /*
> - * Tell the timer to stop, then wait for it to stop. This
> - * avoids problems with race conditions removing the timer
> - * here.
> - */
> - atomic_inc(&stop_operation);
> - del_timer_sync(&ipmi_timer);
> + /*
> + * This can't be called if any interfaces exist, so no worry
> + * about shutting down the interfaces.
> + */
>
> - driver_unregister(&ipmidriver.driver);
> + /*
> + * Tell the timer to stop, then wait for it to stop. This
> + * avoids problems with race conditions removing the timer
> + * here.
> + */
> + atomic_inc(&stop_operation);
> + del_timer_sync(&ipmi_timer);
>
> - initialized = 0;
> + initialized = false;
>
> - /* Check for buffer leaks. */
> - count = atomic_read(&smi_msg_inuse_count);
> - if (count != 0)
> - pr_warn("SMI message count %d at exit\n", count);
> - count = atomic_read(&recv_msg_inuse_count);
> - if (count != 0)
> - pr_warn("recv message count %d at exit\n", count);
> + /* Check for buffer leaks. */
> + count = atomic_read(&smi_msg_inuse_count);
> + if (count != 0)
> + pr_warn("SMI message count %d at exit\n", count);
> + count = atomic_read(&recv_msg_inuse_count);
> + if (count != 0)
> + pr_warn("recv message count %d at exit\n", count);
> + }
> + if (drvregistered)
> + driver_unregister(&ipmidriver.driver);
> }
> module_exit(cleanup_ipmi);
>
> --
> 2.17.1
>
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
