From patchwork Wed Nov 9 15:52:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 9419711 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E98126048E for ; Wed, 9 Nov 2016 15:54:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DA043291BB for ; Wed, 9 Nov 2016 15:54:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CDBBB2933F; Wed, 9 Nov 2016 15:54:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 81050291BB for ; Wed, 9 Nov 2016 15:54:27 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1c4VAv-0002tV-Ph; Wed, 09 Nov 2016 15:52:33 +0000 Received: from mail-pf0-x242.google.com ([2607:f8b0:400e:c00::242]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1c4VAr-0002sc-QR for linux-arm-kernel@lists.infradead.org; Wed, 09 Nov 2016 15:52:31 +0000 Received: by mail-pf0-x242.google.com with SMTP id n85so724814pfi.3 for ; Wed, 09 Nov 2016 07:52:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:reply-to:subject:references:to:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=nlX83TXHENhsNWTsp82uzs8OtIbCBIdwsbIxuIe2+r4=; b=EsqFQDC1VRTElKQBY0iTvUnTrmr7cAfdCQxpN79vCj48+Ye7bk8StVyPuqrpWbd+8K 64mQeffNHh1at7FCPS5VU4JqR1SLOKNrJ7V+k2FS1LyfWjjMwViFRYQepZXnuWQVhvn/ Hs1spp+wHr8kvm9JFTs8Om67ngazWWYHm8tkZB3Iw0FRzXTR7Onp/x2zTIyqpWqMHTgG 7vlSIqbZFFotBBPBmT/tZffzhhCrJpcWfBsZvdKmo20Hpj9zURBF13zjH5JZ6kcbI+7c YMufGvfmrTStLqW5E2F1My/jmw2x7+aerEtiEkD4t9foenJ+uGN9cxAHGzDNDwSPHHVj wdLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:reply-to:subject:references:to:cc:from :message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=nlX83TXHENhsNWTsp82uzs8OtIbCBIdwsbIxuIe2+r4=; b=DndTrROOejGV0GCB9YwTrxA260DRIk1Cd3zz0DkbwWKkNleU8jdCtBt+ZanKvCOoo3 Ae9SJdbchdtPab9pgTiovb/jB2uYwcxtwOuOf2UxU2Zct4DtHKL8gD3q0h8JdIM9QNqn DdD0kiHq9wfcz1h/qQPIcfTrmD2h+lhfadGOkok5AtTGaPJDGtrc/PxVSHL8HoLOpP/2 qU9eQcrNPQluUjeDEUOzNS/fdJLo8aZ+M1gZ+iV27z9EA73FROPwSqqPC9UUEKEEVre4 ny6QVubFJX2dfHX0GGXZWqMzWZfBoWYyracIfl+uBAvi2dbfCqQQe9esCL1Nr21dan0m Yp/Q== X-Gm-Message-State: ABUngvfSAM/fTzqltKYmrcaTt9YnDu7Z7YmDh6Y+y7yH9cJwjW9xtDUsV3pOq+HoqmlCbw== X-Received: by 10.99.139.199 with SMTP id j190mr27927218pge.115.1478706728115; Wed, 09 Nov 2016 07:52:08 -0800 (PST) Received: from serve.minyard.net (serve.minyard.net. [2001:470:b8f6:1b::1]) by smtp.gmail.com with ESMTPSA id yk6sm222792pab.43.2016.11.09.07.52.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 09 Nov 2016 07:52:07 -0800 (PST) Received: from [IPv6:2001:470:b8f6:1b:2467:e409:f2a3:fd2] (unknown [IPv6:2001:470:b8f6:1b:2467:e409:f2a3:fd2]) by serve.minyard.net (Postfix) with ESMTPSA id 1616D1647; Wed, 9 Nov 2016 09:52:04 -0600 (CST) Subject: Re: [PATCH 2/3] ipmi/bt-bmc: maintain a request expiry list References: <1478073426-3714-1-git-send-email-clg@kaod.org> <1478073426-3714-3-git-send-email-clg@kaod.org> <6117c1fd-b969-9394-0be5-d46f64269cac@kaod.org> To: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= , openipmi-developer@lists.sourceforge.net, Joel Stanley From: Corey Minyard Message-ID: <1e0187c4-d503-ce4a-3d4c-cf21f0bffb96@acm.org> Date: Wed, 9 Nov 2016 09:52:03 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <6117c1fd-b969-9394-0be5-d46f64269cac@kaod.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20161109_075229_945785_2761EF0D X-CRM114-Status: GOOD ( 52.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: minyard@acm.org Cc: devicetree@vger.kernel.org, Rob Herring , Brendan Higgins , linux-arm-kernel@lists.infradead.org, Arnd Bergmann Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP 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? 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. From 1a73585a9c1c74ac1d59d82f22e05b30447619a6 Mon Sep 17 00:00:00 2001 From: Corey Minyard 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 --- drivers/char/ipmi/bt-bmc.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) @@ -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) { 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; }