From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E61EEC433FE for ; Thu, 6 Oct 2022 14:11:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Reply-To:List-Subscribe:List-Help: List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=n7sAkbf6bMfMCNuB3+/AHhji4BLLzzPrShO51KVpE7s=; b=PWvWqMJb2XaOPo Vnh7LwjI06Vcd/J0ro1EQA3Indw7lwcdygWbWOji1EO5/8Srl0GeA+u2lpC+9VYbb470tyisci2LK Lus1slt/Ldd/Iaq/A2SLvj5KzNvn/m2ywCKCR1JFYNJJhkAPnTryF5Vk8zqGMWFNoXltWGER85Ohh Pu7BPFZwqFhtoD5/279T/rFIS+0OAW2Hd9PkN4/NferwyydPtdk1aNDMT3jSVKMTIlbyEmYCbTEnV I+Sq8GvmvdFPgP2ZwrvbHEHM1VhN/Fy7N1KE3WJfeyNaQw1P17lh0kagknwofx3Yt+rNGhG81kxgg bMOwxXBaGnNe35wQCqeA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ogRZf-002uZt-Ih; Thu, 06 Oct 2022 14:10:07 +0000 Received: from mail-qk1-x731.google.com ([2607:f8b0:4864:20::731]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ogRZc-002uYU-0f for linux-arm-kernel@lists.infradead.org; Thu, 06 Oct 2022 14:10:05 +0000 Received: by mail-qk1-x731.google.com with SMTP id h6so1050225qkl.11 for ; Thu, 06 Oct 2022 07:10:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:reply-to :message-id:subject:cc:to:from:date:sender:from:to:cc:subject:date :message-id:reply-to; bh=ntnmdCz7v5r64kcnWi8DSkoYbZLsQCVuJJM9qyrDCnw=; b=B/MELwvxGHOUB9n/4IZqtXNP45/GUA/qzSkuclVSpXk/hHNETuFosn1BksqfdAO6YC Uk7s0kBfRcQno51w58ZZeR1JRQ1FTx0KFzVowH5ahSpQFM00sjiLAqu/8a/6V2kjdyFE BOaF5+dyP6N9n6rqssedaIY+lIsodO8jDsqxqQBbqPohnWSKRIraCSTiiKRhNJUswxrA 4OuQOHS8dxZNaz8aBi+/Q0+h81aVB2UEnUYx43vpmtGk6gpAbkgxX8X/+FJPS0zlByMk QEl/foNsH27xCm2+Aj2r75obbZHv8GOoPql58jPWQ960t+qDJUHVZ0NHmJQhg+RSFZuv gUTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:reply-to :message-id:subject:cc:to:from:date:sender:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=ntnmdCz7v5r64kcnWi8DSkoYbZLsQCVuJJM9qyrDCnw=; b=KYq9fQa+LJ72L2/hXK5l1cL1ewAn4EJ7JOc/fp7Kv92z0pHNZPrcmYiT5U2vzXZRFo +KqB+oLzqvCH4thc1+d3SSRPHBnwN1PbXysFtGOm90cz979Z1JNJkK9NiTNfpBTlorJ8 yMRzO66HIPdE5S6Qd7nhGQMyxA40/dGQjDfyFJGu3/v+FH0j73JG6WPM6ZPX0CGUl3IR 1sCqmZddSNbPmYMlBsFvSBPTJCDJWSniTTwjxTC4Olm/fCbB9AN6HoiexqeSp6GKnKrO 4ib68GLGpVpjP/5q5TkvDkqOrXgRmy+I2GJN4bBfff3Yp8B3JtGrjuauwwfHl55MTIRm 4Uvg== X-Gm-Message-State: ACrzQf1ZcPS2Z+T9yMOTNPeSALUXXWYd3TKDxqFFmD2is6yvLXbIzImk d4Vx2mBTBciB85t/7W2Jyg== X-Google-Smtp-Source: AMsMyM73nX69TZE1+MgUVtlQx7xVnxr/lUPIFB2FR88zpn3YmI15D7PxjegZ0vi/YooQMgojpkOXKQ== X-Received: by 2002:a37:9302:0:b0:6ce:3765:eb95 with SMTP id v2-20020a379302000000b006ce3765eb95mr130459qkd.177.1665065401988; Thu, 06 Oct 2022 07:10:01 -0700 (PDT) Received: from serve.minyard.net ([47.184.185.126]) by smtp.gmail.com with ESMTPSA id o18-20020a05620a111200b006b98315c6fbsm3686391qkk.1.2022.10.06.07.10.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Oct 2022 07:10:01 -0700 (PDT) Received: from dell1.minyard.net (unknown [IPv6:2001:470:b8f6:1d::35]) by serve.minyard.net (Postfix) with ESMTPSA id 8D0D2180015; Thu, 6 Oct 2022 14:09:59 +0000 (UTC) Date: Thu, 6 Oct 2022 09:09:57 -0500 From: Corey Minyard To: Andrew Jeffery Cc: Joel Stanley , openipmi-developer@lists.sourceforge.net, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org Subject: Re: [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency Message-ID: References: <20220812144741.240315-1-andrew@aj.id.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221006_071004_083986_297CC67C X-CRM114-Status: GOOD ( 36.63 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: minyard@acm.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Oct 06, 2022 at 01:36:51PM +1030, Andrew Jeffery wrote: > > > On Thu, 6 Oct 2022, at 10:20, Joel Stanley wrote: > > On Fri, 12 Aug 2022 at 14:48, Andrew Jeffery wrote: > >> > >> The ASPEED KCS devices don't provide a BMC-side interrupt for the host > >> reading the output data register (ODR). The act of the host reading ODR > >> clears the output buffer full (OBF) flag in the status register (STR), > >> informing the BMC it can transmit a subsequent byte. > >> > >> On the BMC side the KCS client must enable the OBE event *and* perform a > >> subsequent read of STR anyway to avoid races - the polling provides a > >> window for the host to read ODR if data was freshly written while > >> minimising BMC-side latency. > >> > > > > Fixes...? > > Is it a fix though? It's definitely an *improvement* in behaviour, but > the existing behaviour also wasn't *incorrect*, just kinda unfortunate > under certain timings? Dunno. I'm probably splitting hairs. > > In any case, if we do want a fixes line: > > Fixes: 28651e6c4237 ("ipmi: kcs_bmc: Allow clients to control KCS IRQ state") I added the Fixes and Joel's review. Thanks, -corey > > > > >> Signed-off-by: Andrew Jeffery > > > > Reviewed-by: Joel Stanley > > Thanks! > > > > >> --- > >> drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +++++++++++++++++++++--- > >> 1 file changed, 21 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c > >> index cdc88cde1e9a..417e5a3ccfae 100644 > >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > >> @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list *timer) > >> static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) > >> { > >> struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); > >> + int rc; > >> + u8 str; > > > > str is status, it would be good to spell that out in full. > > I guess if it trips enough people up we can rename it later. > > > > >> > >> /* We don't have an OBE IRQ, emulate it */ > >> if (mask & KCS_BMC_EVENT_TYPE_OBE) { > >> - if (KCS_BMC_EVENT_TYPE_OBE & state) > >> - mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > >> - else > >> + if (KCS_BMC_EVENT_TYPE_OBE & state) { > >> + /* > >> + * Given we don't have an OBE IRQ, delay by polling briefly to see if we can > >> + * observe such an event before returning to the caller. This is not > >> + * incorrect because OBF may have already become clear before enabling the > >> + * IRQ if we had one, under which circumstance no event will be propagated > >> + * anyway. > >> + * > >> + * The onus is on the client to perform a race-free check that it hasn't > >> + * missed the event. > >> + */ > >> + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str, > >> + !(str & KCS_BMC_STR_OBF), 1, 100, false, > >> + &priv->kcs_bmc, priv->kcs_bmc.ioreg.str); > >> + /* Time for the slow path? */ > > > > The mod_timer is the slow path? The question mark threw me. > > Yeah, mod_timer() is the slow path; read_poll_timeout_atomic() is the > fast path and we've exhausted the time we're willing to wait there if > we get -ETIMEDOUT. > > The comment was intended as a description for the question posed by the > condition. It made sense in my head but maybe it's confusing more than > it is enlightening? > > Andrew > > > > >> + if (rc == -ETIMEDOUT) > >> + mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > >> + } else { > >> del_timer(&priv->obe.timer); > >> + } > >> } > >> > >> if (mask & KCS_BMC_EVENT_TYPE_IBF) { > >> -- > >> 2.34.1 > >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel