Re: [mtd:spi-mem-ecc 14/29] drivers/mtd/nand/ecc-mxic.c:548 mxic_ecc_prepare_io_req_external() error: uninitialized symbol 'ret'.

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

[-- Attachment #1: Type: text/plain, Size: 6512 bytes --]

Hi Dan,

dan.carpenter(a)oracle.com wrote on Wed, 9 Feb 2022 10:22:04 +0300:

> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-mem-ecc
> head:   d6986e74ec6ee6a48ce9ee1d8051b2988d747558
> commit: cfe5cf69e97267e9d1de65cc894b7a2310644a15 [14/29] mtd: nand: mxic-ecc: Add Macronix external ECC engine support
> config: x86_64-randconfig-m001-20220207 (https://download.01.org/0day-ci/archive/20220209/202202090415.SS8TwwHj-lkp(a)intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/mtd/nand/ecc-mxic.c:548 mxic_ecc_prepare_io_req_external() error: uninitialized symbol 'ret'.
> 
> vim +/ret +548 drivers/mtd/nand/ecc-mxic.c
> 
> cfe5cf69e97267 Miquel Raynal 2021-12-16  494  static int mxic_ecc_prepare_io_req_external(struct nand_device *nand,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  495  					    struct nand_page_io_req *req)
> cfe5cf69e97267 Miquel Raynal 2021-12-16  496  {
> cfe5cf69e97267 Miquel Raynal 2021-12-16  497  	struct mxic_ecc_engine *mxic = nand_to_mxic(nand);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  498  	struct mxic_ecc_ctx *ctx = nand_to_ecc_ctx(nand);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  499  	struct mtd_info *mtd = nanddev_to_mtd(nand);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  500  	int offset, nents, step, ret;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  501  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  502  	if (req->mode == MTD_OPS_RAW)
> cfe5cf69e97267 Miquel Raynal 2021-12-16  503  		return 0;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  504  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  505  	nand_ecc_tweak_req(&ctx->req_ctx, req);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  506  	ctx->req = req;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  507  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  508  	if (req->type == NAND_PAGE_READ)
> cfe5cf69e97267 Miquel Raynal 2021-12-16  509  		return 0;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  510  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  511  	mxic_ecc_add_room_in_oobbuf(ctx, ctx->oobwithstat,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  512  				    ctx->req->oobbuf.out);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  513  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  514  	sg_set_buf(&ctx->sg[0], req->databuf.out, req->datalen);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  515  	sg_set_buf(&ctx->sg[1], ctx->oobwithstat,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  516  		   req->ooblen + (ctx->steps * STAT_BYTES));
> cfe5cf69e97267 Miquel Raynal 2021-12-16  517  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  518  	nents = dma_map_sg(mxic->dev, ctx->sg, 2, DMA_BIDIRECTIONAL);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  519  	if (!nents)
> cfe5cf69e97267 Miquel Raynal 2021-12-16  520  		return -EINVAL;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  521  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  522  	mutex_lock(&mxic->lock);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  523  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  524  	for (step = 0; step < ctx->steps; step++) {
> cfe5cf69e97267 Miquel Raynal 2021-12-16  525  		writel(sg_dma_address(&ctx->sg[0]) + (step * ctx->data_step_sz),
> cfe5cf69e97267 Miquel Raynal 2021-12-16  526  		       mxic->regs + SDMA_MAIN_ADDR);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  527  		writel(sg_dma_address(&ctx->sg[1]) + (step * (ctx->oob_step_sz + STAT_BYTES)),
> cfe5cf69e97267 Miquel Raynal 2021-12-16  528  		       mxic->regs + SDMA_SPARE_ADDR);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  529  		ret = mxic_ecc_process_data(mxic, ctx->req->type);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  530  		if (ret)
> cfe5cf69e97267 Miquel Raynal 2021-12-16  531  			break;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  532  	}
> cfe5cf69e97267 Miquel Raynal 2021-12-16  533  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  534  	mutex_unlock(&mxic->lock);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  535  
> cfe5cf69e97267 Miquel Raynal 2021-12-16  536  	dma_unmap_sg(mxic->dev, ctx->sg, 2, DMA_BIDIRECTIONAL);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  537  
> 
> Smatch is complaining that ctx->steps might be zero.  I should probably
> try to silence that kind of warning if the cross function DB has not
> been built.  It tends to have false positives.

I'm sorry I don't know what you mean by "if the cross function DB has
not been built"?

Both ->prepare() and ->finish() hooks cannot be called if ->init_ctx()
failed (the core enforces that). In the init implementation, there is
this:

conf->step_size = SZ_1K;
steps = mtd->writesize / conf->step_size;
ctx->steps = steps;

Also, mtd->writesize == 0 is impossible here because:
in drivers/mtd/nand/core.c:nanddev_init():
we check that memorg->pagesize != 0 and then we assign mtd->writesize
to memorg->pagesize.

nanddev_init() is called by the raw NAND core and SPI NAND core, which
are the only two possible users of this driver.

So I would say this is a false positive that you can silence.

> But shouldn't we have an if (ret) return ret; after this dma_unmap_sg()?
> Can we really retrieve the calculated ECC bytes when processing the data
> failed?

Well yeah, that's right, I'll fix it inline, thanks for catching that.

> cfe5cf69e97267 Miquel Raynal 2021-12-16  538  	/* Retrieve the calculated ECC bytes */
> cfe5cf69e97267 Miquel Raynal 2021-12-16  539  	for (step = 0; step < ctx->steps; step++) {
> cfe5cf69e97267 Miquel Raynal 2021-12-16  540  		offset = ctx->meta_sz + (step * ctx->oob_step_sz);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  541  		mtd_ooblayout_get_eccbytes(mtd,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  542  					   (u8 *)ctx->req->oobbuf.out + offset,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  543  					   ctx->oobwithstat + (step * STAT_BYTES),
> cfe5cf69e97267 Miquel Raynal 2021-12-16  544  					   step * ctx->parity_sz,
> cfe5cf69e97267 Miquel Raynal 2021-12-16  545  					   ctx->parity_sz);
> cfe5cf69e97267 Miquel Raynal 2021-12-16  546  	}
> cfe5cf69e97267 Miquel Raynal 2021-12-16  547  
> cfe5cf69e97267 Miquel Raynal 2021-12-16 @548  	return ret;
> cfe5cf69e97267 Miquel Raynal 2021-12-16  549  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
> 


Thanks,
Miquèl