| 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
| ||
|
Message-ID: <CAAyq3SbP4kSW47ZaT7V9ZzeHauj_EwfU5GPipZjNK6j69qm7Vw@mail.gmail.com>
Date: Mon, 7 Oct 2024 13:49:28 +0800
From: Cheng Ming Lin <linchengming884@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: vigneshr@...com, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org, richard@....at, alvinzhou@...c.com.tw,
leoyu@...c.com.tw, Cheng Ming Lin <chengminglin@...c.com.tw>
Subject: Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry
Hi Miquel,
Miquel Raynal <miquel.raynal@...tlin.com> 於 2024年10月1日 週二 下午5:40寫道：
>
> Hi Cheng Ming,
>
> linchengming884@...il.com wrote on Thu, 5 Sep 2024 13:53:32 +0800:
>
> > From: Cheng Ming Lin <chengminglin@...c.com.tw>
> >
> > Add fixups for support read retry:
> > - Initialize the NAND device maximum retry mode.
> > - Set feature on Special Read for Data Recovery register.
> >
> > The Special Read for Data Recovery operation is enabled by Set Feature
> > function.
> >
> > There are 5 modes for the user to recover the lost data.
> >
> > Signed-off-by: Cheng Ming Lin <chengminglin@...c.com.tw>
> > ---
> > drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++-------
> > include/linux/mtd/spinand.h | 17 +++++++
> > 2 files changed, 81 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
> > index 3f9e9c572854..baca67ff1cd6 100644
> > --- a/drivers/mtd/nand/spi/macronix.c
> > +++ b/drivers/mtd/nand/spi/macronix.c
> > @@ -9,6 +9,8 @@
> > #include <linux/kernel.h>
> > #include <linux/mtd/spinand.h>
> >
> > +#define MACRONIX_NUM_READ_RETRY_MODES 6
>
> You said 5 in the cover letter?
Since the original mode is labeled as default in our datasheet,
there are a total of six modes, including five additional modes
numbered from Mode 1 to Mode 5.
>
> > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70
>
> Both definitions should probably come...
>
> > #define SPINAND_MFR_MACRONIX 0xC2
> > #define MACRONIX_ECCSR_MASK 0x0F
>
> ...here
Sure, thanks for your advice.
>
> >
> > @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand,
> > return -EINVAL;
> > }
> >
> > +/**
> > + * macronix_spinand_init_read_retry - Initialize read_retries
> > + * @spinand: SPI NAND device
> > + *
> > + * Return: the number of read retry modes
> > + */
> > +static int macronix_spinand_init_read_retry(struct spinand_device *spinand)
> > +{
> > + return MACRONIX_NUM_READ_RETRY_MODES;
>
> Does not sound very useful as a function?
>
> > +}
> > +
> > +/**
> > + * macronix_spinand_setup_read_retry - Set the retry mode
> > + * @spinand: SPI NAND device
> > + * @retry_mode: Specify which retry mode to set
> > + *
> > + * Return: 0 on success, -error otherwise
>
> , a negative error code otherwise.
>
> > + */
> > +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode)
> > +{
> > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY,
> > + spinand->scratchbuf);
> > +
> > + *spinand->scratchbuf = retry_mode;
> > + return spi_mem_exec_op(spinand->spimem, &op);
> > +}
> > +
> > +static const struct spi_nand_fixups read_retry_fixups = {
> > + .init_read_retry = macronix_spinand_init_read_retry,
> > + .setup_read_retry = macronix_spinand_setup_read_retry,
> > +};
> > +
>
> ...
>
> > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = {
> > &update_cache_variants),
> > SPINAND_HAS_QE_BIT,
> > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
> > - mx35lf1ge4ab_ecc_get_status)),
> > + mx35lf1ge4ab_ecc_get_status),
> > + SPINAND_FIXUPS(&read_retry_fixups)),
> > SPINAND_INFO("MX35UF1GE4AC",
> > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01),
> > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
>
> I expect a patch targeting the core first, and then the changes in the
> Macronix driver.
Got it, so do you prefer that we switch to using flags instead?
>
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index 5c19ead60499..e567d00a2805 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -354,6 +354,7 @@ struct spinand_info {
> > } op_variants;
> > int (*select_target)(struct spinand_device *spinand,
> > unsigned int target);
> > + const struct spi_nand_fixups *fixups;
> > };
> >
> > #define SPINAND_ID(__method, ...) \
> > @@ -379,6 +380,9 @@ struct spinand_info {
> > #define SPINAND_SELECT_TARGET(__func) \
> > .select_target = __func,
> >
> > +#define SPINAND_FIXUPS(__func) \
> > + .fixups = __func,
>
> I don't like the fixups naming. It feels like something is inherently
> wrong.
>
> > +
> > #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \
> > __flags, ...) \
> > { \
> > @@ -398,6 +402,16 @@ struct spinand_dirmap {
> > struct spi_mem_dirmap_desc *rdesc_ecc;
> > };
> >
> > +/**
> > + * struct spi_nand_fixups - SPI NAND fixup hooks
> > + * @init_read_retry: initialize spinand->read_retries
> > + * @setup_read_retry: set the retry mode
> > + */
> > +struct spi_nand_fixups {
> > + int (*init_read_retry)(struct spinand_device *spinand);
> > + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode);
> > +};
> > +
> > /**
> > * struct spinand_device - SPI NAND device instance
> > * @base: NAND device instance
> > @@ -423,6 +437,7 @@ struct spinand_dirmap {
> > * the stack
> > * @manufacturer: SPI NAND manufacturer information
> > * @priv: manufacturer private data
> > + * @read_retries: the number of read retry modes supported
> > */
> > struct spinand_device {
> > struct nand_device base;
> > @@ -449,8 +464,10 @@ struct spinand_device {
> > u8 *databuf;
> > u8 *oobbuf;
> > u8 *scratchbuf;
> > + const struct spinand_info *info;
>
> This looks like a leftover.
>
> > const struct spinand_manufacturer *manufacturer;
> > void *priv;
> > + int read_retries;
>
> Any reason to keep this variable signed?
No, we can simply change from int to u8.
>
> > };
> >
> > /**
>
>
> Thanks,
> Miquèl
Thanks,
Cheng Ming Lin
Powered by blists - more mailing lists