Re: [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

Re: [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

[Tony Camuso]

Hi, Len.

On Jul 23, Lv Zheng submitted this patch (02/13)
https://lkml.org/lkml/2013/7/23/96

.. and early this month, not having seen Lv's patch, I
submitted a virtual identical one, here. 
https://lkml.org/lkml/2013/9/2/305

We've been seeing lockdep splats that are fixed by this
patch, so I'm wondering if Lv's patch will be included
in kernel v3.12.

Re: [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

[Rafael J. Wysocki]

On Thursday, September 12, 2013 10:44:02 AM Tony Camuso wrote:
> Hi, Len.
> 
> On Jul 23, Lv Zheng submitted this patch (02/13)
> https://lkml.org/lkml/2013/7/23/96
> 
> .. and early this month, not having seen Lv's patch, I
> submitted a virtual identical one, here. 
> https://lkml.org/lkml/2013/9/2/305
> 
> We've been seeing lockdep splats that are fixed by this
> patch, so I'm wondering if Lv's patch will be included
> in kernel v3.12.

I actually have a plan to apply your patch, because it doesn't depend on the
additional changes made by Lv that don't belong in 3.12.

Thanks,
Rafael

Re: [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

[Tony Camuso]

On 09/12/2013 11:12 AM, Rafael J. Wysocki wrote:
> On Thursday, September 12, 2013 10:44:02 AM Tony Camuso wrote:
>> Hi, Len.
>>
>> On Jul 23, Lv Zheng submitted this patch (02/13)
>> https://lkml.org/lkml/2013/7/23/96
>>
>> .. and early this month, not having seen Lv's patch, I
>> submitted a virtual identical one, here. 
>> https://lkml.org/lkml/2013/9/2/305
>>
>> We've been seeing lockdep splats that are fixed by this
>> patch, so I'm wondering if Lv's patch will be included
>> in kernel v3.12.
> 
> I actually have a plan to apply your patch, because it doesn't depend on the
> additional changes made by Lv that don't belong in 3.12.
> 
> Thanks,
> Rafael
> 

Yes, that would be great.

Regards,
Tony

RE: [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

[Zheng, Lv]

It's OK, though I think my patch doesn't depend on any other patches in the series.
I'll change my patch order, making this one the first one for the series and rebase the whole series again so that it's up to you to determine to accept which one.

But if you could wait a minute, I can send out an upgraded bug-fix part today as after the internal discussion, I now actually know everything you want.
I do think other bug fixes should also be included in the 3.12.
I hope you could accept my series as I have the environment that be able to do a complete functional test on the ACPI IPMI functionality.
It's just because we have some business reasons (something like internal process, other assignments) that delayed the upgraded version.

Thanks and best regards
-Lv


> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Thursday, September 12, 2013 11:13 PM
> 
> On Thursday, September 12, 2013 10:44:02 AM Tony Camuso wrote:
> > Hi, Len.
> >
> > On Jul 23, Lv Zheng submitted this patch (02/13)
> > https://lkml.org/lkml/2013/7/23/96
> >
> > .. and early this month, not having seen Lv's patch, I
> > submitted a virtual identical one, here.
> > https://lkml.org/lkml/2013/9/2/305
> >
> > We've been seeing lockdep splats that are fixed by this
> > patch, so I'm wondering if Lv's patch will be included
> > in kernel v3.12.
> 
> I actually have a plan to apply your patch, because it doesn't depend on the
> additional changes made by Lv that don't belong in 3.12.
> 
> Thanks,
> Rafael

[PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes

[Lv Zheng]

This patchset tries to fix the following kernel bug:
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=46741
This is fixed by [PATCH 05].

The bug shows IPMI operation region may appear in a device not under the
IPMI system interface device's scope, thus it's required to install the
ACPI IPMI operation region handler from the root of the ACPI namespace.

The original acpi_ipmi implementation includes several issues that break
the test process.  This patchset also includes a re-design of acpi_ipmi
module to make the test possible.
 
[PATCH 01-05] are bug-fix patches that can be applied to the kernels whose
              version is > 2.6.38.  This can be confirmed with:
              # git tag --contains e92b297c
[PATCH 06] is also a bug-fix patch.
           The drivers/acpi/osl.c part can be back ported to the kernels
           whose version > 2.6.14.  This can be confirmed with:
           # git tag --contains 4be44fcd
           The drivers/acpi/acpi_ipmi.c part can be applied on top of
           [PATCH 01-05].
[PATCH 07] is a tuning patch for acpi_ipmi.c.
[PATCH 08-10] are cleanup patches for acpi_ipmi.c.
[PATCH 11] is a cleanup patch not for acpi_ipmi.c.
[PATCH 12-13] are test patches.
              [PATCH 12] may be accepted by upstream kernel as a useful
                         facility to test the loading/unloading of the
                         modules.
              [PATCH 13] should not be merged by any published kernel as it
                         is a driver for a pseudo device with a PnP ID that
                         does not exist in the real machines.

This patchset has passed the test around a fake device accessing IPMI
operation region fields on an IPMI capable platform.  A stress test of
module(acpi_ipmi) load/unload has been performed on such platform.  No
races can be found and the IPMI operation region handler is functioning
now.  It is not possible to test module(ipmi_si) load/unload as it can't be
unloaded due to its' transfer flushing implementation.

Lv Zheng (13):
  ACPI/IPMI: Fix potential response buffer overflow
  ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()
  ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
  ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user
  ACPI/IPMI: Fix issue caused by the per-device registration of the
    IPMI operation region handler
  ACPI/IPMI: Add reference counting for ACPI operation region handlers
  ACPI/IPMI: Add reference counting for ACPI IPMI transfers
  ACPI/IPMI: Cleanup several acpi_ipmi_device members
  ACPI/IPMI: Cleanup some initialization codes
  ACPI/IPMI: Cleanup some inclusion codes
  ACPI/IPMI: Cleanup some Kconfig codes
  Testing: Add module load/unload test suite
  ACPI/IPMI: Add IPMI operation region test device driver

 drivers/acpi/Kconfig                          |   71 +++-
 drivers/acpi/Makefile                         |    1 +
 drivers/acpi/acpi_ipmi.c                      |  513 +++++++++++++++----------
 drivers/acpi/ipmi_test.c                      |  254 ++++++++++++
 drivers/acpi/osl.c                            |  224 +++++++++++
 include/acpi/acpi_bus.h                       |    5 +
 tools/testing/module-unloading/endless_cat.sh |   32 ++
 tools/testing/module-unloading/endless_mod.sh |   81 ++++
 8 files changed, 977 insertions(+), 204 deletions(-)
 create mode 100644 drivers/acpi/ipmi_test.c
 create mode 100755 tools/testing/module-unloading/endless_cat.sh
 create mode 100755 tools/testing/module-unloading/endless_mod.sh

-- 
1.7.10

[PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()

[Lv Zheng]

This patch quick fixes the issues indicated by the test results that
ipmi_msg_handler() is invoked in atomic context.

BUG: scheduling while atomic: kipmi0/18933/0x10000100
Modules linked in: ipmi_si acpi_ipmi ...
CPU: 3 PID: 18933 Comm: kipmi0 Tainted: G       AW    3.10.0-rc7+ #2
Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS QSSC-S4R.QCI.01.00.0027.070120100606 07/01/2010
 ffff8838245eea00 ffff88103fc63c98 ffffffff814c4a1e ffff88103fc63ca8
 ffffffff814bfbab ffff88103fc63d28 ffffffff814c73e0 ffff88103933cbd4
 0000000000000096 ffff88103fc63ce8 ffff88102f618000 ffff881035c01fd8
Call Trace:
 <IRQ>  [<ffffffff814c4a1e>] dump_stack+0x19/0x1b
 [<ffffffff814bfbab>] __schedule_bug+0x46/0x54
 [<ffffffff814c73e0>] __schedule+0x83/0x59c
 [<ffffffff81058853>] __cond_resched+0x22/0x2d
 [<ffffffff814c794b>] _cond_resched+0x14/0x1d
 [<ffffffff814c6d82>] mutex_lock+0x11/0x32
 [<ffffffff8101e1e9>] ? __default_send_IPI_dest_field.constprop.0+0x53/0x58
 [<ffffffffa09e3f9c>] ipmi_msg_handler+0x23/0x166 [ipmi_si]
 [<ffffffff812bf6e4>] deliver_response+0x55/0x5a
 [<ffffffff812c0fd4>] handle_new_recv_msgs+0xb67/0xc65
 [<ffffffff81007ad1>] ? read_tsc+0x9/0x19
 [<ffffffff814c8620>] ? _raw_spin_lock_irq+0xa/0xc
 [<ffffffffa09e1128>] ipmi_thread+0x5c/0x146 [ipmi_si]
 ...

Known issues:
- Replacing tx_msg_lock with spinlock is not performance friendly
  Current solution works but does not have the best performance because it
  is better to make atomic context run as fast as possible.  Given there
  are no many IPMI messages created by ACPI, performance of current
  solution may be OK.  It can be better via linking ipmi_recv_msg into an
  RX message queue and process it in other contexts.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/acpi_ipmi.c |   24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index 28e2b4c..b37c189 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -39,6 +39,7 @@
 #include <linux/ipmi.h>
 #include <linux/device.h>
 #include <linux/pnp.h>
+#include <linux/spinlock.h>
 
 MODULE_AUTHOR("Zhao Yakui");
 MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
@@ -58,7 +59,7 @@ struct acpi_ipmi_device {
 	struct list_head head;
 	/* the IPMI request message list */
 	struct list_head tx_msg_list;
-	struct mutex	tx_msg_lock;
+	spinlock_t	tx_msg_lock;
 	acpi_handle handle;
 	struct pnp_dev *pnp_dev;
 	ipmi_user_t	user_interface;
@@ -146,6 +147,7 @@ static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
 	struct kernel_ipmi_msg *msg;
 	struct acpi_ipmi_buffer *buffer;
 	struct acpi_ipmi_device *device;
+	unsigned long flags;
 
 	msg = &tx_msg->tx_message;
 	/*
@@ -182,10 +184,10 @@ static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
 
 	/* Get the msgid */
 	device = tx_msg->device;
-	mutex_lock(&device->tx_msg_lock);
+	spin_lock_irqsave(&device->tx_msg_lock, flags);
 	device->curr_msgid++;
 	tx_msg->tx_msgid = device->curr_msgid;
-	mutex_unlock(&device->tx_msg_lock);
+	spin_unlock_irqrestore(&device->tx_msg_lock, flags);
 
 	return 0;
 }
@@ -250,6 +252,7 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
 	int msg_found = 0;
 	struct acpi_ipmi_msg *tx_msg;
 	struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+	unsigned long flags;
 
 	if (msg->user != ipmi_device->user_interface) {
 		dev_warn(&pnp_dev->dev,
@@ -258,14 +261,14 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
 		goto out_msg;
 	}
 
-	mutex_lock(&ipmi_device->tx_msg_lock);
+	spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
 	list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
 		if (msg->msgid == tx_msg->tx_msgid) {
 			msg_found = 1;
 			break;
 		}
 	}
-	mutex_unlock(&ipmi_device->tx_msg_lock);
+	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 
 	if (!msg_found) {
 		dev_warn(&pnp_dev->dev,
@@ -394,6 +397,7 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
 	struct acpi_ipmi_device *ipmi_device = handler_context;
 	int err, rem_time;
 	acpi_status status;
+	unsigned long flags;
 
 	/*
 	 * IPMI opregion message.
@@ -416,9 +420,9 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
 		goto out_msg;
 	}
 
-	mutex_lock(&ipmi_device->tx_msg_lock);
+	spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
 	list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
-	mutex_unlock(&ipmi_device->tx_msg_lock);
+	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 	err = ipmi_request_settime(ipmi_device->user_interface,
 				   &tx_msg->addr,
 				   tx_msg->tx_msgid,
@@ -434,9 +438,9 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
 	status = AE_OK;
 
 out_list:
-	mutex_lock(&ipmi_device->tx_msg_lock);
+	spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
 	list_del(&tx_msg->head);
-	mutex_unlock(&ipmi_device->tx_msg_lock);
+	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 out_msg:
 	kfree(tx_msg);
 	return status;
@@ -479,7 +483,7 @@ static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device)
 
 	INIT_LIST_HEAD(&ipmi_device->head);
 
-	mutex_init(&ipmi_device->tx_msg_lock);
+	spin_lock_init(&ipmi_device->tx_msg_lock);
 	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
 	ipmi_install_space_handler(ipmi_device);
 
-- 
1.7.10