On 03/27/2018 08:20 AM, Corey Minyard wrote:
On 03/14/2018 06:00 AM, Kamlakant Patel wrote:
On Tue, Mar 13, 2018 at 09:50:53AM -0500, Corey Minyard wrote:
On 03/13/2018 08:31 AM, Corey Minyard wrote:
On 03/13/2018 06:02 AM, Kamlakant Patel wrote:
Getting following crash while inserting ipmi_ssif after ipmi_watchdog.


[Mar 6 05:59] INFO: task insmod:4232 blocked for more than 120 seconds.
[  +0.006433]       Tainted: G        W  OE   4.11.12+ #19
[  +0.005317] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[  +0.007825] insmod          D    0  4232   2751 0x00000008
[  +0.000004] Call trace:
[  +0.000003] [<ffff000008086198>] __switch_to+0x90/0xa8
[  +0.000005] [<ffff000008a00078>] __schedule+0x340/0x8d8
[  +0.000002] [<ffff000008a00644>] schedule+0x34/0x90
[  +0.000002] [<ffff000008a0098c>] schedule_preempt_disabled+0x14/0x20
[  +0.000002] [<ffff000008a01768>] __mutex_lock.isra.0+0x130/0x480
[  +0.000002] [<ffff000008a01adc>] __mutex_lock_slowpath+0x24/0x30
[  +0.000002] [<ffff000008a01b34>] mutex_lock+0x4c/0x58
[  +0.000002] [<ffff000008817ea4>] i2c_get_adapter+0x2c/0xa0
[  +0.000005] [<ffff000000ac0530>] inc_usecount+0x28/0x50 [ipmi_ssif]
[  +0.000007] [<ffff000001a1c180>] ipmi_create_user+0x140/0x240
[ipmi_msghandler]
[  +0.000007] [<ffff000000ab8d3c>] ipmi_register_watchdog+0x74/0x118
[ipmi_watchdog]
[...]

The crash is caused because ipmi_register_watchdog calls
ipmi_create_user.
This in terns calls i2c_get_adapter which tries to take mutex lock of
core_lock.
The core_lock is already acquired by i2c_for_each_dev called by
i2c_add_driver
>from init_ipmi_ssif.
Calling ipmi_register_smi() after i2c_add_driver() solves this issue.
Well, no it doesn't.  It might solve this issue, but it creates other
issues.
Multiple clients can be registering at the same time, sothe global
variables
are not going to work.  And adapters can be added after ipmi_init_ssif()
is called, so your change will cause any IPMI devices on new adapters
(or if the IPMI code is initialized before the I2C code) to be missed.
The
unregister code may have a similar issue, too.

I can see the issue now, you have clearly laid that out, and this was
an RFC, so I'm guessing it didn't feel right to you.

I'm not quite sure how to fix it.  It would be nice if the I2C code
didn't call probe functions holding a fundamental lock preventing
a call to i2c_get_adapater().  That would be fairly easy to fix, but
I'm not sure if it would be accepted.

I'm thinking that postponing the calls to things waiting for interfaces
to come and go would be the right thing to do.  But that adds
the burden of handling if the device gets deleted before the
calls get made.
Actually, the right thing to do may be to remove the call to
i2c_get_adapter() and the matching i2c_put_adapter().  The
idea was to keep from removing the adapter's module code
from being removed while the IPMI driver was using it, but
with hotplug being so prevalent, I'm not sure there's much
value in that.

You can try that, if you like.

If we remove the call to i2c_get_adapter and i2c_put_adapter, we can remove i2c adapter while IPMI driver using it. This will lead to another issue when we insert ipmi_watchdog and start watchdog service and then remove the i2c adapter.

Getting following error with the given scenario:
[ 5081.636417]       Tainted: G           OE    4.16.0-rc4+ #1
[ 5081.642000] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 5081.649839] watchdog        D    0  7309      1 0x00000000
[ 5081.649848] Call trace:
[ 5081.649860]  __switch_to+0x8c/0xd0
[ 5081.649874]  __schedule+0x344/0x908
[ 5081.649878]  schedule+0x34/0x90
[ 5081.649882]  schedule_timeout+0x1c4/0x3f8
[ 5081.649886]  wait_for_common+0xb8/0x168
[ 5081.649889]  wait_for_completion+0x28/0x38
[ 5081.649904]  ipmi_heartbeat+0xbc/0x200 [ipmi_watchdog]
[ 5081.649911]  ipmi_write+0x110/0x170 [ipmi_watchdog]
[ 5081.649923]  __vfs_write+0x48/0x150
[ 5081.649926]  vfs_write+0xb0/0x1c0
[ 5081.649929]  SyS_write+0x54/0xb0
[ 5081.649931]  el0_svc_naked+0x30/0x34

I was a little afraid of that.  The IPMI users in the kernel were not designed
to handle hot removal, though they probably should be.  Let me look at
this.

In this case, the watchdog is going to go off if you remove the adapter.
I guess there's not much you can do about that.


I've finally gotten to this.  Unfortunately, it's a little more complex that I
would like.

The IPMI driver was written before there was any hot swap, and it would
refcount modules it used and it was not designed to handle hot-removal.
I have modified the driver to fix those issues, but the changes are fairly
large.  Well, not too large, but the total diffs are 1300 lines or so.

Now the driver uses the same sorts of mechanisms as other subsystems,
with a remove callback, and it actually cleaned up a few pieces of
messiness elsewhere.

Anyway, there are six patches on top of v4.14 at:

https://github.com/cminyard/linux-ipmi.git v4.14-hotremove

