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-next>] [day] [month] [year] [list]
Message-ID:
 <SE2P216MB2102DA0C880653E08D03747383222@SE2P216MB2102.KORP216.PROD.OUTLOOK.COM>
Date: Tue, 5 Mar 2024 02:28:31 +0000
From: Kyeongrho.Kim <kr.kim@...highmemory.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
CC: "richard@....at" <richard@....at>, "vigneshr@...com" <vigneshr@...com>,
	"mmkurbanov@...utedevices.com" <mmkurbanov@...utedevices.com>,
	"ddrokosov@...rdevices.ru" <ddrokosov@...rdevices.ru>, Kyeongrho.Kim
	<kr.kim@...highmemory.com>, "gch981213@...il.com" <gch981213@...il.com>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Mohamed Sardi
	<moh.sardi@...highmemory.com>, Changsub.Shim
	<changsub.shim@...highmemory.com>
Subject: RE: [PATCH] Skyhighmemory

Hello Miquel,

Please find the reply in below.

I tried to send an email using 'git send-email', but '5.1.3 Invalid address' error is occurring, so I send it to Outlook first.

Please help me solve this problem.



Thanks,

KR

-----Original Message-----
From: Miquel Raynal <miquel.raynal@...tlin.com>
Sent: Friday, February 23, 2024 6:02 PM
To: Kyeongrho.Kim <kr.kim@...highmemory.com>
Cc: richard@....at; vigneshr@...com; Mohamed Sardi <moh.sardi@...highmemory.com>; Changsub.Shim <changsub.shim@...highmemory.com>; linux-mtd@...ts.infradead.org
Subject: Re: [PATCH] Skyhighmemory



Hello Kim,



Thanks for your contribution, a few comments below.



First, please consider always adding the relevant Maintainers and List in Copy, I already reviewed the patch so here is my answer, but next time I will not answer if you send another message in private.



kr.kim@...highmemory.com<mailto:kr.kim@...highmemory.com> wrote on Fri, 23 Feb 2024 15:35:22 +0900:



> From: “Kyeongrho <kr.kim@...highmemory.com<mailto:kr.kim@...highmemory.com>>



Please write a commit log and fix the commit title.



> Signed-off-by: “Kyeongrho <kr.kim@...highmemory.com<mailto:kr.kim@...highmemory.com>>

> ---

>  drivers/mtd/nand/spi/Makefile  |   2 +-

>  drivers/mtd/nand/spi/core.c    |   7 +-

>  drivers/mtd/nand/spi/skyhigh.c | 191 +++++++++++++++++++++++++++++++++

>  include/linux/mtd/spinand.h    |   3 +

>  4 files changed, 201 insertions(+), 2 deletions(-)  mode change

> 100644 => 100755 drivers/mtd/nand/spi/Makefile  mode change 100644 =>

> 100755 drivers/mtd/nand/spi/core.c  create mode 100644

> drivers/mtd/nand/spi/skyhigh.c  mode change 100644 => 100755

> include/linux/mtd/spinand.h

>

> diff --git a/drivers/mtd/nand/spi/Makefile

> b/drivers/mtd/nand/spi/Makefile old mode 100644 new mode 100755 index

> 19cc77288ebb..48b429d90460

> --- a/drivers/mtd/nand/spi/Makefile

> +++ b/drivers/mtd/nand/spi/Makefile

> @@ -1,4 +1,4 @@

>  # SPDX-License-Identifier: GPL-2.0

>  spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o

> gigadevice.o macronix.o -spinand-objs += micron.o paragon.o toshiba.o

> winbond.o xtx.o

> +spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o

> +skyhigh.o



Please keep the alphabetical ordering.

Yes, it is update like below.

+spinand-objs += micron.o paragon.o skyhigh.o toshiba.o winbond.o xtx.o





>  obj-$(CONFIG_MTD_SPI_NAND) += spinand.o diff --git

> a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c old mode

> 100644 new mode 100755 index e0b6715e5dfe..e3f0a7544ba4

> --- a/drivers/mtd/nand/spi/core.c

> +++ b/drivers/mtd/nand/spi/core.c

> @@ -34,7 +34,7 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)

>     return 0;

>  }

>

> -static int spinand_write_reg_op(struct spinand_device *spinand, u8

> reg, u8 val)

> +int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8

> +val)



If this is really needed, please do it in a preparation patch. But TBH I'm not really sure it is needed.

That function is needed because it is called by "skyhigh.c".

>  {

>     struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,

>                                         spinand->scratchbuf);

> @@ -196,6 +196,10 @@ static int spinand_init_quad_enable(struct

> spinand_device *spinand)  static int spinand_ecc_enable(struct spinand_device *spinand,

>                       bool enable)

>  {

> +   /* SHM : always ECC enable */

> +   if (spinand->flags & SPINAND_ON_DIE_ECC_MANDATORY)

> +         return 0;

> +



Can you please move this logic to your vendor driver?

SHM's SPI Nand must have “ECC Always on” applied.

However, I think there is no way to be moved to the vendor driver.



>     return spinand_upd_cfg(spinand, CFG_ECC_ENABLE,

>                        enable ? CFG_ECC_ENABLE : 0);  } @@ -945,6 +949,7 @@ static

