[PATCH 2/3] ipmi/bt-bmc: maintain a request expiry list
From: clg@kaod.org (Cédric Le Goater)
Date: 2016-11-09 19:08:31
Also in:
linux-devicetree
On 11/09/2016 04:52 PM, Corey Minyard wrote:
On 11/09/2016 08:30 AM, C?dric Le Goater wrote:quoted
On 11/07/2016 08:04 PM, Corey Minyard wrote:quoted
On 11/02/2016 02:57 AM, C?dric Le Goater wrote:quoted
Regarding the response expiration handling, the IPMI spec says : The BMC must not return a given response once the corresponding Request-to-Response interval has passed. The BMC can ensure this by maintaining its own internal list of outstanding requests through the interface. The BMC could age and expire the entries in the list by expiring the entries at an interval that is somewhat shorter than the specified Request-to-Response interval.... To handle such case, we maintain list of received requests using the seq number of the BT message to identify them. The list is updated each time a request is received and a response is sent. The expiration of the reponses is handled at each updates but also with a timer.This looks correct, but it seems awfully complicated. Why can't you get the current time before the wait_event_interruptible() and then compare the time before you do the write? That would seem to accomplish the same thing without any lists or extra locks.Well, the expiry list needs a request identifier and it is currently using the Seq byte for this purpose. So the BT message needs to be read to grab that byte. The request is added to a list and that involves some locking. When the response is written, the first matching request is removed from the list and a garbage collector loop is also run. Then, as we might not get any responses to run that loop, we use a timer to empty the list from any expired requests. The read/write ops of the driver are protected with a mutex, the list and the timer add their share of locking. That could have been done with RCU surely but we don't really need performance in this driver. Caveats : bt_bmc_remove_request() should not be done in the writing loop though. It needs a fix. The request identifier is currently Seq but the spec say we should use Seq, NetFn, and Command or an internal Seq value as a request identifier. Google is also working on an OEM/Group extension (Brendan in CC: ) which has a different message format. I need to look closer at what should be done in this case.I'm still not sure why the list is necessary. You have a separate thread of execution for each writer, why not just time it in that thread?
No, we don't in the current design. This is only a single process acting as a proxy and dispatching commands on dbus to other processes doing whatever they need to do. So the request/responses can interlace. The current daemon already handles an expiry list but I thought it would be better to move it in the kernel to have a better response time. The BMC can be quite slow when busy. It seems that keeping the logic in user space is better. So let's have it that way. Not a problem.
What about the following, not even compile-tested, patch? I'm sure my mailer will munge this up, I can send you a clean version if you like.
No it is ok. I will give your fix a try on our system and resend. Thanks, C.
quoted hunk
From 1a73585a9c1c74ac1d59d82f22e05b30447619a6 Mon Sep 17 00:00:00 2001 From: Corey Minyard <redacted> Date: Wed, 9 Nov 2016 09:07:48 -0600 Subject: [PATCH] ipmi:bt-bmc: Fix a multi-user race, time out responses The IPMI spec says to time out responses after a given amount of time, so don't let a writer send something after an amount of time has elapsed. Also, fix a race condition in the same area where if you have two writers at the same time, one can get a EIO return when it should still be waiting its turn to send. A mutex_lock_interruptible_timeout() would be handy here, but it doesn't exist. Signed-off-by: Corey Minyard <redacted> --- drivers/char/ipmi/bt-bmc.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c index b49e613..5be94cf 100644 --- a/drivers/char/ipmi/bt-bmc.c +++ b/drivers/char/ipmi/bt-bmc.c@@ -57,6 +57,8 @@ #define BT_BMC_BUFFER_SIZE 256 +#define BT_BMC_RESPONSE_JIFFIES (5 * HZ) + struct bt_bmc { struct device dev; struct miscdevice miscdev;@@ -190,14 +192,12 @@ static ssize_t bt_bmc_read(struct file *file, char __user *buf, WARN_ON(*ppos); - if (wait_event_interruptible(bt_bmc->queue, - bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN)) + if (mutex_lock_interruptible(&bt_bmc->mutex)) return -ERESTARTSYS; - mutex_lock(&bt_bmc->mutex); - - if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) { - ret = -EIO; + if (wait_event_interruptible(bt_bmc->queue, + bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN)) { + ret = -ERESTARTSYS; goto out_unlock; }@@ -251,6 +251,7 @@ static ssize_t bt_bmc_write(struct file *file, const char __user *buf, u8 kbuffer[BT_BMC_BUFFER_SIZE]; ssize_t ret = 0; ssize_t nwritten; + unsigned long start_jiffies = jiffies, wait_time; /* * send a minimum response size@@ -263,23 +264,31 @@ static ssize_t bt_bmc_write(struct file *file, const char __user *buf, WARN_ON(*ppos); + if (mutex_lock_interruptible(&bt_bmc->mutex)) + return -ERESTARTSYS; + + wait_time = jiffies - start_jiffies; + if (wait_time >= BT_BMC_RESPONSE_TIME_JIFFIES) { + ret = -ETIMEDOUT; + goto out_unlock; + } + wait_time = BT_BMC_RESPONSE_TIME_JIFFIES - wait_time; + /* * There's no interrupt for clearing bmc busy so we have to * poll */ - if (wait_event_interruptible(bt_bmc->queue, + ret = wait_event_interruptible_timeout(bt_bmc->queue, !(bt_inb(bt_bmc, BT_CTRL) & - (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)))) - return -ERESTARTSYS; - - mutex_lock(&bt_bmc->mutex); - - if (unlikely(bt_inb(bt_bmc, BT_CTRL) & - (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))) { - ret = -EIO; + (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)), + wait_time); + if (ret <= 0) { + if (ret == 0) + ret = -ETIMEDOUT; goto out_unlock; } + ret = 0; clr_wr_ptr(bt_bmc); while (count) {