| 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: <MN2PR08MB5951A622B36705BC26CE36E2B8340@MN2PR08MB5951.namprd08.prod.outlook.com>
Date: Thu, 2 May 2019 11:37:10 +0000
From: "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@...ron.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
CC: Boris Brezillon <bbrezillon@...nel.org>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Richard Weinberger <richard@....at>,
David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
Marek Vasut <marek.vasut@...il.com>,
Frieder Schrempf <frieder.schrempf@...eet.de>
Subject: RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Enabled support to detect
parameter page
Hi Miquel,
>
> Hi Shivamurthy,
>
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@...ron.com> wrote
> on
> Tue, 26 Mar 2019 10:52:00 +0000:
>
> > Some of the SPI NAND devices has parameter page which is similar to ONFI
> > table.
> >
> > But, it may not be self sufficient to propagate all the required
> > parameters. Fixup function has been added in struct manufacturer to
> > accommodate this.
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@...ron.com>
> > ---
> > drivers/mtd/nand/spi/core.c | 113
> +++++++++++++++++++++++++++++++++++-
> > include/linux/mtd/spinand.h | 5 ++
> > 2 files changed, 117 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 985ad52cdaa7..40882a1d2bc1 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -574,6 +574,108 @@ static int spinand_lock_block(struct
> spinand_device *spinand, u8 lock)
> > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> > }
> >
> > +/**
> > + * spinand_read_param_page_op - Read parameter page operation
> > + * @spinand: the spinand device
> > + * @page: page number where parameter page tables can be found
> > + * @parameters: buffer used to store the parameter page
>
> Does not match the prototype
I will fix this in next version.
>
> > + * @len: length of the buffer
> > + *
> > + * Read parameter page
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +static int spinand_parameter_page_read(struct nand_device *base,
>
> Please use a spinand structure as parameter, you don't need a
> nand_device here (same for other spinand functions).
This function is helper function for generic ONFI layer.
From generic ONFI layer, I can get only nand_device.
>
> > + u8 page, void *buf, unsigned int len)
> > +{
> > + struct spinand_device *spinand = nand_to_spinand(base);
> > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > + struct spi_mem_op pread_cache_op =
> > +
> SPINAND_PAGE_READ_FROM_CACHE_OP(false,
> > + 0,
> > + 1,
> > + buf,
> > + len);
> > + u8 feature;
> > + u8 status;
> > + int ret;
> > +
> > + if (len && !buf)
> > + return -EINVAL;
> > +
> > + ret = spinand_read_reg_op(spinand, REG_CFG,
> > + &feature);
> > + if (ret)
> > + return ret;
> > +
> > + /* CFG_OTP_ENABLE is used to enable parameter page access */
> > + feature |= CFG_OTP_ENABLE;
> > +
> > + spinand_write_reg_op(spinand, REG_CFG, feature);
> > +
> > + ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > + if (ret)
> > + return ret;
> > +
> > + ret = spinand_wait(spinand, &status);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > + if (ret)
> > + return ret;
> > +
> > + ret = spinand_read_reg_op(spinand, REG_CFG,
> > + &feature);
> > + if (ret)
> > + return ret;
> > +
> > + feature &= ~CFG_OTP_ENABLE;
> > +
> > + spinand_write_reg_op(spinand, REG_CFG, feature);
> > +
> > + return 1;
>
> Should return 0
I will fix this in next version.
>
> > +}
> > +
> > +static int check_version(struct nand_device *base,
> > + struct nand_onfi_params *p, int *onfi_version)
> > +{
> > + /**
>
> I don't think you need these /** here, just use /*
I will fix this in next version.
>
> > + * SPI NAND do not support ONFI standard
>
> What about:
>
> /*
> * SPI NANDs do not necessarily support ONFI standard, but
> * parameter page looks the same as an ONFI table.
> */
>
This looks better, I will use this.
> > + * But, parameter page looks same as ONFI table
> > + */
> > + if (!le16_to_cpu(p->revision))
> > + *onfi_version = 0;
> > +
> > + return 1;
>
> Functions should return 0 in normal state, not 1.
>
I will fix this in next version.
> > +}
> > +
> > +static int spinand_intf_data(struct nand_device *base,
> > + struct nand_onfi_params *p)
> > +{
> > + struct spinand_device *spinand = nand_to_spinand(base);
> > +
> > + /**
> > + * Manufacturers may interpret the parameter page differently
>
> ^^^^^ extra spaces
>
I will fix this in next version.
> > + */
>
> One-line comment should be in the form /* <comment> */
>
> > + if (spinand->manufacturer->ops->fixup_param_page)
> > + spinand->manufacturer->ops->fixup_param_page(spinand,
> p);
> > +
> > + return 1;
>
> return 0;
I will fix this in next version.
>
> > +}
> > +
> > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > +{
> > + struct nand_device *base = spinand_to_nand(spinand);
> > +
> > + base->helper.page = 0x01;
> > + base->helper.check_revision = check_version;
> > + base->helper.parameter_page_read =
> spinand_parameter_page_read;
> > + base->helper.init_intf_data = spinand_intf_data;
> > +
> > + return nand_onfi_detect(base);
> > +}
> > +
> > static int spinand_read_page(struct spinand_device *spinand,
> > const struct nand_page_io_req *req)
> > {
> > @@ -896,7 +998,7 @@ static void spinand_manufacturer_cleanup(struct
> spinand_device *spinand)
> > return spinand->manufacturer->ops->cleanup(spinand);
> > }
> >
> > -static const struct spi_mem_op *
> > +const struct spi_mem_op *
> > spinand_select_op_variant(struct spinand_device *spinand,
> > const struct spinand_op_variants *variants)
> > {
> > @@ -1012,6 +1114,15 @@ static int spinand_detect(struct spinand_device
> *spinand)
> > return ret;
> > }
> >
> > + if (!spinand->base.memorg.pagesize) {
> > + ret = spinand_param_page_detect(spinand);
> > + if (ret < 0) {
> > + dev_err(dev, "no parameter page for %*phN\n",
> > + SPINAND_MAX_ID_LEN, spinand->id.data);
> > + return ret;
> > + }
> > + }
> > +
>
> I think this could be added in a separate patch. Is this the only
> condition where we want to read the param page ?
>
I will make separate patch if possible.
spinand_manufacturer_detect will not initialize memorg, I think it is
enough to check memorg.pagesize is initialized or not.
Suggest me if you have any other thoughts.
> > if (nand->memorg.ntargets > 1 && !spinand->select_target) {
> > dev_err(dev,
> > "SPI NANDs with more than one die must implement
> ->select_target()\n");
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index d093d237fba8..57b3b5b075f2 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -179,6 +179,8 @@ struct spinand_manufacturer_ops {
> > int (*detect)(struct spinand_device *spinand);
> > int (*init)(struct spinand_device *spinand);
> > void (*cleanup)(struct spinand_device *spinand);
> > + void (*fixup_param_page)(struct spinand_device *spinand,
> > + struct nand_onfi_params *p);
>
> You could probably add this hook in a separate patch.
I will fix this in new version.
>
> > };
> >
> > /**
> > @@ -429,4 +431,7 @@ int spinand_match_and_init(struct spinand_device
> *dev,
> > int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
> > int spinand_select_target(struct spinand_device *spinand, unsigned int
> target);
> >
> > +const struct spi_mem_op *spinand_select_op_variant(struct
> spinand_device *spinand,
> > + const struct spinand_op_variants *variants);
>
> What is it for?
Ah, it should be in next patch no here.
>
> > +
> > #endif /* __LINUX_MTD_SPINAND_H */
>
>
>
>
> Thanks,
> Miquèl
Powered by blists - more mailing lists