> const struct spinand_manufacturer *spinand_manufacturers[] = {

>     &macronix_spinand_manufacturer,

>     &micron_spinand_manufacturer,

>     &paragon_spinand_manufacturer,

> +   &skyhigh_spinand_manufacturer,

>     &toshiba_spinand_manufacturer,

>     &winbond_spinand_manufacturer,

>     &xtx_spinand_manufacturer,

> diff --git a/drivers/mtd/nand/spi/skyhigh.c

> b/drivers/mtd/nand/spi/skyhigh.c new file mode 100644 index

> 000000000000..71de4fa34406

> --- /dev/null

> +++ b/drivers/mtd/nand/spi/skyhigh.c

> @@ -0,0 +1,191 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * Copyright (c) 2022 SkyHigh Memory Limited

> + *

> + * Author: Takahiro Kuwano <takahiro.kuwano@...ineon.com<mailto:takahiro.kuwano@...ineon.com>>  */

> +

> +#include <linux/device.h>

> +#include <linux/kernel.h>

> +#include <linux/mtd/spinand.h>

> +

> +#define SPINAND_MFR_SKYHIGH      0x01

> +

> +#define SKYHIGH_STATUS_ECC_1TO2_BITFLIPS     (1 << 4)

> +#define SKYHIGH_STATUS_ECC_3TO4_BITFLIPS     (2 << 4)

> +#define SKYHIGH_STATUS_ECC_5TO6_BITFLIPS     (3 << 4)

> +

> +#define SKYHIGH_CONFIG_PROTECT_EN BIT(1)

> +

> +static SPINAND_OP_VARIANTS(read_cache_variants,

> +         SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 4, NULL, 0),

> +         SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

> +         SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 2, NULL, 0),

> +         SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),

> +         SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),

> +         SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));

> +

> +static SPINAND_OP_VARIANTS(write_cache_variants,

> +         SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),

> +         SPINAND_PROG_LOAD(true, 0, NULL, 0));

> +

> +static SPINAND_OP_VARIANTS(update_cache_variants,

> +         SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),

> +         SPINAND_PROG_LOAD(false, 0, NULL, 0));

> +

> +static int skyhigh_spinand_ooblayout_ecc(struct mtd_info *mtd, int section,

> +                           struct mtd_oob_region *region)

> +{

> +   if (section)

> +         return -ERANGE;

> +

> +   region->length = 0;

> +   region->offset = mtd->oobsize;



The ECC bytes are not stored anywhere?

There is no problem because skyhigh's ecc parity is stored in the internal hidden area and is not needed for them.

> +

> +   return 0;

> +}

> +

> +static int skyhigh_spinand_ooblayout_free(struct mtd_info *mtd, int section,

> +                             struct mtd_oob_region *region) {

> +   if (section)

> +         return -ERANGE;

> +

> +   region->length = mtd->oobsize - 2;

> +   region->offset = 2;

> +

> +   return 0;

> +}

> +

> +static const struct mtd_ooblayout_ops skyhigh_spinand_ooblayout = {

> +   .ecc = skyhigh_spinand_ooblayout_ecc,

> +   .free = skyhigh_spinand_ooblayout_free, };

> +

> +#if 0



Please drop debug code from your submission.

It was dropped.



> +bool skyhigh_spinand_isbad(struct spinand_device *spinand,

> +                  const struct nand_pos *pos)

> +{

> +   u8 marker;

> +   struct nand_page_io_req req = {

> +         .pos = *pos,

> +         .ooblen = 1,

> +         .ooboffs = 0,

> +         .oobbuf.in = &marker,

> +         .mode = MTD_OPS_RAW,

> +   };

> +

> +   req.pos.page = 0;

> +   spinand_read_page(spinand, &req);

> +   if (marker != 0xff)

> +         return true;

> +

> +#if 0

> +   req.pos.page = 1;

> +   spinand_read_page(spinand, &req);

> +   if (marker != 0xff)

> +         return true;

> +

> +   req.pos.page = 63;

> +   spinand_read_page(spinand, &req);

> +   if (marker != 0xff)

> +         return true;

> +#endif

> +

> +   return false;

> +}

> +#endif

> +

> +static int skyhigh_spinand_ecc_get_status(struct spinand_device *spinand,

> +                       u8 status)

> +{

> +   /* SHM

> +   00 : No bit-flip

> +   01 : 1-2 errors corrected

> +   10 : 3-6 errors corrected

> +   11 : uncorrectable

> +   */



The style is wrong, checkpatch.pl --strict?

It was updated.



> +

> +   switch (status & STATUS_ECC_MASK) {

> +   case STATUS_ECC_NO_BITFLIPS:

> +         return 0;

> +

> +   case SKYHIGH_STATUS_ECC_1TO2_BITFLIPS:

> +         return 2;

> +

> +   /* change from 4 to 6 */

> +   case SKYHIGH_STATUS_ECC_3TO4_BITFLIPS:

> +         return 6;

> +

> +   /* uncorrectable for '11' */

> +   case SKYHIGH_STATUS_ECC_5TO6_BITFLIPS:

> +         return -EBADMSG;;



The last two macro names definitely don't match what they actually mean. It's very strange. Please align the names with the real impact.

The Macros were updated.



> +

> +   default:

> +         break;

> +   }

> +

> +   return -EINVAL;

> +}

