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 CF2932877F6; Fri, 13 Mar 2026 11:46:41 +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=1773402401; cv=none; b=t+6Oul0Qv6v/Qbor9YWA8uIY9j4jXlk2ByJI3n/tdqJzge1/D8c/Da/R9nb0ms52lSYulCJjvrOyzKoFZfUK6ZyBpCX69ClCdZXk0EHmk57ul4pSZLz2Ix165Tt/eyOqrF/5bwuKTOjXNK5Pkvv7prSiKcf0GqGV83MbwCTZi/g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773402401; c=relaxed/simple; bh=MKpOcpqNGirnFljtrZqJxY8MJ+ogzgLHLzhDGY7ue5Q=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=d+b0ELSgiacd436Q9kwpmtJuSNjeJ8962dEu60KaedrGPEiRG3R8f7Ef+66wAiAI7f243FgZbTYJ4HIifHuRxlogW5xzri3JoLOateTUDbxGa7qxdC1/IEA75jQK/Qmg0/ZBXR5YlKtg7SK1FN4amyFdOVZd0myD0xFUeR7x8tM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=l18N0UmG; 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="l18N0UmG" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4BD1EC19421; Fri, 13 Mar 2026 11:46:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773402401; bh=MKpOcpqNGirnFljtrZqJxY8MJ+ogzgLHLzhDGY7ue5Q=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=l18N0UmGqs8YhyTgBxhgsryVSokfa0KC9rRLhpCJcfCuwTu6UHZLHiZZv3r63bfP4 zNLhiJJelDcn4NLand0iZ2QvMxCsKIgYSF97upXRKiUBeQSYH5An6lOtU7dz4e4p0H RXM5bjIJuy1SJxUJurGpsw/XpizQ9kZDg0rGIpp9pjKxaguXl67kmWEhTS68VJdUfA Wed5QGqCqXXqUL79wLeRPR7y2snvwso3d7rlzFoyi4IlCDVMdfL+5EzUSIZD3jG7QE pZhe3J8El830NhFLB6Y3EqL39jRP9sTGj5BTj7GQuZJxHEULlXcMy9pdmuYmKJdAl+ QQNdwP99XkDHQ== From: Pratyush Yadav To: Hendrik Donner Cc: Michael Walle , Sanjaikumar V S , linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com, pratyush@kernel.org, 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: <8a1db3a5-09b3-4ef1-87e8-66553a81ec27@os-cillation.de> (Hendrik Donner's message of "Fri, 6 Mar 2026 23:36:03 +0100") References: <20260223091733.47-1-sanjaikumarvs@gmail.com> <8a1db3a5-09b3-4ef1-87e8-66553a81ec27@os-cillation.de> Date: Fri, 13 Mar 2026 11:46:37 +0000 Message-ID: <2vxzpl58dk9u.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 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. > nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val, > op.data.nbytes, op.data.buf.out); > } else { > > > I think patch 2 of this series is the better approach though. There is a v4 here: https://lore.kernel.org/linux-mtd/20260311103057.29-1-sanjaikumarvs@gmail.com/T/#u Can you please review and test it so we can apply it? > > Regards, > Hendrik > >>> Please let me know if you'd like me to send a v3 with the >>> simplified unconditional write_enable. >> Please see above. >> -michael >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ > -- Regards, Pratyush Yadav