[PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups

[PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups

[Manivannan Sadhasivam]

Hi Miquel,

This series has fixes for the smatch warnings reported by Kbuild bot [1]
and also several cleanup patches based on my code observation.

I've only compile tested this series. So let's wait for Sadre/Sricharan to
give a tested-by tag to make sure I didn't mess up anything.

- Mani

[1] https://lore.kernel.org/all/202308040141.gUjtZ32J-lkp@intel.com/

Manivannan Sadhasivam (10):
  mtd: rawnand: qcom: Remove superfluous initialization of "ret"
  mtd: rawnand: qcom: Rename variables in qcom_op_cmd_mapping()
  mtd: rawnand: qcom: Handle unsupported opcode in qcom_op_cmd_mapping()
  mtd: rawnand: qcom: Fix the opcode check in qcom_check_op()
  mtd: rawnand: qcom: Use EOPNOTSUPP instead of ENOTSUPP
  mtd: rawnand: qcom: Wrap qcom_nand_exec_op() to 80 columns
  mtd: rawnand: qcom: Unmap sg_list and free desc within submic_descs()
  mtd: rawnand: qcom: Simplify the call to nand_prog_page_end_op()
  mtd: rawnand: qcom: Do not override the error no of submit_descs()
  mtd: rawnand: qcom: Sort includes alphabetically

 drivers/mtd/nand/raw/qcom_nandc.c | 192 ++++++++++++++----------------
 1 file changed, 91 insertions(+), 101 deletions(-)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 01/10] mtd: rawnand: qcom: Remove superfluous initialization of "ret"

[Manivannan Sadhasivam]

In all the cases, "ret" variable is assigned a value before returning it.
So there is no need to explicitly initialize it with 0.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b485d8517fce..b6751fb17587 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1554,7 +1554,7 @@ check_for_erased_page(struct qcom_nand_host *host, u8 *data_buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	u8 *cw_data_buf, *cw_oob_buf;
-	int cw, data_size, oob_size, ret = 0;
+	int cw, data_size, oob_size, ret;
 
 	if (!data_buf)
 		data_buf = nand_get_data_buf(chip);
@@ -2684,7 +2684,7 @@ static int qcom_read_status_exec(struct nand_chip *chip,
 	const struct nand_op_instr *instr = NULL;
 	unsigned int op_id = 0;
 	unsigned int len = 0;
-	int ret = 0, num_cw, i;
+	int ret, num_cw, i;
 	u32 flash_status;
 
 	host->status = NAND_STATUS_READY | NAND_STATUS_WP;
@@ -2747,7 +2747,7 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo
 	const struct nand_op_instr *instr = NULL;
 	unsigned int op_id = 0;
 	unsigned int len = 0;
-	int ret = 0;
+	int ret;
 
 	qcom_parse_instructions(chip, subop, &q_op);
 
@@ -2795,7 +2795,7 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	struct qcom_op q_op = {};
-	int ret = 0;
+	int ret;
 
 	qcom_parse_instructions(chip, subop, &q_op);
 
@@ -2841,7 +2841,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	const struct nand_op_instr *instr = NULL;
 	unsigned int op_id = 0;
 	unsigned int len = 0;
-	int ret = 0;
+	int ret;
 
 	qcom_parse_instructions(chip, subop, &q_op);
 
@@ -2935,7 +2935,7 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su
 	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	struct qcom_op q_op = {};
-	int ret = 0;
+	int ret;
 
 	qcom_parse_instructions(chip, subop, &q_op);
 
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 01/10] mtd: rawnand: qcom: Remove superfluous initialization of "ret"

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:37 UTC, Manivannan Sadhasivam wrote:
> In all the cases, "ret" variable is assigned a value before returning it.
> So there is no need to explicitly initialize it with 0.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 02/10] mtd: rawnand: qcom: Rename variables in qcom_op_cmd_mapping()

[Manivannan Sadhasivam]

qcom_op_cmd_mapping() function accepts opcode and returns the corresponding
command register. So let's rename the local variables and parameters to
reflect the same.

Reported-by: Miquel Raynal <miquel.raynal@bootlin.com>
Closes: https://lore.kernel.org/all/20230804190750.3367a044@xps-13/
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index b6751fb17587..be428b80a39b 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2555,39 +2555,39 @@ static int qcom_nand_attach_chip(struct nand_chip *chip)
 	return 0;
 }
 
