linux-kernel - Re: [PATCH] mtd: spi-nor: sst: Factor out common write operation to `sst_nor_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <mafs07cdto0t9.fsf@kernel.org>
Date: Wed, 10 Jul 2024 15:04:34 +0200
From: Pratyush Yadav <pratyush@...nel.org>
To: Csókás, Bence <csokas.bence@...lan.hu>
Cc: <linux-mtd@...ts.infradead.org>,  <linux-kernel@...r.kernel.org>,
  "Tudor Ambarus" <tudor.ambarus@...aro.org>,  Pratyush Yadav
 <pratyush@...nel.org>,  Michael Walle <mwalle@...nel.org>,  Miquel Raynal
 <miquel.raynal@...tlin.com>,  Richard Weinberger <richard@....at>,
  Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH] mtd: spi-nor: sst: Factor out common write operation to
 `sst_nor_write_data()`

Hi,

On Wed, Jul 10 2024, Csókás, Bence wrote:

> Writing to the Flash in `sst_nor_write()` is a 3-step process:
> first an optional one-byte write to get 2-byte-aligned, then the
> bulk of the data is written out in vendor-specific 2-byte writes.
> Finally, if there's a byte left over, another one-byte write.
> This was implemented 3 times in the body of `sst_nor_write()`.
> To reduce code duplication, factor out these sub-steps to their
> own function.
>
> Signed-off-by: Csókás, Bence <csokas.bence@...lan.hu>
> ---
>
> Notes:
>     RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>     SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>     is the "standard" name for the elementary unit-sized
>     (1 byte, in the case of NOR) write operation. I find it
>     confusing to have two names for the same operation,
>     so in a followup I plan to remove the vendor-specific
>     name in favor of the standard one.

Even though the operations have the same opcode, I see them as different
operations. One is a byte program: it can only write one byte at a time.
The other is a page program: it can write up to one page (256 bytes
usually) at a time.

So I would actually find it more confusing if you use page program in a
situation where the operation is actually a byte program, and attempting
to program the whole page will fail.

>
>  drivers/mtd/spi-nor/sst.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 180b7390690c..fec71689e644 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -167,6 +167,21 @@ static const struct flash_info sst_nor_parts[] = {
>  	}
>  };
>  
> +static int sst_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> +			const u_char *buf)
> +{
> +	u8 op = (len == 1) ? SPINOR_OP_BP : SPINOR_OP_AAI_WP;

Hmm, this looks a bit hacky. But the whole sst_nor_writa() is kinda
hacky anyway so it is fine I guess... LGTM otherwise.

Reviewed-by: Pratyush Yadav <pratyush@...nel.org>

Not directly related to this patch, but when reviewing this patch I
noticed another small improvement you can make. since you are already
looking at this code. (to be clear, this is not needed for this patch to
get merged, this can be a follow up patch).

sst_nor_write() looks like:

	/* Write out most of the data here. */
	for (; actual < len - 1; actual += 2) {
        	[...]
	}
	nor->sst_write_second = false;

	ret = spi_nor_write_disable(nor);
	if (ret)
		goto out;

	ret = spi_nor_wait_till_ready(nor);
	if (ret)
		goto out;

	/* Write out trailing byte if it exists. */
	if (actual != len) {
		ret = spi_nor_write_enable(nor);
                [...]

		ret = spi_nor_write_data(nor, to, 1, buf + actual);
                [...]

		ret = spi_nor_wait_till_ready(nor);
		if (ret)
			goto out;


		ret = spi_nor_write_disable(nor);
	}

Here, we do a write disable. Then if a one-byte write is needed, do a
write enable again, write the data and write disable.

Do we really need to toggle write enable between these? If not, it can
be simplified to only do the write disable after all bytes have been
written.

[...]

-- 
Regards,
Pratyush Yadav