that should clear up this issue.  I still may hack on this a little bit,
but it should be close to final.

Thanks,

-corey

-corey

Thanks,
Kamlakant Patel
-corey

Signed-off-by: Kamlakant Patel <kamlakant.pa...@cavium.com>
---
   drivers/char/ipmi/ipmi_ssif.c | 102
+++++++++++++++++++++++-------------------
   1 file changed, 55 insertions(+), 47 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c
b/drivers/char/ipmi/ipmi_ssif.c
index 16d7fb5..c71e707 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -192,6 +192,8 @@ struct ssif_addr_info {
   };
     struct ssif_info;
+static struct ssif_info *local_ssif_info;
+static int local_slave_addr;
     typedef void (*ssif_i2c_done)(struct ssif_info *ssif_info, int
result,
                    unsigned char *data, unsigned int len);
@@ -1534,6 +1536,7 @@ static int ssif_probe(struct i2c_client *client,
const struct i2c_device_id *id)
       }
         slave_addr = find_slave_address(client, slave_addr);
+    local_slave_addr = slave_addr;
         pr_info(PFX "Trying %s-specified SSIF interface at i2c address
0x%x, adapter %s, slave address 0x%x\n",
ipmi_addr_src_to_str(ssif_info->addr_source),
@@ -1725,43 +1728,7 @@ static int ssif_probe(struct i2c_client *client,
const struct i2c_device_id *id)
           }
       }
   -    dev_set_drvdata(&ssif_info->client->dev, ssif_info);
-    rv = device_add_group(&ssif_info->client->dev,
-                  &ipmi_ssif_dev_attr_group);
-    if (rv) {
-        dev_err(&ssif_info->client->dev,
-            "Unable to add device attributes: error %d\n",
-            rv);
-        goto out;
-    }
-
-    rv = ipmi_register_smi(&ssif_info->handlers,
-                   ssif_info,
-                   &ssif_info->client->dev,
-                   slave_addr);
-     if (rv) {
-        pr_err(PFX "Unable to register device: error %d\n", rv);
-        goto out_remove_attr;
-    }
-
-#ifdef CONFIG_IPMI_PROC_INTERFACE
-    rv = ipmi_smi_add_proc_entry(ssif_info->intf, "type",
-                     &smi_type_proc_ops,
-                     ssif_info);
-    if (rv) {
-        pr_err(PFX "Unable to create proc entry: %d\n", rv);
-        goto out_err_unreg;
-    }
-
-    rv = ipmi_smi_add_proc_entry(ssif_info->intf, "ssif_stats",
-                     &smi_stats_proc_ops,
-                     ssif_info);
-    if (rv) {
-        pr_err(PFX "Unable to create proc entry: %d\n", rv);
-        goto out_err_unreg;
-    }
-#endif
-
+    local_ssif_info = ssif_info;
    out:
       if (rv) {
           /*
@@ -1778,16 +1745,6 @@ static int ssif_probe(struct i2c_client *client,
const struct i2c_device_id *id)
       }
       kfree(resp);
       return rv;
-
-#ifdef CONFIG_IPMI_PROC_INTERFACE
-out_err_unreg:
-    ipmi_unregister_smi(ssif_info->intf);
-#endif
-
-out_remove_attr:
- device_remove_group(&ssif_info->client->dev,
&ipmi_ssif_dev_attr_group);
-    dev_set_drvdata(&ssif_info->client->dev, NULL);
-    goto out;
   }
     static int ssif_adapter_handler(struct device *adev, void *opaque)
@@ -2127,7 +2084,58 @@ static int init_ipmi_ssif(void)
       if (!rv)
           initialized = true;
   +    if (local_ssif_info == NULL)
+        goto out;
+
+ dev_set_drvdata(&local_ssif_info->client->dev, local_ssif_info);
+    rv = device_add_group(&local_ssif_info->client->dev,
+                  &ipmi_ssif_dev_attr_group);
+    if (rv) {
+        dev_err(&local_ssif_info->client->dev,
+            "Unable to add device attributes: error %d\n",
+            rv);
+        goto out;
+    }
+
+    rv = ipmi_register_smi(&local_ssif_info->handlers,
+                   local_ssif_info,
+ &local_ssif_info->client->dev,
+                   local_slave_addr);
+    if (rv) {
+        pr_err(PFX "Unable to register device: error %d\n", rv);
+        goto out_remove_attr;
+    }
+
+#ifdef CONFIG_IPMI_PROC_INTERFACE
+    rv = ipmi_smi_add_proc_entry(local_ssif_info->intf, "type",
+                     &smi_type_proc_ops,
+                     local_ssif_info);
+    if (rv) {
+        pr_err(PFX "Unable to create proc entry: %d\n", rv);
+        goto out_err_unreg;
+    }
+
+    rv = ipmi_smi_add_proc_entry(local_ssif_info->intf, "ssif_stats",
+                     &smi_stats_proc_ops,
+                     local_ssif_info);
+    if (rv) {
+        pr_err(PFX "Unable to create proc entry: %d\n", rv);
+        goto out_err_unreg;
+    }
+#endif
+ out:
       return rv;
+
+#ifdef CONFIG_IPMI_PROC_INTERFACE
+ out_err_unreg:
+    ipmi_unregister_smi(local_ssif_info->intf);
+#endif
+
+ out_remove_attr:
+ device_remove_group(&local_ssif_info->client->dev,
+                &ipmi_ssif_dev_attr_group);
+ dev_set_drvdata(&local_ssif_info->client->dev, NULL);
+    goto out;
   }
   module_init(init_ipmi_ssif);


------------------------------------------------------------------------------

Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Reply via email to