-static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 cmd,
+static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 opcode,
 			       struct qcom_op *q_op)
 {
-	int ret;
+	int cmd;
 
-	switch (cmd) {
+	switch (opcode) {
 	case NAND_CMD_RESET:
-		ret = OP_RESET_DEVICE;
+		cmd = OP_RESET_DEVICE;
 		break;
 	case NAND_CMD_READID:
-		ret = OP_FETCH_ID;
+		cmd = OP_FETCH_ID;
 		break;
 	case NAND_CMD_PARAM:
 		if (nandc->props->qpic_v2)
-			ret = OP_PAGE_READ_ONFI_READ;
+			cmd = OP_PAGE_READ_ONFI_READ;
 		else
-			ret = OP_PAGE_READ;
+			cmd = OP_PAGE_READ;
 		break;
 	case NAND_CMD_ERASE1:
 	case NAND_CMD_ERASE2:
-		ret = OP_BLOCK_ERASE;
+		cmd = OP_BLOCK_ERASE;
 		break;
 	case NAND_CMD_STATUS:
-		ret = OP_CHECK_STATUS;
+		cmd = OP_CHECK_STATUS;
 		break;
 	case NAND_CMD_PAGEPROG:
-		ret = OP_PROGRAM_PAGE;
+		cmd = OP_PROGRAM_PAGE;
 		q_op->flag = OP_PROGRAM_PAGE;
 		nandc->exec_opwrite = true;
 		break;
 	}
 
-	return ret;
+	return cmd;
 }
 
 /* NAND framework ->exec_op() hooks and related helpers */
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 02/10] mtd: rawnand: qcom: Rename variables in qcom_op_cmd_mapping()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:38 UTC, Manivannan Sadhasivam wrote:
> qcom_op_cmd_mapping() function accepts opcode and returns the corresponding
> command register. So let's rename the local variables and parameters to
> reflect the same.
> 
> Reported-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Closes: https://lore.kernel.org/all/20230804190750.3367a044@xps-13/
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 03/10] mtd: rawnand: qcom: Handle unsupported opcode in qcom_op_cmd_mapping()

[Manivannan Sadhasivam]

Handle the scenario where the caller has passed an unsupported opcode to
qcom_op_cmd_mapping(). In that case, log the error and return the
-EOPNOTSUPP errono. Also, let's propagate this error code all the way up.

This also fixes the following smatch warning:

drivers/mtd/nand/raw/qcom_nandc.c:2941 qcom_op_cmd_mapping() error: uninitialized symbol 'ret'.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202308032022.SnXkKyFs-lkp@intel.com/
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 35 ++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index be428b80a39b..4957e9253878 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2585,20 +2585,23 @@ static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 opcode,
 		q_op->flag = OP_PROGRAM_PAGE;
 		nandc->exec_opwrite = true;
 		break;
+	default:
+		dev_err(nandc->dev, "Opcode not supported: %u\n", opcode);
+		return -EOPNOTSUPP;
 	}
 
 	return cmd;
 }
 
 /* NAND framework ->exec_op() hooks and related helpers */
