From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 28ADC392802; Fri, 13 Mar 2026 13:39:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773409177; cv=none; b=Axu0odPMfD5d5Uko9qzRUX3sQw89/euWgWgh1bSbQR+HsWEHzDC91MzThb2VYM90qsZD7+lvM/ZXqD/xkJy2zxGj9Zv104z3SIdyTyg98N8UdU6XiB67VrPSUNseDu5hyNKujOWp+jxaIy4gAqRqek+9CydgNkHH92Q+gFZ+9Jc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773409177; c=relaxed/simple; bh=xnp+Riyh/w0YpgxEvhFWS/9bdMiL79ATIFLfP7pHKwI=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=J0fBfIvLXxTdduhOmG7YDfrxQhs/EVR1euXOfmCQju5b6djes5SCh1J7zAWanp0yKtEDyrPOVP0PdLrvhJcKZyzYr7XtlV4g8igHwuD5GvOfcIZfTVMs9HWpJJA2GyD1FZeZACjA8ccD6lQ+F+uG45TSMFO7fNUbdsGwAQ+bAck= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=uq+3PqYk; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="uq+3PqYk" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5AD73C19425; Fri, 13 Mar 2026 13:39:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773409176; bh=xnp+Riyh/w0YpgxEvhFWS/9bdMiL79ATIFLfP7pHKwI=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=uq+3PqYk5bxRdIZxNbbFeNn6Z2tT9FmuCHBT9bGu23AKK/FAm/m42z16iZ3L/IMoJ u6aIcaigAKV2VxMz9c30y1iwOrvAL5f/FzeJTzva5tMrBWkbXIXBFEiY58G4hcKFK4 OvzixvLmvGLntJy3+xMTnRU6dt0h1rD85ZDdStA2yTUwudy3TVvdvHVlAey+feOuLK 9juPeU/F2JxozjlkQPtXsx4uhLE7IYIVcyT1kDluoiUXdxmjZmR2pLs9UOE82UA8fK HkDaiej8qSJhVncd5R260aZlb53SEwjvD4f7A2mFT8393hFF1Xu7NX5TjfFOAazmv8 2N6bxXhE+OihQ== From: Pratyush Yadav To: Hendrik Donner Cc: Pratyush Yadav , Michael Walle , Sanjaikumar V S , linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com, richard@nod.at, sanjaikumar.vs@dicortech.com, stable@vger.kernel.org, tudor.ambarus@linaro.org, vigneshr@ti.com Subject: Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence In-Reply-To: (Hendrik Donner's message of "Fri, 13 Mar 2026 13:50:42 +0100") References: <20260223091733.47-1-sanjaikumarvs@gmail.com> <8a1db3a5-09b3-4ef1-87e8-66553a81ec27@os-cillation.de> <2vxzpl58dk9u.fsf@kernel.org> Date: Fri, 13 Mar 2026 13:39:32 +0000 Message-ID: <2vxz8qbvetm3.fsf@kernel.org> User-Agent: Gnus/5.13 (Gnus v5.13) Precedence: bulk X-Mailing-List: stable@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Fri, Mar 13 2026, Hendrik Donner wrote: > Hello, > > On 3/13/26 12:46, Pratyush Yadav wrote: >> On Fri, Mar 06 2026, Hendrik Donner wrote: >> >>> Hello, >>> >>> On 2/23/26 10:29, Michael Walle wrote: >>>> Hi, >>>> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote: >>>>>> Raises concern about writes ending at odd offsets potentially >>>>>> having the same issue >>>>> >>>>> The odd end address case (trailing byte) is already handled in the >>>>> existing code at lines 243-255: >>>>> >>>>> /* Write out trailing byte if it exists. */ >>>>> if (actual != len) { >>>>> ret = spi_nor_write_enable(nor); >>>>> ... >>>>> ret = sst_nor_write_data(nor, to, 1, buf + actual); >>>>> } >>>> Ah, I must be blind. I stopped reading at the write_disable. >>>> >>>>> So write_enable is already called before writing the trailing >>>>> byte. My patch only addresses the odd start case where BP clears >>>>> WEL before the AAI sequence begins. >>>>> >>>>>> Suggests simplifying the conditional logic by removing the length >>>>>> check >>>>> >>>>> The condition `if (actual < len - 1)` avoids an unnecessary >>>>> write_enable when len == 1 (single byte write at odd address, no >>>>> AAI follows). But if you prefer unconditional write_enable for >>>>> simplicity, I can change it in v3. >>>> I know, but I actually don't like repeating the condition in the for >>>> loop. So I'd prefer to have a local "needs_write_enable" boolean >>>> which will be set to true. But then, I wouldn't care too much if >>>> there is a write enable followed by a write disable for a rare case. >>>> >>>>>> Notes the patch lacks runtime testing >>>>> >>>>> I don't have the hardware setup to test odd-address writes at the >>>>> moment. The fix is based on code analysis. I have tested patch 2/2 >>>>> (dirmap fallback) on hardware. >>>> I'm hesitant - because like I said, if there is really a bug - it >>>> would have never worked correctly, since day 1. But yeah, I've also >>>> read the datasheet and it clearly states that the byte write will >>>> clear the write enable latch. >>>> >>> >>> i can confirm both patches fix real issues, i have similiar fixes >>> on a kernel tree i always wanted to clean up and upstream. Diffs >>> based on 6.6.127: >>> >>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c >>> index 197d2c1101ed5..eaa50561ede2c 100644 >>> --- a/drivers/mtd/spi-nor/sst.c >>> +++ b/drivers/mtd/spi-nor/sst.c >>> @@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len, >>> if (ret) >>> goto out; >>> >>> + ret = spi_nor_write_enable(nor); >>> + if (ret) >>> + goto out; >>> + ret = spi_nor_wait_till_ready(nor); >>> + if (ret) >>> + goto out; >>> + >>> to++; >>> actual++; >>> } >>> >>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index 1b0c6770c14e4..646bfb2e91a65 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >>> @@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to, >>> if (spi_nor_spimem_bounce(nor, &op)) >>> memcpy(nor->bouncebuf, buf, op.data.nbytes); >>> >>> - if (nor->dirmap.wdesc) { >>> + if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) { >> Why is this better? This removes the use of dirmap for all flashes other >> than SST. > > i claim the opposite down below? That patch 2 of the posted patch series > looks better to me. Sorry if that was unclear. Oh, then I misunderstood. Please review and test the v4, it will help land those fixes. [...] -- Regards, Pratyush Yadav