Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Secumail-id: <354e.65017b0d.1a69.0>
Message-ID: <4537fa1c-b622-674c-026e-8453eda0a4d1@kalrayinc.com>
Date:   Wed, 13 Sep 2023 11:04:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] i2c: designware: Fix corrupted memory seen in the ISR
To:     Jan Bottorff <janb@os.amperecomputing.com>,
        Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Jan Dabros <jsd@semihalf.com>,
        Andi Shyti <andi.shyti@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230913010313.418159-1-janb@os.amperecomputing.com>
Content-Language: en-us
From:   Yann Sionneau <ysionneau@kalrayinc.com>
In-Reply-To: <20230913010313.418159-1-janb@os.amperecomputing.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: Ocr2VbArVOJfzMdKmIwvq6KAwadAr2QZ/pGytG/a2pi5uZdhCgal8lSM+/wWoqGC7rOIMg3n/nHwUKcrrouBfuS8oa3VRAzV9HbsdJGA1aH0tGBiurDqkGiAJWS2znrMhKQVKpIEYtXcxrDi+om0e+mybQJbSeJSMmxt+rAKM/oqw/+0xu1gzOny0hNQW6OTpi/zPcE+O0vv2HWk3+ji/zAmABbNqHPc7Kx6B1NPPrlQf6m7/5oY7VW3a4NEzkd/f/Xt+P/EUcVA2KdrVfLz5a+ADWB/Hxs8awjrUir5Xxdk2AVORXquwH8HGFV6L8jluI0BmCCzsECxHMTTm2vJPDfFmJmFFoh8N5pvzpaorfLop8oE9fJv3aoAYAQQmvu3EjJZqC8zyhQIO8fQH1j/FAi21zYrXfHnKqSghONCsdavt3kvoOz6H2BRXtSArPrVkERHYVFwipwogwNWjC9hBr+FT3CsXXZMxDDcKSCfszcixgwkAMHOqEazxu/BUKo/PUy5kURQ2dk6QgaiQxoIdGxF6bNw/JuT3sB1aubTwTO7zTsm6bhWnkTmoOodhgy2dvTSAfFi8ObyjVF5BE0x9abxaqgnkR6GJ3uCyIIKKUrV197MqUqJgy3zdsdH+bIJZYy4qxB59vbx30QLw3iFiwnJMFqtUjpntZ0f7OtcGVWSs+8cC2UJ133e0A9yfYCvUh2vk1EaRnBm4tirzZjjxQHsViw1vvX/LDTG1TEWi23L9LqPXfHNqy3f5zFHvYskT4KXhSnypQrf/iyr8ItbI4oAHKBB7MwBVnvxsmICiiC2u0j/+FycgXYmHYrRq0IDtTtBho0KjUSYVrhg/9SavZqj3WlNI1JzH4vtOYPDx14GaOujI23/IWFBcRUw1rzJNFGJEVt8+3/uZ000tXB/uyRZhXDqoQmWhCwULLDzAMpN02FVc5TPJmPLGxRSwgcR
 br1tAQNtdiooAmBP1faY+/tLjb3F9JP0KjGTSgv8nfLnbDVd0bS10SEkJqoh2Ea7SrBDKKhNa8UagXjYfHW3nSwAQ0gWJNzo0LkaMNWjUEfHcCb0cRQmcOpuViahQdXFsswyWbgzloTLgDZZaUO+psYY00YzL/NC5vKsyg9JZfD0wGV7xZsZDXFfiAJ5nlZfdHMW02W9PWXG4K5Q27M7bwNfaHkN8uiXkzKYIEAXDr/pWclnczz35c3l9sp44kP0W9xRN10uneb34wYpnPmXWRe1GDxqPzBPpL8WD4O13lRrjkst3ZdWorufXUM9dz45WteBMNxAKV5hrHEdcw4qiGx97NQCRmDfbiYG07oh71nAuCgzZ3iWw6GgH6ujA1tgkUNI+VnTYAFjUrBA6KCT60K2NRKrXYLxRu7eMsVPGeUuDD/rtQ35kW25Y4lcSpSHMgMqeGRcBLg3JAt/C3Ef+nh2fgqjtZLJZ8lak5cl/TfPKSQjFp2Kfr9gdT6mLv7kjVsfVuI06zxgPBJpbw1Q9GDKU5bYewFd/i/8xH+ZOJe0D523wl/JNvVQiUvuz9PDPJWIm56twp7NfxQQ9mmcpN6/+ZuJm3yq8zfz531X/EUIlmbKlQDnVwPtdYgiPW/Qrg1hVPn8T7DYmpPFgwnaCvQWpYyCkRAD0iR5BAC984u6he278y8Dq3kDe7bFN9MHEtTeyLq+HRh0wD5S4QJfGg==
X-OriginatorOrg: kalrayinc.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 638b07f5-8250-40ad-ceae-08dbb438657a
X-MS-Exchange-CrossTenant-AuthSource: MR1P264MB1890.FRAP264.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2023 09:04:11.8349
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8931925d-7620-4a64-b7fe-20afd86363d3
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aMMm2qkbbgr/10Qj/OlXSxDsXHLbM6uGP4Ss0t5rv5fDhAoIof+0wFJ3jETQlbFhwadaC1ZwxhPWDXrmuVIzNg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MRZP264MB3049
X-ALTERMIMEV2_out: done
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 13 Sep 2023 02:04:25 -0700 (PDT)

Hi Jan,

On 13/09/2023 03:03, Jan Bottorff wrote:
> Errors were happening in the ISR that looked like corrupted
> memory. This was because writes from the core enabling interrupts
> where not yet visible to the core running the ISR. A write barrier
Typo where => were
> assures writes to driver data structures are visible to all cores
> before interrupts are enabled.
Maybe rephrase this using the direct style describing what the commit 
does like "Add a write barrier before enabling interrupts to assure [...]"
>
> The ARM Barrier Litmus Tests and Cookbook has an example under
> Sending Interrupts and Barriers that matches the usage in this
> driver. That document says a DSB barrier is required.
>
> Signed-off-by: Jan Bottorff <janb@os.amperecomputing.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> 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();
> +
>   	/* Clear and enable interrupts */
>   	regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
>   	regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK);

Apart from the commit message it looks good to me.

If I understand correctly without this wmb() it is possible that the 
writes to dev->msg_write_idx , dev->msg_read_idx = 0 etc would not yet 
be visible to another CPU running the ISR handler right after enabling 
those.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>

Tested-by: Yann Sionneau <ysionneau@kalrayinc.com>

Thanks!

Regards,

-- 

Yann