| 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: <CAJhJPsWe+maw+zK6uiwvObTd_Ew73yjH=KddkgxwY7Zp0Y7ZYw@mail.gmail.com>
Date: Fri, 17 Jan 2025 19:58:39 +0800
From: Keguang Zhang <keguang.zhang@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Keguang Zhang via B4 Relay <devnull+keguang.zhang.gmail.com@...nel.org>,
Richard Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-media@...r.kernel.org
Subject: Re: [PATCH v11 2/2] mtd: rawnand: Add Loongson-1 NAND Controller Driver
Hello Miquel,
On Thu, Jan 16, 2025 at 2:54 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hello Keguang,
>
> On 17/12/2024 at 18:16:50 +08, Keguang Zhang via B4 Relay <devnull+keguang.zhang.gmail.com@...nel.org> wrote:
>
> > +static int ls1x_nand_op_cmd_mapping(struct nand_chip *chip, struct ls1x_nand_op *op, u8 opcode)
> > +{
> > + struct ls1x_nand_host *host = nand_get_controller_data(chip);
> > + int ret = 0;
>
> This return code is unused.
>
> > +
> > + op->row_start = chip->page_shift + 1;
> > +
> > + /* The controller abstracts the following NAND operations. */
> > + switch (opcode) {
> > + case NAND_CMD_STATUS:
> > + op->cmd_reg = LS1X_NAND_CMD_STATUS;
> > + break;
> > + case NAND_CMD_RESET:
> > + op->cmd_reg = LS1X_NAND_CMD_RESET;
> > + break;
> > + case NAND_CMD_READID:
> > + op->is_readid = true;
> > + op->cmd_reg = LS1X_NAND_CMD_READID;
> > + break;
> > + case NAND_CMD_ERASE1:
> > + op->is_erase = true;
> > + op->addrs_offset = 2;
> > + break;
> > + case NAND_CMD_ERASE2:
> > + if (!op->is_erase)
> > + return -EOPNOTSUPP;
> > + /* During erasing, row_start differs from the default value. */
>
> ...
>
> > +static void ls1x_nand_trigger_op(struct ls1x_nand_host *host, struct ls1x_nand_op *op)
> > +{
> > + struct nand_chip *chip = &host->chip;
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + int col0 = op->addrs[0];
> > + short col;
> > +
> > + /* restore row address for column change */
> > + if (op->is_change_column) {
> > + op->addr2_reg = readl(host->reg_base + LS1X_NAND_ADDR2);
> > + op->addr1_reg = readl(host->reg_base + LS1X_NAND_ADDR1);
> > + op->addr1_reg &= ~(mtd->writesize - 1);
> > + }
>
> This looks very suspicious. You should not have to do that and to be
> honest, I don't undertand what this means.
>
The Loongson-1 NAND controller requires a full address (column address
+ row address).
However, nand_change_read_column_op() function only provides the
column address. Therefore, the row address must be restored.
The above logic retrieves the row address from the addr1_reg in order
to restore the row address.
> > +
> > + if (!IS_ALIGNED(col0, chip->buf_align)) {
> > + col0 = ALIGN_DOWN(op->addrs[0], chip->buf_align);
> > + op->aligned_offset = op->addrs[0] - col0;
> > + op->addrs[0] = col0;
> > + }
> > +
> > + if (host->data->parse_address)
> > + host->data->parse_address(op);
> > +
> > + /* set address */
> > + writel(op->addr1_reg, host->reg_base + LS1X_NAND_ADDR1);
> > + writel(op->addr2_reg, host->reg_base + LS1X_NAND_ADDR2);
> > +
> > + /* set operation length */
> > + if (op->is_write || op->is_read || op->is_change_column)
> > + op->len = ALIGN(op->orig_len + op->aligned_offset, chip->buf_align);
> > + else if (op->is_erase)
> > + op->len = 1;
> > + else
> > + op->len = op->orig_len;
> > +
> > + writel(op->len, host->reg_base + LS1X_NAND_OP_NUM);
> > +
> > + /* set operation area */
> > + col = op->addrs[1] << BITS_PER_BYTE | op->addrs[0];
> > + if (op->orig_len && !op->is_readid) {
> > + if (col < mtd->writesize)
> > + op->cmd_reg |= LS1X_NAND_CMD_OP_MAIN;
> > +
> > + op->cmd_reg |= LS1X_NAND_CMD_OP_SPARE;
> > + }
> > +
> > + /* set operation scope */
> > + if (host->data->op_scope_field) {
> > + unsigned int op_scope;
> > +
> > + switch (op->cmd_reg & LS1X_NAND_CMD_OP_AREA_MASK) {
> > + case LS1X_NAND_CMD_OP_MAIN:
> > + op_scope = mtd->writesize;
> > + break;
> > + case LS1X_NAND_CMD_OP_SPARE:
> > + op_scope = mtd->oobsize;
> > + break;
> > + case LS1X_NAND_CMD_OP_AREA_MASK:
> > + op_scope = mtd->writesize + mtd->oobsize;
> > + break;
> > + default:
> > + op_scope = 0;
> > + break;
> > + }
>
> Please get rid of this extra step. I'm not a big fan of it, but this can
> be very well simplified and this whole switch removed.
>
Will simplify this logic.
> > +
> > + op_scope <<= __ffs(host->data->op_scope_field);
> > + regmap_update_bits(host->regmap, LS1X_NAND_PARAM,
> > + host->data->op_scope_field, op_scope);
> > + }
> > +
> > + /* set command */
> > + writel(op->cmd_reg, host->reg_base + LS1X_NAND_CMD);
> > +
> > + /* trigger operation */
> > + regmap_write_bits(host->regmap, LS1X_NAND_CMD, LS1X_NAND_CMD_VALID, LS1X_NAND_CMD_VALID);
> > +}
> > +
>
> ...
>
> > +static const struct nand_op_parser ls1x_nand_op_parser = NAND_OP_PARSER(
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_read_id_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)),
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_read_status_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 1)),
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_zerolen_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_zerolen_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_data_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 0)),
> > + NAND_OP_PARSER_PATTERN(
> > + ls1x_nand_data_type_exec,
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> > + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 0),
> > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> > + );
> > +
> > +static inline bool ls1x_nand_is_valid_cmd(u8 opcode)
> > +{
> > + return opcode == NAND_CMD_RESET ||
> > + opcode == NAND_CMD_READID ||
> > + opcode == NAND_CMD_ERASE1 ||
> > + opcode == NAND_CMD_ERASE2 ||
> > + opcode == NAND_CMD_STATUS ||
> > + opcode == NAND_CMD_SEQIN ||
> > + opcode == NAND_CMD_PAGEPROG ||
> > + opcode == NAND_CMD_RNDOUT ||
> > + opcode == NAND_CMD_RNDOUTSTART ||
> > + opcode == NAND_CMD_READ0 ||
> > + opcode == NAND_CMD_READSTART;
> > +}
> > +
> > +static inline bool ls1x_nand_is_cmd_sequence(const struct nand_op_instr *instr1,
> > + const struct nand_op_instr *instr2)
> > +{
> > + return instr1->type == NAND_OP_CMD_INSTR && instr2->type == NAND_OP_CMD_INSTR;
> > +}
> > +
> > +static inline bool ls1x_nand_is_erase_sequence(const struct nand_op_instr *instr1,
> > + const struct nand_op_instr *instr2)
> > +{
> > + return instr1->ctx.cmd.opcode == NAND_CMD_ERASE1 &&
> > + instr2->ctx.cmd.opcode == NAND_CMD_ERASE2;
> > +}
> > +
> > +static inline bool ls1x_nand_is_write_sequence(const struct nand_op_instr *instr1,
> > + const struct nand_op_instr *instr2)
> > +{
> > + return instr1->ctx.cmd.opcode == NAND_CMD_SEQIN &&
> > + instr2->ctx.cmd.opcode == NAND_CMD_PAGEPROG;
> > +}
> > +
> > +static inline bool ls1x_nand_is_read_sequence(const struct nand_op_instr *instr1,
> > + const struct nand_op_instr *instr2)
> > +{
> > + return (instr1->ctx.cmd.opcode == NAND_CMD_READ0 &&
> > + instr2->ctx.cmd.opcode == NAND_CMD_READSTART) ||
> > + (instr1->ctx.cmd.opcode == NAND_CMD_RNDOUT &&
> > + instr2->ctx.cmd.opcode == NAND_CMD_RNDOUTSTART);
> > +}
> > +
> > +static int ls1x_nand_check_op(struct nand_chip *chip, const struct nand_operation *op)
> > +{
> > + const struct nand_op_instr *instr;
> > + int op_id;
> > +
> > + for (op_id = 0; op_id < op->ninstrs; op_id++) {
> > + instr = &op->instrs[op_id];
> > +
> > + switch (instr->type) {
> > + case NAND_OP_CMD_INSTR:
> > + if (!ls1x_nand_is_valid_cmd(instr->ctx.cmd.opcode))
> > + return -EOPNOTSUPP;
> > + break;
> > + case NAND_OP_ADDR_INSTR:
> > + if (instr->ctx.addr.naddrs > LS1X_NAND_MAX_ADDR_CYC)
> > + return -EOPNOTSUPP;
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (op->ninstrs == 4 &&
> > + ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[2]) &&
> > + !ls1x_nand_is_erase_sequence(&op->instrs[0], &op->instrs[2]))
> > + return -EOPNOTSUPP;
> > +
> > + if (op->ninstrs == 5) {
> > + if (ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[2]) &&
> > + !ls1x_nand_is_read_sequence(&op->instrs[0], &op->instrs[2]))
> > + return -EOPNOTSUPP;
> > +
> > + if (ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[3]) &&
> > + !ls1x_nand_is_write_sequence(&op->instrs[0], &op->instrs[3]))
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ls1x_nand_exec_op(struct nand_chip *chip,
> > + const struct nand_operation *op,
> > + bool check_only)
> > +{
> > + if (check_only)
> > + return ls1x_nand_check_op(chip, op);
> > +
>
> It lookse like you're re-encoding all your requirements in
> ls1x_nand_check_op(), whereas nand_op_parser_exec_op(check_only := true)
> should give you already a certain number of verifications which are
> skipped here. I'd suggest to improve this to avoid repetitions between
> the two. Of course the second part of nand_check_op is necessary, but
> the initial checks seem redundant and would better be performed by the parser.
>
Indeed, this logic seems a little wierd.
In addition, ls1x_nand_check_op() must always be executed whenever
check_only is set.
Will fix this logic and drop the first part of ls1x_nand_check_op().
> > + return nand_op_parser_exec_op(chip, &ls1x_nand_op_parser, op, check_only);
> > +}
> > +
> > +static int ls1x_nand_attach_chip(struct nand_chip *chip)
> > +{
>
> ...
>
> > +static int ls1x_nand_controller_init(struct ls1x_nand_host *host)
> > +{
> > + struct device *dev = host->dev;
> > + struct dma_chan *chan;
> > + struct dma_slave_config cfg = {};
> > + int ret;
> > +
> > + host->regmap = devm_regmap_init_mmio(dev, host->reg_base, &ls1x_nand_regmap_config);
> > + if (IS_ERR(host->regmap))
> > + return dev_err_probe(dev, PTR_ERR(host->regmap), "failed to init regmap\n");
> > +
> > + chan = dma_request_chan(dev, "rxtx");
> > + if (IS_ERR(chan))
> > + return dev_err_probe(dev, PTR_ERR(chan), "failed to request DMA channel\n");
> > + host->dma_chan = chan;
> > +
> > + cfg.src_addr = host->dma_base;
> > + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + cfg.dst_addr = host->dma_base;
>
> Don't you need a dma_addr_t here instead? You shall remap the resource.
>
Sorry, I don't quite understand.
'dma_base' is already of type dma_addr_t.
Thanks for your review!
> > + cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + ret = dmaengine_slave_config(host->dma_chan, &cfg);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to config DMA channel\n");
> > +
> > + init_completion(&host->dma_complete);
> > +
> > + dev_dbg(dev, "got %s for %s access\n", dma_chan_name(host->dma_chan), dev_name(dev));
> > +
> > + return 0;
> > +}
> > +
> > +static int ls1x_nand_chip_init(struct ls1x_nand_host *host)
> > +{
> > + struct device *dev = host->dev;
> > + int nchips = of_get_child_count(dev->of_node);
> > + struct device_node *chip_np;
> > + struct nand_chip *chip = &host->chip;
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + int ret = 0;
> > +
> > + if (nchips != 1)
> > + return dev_err_probe(dev, -EINVAL, "Currently one NAND chip supported\n");
> > +
> > + chip_np = of_get_next_child(dev->of_node, NULL);
> > + if (!chip_np)
> > + return dev_err_probe(dev, -ENODEV, "failed to get child node for NAND chip\n");
> > +
> > + chip->controller = &host->controller;
> > + chip->options = NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA | NAND_BROKEN_XD;
> > + chip->buf_align = 16;
> > + nand_set_controller_data(chip, host);
> > + nand_set_flash_node(chip, chip_np);
> > +
> > + mtd->dev.parent = dev;
> > + mtd->name = "ls1x-nand";
>
> No, the name is gonna be filled automatically when you call
> nand_set_flash_node IIRC.
>
> > + mtd->owner = THIS_MODULE;
> > +
> > + ret = nand_scan(chip, 1);
> > + if (ret) {
> > + of_node_put(chip_np);
> > + return ret;
> > + }
> > +
>
> It looks like your controller does not support any ECC correction, if
> that's the case you must make sure it's properly handled in attach_chip
> hook by refusing to probe if the on_host engine is used.
>
> Thanks,
> Miquèl
--
Best regards,
Keguang Zhang
Powered by blists - more mailing lists