-static void qcom_parse_instructions(struct nand_chip *chip,
+static int qcom_parse_instructions(struct nand_chip *chip,
 				    const struct nand_subop *subop,
 				    struct qcom_op *q_op)
 {
 	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
 	const struct nand_op_instr *instr = NULL;
 	unsigned int op_id;
-	int i;
+	int i, ret;
 
 	for (op_id = 0; op_id < subop->ninstrs; op_id++) {
 		unsigned int offset, naddrs;
@@ -2608,7 +2611,11 @@ static void qcom_parse_instructions(struct nand_chip *chip,
 
 		switch (instr->type) {
 		case NAND_OP_CMD_INSTR:
-			q_op->cmd_reg = qcom_op_cmd_mapping(nandc, instr->ctx.cmd.opcode, q_op);
+			ret = qcom_op_cmd_mapping(nandc, instr->ctx.cmd.opcode, q_op);
+			if (ret < 0)
+				return ret;
+
+			q_op->cmd_reg = ret;
 			q_op->rdy_delay_ns = instr->delay_ns;
 			break;
 
@@ -2641,6 +2648,8 @@ static void qcom_parse_instructions(struct nand_chip *chip,
 			break;
 		}
 	}
+
+	return 0;
 }
 
 static void qcom_delay_ns(unsigned int ns)
@@ -2689,7 +2698,9 @@ static int qcom_read_status_exec(struct nand_chip *chip,
 
 	host->status = NAND_STATUS_READY | NAND_STATUS_WP;
 
-	qcom_parse_instructions(chip, subop, &q_op);
+	ret = qcom_parse_instructions(chip, subop, &q_op);
+	if (ret)
+		return ret;
 
 	num_cw = nandc->exec_opwrite ? ecc->steps : 1;
 	nandc->exec_opwrite = false;
@@ -2749,7 +2760,9 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo
 	unsigned int len = 0;
 	int ret;
 
-	qcom_parse_instructions(chip, subop, &q_op);
+	ret = qcom_parse_instructions(chip, subop, &q_op);
+	if (ret)
+		return ret;
 
 	nandc->buf_count = 0;
 	nandc->buf_start = 0;
@@ -2797,7 +2810,9 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub
 	struct qcom_op q_op = {};
 	int ret;
 
-	qcom_parse_instructions(chip, subop, &q_op);
+	ret = qcom_parse_instructions(chip, subop, &q_op);
+	if (ret)
+		return ret;
 
 	if (q_op.flag == OP_PROGRAM_PAGE)
 		goto wait_rdy;
@@ -2843,7 +2858,9 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	unsigned int len = 0;
 	int ret;
 
-	qcom_parse_instructions(chip, subop, &q_op);
+	ret = qcom_parse_instructions(chip, subop, &q_op);
+	if (ret)
+		return ret;
 
 	q_op.cmd_reg |= PAGE_ACC | LAST_PAGE;
 
@@ -2937,7 +2954,9 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su
 	struct qcom_op q_op = {};
 	int ret;
 
-	qcom_parse_instructions(chip, subop, &q_op);
+	ret = qcom_parse_instructions(chip, subop, &q_op);
+	if (ret)
+		return ret;
 
 	q_op.cmd_reg |= PAGE_ACC | LAST_PAGE;
 
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 03/10] mtd: rawnand: qcom: Handle unsupported opcode in qcom_op_cmd_mapping()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:39 UTC, Manivannan Sadhasivam wrote:
> Handle the scenario where the caller has passed an unsupported opcode to
> qcom_op_cmd_mapping(). In that case, log the error and return the
> -EOPNOTSUPP errono. Also, let's propagate this error code all the way up.
> 
> This also fixes the following smatch warning:
> 
> drivers/mtd/nand/raw/qcom_nandc.c:2941 qcom_op_cmd_mapping() error: uninitialized symbol 'ret'.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202308032022.SnXkKyFs-lkp@intel.com/
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 04/10] mtd: rawnand: qcom: Fix the opcode check in qcom_check_op()

[Manivannan Sadhasivam]

qcom_check_op() function checks for the invalid opcode for the instruction
types. Currently, it just returns -ENOTSUPP for all opcodes of
NAND_OP_CMD_INSTR type due to the use of "||" operator instead of "&&".
Fix it!

This also fixes the following smatch warning:

drivers/mtd/nand/raw/qcom_nandc.c:3036 qcom_check_op() warn: was && intended here instead of ||?

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202308032022.SnXkKyFs-lkp@intel.com/
Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 4957e9253878..29fcdac932f1 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -3033,12 +3033,12 @@ static int qcom_check_op(struct nand_chip *chip,
 
 		switch (instr->type) {
 		case NAND_OP_CMD_INSTR:
-			if (instr->ctx.cmd.opcode != NAND_CMD_RESET ||
-			    instr->ctx.cmd.opcode != NAND_CMD_READID ||
-			    instr->ctx.cmd.opcode != NAND_CMD_PARAM ||
-			    instr->ctx.cmd.opcode != NAND_CMD_ERASE1 ||
-			    instr->ctx.cmd.opcode != NAND_CMD_ERASE2 ||
-			    instr->ctx.cmd.opcode != NAND_CMD_STATUS ||
+			if (instr->ctx.cmd.opcode != NAND_CMD_RESET  &&
+			    instr->ctx.cmd.opcode != NAND_CMD_READID &&
+			    instr->ctx.cmd.opcode != NAND_CMD_PARAM  &&
+			    instr->ctx.cmd.opcode != NAND_CMD_ERASE1 &&
+			    instr->ctx.cmd.opcode != NAND_CMD_ERASE2 &&
+			    instr->ctx.cmd.opcode != NAND_CMD_STATUS &&
 			    instr->ctx.cmd.opcode != NAND_CMD_PAGEPROG)
 				return -ENOTSUPP;
 			break;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 04/10] mtd: rawnand: qcom: Fix the opcode check in qcom_check_op()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:40 UTC, Manivannan Sadhasivam wrote:
> qcom_check_op() function checks for the invalid opcode for the instruction
> types. Currently, it just returns -ENOTSUPP for all opcodes of
> NAND_OP_CMD_INSTR type due to the use of "||" operator instead of "&&".
> Fix it!
> 
> This also fixes the following smatch warning:
> 
> drivers/mtd/nand/raw/qcom_nandc.c:3036 qcom_check_op() warn: was && intended here instead of ||?
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202308032022.SnXkKyFs-lkp@intel.com/
> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 05/10] mtd: rawnand: qcom: Use EOPNOTSUPP instead of ENOTSUPP

[Manivannan Sadhasivam]

Checkpatch complains over the usage of ENOTSUPP for new patches as below:

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

So let's fix the error code which is already present in qcom_check_op().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 29fcdac932f1..a7a9421ef003 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -3040,7 +3040,7 @@ static int qcom_check_op(struct nand_chip *chip,
 			    instr->ctx.cmd.opcode != NAND_CMD_ERASE2 &&
 			    instr->ctx.cmd.opcode != NAND_CMD_STATUS &&
 			    instr->ctx.cmd.opcode != NAND_CMD_PAGEPROG)
-				return -ENOTSUPP;
+				return -EOPNOTSUPP;
 			break;
 		default:
 			break;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 05/10] mtd: rawnand: qcom: Use EOPNOTSUPP instead of ENOTSUPP

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:41 UTC, Manivannan Sadhasivam wrote:
> Checkpatch complains over the usage of ENOTSUPP for new patches as below:
> 
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> 
> So let's fix the error code which is already present in qcom_check_op().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 06/10] mtd: rawnand: qcom: Wrap qcom_nand_exec_op() to 80 columns

[Manivannan Sadhasivam]

Both the function arguments and the definition could be wrapped to 80
columns to save line space.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index a7a9421ef003..4f38579ae03e 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -3051,14 +3051,12 @@ static int qcom_check_op(struct nand_chip *chip,
 }
 
 static int qcom_nand_exec_op(struct nand_chip *chip,
-			     const struct nand_operation *op,
-			bool check_only)
+			     const struct nand_operation *op, bool check_only)
 {
 	if (check_only)
 		return qcom_check_op(chip, op);
 
-	return nand_op_parser_exec_op(chip, &qcom_op_parser,
-			op, check_only);
+	return nand_op_parser_exec_op(chip, &qcom_op_parser, op, check_only);
 }
 
 static const struct nand_controller_ops qcom_nandc_ops = {
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: rawnand: qcom: Wrap qcom_nand_exec_op() to 80 columns

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:42 UTC, Manivannan Sadhasivam wrote:
> Both the function arguments and the definition could be wrapped to 80
> columns to save line space.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 07/10] mtd: rawnand: qcom: Unmap sg_list and free desc within submic_descs()

