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: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.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 variablesare 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 interfacesto 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.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/0x34I was a little afraid of that. The IPMI users in the kernel were not designedto 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
-coreyThanks, Kamlakant Patel-coreySigned-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 address0x%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
