| 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: <CAKR-sGcg5yCiQFV5yriD+Lv1MiV1wXVQo1_q_Eh-=UNAP-idxA@mail.gmail.com>
Date: Thu, 15 May 2025 07:06:39 +0200
From: Álvaro Fernández Rojas <noltari@...il.com>
To: William Zhang <william.zhang@...adcom.com>
Cc: linux-mtd@...ts.infradead.org, dregan@...adcom.com,
miquel.raynal@...tlin.com, bcm-kernel-feedback-list@...adcom.com,
Florian Fainelli <florian.fainelli@...adcom.com>, rafal@...ecki.pl,
computersforpeace@...il.com, Kamal Dasu <kamal.dasu@...adcom.com>,
Dan Beygelman <dan.beygelman@...adcom.com>, frieder.schrempf@...tron.de,
linux-kernel@...r.kernel.org, vigneshr@...com, richard@....at,
bbrezillon@...nel.org, kdasu.kdev@...il.com, jaimeliao.tw@...il.com,
kilobyte@...band.pl, jonas.gorski@...il.com, dgcbueu@...il.com
Subject: Re: [PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation
Hi William
El jue, 15 may 2025 a las 3:42, William Zhang
(<william.zhang@...adcom.com>) escribió:
>
> Hi Alvaro,
>
> > -----Original Message-----
> > From: Álvaro Fernández Rojas <noltari@...il.com>
> > Sent: Tuesday, May 13, 2025 11:24 PM
> > To: linux-mtd@...ts.infradead.org; dregan@...adcom.com;
> > miquel.raynal@...tlin.com; bcm-kernel-feedback-list@...adcom.com;
> > florian.fainelli@...adcom.com; rafal@...ecki.pl;
> > computersforpeace@...il.com; kamal.dasu@...adcom.com;
> > dan.beygelman@...adcom.com; william.zhang@...adcom.com;
> > frieder.schrempf@...tron.de; linux-kernel@...r.kernel.org;
> > vigneshr@...com;
> > richard@....at; bbrezillon@...nel.org; kdasu.kdev@...il.com;
> > jaimeliao.tw@...il.com; kilobyte@...band.pl; jonas.gorski@...il.com;
> > dgcbueu@...il.com
> > Cc: Álvaro Fernández Rojas <noltari@...il.com>
> > Subject: [PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation
> >
> > Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > removed legacy interface functions, breaking < v5.0 controllers support.
> > In order to fix older controllers we need to add an alternative exec_op
> > implementation which doesn't rely on low level registers.
> >
> > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> > ---
> > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 178
> > ++++++++++++++++++++++-
> > 1 file changed, 172 insertions(+), 6 deletions(-)
> >
> > v3: add changes requested by Florian and other improvements:
> > - Add associative array for native command conversion.
> > - Add function pointer to brcmnand_controller for exec_instr
> > functionality.
> > - Fix CMD_BLOCK_ERASE address.
> > - Drop NAND_CMD_READOOB support.
> >
> > v2: multiple improvements:
> > - Use proper native commands for checks.
> > - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
> > - Remove host->last_addr usage.
> > - Remove sector_size_1k since it only applies to v5.0+ controllers.
> > - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
> > - Use j instead of i for flash_cache loop.
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > index 17f6d9723df9..f4fabe7ffd9d 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -65,6 +65,7 @@ module_param(wp_on, int, 0444);
> > #define CMD_PARAMETER_READ 0x0e
> > #define CMD_PARAMETER_CHANGE_COL 0x0f
> > #define CMD_LOW_LEVEL_OP 0x10
> > +#define CMD_NOT_SUPPORTED 0xff
> >
> > struct brcm_nand_dma_desc {
> > u32 next_desc;
> > @@ -199,6 +200,30 @@ static const u16 flash_dma_regs_v4[] = {
> > [FLASH_DMA_CURRENT_DESC_EXT] = 0x34,
> > };
> >
> > +/* Native command conversion */
> > +static const u8 native_cmd_conv[] = {
> > + [NAND_CMD_READ0] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READ1] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDOUT] = CMD_PARAMETER_CHANGE_COL,
> > + [NAND_CMD_PAGEPROG] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READOOB] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_ERASE1] = CMD_BLOCK_ERASE,
> > + [NAND_CMD_STATUS] = CMD_NOT_SUPPORTED,
> Do we not need to support nand_status_op()?
We do, but NAND_CMD_STATUS and NAND_CMD_RESET are handled in brcmnand_exec_op():
https://github.com/torvalds/linux/blob/546bce579204685a0b204beebab98c3aa496e651/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L2506-L2523
>
> > + [NAND_CMD_SEQIN] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDIN] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READID] = CMD_DEVICE_ID_READ,
> > + [NAND_CMD_ERASE2] = CMD_NULL,
> > + [NAND_CMD_PARAM] = CMD_PARAMETER_READ,
> > + [NAND_CMD_GET_FEATURES] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_SET_FEATURES] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RESET] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READSTART] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READCACHESEQ] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READCACHEEND] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDOUTSTART] = CMD_NULL,
> > + [NAND_CMD_CACHEDPROG] = CMD_NOT_SUPPORTED,
> > +};
> > +
> > /* Controller feature flags */
> > enum {
> > BRCMNAND_HAS_1K_SECTORS = BIT(0),
> > @@ -237,6 +262,10 @@ struct brcmnand_controller {
> > /* List of NAND hosts (one for each chip-select) */
> > struct list_head host_list;
> >
> > + /* Function to be called from exec_op */
> > + int (*exec_instr)(struct nand_chip *chip,
> > + const struct nand_operation *op);
> > +
> > /* EDU info, per-transaction */
> > const u16 *edu_offsets;
> > void __iomem *edu_base;
> > @@ -2490,14 +2519,149 @@ static int brcmnand_op_is_reset(const struct
> > nand_operation *op)
> > return 0;
> > }
> >
> > +static int brcmnand_exec_instructions(struct nand_chip *chip,
> > + const struct nand_operation *op)
> > +{
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + for (i = 0; i < op->ninstrs; i++) {
> > + ret = brcmnand_exec_instr(host, i, op);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
> > + const struct nand_operation *op)
> > +{
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > + const struct nand_op_instr *instr;
> > + unsigned int i, j;
> > + u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
> > + int ret = 0;
> > + u64 last_addr;
> > +
> > + for (i = 0; i < op->ninstrs; i++) {
> > + instr = &op->instrs[i];
> > +
> > + if (instr->type == NAND_OP_CMD_INSTR) {
> > + cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > + if (cmd == CMD_NOT_SUPPORTED) {
> > + dev_err(ctrl->dev, "unsupported cmd=%d\n",
> > + instr->ctx.cmd.opcode);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > + } else if (instr->type == NAND_OP_ADDR_INSTR) {
> > + u64 addr = 0;
> > +
> > + if (cmd == CMD_NULL)
> > + continue;
> > +
> > + if (instr->ctx.addr.naddrs > 8) {
> > + dev_err(ctrl->dev, "unsupported naddrs=%u\n",
> > + instr->ctx.addr.naddrs);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + for (j = 0; j < instr->ctx.addr.naddrs; j++)
> > + addr |= (instr->ctx.addr.addrs[j]) << (j << 3);
> > +
> > + if (cmd == CMD_BLOCK_ERASE)
> > + addr <<= chip->page_shift;
> > + else if (cmd == CMD_PARAMETER_CHANGE_COL)
> > + addr &= ~((u64)(FC_BYTES - 1));
> > +
> > + brcmnand_set_cmd_addr(mtd, addr);
> > + brcmnand_send_cmd(host, cmd);
> > + last_addr = addr;
> > + last_cmd = cmd;
> > + cmd = CMD_NULL;
> > + brcmnand_waitfunc(chip);
> > +
> > + if (last_cmd == CMD_PARAMETER_READ ||
> > + last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > + /* Copy flash cache word-wise */
> > + u32 *flash_cache = (u32 *)ctrl->flash_cache;
> > +
> > + brcmnand_soc_data_bus_prepare(ctrl->soc, true);
> > +
> > + /*
> > + * Must cache the FLASH_CACHE now, since
> > changes in
> > + * SECTOR_SIZE_1K may invalidate it
> > + */
> > + for (j = 0; j < FC_WORDS; j++)
> > + /*
> > + * Flash cache is big endian for parameter
> > pages, at
> > + * least on STB SoCs
> > + */
> > + flash_cache[j] =
> > be32_to_cpu(brcmnand_read_fc(ctrl, j));
> > +
> > + brcmnand_soc_data_bus_unprepare(ctrl->soc,
> > true);
> > + }
> > + } else if (instr->type == NAND_OP_DATA_IN_INSTR) {
> > + u8 *in = instr->ctx.data.buf.in;
> > +
> > + if (last_cmd == CMD_DEVICE_ID_READ) {
> > + u32 val;
> > +
> > + if (instr->ctx.data.len > 8) {
> > + dev_err(ctrl->dev, "unsupported
> > len=%u\n",
> > + instr->ctx.data.len);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + for (j = 0; j < instr->ctx.data.len; j++) {
> > + if (j == 0)
> > + val = brcmnand_read_reg(ctrl,
> > BRCMNAND_ID);
> > + else if (j == 4)
> > + val = brcmnand_read_reg(ctrl,
> > BRCMNAND_ID_EXT);
> > +
> > + in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
> > + }
> > + } else if (last_cmd == CMD_PARAMETER_READ ||
> > + last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > + u64 addr;
> > + u32 offs;
> > +
> > + for (j = 0; j < instr->ctx.data.len; j++) {
> > + addr = last_addr + j;
> > + offs = addr & (FC_BYTES - 1);
> > +
> > + if (j > 0 && offs == 0)
> > +
> > nand_change_read_column_op(chip, addr, NULL, 0,
> > + false);
> > +
> > + in[j] = ctrl->flash_cache[offs];
> > + }
> > + }
> > + } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
> > + ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY,
> > NAND_CTRL_RDY, 0);
> > + } else {
> > + dev_err(ctrl->dev, "unsupported instruction type: %d\n",
> > instr->type);
> > + ret = -EINVAL;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int brcmnand_exec_op(struct nand_chip *chip,
> > const struct nand_operation *op,
> > bool check_only)
> > {
> > struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > u8 *status;
> > - unsigned int i;
> > int ret = 0;
> >
> > if (check_only)
> > @@ -2525,11 +2689,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
> > if (op->deassert_wp)
> > brcmnand_wp(mtd, 0);
> >
> > - for (i = 0; i < op->ninstrs; i++) {
> > - ret = brcmnand_exec_instr(host, i, op);
> > - if (ret)
> > - break;
> > - }
> > + ctrl->exec_instr(chip, op);
> >
> > if (op->deassert_wp)
> > brcmnand_wp(mtd, 1);
> > @@ -3142,6 +3302,12 @@ int brcmnand_probe(struct platform_device *pdev,
> > struct brcmnand_soc *soc)
> > if (ret)
> > goto err;
> >
> > + /* Only v5.0+ controllers have low level ops support */
> > + if (ctrl->nand_version >= 0x0500)
> > + ctrl->exec_instr = brcmnand_exec_instructions;
> > + else
> > + ctrl->exec_instr = brcmnand_exec_instructions_legacy;
> > +
> > /*
> > * Most chips have this cache at a fixed offset within 'nand' block.
> > * Some must specify this region separately.
> > --
> > 2.39.5
BTW, can anyone from Broadcom confirm any of the following?
1. There are no low level registers on v2.1 and v2.2 controllers.
2. Do low level registers exist on v4.0 controllers? They are defined
on 63268_map_part.h but the NAND drivers I could find never use them.
3. Are the low level registers bugged on v4.0 controllers?
4. Should the low level registers be handled differently on v4.0 controllers?
The issue is that trying to use them on v4.0 controllers for
GET_FEATURES would leave the NAND controller in a weird state that
results in hangs or timeouts while trying to read the NAND.
This happens on the Sercomm H500-s, which is a BCM63268 with a
Macronix NAND that tries to unlock through ONFI.
Best regards,
Álvaro.
Powered by blists - more mailing lists