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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C800CD54AB for ; Tue, 19 Sep 2023 09:56:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231945AbjISJ47 (ORCPT ); Tue, 19 Sep 2023 05:56:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41516 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231372AbjISJ4r (ORCPT ); Tue, 19 Sep 2023 05:56:47 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A393F1990; Tue, 19 Sep 2023 02:55:29 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2BCBEC433C8; Tue, 19 Sep 2023 09:55:26 +0000 (UTC) Date: Tue, 19 Sep 2023 10:55:23 +0100 From: Catalin Marinas To: Jan Bottorff Cc: Serge Semin , Yann Sionneau , Will Deacon , Jarkko Nikula , Andy Shevchenko , Mika Westerberg , Jan Dabros , Andi Shyti , Philipp Zabel , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Yann Sionneau Subject: Re: [PATCH v2] i2c: designware: Fix corrupted memory seen in the ISR Message-ID: References: <20230913232938.420423-1-janb@os.amperecomputing.com> <37e10c3d-b5ab-75ec-3c96-76e15eb9bef8@sionneau.net> <9de89e14-35bd-415d-97f1-4b6db1258997@os.amperecomputing.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9de89e14-35bd-415d-97f1-4b6db1258997@os.amperecomputing.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 18, 2023 at 08:45:42PM -0700, Jan Bottorff wrote: > > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > > > > > > > index ca1035e010c7..1694ac6bb592 100644 > > > > > > > --- a/drivers/i2c/busses/i2c-designware-master.c > > > > > > > +++ b/drivers/i2c/busses/i2c-designware-master.c > > > > > > > @@ -248,6 +248,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) > > > > > > > /* Dummy read to avoid the register getting stuck on Bay Trail */ > > > > > > > regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy); > > > > > > > > > > > > > > + /* > > > > > > > + * To guarantee data written by the current core is visible to > > > > > > > + * all cores, a write barrier is required. This needs to be > > > > > > > + * before an interrupt causes execution on another core. > > > > > > > + * For ARM processors, this needs to be a DSB barrier. > > > > > > > + */ > > > > > > > + wmb(); [...] > I did find the below text in the Arm Architectural Reference Manual (DDI > 0487I.a) section K13.4 "Using a mailbox to send an interrupt". It was nearly > the same wording as the ARM barrier document I previously referenced at > https://developer.arm.com/documentation/genc007826/latest/ This too says a > DSB barrier is required for memory updates to be observable in the ISR. > > " > K13.4 Using a mailbox to send an interrupt > In some message passing systems, it is common for one observer to update > memory and then notify a second observer of the update by sending an > interrupt, using a mailbox. Although a memory access might be made to > initiate the sending of the mailbox interrupt, a DSB instruction is > required to ensure the completion of previous memory accesses. > > Therefore, the following sequence is required to ensure that P2 observes the > updated value: > > AArch32 > P1 > STR R5, [R1] ; message stored to shared memory location > DSB ST > STR R0, [R4] ; R4 contains the address of a mailbox > P2 > ; interrupt service routine > LDR R5, [R1] > > AArch64 > P1 > STR W5, [X1] ; message stored to shared memory location > DSB ST > STR W0, [X4] ; R4 contains the address of a mailbox > P2 > ; interrupt service routine > LDR W5, [X1] > " Will convinced me in the past that a DMB is sufficient here unless the peripheral is CPU-local. The Arm ARM is not entirely clear here. > I hear your concern about how this barrier in platform portable code may > impact platforms other than the one I'm trying to fix. It almost seems like > there is some missing type of barrier macro that on ARM64 does what is > required for cases like this and on other platforms does whatever is > appropriate for that platform, often nothing. I also agree that a wmb() in the i2c driver is not the more elegant fix. For similar reasons, we hid barriers in the write*() macros, drivers need to stay architecture-agnostic as much as possible. Where does the regmap_write() end up? I think the barrier should be somewhere down this path. -- Catalin