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 Dan,

dan.carpenter@linaro.org wrote on Fri, 4 Aug 2023 19:52:50 +0300:

> On Fri, Aug 04, 2023 at 06:45:50PM +0200, Miquel Raynal wrote:
> > Hi Sadre, Sricharan & Manivannan,
> > 
> > A couple of questions for you below.
> > 
> > dan.carpenter@linaro.org wrote on Thu, 3 Aug 2023 15:20:56 +0300:
> >   
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > > head:   fb4327106e5250ee360d0d8b056c1eef7eeb9a98
> > > commit: 89550beb098e04b951df079483fb064eafc0e5fa [1937/6910] mtd: rawnand: qcom: Implement exec_op()
> > > config: riscv-randconfig-m031-20230730 (https://download.01.org/0day-ci/archive/20230803/202308032022.SnXkKyFs-lkp@intel.com/config)
> > > compiler: riscv64-linux-gcc (GCC) 12.3.0
> > > reproduce: (https://download.01.org/0day-ci/archive/20230803/202308032022.SnXkKyFs-lkp@intel.com/reproduce)
> > > 
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | 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/
> > > 
> > > New smatch warnings:
> > > drivers/mtd/nand/raw/qcom_nandc.c:2941 qcom_op_cmd_mapping() error: uninitialized symbol 'ret'.
> > > drivers/mtd/nand/raw/qcom_nandc.c:3369 qcom_check_op() warn: was && intended here instead of ||?
> > > 
> > > Old smatch warnings:
> > > drivers/mtd/nand/raw/qcom_nandc.c:3076 qcom_read_status_exec() warn: inconsistent indenting
> > > 
> > > vim +/ret +2941 drivers/mtd/nand/raw/qcom_nandc.c
> > > 
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2909  static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 cmd,
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2910  			       struct qcom_op *q_op)
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2911  {
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2912  	int ret;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2913  
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2914  	switch (cmd) {
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2915  	case NAND_CMD_RESET:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2916  		ret = OP_RESET_DEVICE;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2917  		break;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2918  	case NAND_CMD_READID:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2919  		ret = OP_FETCH_ID;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2920  		break;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2921  	case NAND_CMD_PARAM:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2922  		if (nandc->props->qpic_v2)
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2923  			ret = OP_PAGE_READ_ONFI_READ;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2924  		else
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2925  			ret = OP_PAGE_READ;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2926  		break;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2927  	case NAND_CMD_ERASE1:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2928  	case NAND_CMD_ERASE2:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2929  		ret = OP_BLOCK_ERASE;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2930  		break;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2931  	case NAND_CMD_STATUS:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2932  		ret = OP_CHECK_STATUS;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2933  		break;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2934  	case NAND_CMD_PAGEPROG:
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2935  		ret = OP_PROGRAM_PAGE;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2936  		q_op->flag = OP_PROGRAM_PAGE;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2937  		nandc->exec_opwrite = true;
> > > 89550beb098e04 drivers/mtd/nand/raw/qcom_nandc.c Md Sadre Alam 2023-07-10  2938  		break;
> > > 
> > > No default case.  I'm more concerned about the && vs || warning, but
> > > the zero day bot doesn't include that code into the email.  
> > 
> > The default case here is in theory not possible, as long as
> > qcom_check_op() is properly implemented. We should however silence the
> > warning by handling it. Maybe a WARN_ON_ONCE() + ret = <anything that
> > will produce an error when executing the command> would work.
> >   
> 
> The kbuild-bot runs without the cross function database so it has a lot
> of false positives that you wouldn't see if you had the DB.  (There
> isn't really a way to build with the cross function DB.  It takes way
> way way too long.  Also it sometimes finds bug which are complicated to
> explain or to assign blame for in an automated way).  But these false
> positives are frustrating to me.

I understand.

> In real life, GCC is going to automatically initialize stack variables
> to zero on production systems.

Well, we must silence this by setting reg in all cases, but here the
variable is badly named, it should be s/ret/cmd/. That's why, if we
silence it properly, I would go for a runtime warning if we ever
encounter the situation because this is a purely software bug.

> > However I doubt the following piece of code has been successfully
> > tested:
> > 
> >         for (op_id = 0; op_id < op->ninstrs; op_id++) {
> >                 instr = &op->instrs[op_id];
> > 
> >                 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 ||
> >                             instr->ctx.cmd.opcode != NAND_CMD_PAGEPROG)
> >                                 return -ENOTSUPP;
> >                         break;
> > 
> > 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.
> 
> regards,
> dan carpenter
> 


Thanks,
Miquèl