[Manivannan Sadhasivam]

There are two types of dma descriptors being used in this driver allocated
by, prepare_bam_async_desc() and prep_adm_dma_desc() helper functions.
These functions map and prepare the descriptors to be used for dma
transfers.

And all the descriptors are submitted inside the submit_descs() function.
Once the transfer completion happens, those descriptors should be unmapped
and freed as a part of cleanup.

Currently, free_descs() function is doing the said cleanup of descriptors.
But the callers of submit_descs() are required to call free_descs() in both
the success and error cases.

Since there are no other transactions need to be done after submit_descs(),
let's just move the contents of free_descs() inside submit_descs() itself.

This makes sure that the cleanup is handled within the submit_descs()
thereby offloading the cleanup part from callers.

While at it, let's also rename the return variable from "r" to "ret".

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++---------------------
 1 file changed, 20 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 4f38579ae03e..1dbe17dcdb84 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1306,30 +1306,30 @@ static void config_nand_cw_write(struct nand_chip *chip)
 /* helpers to submit/free our list of dma descriptors */
 static int submit_descs(struct qcom_nand_controller *nandc)
 {
-	struct desc_info *desc;
+	struct desc_info *desc, *n;
 	dma_cookie_t cookie = 0;
 	struct bam_transaction *bam_txn = nandc->bam_txn;
-	int r;
+	int ret = 0;
 
 	if (nandc->props->is_bam) {
 		if (bam_txn->rx_sgl_pos > bam_txn->rx_sgl_start) {
-			r = prepare_bam_async_desc(nandc, nandc->rx_chan, 0);
-			if (r)
-				return r;
+			ret = prepare_bam_async_desc(nandc, nandc->rx_chan, 0);
+			if (ret)
+				goto err_unmap_free_desc;
 		}
 
 		if (bam_txn->tx_sgl_pos > bam_txn->tx_sgl_start) {
-			r = prepare_bam_async_desc(nandc, nandc->tx_chan,
+			ret = prepare_bam_async_desc(nandc, nandc->tx_chan,
 						   DMA_PREP_INTERRUPT);
-			if (r)
-				return r;
+			if (ret)
+				goto err_unmap_free_desc;
 		}
 
 		if (bam_txn->cmd_sgl_pos > bam_txn->cmd_sgl_start) {
-			r = prepare_bam_async_desc(nandc, nandc->cmd_chan,
+			ret = prepare_bam_async_desc(nandc, nandc->cmd_chan,
 						   DMA_PREP_CMD);
-			if (r)
-				return r;
+			if (ret)
+				goto err_unmap_free_desc;
 		}
 	}
 
@@ -1351,19 +1351,17 @@ static int submit_descs(struct qcom_nand_controller *nandc)
 
 		if (!wait_for_completion_timeout(&bam_txn->txn_done,
 						 QPIC_NAND_COMPLETION_TIMEOUT))
-			return -ETIMEDOUT;
+			ret = -ETIMEDOUT;
 	} else {
 		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
-			return -ETIMEDOUT;
+			ret = -ETIMEDOUT;
 	}
 
-	return 0;
-}
-
-static void free_descs(struct qcom_nand_controller *nandc)
-{
-	struct desc_info *desc, *n;
-
+err_unmap_free_desc:
+	/*
+	 * Unmap the dma sg_list and free the desc allocated by both
+	 * prepare_bam_async_desc() and prep_adm_dma_desc() functions.
+	 */
 	list_for_each_entry_safe(desc, n, &nandc->desc_list, node) {
 		list_del(&desc->node);
 
@@ -1376,6 +1374,8 @@ static void free_descs(struct qcom_nand_controller *nandc)
 
 		kfree(desc);
 	}
+
+	return ret;
 }
 
 /* reset the register read buffer for next NAND operation */
@@ -1521,7 +1521,6 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
 	read_data_dma(nandc, reg_off, oob_buf + oob_size1, oob_size2, 0);
 
 	ret = submit_descs(nandc);
-	free_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure to read raw cw %d\n", cw);
 		return ret;
@@ -1775,8 +1774,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
 	}
 
 	ret = submit_descs(nandc);
