| 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: <20241001113108.4fdb6360@xps-13>
Date: Tue, 1 Oct 2024 11:31:08 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Martin Kurbanov <mmkurbanov@...utedevices.com>
Cc: Richard Weinberger <richard@....at>, Vignesh Raghavendra
<vigneshr@...com>, Mika Westerberg <mika.westerberg@...ux.intel.com>,
Michael Walle <michael@...le.cc>, "Mark Brown" <broonie@...nel.org>,
Chia-Lin Kao <acelan.kao@...onical.com>, "Md Sadre Alam"
<quic_mdalam@...cinc.com>, Ezra Buehler <ezra.buehler@...qvarnagroup.com>,
Sridharan S N <quic_sridsn@...cinc.com>, Frieder Schrempf
<frieder.schrempf@...tron.de>, Alexey Romanov
<avromanov@...utedevices.com>, <linux-kernel@...r.kernel.org>,
<linux-mtd@...ts.infradead.org>, <kernel@...utedevices.com>
Subject: Re: [PATCH v2 4/5] mtd: spinand: micron: OTP access for
MT29F2G01ABAGD
Hi Martin,
mmkurbanov@...utedevices.com wrote on Tue, 27 Aug 2024 20:49:02 +0300:
> Support for OTP area access on Micron MT29F2G01ABAGD chip.
>
> Signed-off-by: Martin Kurbanov <mmkurbanov@...utedevices.com>
> ---
> drivers/mtd/nand/spi/micron.c | 117 +++++++++++++++++++++++++++++++++-
> 1 file changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 8d741be6d5f3e..a538409db4ccd 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -9,6 +9,7 @@
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/mtd/spinand.h>
> +#include <linux/spi/spi-mem.h>
>
> #define SPINAND_MFR_MICRON 0x2c
>
> @@ -28,6 +29,16 @@
>
> #define MICRON_SELECT_DIE(x) ((x) << 6)
>
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGES 12
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE 2176
In the core we did add the data size and the OOB size to get the OTP
page size. I would prefer something dynamic here as well, otherwise the
implementation is very device specific for now reason?
> +#define MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES \
> + (MICRON_MT29F2G01ABAGD_OTP_PAGES * \
> + MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE)
This is a function from the core as well once you've filled all the
information in the core structures, so why hardcoding it?
> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_STATE BIT(7)
> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK \
> + (CFG_OTP_ENABLE | MICRON_MT29F2G01ABAGD_CFG_OTP_STATE)
> +
> static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
> //SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -182,6 +193,108 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
> return -EINVAL;
> }
>
> +static int mt29f2g01abagd_otp_is_locked(struct spinand_device *spinand)
> +{
> + size_t buf_size = MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE;
> + size_t retlen;
> + u8 *buf;
> + int ret;
> +
> + buf = kmalloc(buf_size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = spinand_upd_cfg(spinand,
> + MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> + MICRON_MT29F2G01ABAGD_CFG_OTP_STATE);
> + if (ret)
> + goto out;
can we name the label free_buf?
> +
> + ret = spinand_otp_read(spinand, 0, buf_size, buf, &retlen);
> +
> + if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> + 0)) {
> + WARN(1, "Can not disable OTP mode\n");
I prefer dev_warn as well here, so we know which device is concerned.
> + ret = -EIO;
> + }
> +
> + if (!ret) {
if (ret)
goto out;
> + size_t i = 0;
> +
> + /* If all zeros, then the OTP area is locked. */
> + while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
> + i += 4;
Shall we expect buf_size to always be a multiple of 4? (real question)
I am not a big fan of the casting game here. I see the optimization
you're attempting to do, but I'm a little bit skeptical. I must admit I
didn't find a helper for that, buf at least maybe you can use an
intermediate variable and loop over it.
> +
> + if (i == buf_size)
> + ret = 1;
If buf_size is not a multiple of 4, this is not gonna work.
> + }
> +
> +out:
> + kfree(buf);
> + return ret;
> +}
> +
> +static int mt29f2g01abagd_otp_info(struct spinand_device *spinand, size_t len,
> + struct otp_info *buf, size_t *retlen)
> +{
> + int locked;
> +
> + if (len < sizeof(*buf))
> + return -EINVAL;
> +
> + locked = mt29f2g01abagd_otp_is_locked(spinand);
> + if (locked < 0)
> + return locked;
> +
> + buf->locked = locked;
> + buf->start = 0;
> + buf->length = MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES;
> +
> + *retlen = sizeof(*buf);
> + return 0;
> +}
> +
> +static int mt29f2g01abagd_otp_lock(struct spinand_device *spinand, loff_t from,
> + size_t len)
> +{
> + struct spi_mem_op write_op = SPINAND_WR_EN_DIS_OP(true);
> + struct spi_mem_op exec_op = SPINAND_PROG_EXEC_OP(0);
> + int ret;
> +
> + ret = spinand_upd_cfg(spinand,
> + MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> + MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK);
> + if (!ret)
> + return ret;
> +
> + ret = spi_mem_exec_op(spinand->spimem, &write_op);
> + if (!ret)
> + goto out;
> +
> + ret = spi_mem_exec_op(spinand->spimem, &exec_op);
> + if (!ret)
> + goto out;
> +
> + ret = spinand_wait(spinand, 10, 5, NULL);
Usually I expect timeouts to be bigger.
> + if (!ret)
> + goto out;
This goto seems to have not impact :)
> +
> +out:
> + if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK, 0)) {
> + WARN(1, "Can not disable OTP mode\n");
dev_warn()
> + ret = -EIO;
> + }
> +
> + return ret;
> +}
> +
> +static const struct spinand_otp_ops mt29f2g01abagd_otp_ops = {
> + .info = mt29f2g01abagd_otp_info,
> + .lock = mt29f2g01abagd_otp_lock,
> + .read = spinand_otp_read,
> + .write = spinand_otp_write,
> +};
> +
> static const struct spinand_info micron_spinand_table[] = {
> /* M79A 2Gb 3.3V */
> SPINAND_INFO("MT29F2G01ABAGD",
> @@ -193,7 +306,9 @@ static const struct spinand_info micron_spinand_table[] = {
> &x4_update_cache_variants),
> 0,
> SPINAND_ECCINFO(µn_8_ooblayout,
> - micron_8_ecc_get_status)),
> + micron_8_ecc_get_status),
> + SPINAND_OTP_INFO(MICRON_MT29F2G01ABAGD_OTP_PAGES,
> + &mt29f2g01abagd_otp_ops)),
> /* M79A 2Gb 1.8V */
> SPINAND_INFO("MT29F2G01ABBGD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),
Thanks,
Miquèl
Powered by blists - more mailing lists