On 12/20/18 5:59 PM, Paul E. McKenney wrote:
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?

Thanks for the review.  I really meant to put that there :).

-corey

                                                        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

Reply via email to