-	free_descs(nandc);
-
 	if (ret) {
 		dev_err(nandc->dev, "failure to read page/oob\n");
 		return ret;
@@ -1815,8 +1812,6 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
 	if (ret)
 		dev_err(nandc->dev, "failed to copy last codeword\n");
 
-	free_descs(nandc);
-
 	return ret;
 }
 
@@ -2024,8 +2019,6 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const u8 *buf,
 	if (ret)
 		dev_err(nandc->dev, "failure to write page\n");
 
-	free_descs(nandc);
-
 	if (!ret)
 		ret = nand_prog_page_end_op(chip);
 
@@ -2100,8 +2093,6 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
 	if (ret)
 		dev_err(nandc->dev, "failure to write raw page\n");
 
-	free_descs(nandc);
-
 	if (!ret)
 		ret = nand_prog_page_end_op(chip);
 
@@ -2149,9 +2140,6 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
 	config_nand_cw_write(chip);
 
 	ret = submit_descs(nandc);
-
-	free_descs(nandc);
-
 	if (ret) {
 		dev_err(nandc->dev, "failure to write oob\n");
 		return -EIO;
@@ -2228,9 +2216,6 @@ static int qcom_nandc_block_markbad(struct nand_chip *chip, loff_t ofs)
 	config_nand_cw_write(chip);
 
 	ret = submit_descs(nandc);
-
-	free_descs(nandc);
-
 	if (ret) {
 		dev_err(nandc->dev, "failure to update BBM\n");
 		return -EIO;
@@ -2722,10 +2707,8 @@ static int qcom_read_status_exec(struct nand_chip *chip,
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure in submitting status descriptor\n");
-		free_descs(nandc);
 		goto err_out;
 	}
-	free_descs(nandc);
 
 	nandc_read_buffer_sync(nandc, true);
 
@@ -2787,10 +2770,8 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure in submitting read id descriptor\n");
-		free_descs(nandc);
 		goto err_out;
 	}
-	free_descs(nandc);
 
 	instr = q_op.data_instr;
 	op_id = q_op.data_instr_idx;
@@ -2835,10 +2816,8 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure in submitting misc descriptor\n");
-		free_descs(nandc);
 		goto err_out;
 	}
