linux-kernel - Re: [PATCH v2 2/3] mtd: nand: mxc_nand: implement exec

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240513091902.2c00d30a@xps-13>
Date: Mon, 13 May 2024 09:19:02 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Sascha Hauer <s.hauer@...gutronix.de>
Cc: Richard Weinberger <richard@....at>, Vignesh Raghavendra
 <vigneshr@...com>, linux-mtd@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] mtd: nand: mxc_nand: implement exec_op

Hi Sascha,

> @@ -1717,9 +1465,111 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
>  	return host->devtype_data->setup_interface(chip, chipnr, conf);
>  }
>  
> +static int mxcnd_exec_op(struct nand_chip *chip,
> +			 const struct nand_operation *op,
> +			 bool check_only)
> +{
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int i, j, buf_len;
> +	void *buf_read = NULL;
> +	const void *buf_write = NULL;
> +	const struct nand_op_instr *instr;
> +	bool readid = false;
> +	bool statusreq = false;
> +
> +	dev_dbg(host->dev, "%s: %d instructions\n", __func__, op->ninstrs);

Maybe you want to get rid of this debug line.

> +
> +	if (check_only)
> +		return 0;
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		instr = &op->instrs[i];
> +
> +		nand_op_trace("  ", instr);
> +
> +		switch (instr->type) {
> +		case NAND_OP_WAITRDY_INSTR:
> +			/*
> +			 * NFC handles R/B internally. Therefore, this function
> +			 * always returns status as ready.

This is no longer a standalone function, maybe:

"The controller handles the R/B pin internally, therefore there is
nothing to do here."

> +			 */
> +			break;
> +		case NAND_OP_CMD_INSTR:
> +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> +				host->devtype_data->send_page(mtd, NFC_INPUT);
> +
> +			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> +
> +			if (instr->ctx.cmd.opcode == NAND_CMD_READID)
> +				readid = true;
> +			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
> +				statusreq = true;
> +
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> +				bool islast = j == instr->ctx.addr.naddrs - 1;
> +				host->devtype_data->send_addr(host, instr->ctx.addr.addrs[j], islast);
> +			}
> +			break;
> +		case NAND_OP_DATA_OUT_INSTR:
> +			buf_write = instr->ctx.data.buf.out;
> +			buf_len = instr->ctx.data.len;
> +
> +			memcpy32_toio(host->main_area0, buf_write, buf_len);
> +			if (chip->oob_poi)
> +				copy_spare(mtd, false, chip->oob_poi);

This copy should not be needed. It should be in your page accessors if
needed.

> +
> +			break;
> +		case NAND_OP_DATA_IN_INSTR:
> +
> +			buf_read = instr->ctx.data.buf.in;
> +			buf_len = instr->ctx.data.len;
> +
> +			if (readid) {
> +				host->devtype_data->send_read_id(host);
> +				readid = false;
> +
> +				memcpy32_fromio(host->data_buf, host->main_area0, buf_len * 2);
> +
> +				if (chip->options & NAND_BUSWIDTH_16) {
> +					u8 *bufr = buf_read;
> +					u16 *bufw = host->data_buf;
> +					for (j = 0; j < buf_len; j++)
> +						bufr[j] = bufw[j];
> +				} else {
> +					memcpy(buf_read, host->data_buf, buf_len);
> +				}
> +				break;
> +			}
> +
> +			if (statusreq) {
> +				*(u8*)buf_read = host->devtype_data->get_dev_status(host);
> +				statusreq = false;
> +				break;
> +			}
> +
> +			host->devtype_data->read_page(chip);
> +
> +			if (IS_ALIGNED(buf_len, 4)) {
> +				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +			} else {
> +				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> +				memcpy(buf_read, host->data_buf, buf_len);
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

Otherwise I'm very happy with the look.

Thanks,
Miquèl