> +

> +static const struct spinand_info skyhigh_spinand_table[] = {

> +   SPINAND_INFO("S35ML01G301",

> +              SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x15),

> +              NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),

> +              NAND_ECCREQ(6, 32),

> +              SPINAND_INFO_OP_VARIANTS(&read_cache_variants,

> +                                 &write_cache_variants,

> +                                 &update_cache_variants),

> +              SPINAND_ON_DIE_ECC_MANDATORY,

> +              SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,

> +                           skyhigh_spinand_ecc_get_status)),

> +   SPINAND_INFO("S35ML01G300",

> +              SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14),

> +              NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),

> +              NAND_ECCREQ(6, 32),

> +              SPINAND_INFO_OP_VARIANTS(&read_cache_variants,

> +                                 &write_cache_variants,

> +                                 &update_cache_variants),

> +              SPINAND_ON_DIE_ECC_MANDATORY,

> +              SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,

> +                           skyhigh_spinand_ecc_get_status)),

> +   SPINAND_INFO("S35ML02G300",

> +              SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),

> +              NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),

> +              NAND_ECCREQ(6, 32),

> +              SPINAND_INFO_OP_VARIANTS(&read_cache_variants,

> +                                 &write_cache_variants,

> +                                 &update_cache_variants),

> +              SPINAND_ON_DIE_ECC_MANDATORY,

> +              SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,

> +                           skyhigh_spinand_ecc_get_status)),

> +   SPINAND_INFO("S35ML04G300",

> +              SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35),

> +              NAND_MEMORG(1, 2048, 128, 64, 4096, 80, 2, 1, 1),

> +              NAND_ECCREQ(6, 32),

> +              SPINAND_INFO_OP_VARIANTS(&read_cache_variants,

> +                                 &write_cache_variants,

> +                                 &update_cache_variants),

> +              SPINAND_ON_DIE_ECC_MANDATORY,

> +              SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,

> +                           skyhigh_spinand_ecc_get_status)), };

> +

> +static int skyhigh_spinand_init(struct spinand_device *spinand) {

> +   return spinand_write_reg_op(spinand, REG_BLOCK_LOCK,

> +                         SKYHIGH_CONFIG_PROTECT_EN);

> +}

> +

> +static const struct spinand_manufacturer_ops skyhigh_spinand_manuf_ops = {

> +   .init = skyhigh_spinand_init,

> +/* .isbad = skyhigh_spinand_isbad,*/



Drop this line as well

It was dropped.



> +};

> +

> +const struct spinand_manufacturer skyhigh_spinand_manufacturer = {

> +   .id = SPINAND_MFR_SKYHIGH,

> +   .name = "SkyHigh",

> +   .chips = skyhigh_spinand_table,

> +   .nchips = ARRAY_SIZE(skyhigh_spinand_table),

> +   .ops = &skyhigh_spinand_manuf_ops,

> +};

> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h

> old mode 100644 new mode 100755 index badb4c1ac079..0e135076df24

> --- a/include/linux/mtd/spinand.h

> +++ b/include/linux/mtd/spinand.h

> @@ -268,6 +268,7 @@ extern const struct spinand_manufacturer

> gigadevice_spinand_manufacturer;  extern const struct

> spinand_manufacturer macronix_spinand_manufacturer;  extern const

> struct spinand_manufacturer micron_spinand_manufacturer;  extern const

> struct spinand_manufacturer paragon_spinand_manufacturer;

> +extern const struct spinand_manufacturer

> +skyhigh_spinand_manufacturer;

>  extern const struct spinand_manufacturer

> toshiba_spinand_manufacturer;  extern const struct

> spinand_manufacturer winbond_spinand_manufacturer;  extern const

> struct spinand_manufacturer xtx_spinand_manufacturer; @@ -312,6 +313,7

> @@ struct spinand_ecc_info {

>

>  #define SPINAND_HAS_QE_BIT        BIT(0)

>  #define SPINAND_HAS_CR_FEAT_BIT         BIT(1)

> +#define SPINAND_ON_DIE_ECC_MANDATORY   BIT(2) /* SHM */

>

>  /**

>   * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine

> structure @@ -518,5 +520,6 @@ int spinand_match_and_init(struct

> spinand_device *spinand,

>

>  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);

> int spinand_select_target(struct spinand_device *spinand, unsigned int

> target);

> +int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8

> +val);

>

>  #endif /* __LINUX_MTD_SPINAND_H */





Thanks,

Miquèl

Content of type "text/html" skipped

Download attachment "0002-Skyhighmemory2.patch" of type "application/octet-stream" (3587 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