-	free_descs(nandc);
 
 wait_rdy:
 	qcom_delay_ns(q_op.rdy_delay_ns);
@@ -2932,10 +2911,8 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure in submitting param page descriptor\n");
-		free_descs(nandc);
 		goto err_out;
 	}
-	free_descs(nandc);
 
 	ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms);
 	if (ret)
@@ -2981,10 +2958,8 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure in submitting erase descriptor\n");
-		free_descs(nandc);
 		goto err_out;
 	}
-	free_descs(nandc);
 
 	ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms);
 	if (ret)
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 07/10] mtd: rawnand: qcom: Unmap sg_list and free desc within submic_descs()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:43 UTC, Manivannan Sadhasivam wrote:
> There are two types of dma descriptors being used in this driver allocated
> by, prepare_bam_async_desc() and prep_adm_dma_desc() helper functions.
> These functions map and prepare the descriptors to be used for dma
> transfers.
> 
> And all the descriptors are submitted inside the submit_descs() function.
> Once the transfer completion happens, those descriptors should be unmapped
> and freed as a part of cleanup.
> 
> Currently, free_descs() function is doing the said cleanup of descriptors.
> But the callers of submit_descs() are required to call free_descs() in both
> the success and error cases.
> 
> Since there are no other transactions need to be done after submit_descs(),
> let's just move the contents of free_descs() inside submit_descs() itself.
> 
> This makes sure that the cleanup is handled within the submit_descs()
> thereby offloading the cleanup part from callers.
> 
> While at it, let's also rename the return variable from "r" to "ret".
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 08/10] mtd: rawnand: qcom: Simplify the call to nand_prog_page_end_op()

[Manivannan Sadhasivam]

Now that the dma desc cleanup is moved inside submit_descs(), let's
simplify the call to nand_prog_page_end_op() inside qcom_nandc_write_page()
and qcom_nandc_write_page_raw() to match other functions.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 1dbe17dcdb84..6b81781aa3ad 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2016,13 +2016,12 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const u8 *buf,
 	}
 
 	ret = submit_descs(nandc);
-	if (ret)
+	if (ret) {
 		dev_err(nandc->dev, "failure to write page\n");
+		return ret;
+	}
 
-	if (!ret)
-		ret = nand_prog_page_end_op(chip);
-
-	return ret;
+	return nand_prog_page_end_op(chip);
 }
 
 /* implements ecc->write_page_raw() */
@@ -2090,13 +2089,12 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
 	}
 
 	ret = submit_descs(nandc);
-	if (ret)
+	if (ret) {
 		dev_err(nandc->dev, "failure to write raw page\n");
+		return ret;
+	}
 
