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.

-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

Reply via email to