From patchwork Fri May 20 10:33:24 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wolfram Sang X-Patchwork-Id: 574692 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 E996CC43219 for ; Fri, 20 May 2022 10:33:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1348141AbiETKdh (ORCPT ); Fri, 20 May 2022 06:33:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37196 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1348136AbiETKdf (ORCPT ); Fri, 20 May 2022 06:33:35 -0400 Received: from mail.zeus03.de (www.zeus03.de [194.117.254.33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8E9AC8CCC2 for ; Fri, 20 May 2022 03:33:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=sang-engineering.com; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=k1; bh=whHSXYbHza0AY8 tPCXiERYHBIKvKIgdfZhENGZbjFiQ=; b=JlAjoJYAiEuOaTjZ6fFuw+nCs+TvSK Wdj/x/+6TJsX9AVTqmksIdzTdM6ThdtzeKhfvT+/tL02GaUwVcHGAwIcc2FA7ZYd PclA3ubraOg9pC19hvs4Fr3C5jkkUvO4fPKpkvTXWUSbj5mpp2JxN9cB8dN7E9e+ AA1LyHjNfEo0U= Received: (qmail 3736505 invoked from network); 20 May 2022 12:33:31 +0200 Received: by mail.zeus03.de with ESMTPSA (TLS_AES_256_GCM_SHA384 encrypted, authenticated); 20 May 2022 12:33:31 +0200 X-UD-Smtp-Session: l3s3148p1@xgCRBW/fOXJZD+8D From: Wolfram Sang To: linux-i2c@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org, Yoshihiro Shimoda , Wolfram Sang Subject: [PATCH 1/2] i2c: rcar: avoid race condition with SMIs Date: Fri, 20 May 2022 12:33:24 +0200 Message-Id: <20220520103325.81110-2-wsa+renesas@sang-engineering.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220520103325.81110-1-wsa+renesas@sang-engineering.com> References: <20220520103325.81110-1-wsa+renesas@sang-engineering.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org A customer experienced a race condition with 'repeated starts' when a System Management Interrupt took over for 30us and more. The problem was that during the SMI a new MAT interrupt came in because we set up the 'repeated start' condition. But the old one was not acknowledged yet. So, when it was acknowledged after the SMI, the new MAT interrupt was lost, confusing the state machine of the driver. The fix consists of two parts. First, we do not clear the status register for 'repeated starts' when preparing the next message anymore. The interrupt handlers for sending and receiving data is now solely responsible for that and it makes the code easier to follow, in fact. Secondly, clearing the status register is now split up to handle MAT interrupts independently. This avoids the race condition because the old MAT interrupt will be now cleared before we initiate the "repeated start" condition. Reported-by: Yoshihiro Shimoda Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-rcar.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index ca61dbe218bf..143cb4186daa 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -97,9 +97,6 @@ #define RCAR_IRQ_RECV (MNR | MAL | MST | MAT | MDR) #define RCAR_IRQ_STOP (MST) -#define RCAR_IRQ_ACK_SEND (~(MAT | MDE) & 0x7F) -#define RCAR_IRQ_ACK_RECV (~(MAT | MDR) & 0x7F) - #define ID_LAST_MSG (1 << 0) #define ID_FIRST_MSG (1 << 1) #define ID_DONE (1 << 2) @@ -161,6 +158,11 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg) return readl(priv->io + reg); } +static void rcar_i2c_clear_irq(struct rcar_i2c_priv *priv, u32 val) +{ + writel(~val & 0x7f, priv->io + ICMSR); +} + static int rcar_i2c_get_scl(struct i2c_adapter *adap) { struct rcar_i2c_priv *priv = i2c_get_adapdata(adap); @@ -356,7 +358,7 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv) priv->flags &= ~ID_P_REP_AFTER_RD; else rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START); - rcar_i2c_write(priv, ICMSR, 0); + /* ICMSR is cleared in interrupt handlers */ } } @@ -476,11 +478,15 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) { struct i2c_msg *msg = priv->msg; + u32 irqs_to_clear = MDE; /* FIXME: sometimes, unknown interrupt happened. Do nothing */ if (!(msr & MDE)) return; + if (msr & MAT) + irqs_to_clear |= MAT; + /* Check if DMA can be enabled and take over */ if (priv->pos == 1 && rcar_i2c_dma(priv)) return; @@ -504,32 +510,32 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) * [ICRXTX] -> [SHIFT] -> [I2C bus] */ - if (priv->flags & ID_LAST_MSG) { + if (priv->flags & ID_LAST_MSG) /* * If current msg is the _LAST_ msg, * prepare stop condition here. * ID_DONE will be set on STOP irq. */ rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); - } else { + else rcar_i2c_next_msg(priv); - return; - } } - rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_SEND); + rcar_i2c_clear_irq(priv, irqs_to_clear); } static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) { struct i2c_msg *msg = priv->msg; bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN; + u32 irqs_to_clear = MDR; /* FIXME: sometimes, unknown interrupt happened. Do nothing */ if (!(msr & MDR)) return; if (msr & MAT) { + irqs_to_clear |= MAT; /* * Address transfer phase finished, but no data at this point. * Try to use DMA to receive data. @@ -570,8 +576,8 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) rcar_i2c_next_msg(priv); - else - rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV); + + rcar_i2c_clear_irq(priv, irqs_to_clear); } static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv) From patchwork Fri May 20 10:33:25 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wolfram Sang X-Patchwork-Id: 575352 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 6920BC4332F for ; Fri, 20 May 2022 10:33:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1348143AbiETKdg (ORCPT ); Fri, 20 May 2022 06:33:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37210 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1348141AbiETKdg (ORCPT ); Fri, 20 May 2022 06:33:36 -0400 Received: from mail.zeus03.de (www.zeus03.de [194.117.254.33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D105D8CB20 for ; Fri, 20 May 2022 03:33:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=sang-engineering.com; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; s=k1; bh=EsubzTIPZKYeLm X/QaAIPlUCB5hQR3IE88ztaYIh7mg=; b=waTEqvh1SDRuSOsEPR2KK1BzX6Z5zM RmF70kcdPylImbiaX8fdrs53134zz7RlfD5fd0ZM6kvMFiAxFcVsc6j7W5HpECha scWIPgbZnvPUu0yudD6IxZjZ/fIgsFG5Us1c4ZZgPhDEXsc0xEU9W6uzSQCZK+/b yqLtjIYSoduic= Received: (qmail 3736535 invoked from network); 20 May 2022 12:33:32 +0200 Received: by mail.zeus03.de with ESMTPSA (TLS_AES_256_GCM_SHA384 encrypted, authenticated); 20 May 2022 12:33:32 +0200 X-UD-Smtp-Session: l3s3148p1@ug2gBW/fqw1ZD+8D From: Wolfram Sang To: linux-i2c@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org, Yoshihiro Shimoda , Wolfram Sang Subject: [PATCH 2/2] i2c: rcar: refactor handling of first message Date: Fri, 20 May 2022 12:33:25 +0200 Message-Id: <20220520103325.81110-3-wsa+renesas@sang-engineering.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220520103325.81110-1-wsa+renesas@sang-engineering.com> References: <20220520103325.81110-1-wsa+renesas@sang-engineering.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org After moving ICMSR handling to interrupt handlers previously to fix a race condition, we can now also move ICMSR handling for the first message out of the function to prepare a message. By introducing a seperate function to initialize the first message, we can not only remove some code duplication but the remaining code is also easier to follow. The function to prepare a message is much simpler without ICMSR handling. Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-rcar.c | 50 ++++++++++++++++------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 143cb4186daa..f69903397285 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -98,7 +98,6 @@ #define RCAR_IRQ_STOP (MST) #define ID_LAST_MSG (1 << 0) -#define ID_FIRST_MSG (1 << 1) #define ID_DONE (1 << 2) #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) @@ -333,11 +332,19 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv) return 0; } +/* + * We don't have a test case but the HW engineers say that the write order of + * ICMSR and ICMCR depends on whether we issue START or REP_START. So, ICMSR + * handling is outside of this function. First messages clear ICMSR before this + * function, interrupt handlers clear the relevant bits after this function. + */ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv) { int read = !!rcar_i2c_is_recv(priv); priv->pos = 0; + priv->flags &= ID_P_MASK; + if (priv->msgs_left == 1) priv->flags |= ID_LAST_MSG; @@ -345,29 +352,27 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv) if (!priv->atomic_xfer) rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND); - /* - * We don't have a test case but the HW engineers say that the write order - * of ICMSR and ICMCR depends on whether we issue START or REP_START. Since - * it didn't cause a drawback for me, let's rather be safe than sorry. - */ - if (priv->flags & ID_FIRST_MSG) { - rcar_i2c_write(priv, ICMSR, 0); + if (priv->flags & ID_P_REP_AFTER_RD) + priv->flags &= ~ID_P_REP_AFTER_RD; + else rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START); - } else { - if (priv->flags & ID_P_REP_AFTER_RD) - priv->flags &= ~ID_P_REP_AFTER_RD; - else - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START); - /* ICMSR is cleared in interrupt handlers */ - } +} + +static void rcar_i2c_first_msg(struct rcar_i2c_priv *priv, + struct i2c_msg *msgs, int num) +{ + priv->msg = msgs; + priv->msgs_left = num; + rcar_i2c_write(priv, ICMSR, 0); /* must be before preparing msg */ + rcar_i2c_prepare_msg(priv); } static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv) { priv->msg++; priv->msgs_left--; - priv->flags &= ID_P_MASK; rcar_i2c_prepare_msg(priv); + /* ICMSR handling must come afterwards in the irq handler */ } static void rcar_i2c_cleanup_dma(struct rcar_i2c_priv *priv, bool terminate) @@ -852,11 +857,7 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, for (i = 0; i < num; i++) rcar_i2c_request_dma(priv, msgs + i); - /* init first message */ - priv->msg = msgs; - priv->msgs_left = num; - priv->flags = (priv->flags & ID_P_MASK) | ID_FIRST_MSG; - rcar_i2c_prepare_msg(priv); + rcar_i2c_first_msg(priv, msgs, num); time_left = wait_event_timeout(priv->wait, priv->flags & ID_DONE, num * adap->timeout); @@ -906,12 +907,7 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap, goto out; rcar_i2c_init(priv); - - /* init first message */ - priv->msg = msgs; - priv->msgs_left = num; - priv->flags = (priv->flags & ID_P_MASK) | ID_FIRST_MSG; - rcar_i2c_prepare_msg(priv); + rcar_i2c_first_msg(priv, msgs, num); j = jiffies + num * adap->timeout; do {