From nobody Wed Dec 31 19:00:54 2025 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F8C9C00142 for ; Mon, 30 Oct 2023 16:26:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233594AbjJ3Q0p (ORCPT ); Mon, 30 Oct 2023 12:26:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44858 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232555AbjJ3Q0m (ORCPT ); Mon, 30 Oct 2023 12:26:42 -0400 X-Greylist: delayed 308 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Mon, 30 Oct 2023 09:26:40 PDT Received: from cnc.isely.net (cnc.isely.net [192.69.181.175]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4020CD9 for ; Mon, 30 Oct 2023 09:26:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=isely.net; s=deb; t=1698682891; bh=hfdYzCdm+x+tKMPgcOZzIeuFJxxf0KYbhVmSfhSKie0=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=AEhPwc3ik23Rivj/BYW/k5RTjxSASvnPmmZsZ2pJKq0Fu3t/B+OIYQ2KW1Fi3xX9w n+owQehpIKD4JXkT5R0h5K7i2Oatqp3g2yXkk0DZRbLHc1th1QKUcqK4XfX1zvzfjC jVgaAnB5Kv/vejUF++ujkzN3mZbPxg3HOSO9Wf7QL7gdoacwfFZOoKvFBEyM7 Original-Subject: [PATCH 1/2] [i2c-bcm2835] Fully clean up hardware state machine after a timeout Author: mike.isely@cobaltdigital.com Original-Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Received: from cobalt1.eng.cobalt.local (ts3-dock1.isely.net [::ffff:192.168.23.13]) (AUTH: PLAIN isely, TLS: TLS1.3,256bits,ECDHE_RSA_AES_256_GCM_SHA384) by cnc.isely.net with ESMTPSA id 000000000008093D.00000000653FD80B.000079FF; Mon, 30 Oct 2023 11:21:31 -0500 From: mike.isely@cobaltdigital.com To: Andi Shyti , Florian Fainelli Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] [i2c-bcm2835] Fully clean up hardware state machine after a timeout Date: Mon, 30 Oct 2023 11:21:13 -0500 Message-Id: <20231030162114.3603829-2-mike.isely@cobaltdigital.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> References: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mime-Autoconverted: from 8bit to 7bit by courier 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" From: Mike Isely When the driver detects a timeout, there's no guarantee that the ISR would have fired. Thus after a timeout, it's the foreground that becomes responsible to reset the hardware state machine. The change here just duplicates what is already implemented in the ISR. Signed-off-by: Mike Isely --- drivers/i2c/busses/i2c-bcm2835.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2= 835.c index 8ce6d3f49551..96de875394e1 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -345,42 +345,46 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void= *data) static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[= ], int num) { struct bcm2835_i2c_dev *i2c_dev =3D i2c_get_adapdata(adap); unsigned long time_left; int i; =20 for (i =3D 0; i < (num - 1); i++) if (msgs[i].flags & I2C_M_RD) { dev_warn_once(i2c_dev->dev, "only one read message supported, has to be last\n"); return -EOPNOTSUPP; } =20 i2c_dev->curr_msg =3D msgs; i2c_dev->num_msgs =3D num; reinit_completion(&i2c_dev->completion); =20 bcm2835_i2c_start_transfer(i2c_dev); =20 time_left =3D wait_for_completion_timeout(&i2c_dev->completion, adap->timeout); =20 bcm2835_i2c_finish_transfer(i2c_dev); =20 if (!time_left) { + /* Since we can't trust the ISR to have cleaned up, do the + * full cleanup here... */ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR); + bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT | + BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE); dev_err(i2c_dev->dev, "i2c transfer timed out\n"); return -ETIMEDOUT; } =20 if (!i2c_dev->msg_err) return num; =20 dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err); =20 if (i2c_dev->msg_err & BCM2835_I2C_S_ERR) return -EREMOTEIO; =20 return -EIO; } --=20 2.39.2 From nobody Wed Dec 31 19:00:54 2025 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E5AEFC4332F for ; Mon, 30 Oct 2023 16:26:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233812AbjJ3Q0r (ORCPT ); Mon, 30 Oct 2023 12:26:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44872 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232887AbjJ3Q0m (ORCPT ); Mon, 30 Oct 2023 12:26:42 -0400 Received: from cnc.isely.net (cnc.isely.net [192.69.181.175]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 41A44DA for ; Mon, 30 Oct 2023 09:26:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=isely.net; s=deb; t=1698682891; bh=RJdQVRnJ416Wgcvb+WG6qIsjnzbvVyX8BWqzPyjn0Lw=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=B3htted8H4OPvFO0ZKlY6OyIk66ctUL+MsAlwryE2lPloLVr5B5eHDasV4oHbwKR8 6pED6Dp6FaQRoCIoTKhA8xonLSXFyaUzqlPEwCA4One02DRulNggogOjce3FFFc9N5 bXWeaPBB20PITelVzhY4T22o2ilSs1oawHM7KPWfYkEMw9M5IMeD1oK99CySy Original-Subject: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD Author: mike.isely@cobaltdigital.com Original-Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Received: from cobalt1.eng.cobalt.local (ts3-dock1.isely.net [::ffff:192.168.23.13]) (AUTH: PLAIN isely, TLS: TLS1.3,256bits,ECDHE_RSA_AES_256_GCM_SHA384) by cnc.isely.net with ESMTPSA id 000000000008096B.00000000653FD80B.00007A07; Mon, 30 Oct 2023 11:21:31 -0500 From: mike.isely@cobaltdigital.com To: Andi Shyti , Florian Fainelli Cc: Mike Isely , Mike Isely , Broadcom internal kernel review list , Ray Jui , Scott Branden , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD Date: Mon, 30 Oct 2023 11:21:14 -0500 Message-Id: <20231030162114.3603829-3-mike.isely@cobaltdigital.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> References: <20231030162114.3603829-1-mike.isely@cobaltdigital.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mime-Autoconverted: from 8bit to 7bit by courier 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" From: Mike Isely There is a race in the bcm2835 i2c hardware: When one starts a write transaction, two things apparently take place at the same time: (1) an interrupt is posted to cause the FIFO to be filled with TX data, and (2) an I2C transaction is started on the wire with the slave select byte. The race happens if there's no slave, as this causes a slave selection timeout, raising the ERR flag in the hardware and setting DONE. The setting of that DONE flag races against TXW. If TXW gets set first, then an interrupt is raised if INTT was set. If ERR gets set first, then an interrupt is raised if INTD was set. It's one or the other, not both - probably because DONE being set disables the hardware INTT interrupt path. MOST of the time, TXW gets set first, the ISR runs, sees ERR is set and cleanly fails the transaction. However some of the time DONE gets set first - but since the driver doesn't enable INTD until it's on the last message - there's no interrupt at all. Thus the ISR never fires and the driver detects a timeout instead. At best, the "wrong" error code is delivered to the owner of the transaction. At worst, if the timeout doesn't propertly clean up the hardware (see prior commit fixing that), the next - likely unrelated - transaction will get fouled, leading to bizarre behavior in logic otherwise unrelated to the source of the original error. The fix here is to set INTD on for all messages not just the last one. In that way, unexpected failures which might set DONE earlier than expected will always trigger an interrupt and be handled correctly. The datasheet for this hardware doesn't describe any scenario where the hardware can realistically hang - even a stretched clock will be noticed if it takes too long. So in theory a timeout should really NEVER happen, and with this fix I was completely unable to trigger any further timeouts at all. Signed-off-by: Mike Isely --- drivers/i2c/busses/i2c-bcm2835.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2= 835.c index 96de875394e1..70005c037ff9 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -235,26 +235,22 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_d= ev *i2c_dev) =20 static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev) { - u32 c =3D BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN; + u32 c =3D BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN | BCM2835_I2C_C_INTD; struct i2c_msg *msg =3D i2c_dev->curr_msg; - bool last_msg =3D (i2c_dev->num_msgs =3D=3D 1); =20 if (!i2c_dev->num_msgs) return; =20 i2c_dev->num_msgs--; i2c_dev->msg_buf =3D msg->buf; i2c_dev->msg_buf_remaining =3D msg->len; =20 if (msg->flags & I2C_M_RD) c |=3D BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR; else c |=3D BCM2835_I2C_C_INTT; =20 - if (last_msg) - c |=3D BCM2835_I2C_C_INTD; - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr); bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len); bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c); } --=20 2.39.2