On 11/09/2016 04:52 PM, Corey Minyard wrote:
> On 11/09/2016 08:30 AM, Cédric Le Goater wrote:
>> On 11/07/2016 08:04 PM, Corey Minyard wrote:
>>> On 11/02/2016 02:57 AM, Cédric Le Goater wrote:
>>>> 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.
> From 1a73585a9c1c74ac1d59d82f22e05b30447619a6 Mon Sep 17 00:00:00 2001
> From: Corey Minyard <cminyard@xxxxxxxxxx>
> 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 <cminyard@xxxxxxxxxx>
> ---
> 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) {
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