-	if (!ret)
-		ret = nand_prog_page_end_op(chip);
-
-	return ret;
+	return nand_prog_page_end_op(chip);
 }
 
 /*
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 08/10] mtd: rawnand: qcom: Simplify the call to nand_prog_page_end_op()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:44 UTC, Manivannan Sadhasivam wrote:
> Now that the dma desc cleanup is moved inside submit_descs(), let's
> simplify the call to nand_prog_page_end_op() inside qcom_nandc_write_page()
> and qcom_nandc_write_page_raw() to match other functions.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 09/10] mtd: rawnand: qcom: Do not override the error no of submit_descs()

[Manivannan Sadhasivam]

Just use the error no returned by submit_descs() instead of overriding it
with -EIO.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 6b81781aa3ad..0fbc6d1a558c 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2140,7 +2140,7 @@ static int qcom_nandc_write_oob(struct nand_chip *chip, int page)
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure to write oob\n");
-		return -EIO;
+		return ret;
 	}
 
 	return nand_prog_page_end_op(chip);
@@ -2216,7 +2216,7 @@ static int qcom_nandc_block_markbad(struct nand_chip *chip, loff_t ofs)
 	ret = submit_descs(nandc);
 	if (ret) {
 		dev_err(nandc->dev, "failure to update BBM\n");
-		return -EIO;
+		return ret;
 	}
 
 	return nand_prog_page_end_op(chip);
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 09/10] mtd: rawnand: qcom: Do not override the error no of submit_descs()

[Miquel Raynal]

On Sat, 2023-08-05 at 17:41:45 UTC, Manivannan Sadhasivam wrote:
> Just use the error no returned by submit_descs() instead of overriding it
> with -EIO.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 10/10] mtd: rawnand: qcom: Sort includes alphabetically

[Manivannan Sadhasivam]

Sort includes in alphabetical order.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 0fbc6d1a558c..d4ba0d04c970 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2,19 +2,19 @@
 /*
  * Copyright (c) 2016, The Linux Foundation. All rights reserved.
  */
-#include <linux/clk.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
 #include <linux/bitops.h>
-#include <linux/dma/qcom_adm.h>
-#include <linux/dma-mapping.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma/qcom_adm.h>
+#include <linux/dma/qcom_bam_dma.h>
 #include <linux/module.h>
-#include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
+#include <linux/mtd/rawnand.h>
 #include <linux/of.h>
-#include <linux/delay.h>
-#include <linux/dma/qcom_bam_dma.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 
 /* NANDc reg offsets */
 #define	NAND_FLASH_CMD			0x00
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups

[Miquel Raynal]

Hi Manivannan,

manivannan.sadhasivam@linaro.org wrote on Sat,  5 Aug 2023 23:11:36
+0530:

> Hi Miquel,
> 
> This series has fixes for the smatch warnings reported by Kbuild bot [1]
> and also several cleanup patches based on my code observation.
> 
> I've only compile tested this series. So let's wait for Sadre/Sricharan to
> give a tested-by tag to make sure I didn't mess up anything.

I reviewed all the patches, they look good to me. I'm waiting for the
tests. Please provide the output of nandbiterrs -i.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups

[Manivannan Sadhasivam]

On Sun, Aug 06, 2023 at 03:53:09PM +0200, Miquel Raynal wrote:
> Hi Manivannan,
> 
> manivannan.sadhasivam@linaro.org wrote on Sat,  5 Aug 2023 23:11:36
> +0530:
> 
> > Hi Miquel,
> > 
> > This series has fixes for the smatch warnings reported by Kbuild bot [1]
> > and also several cleanup patches based on my code observation.
> > 
> > I've only compile tested this series. So let's wait for Sadre/Sricharan to
> > give a tested-by tag to make sure I didn't mess up anything.
> 
> I reviewed all the patches, they look good to me. I'm waiting for the
> tests. Please provide the output of nandbiterrs -i.
> 

I tested the series on Qcom SDX55 based dev board, but it was broken already due
to commit <3fc92384b654> ("mtd: rawnand: qcom: Implement exec_op()").

I'm seeing a string of this error while the partitions were being enumerated:
qcom-nandc 1b30000.nand-controller: failed to copy last codeword

It just goes on...

Sadre, on what platform did you test 3fc92384b654? From a quick look, the BAM
DMA transactions are timing out.

- Mani

> Thanks,
> Miquèl

-- 
மணிவண்ணன் சதாசிவம்

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/