Re: [linux-next:master 1937/6910] drivers/mtd/nand/raw/qcom_nandc.c:2941 qcom_op_cmd_mapping() error: uninitialized symbol 'ret'.

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Sricharan,

quic_srichara@quicinc.com wrote on Tue, 8 Aug 2023 00:24:08 +0530:

> Hi Mani/Miquel,
> 
> On 8/6/2023 1:28 PM, Sricharan Ramabadhran wrote:
> > Hi Miquel/Mani/Dan,
> > 
> > <..>
> > 
> >>>>> The || should be &&, otherwise it cannot work, or am I missing
> >>>>> something?
> >>>>
> >>>> Yeah.  That's how this bug normally looks like.  NAND_OP_CMD_INSTR
> >>>> always returns -ENOTSUPP.
> >>>>
> >>
> >> Yes, this is a bug.
> >>
> >> I didn't get a chance to review the exec_op conversion series (blame >> myself).
> >> Now I see scope for a cleaup series :/
> >>
> >> Let me spin something by the end of today.
> >>
> >    Thanks a lot Mani for the quick fixes. Will test it.
> >    Sorry it was a holiday for us past few days, so just checked this.
> > 
> > Regards,
> >   Sricharan
> 
>   With this series applied on linux-next, started seeing the below
>   messages flooded on console while doing mtd r/w.
>   "xxx "Opcode not supported: 0"
> 
>   opcode '0' corresponds to NAND_CMD_READ0. This command inturn was
>   invoked from qcom_nandc.c driver from below places. For read/write_page
>   driver does not use the exec ops. Hence these calls just ends up
>   being -ENOTSUPP and ignored. So removed their invocations.
>   If this is fine, can this be added to your series ? (will send
>   git-format patch to add to your cleanup series). So far, tested
>   mtd raw/block read/writes and all works fine. Will do further tests
>   as well.

Unless I really don't understand the controller, this is non sense.
nand_read_page_op() is precisely what allows your NAND to perform a
read. Removing this call cannot work.

What you need is a proper ->exec_op() implementation, and repeating
this becomes slightly annoying. 

Look at your qcom_op_cmd_mapping, you don't even have a path for reads.
I bet something along:
	CMD_READ0:
		ret = XXX_OPCODE_READ;
		break;
will make it work.

Please fix the driver and test with nandbiterrs -i. If this test works,
it is encouraging. Otherwise it is still broken.
 
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1470,7 +1470,6 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
>          int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
>          int raw_cw = cw;
> 
> -       nand_read_page_op(chip, page, 0, NULL, 0);
>          host->use_ecc = false;
> 
>          if (nandc->props->qpic_v2)
> @@ -1890,7 +1889,6 @@ static int qcom_nandc_read_page(struct nand_chip *chip, u8 *buf,
>          if (host->nr_boot_partitions)
>                  qcom_nandc_codeword_fixup(host, page);
> 
> -       nand_read_page_op(chip, page, 0, NULL, 0);
>          nandc->buf_count = 0;
>          nandc->buf_start = 0;
>          host->use_ecc = true;
> @@ -1965,8 +1963,6 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const u8 *buf,
>          if (host->nr_boot_partitions)
>                  qcom_nandc_codeword_fixup(host, page);
> 
> -       nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> -
>          set_address(host, 0, page);
>          nandc->buf_count = 0;
>          nandc->buf_start = 0;
> @@ -2039,7 +2035,6 @@ static int qcom_nandc_write_page_raw(struct nand_chip *chip,
>          if (host->nr_boot_partitions)
>                  qcom_nandc_codeword_fixup(host, page);
> 
> -       nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>          clear_read_regs(nandc);
>          clear_bam_transaction(nandc);
> 
> 
> Regards,
>   Sricharan


Miquèl