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] [day] [month] [year] [list]

Message-ID: <mafs07cd42qog.fsf@kernel.org>
Date: Mon, 29 Jul 2024 16:55:43 +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>

Applied to spi-nor/next, thanks!

FYI, I spotted a couple small issues and fixed them up when applying.
See below...

> ---
>
> 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.
>
>  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)

Whitespace issue, checkpatch complains on this. It should be aligned
with the opening parenthesis above.

> +{
> +	u8 op = (len == 1) ? SPINOR_OP_BP : SPINOR_OP_AAI_WP;
> +	int ret;
> +
> +	nor->program_opcode = op;
> +	ret = spi_nor_write_data(nor, to, 1, buf);
> +	if (ret < 0)
> +		return ret;
> +	WARN(ret != len, "While writing %i byte written %i bytes\n", len, ret);

I get a build warning because of incorrect format specifier. Should use
%zu since len is size_t.

> +
> +	return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  			 size_t *retlen, const u_char *buf)
>  {
[...]

-- 
Regards,
Pratyush Yadav