Re: [PATCH] mtd: rawnand: tegra: fix error handling of subop helpers

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Tue, 17 Jul 2018 14:22:54 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 17 Jul 2018
> 14:17:33 +0200:
> 
> > On Sat, 14 Jul 2018 18:32:51 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > A report from Colin Ian King pointed a CoverityScan issue where error
> > > values on these helpers where not checked in the drivers. These
> > > helpers could error out only in case of a software bug in driver code,
> > > not because of a runtime/hardware error but in any cases it is safer
> > > to handle these errors properly.
> > > 
> > > Fix the Tegra NAND controller driver implementation by checking
> > > potential negative error values coming from these helpers.    
> > 
> > Hm, ok. I thought you were opting for a return 0 + WARN_ON() approach,
> > what made you change your mind?  
> 
> Wise people told me WARN_ON() should be avoided as much as possible.

I think I mentioned BUG_ON(), not WARN_ON() :P.

> Hence after more discussion with myself I choose to implement the most
> standard C solution: check the returned value...
> 
> But if you think a return 0 + WARN_ON() would be better I'm ready to
> change this as it was my initial idea :)

Well, if this cannot happen without a SW bug, then I'd recommend the
WARN_ON() + unsigned int ret approach. That should force people debug
their implementation while keeping drivers code simple.

> 
> >   
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > > 
> > > This patch should have been part of the following series:
> > > http://lists.infradead.org/pipermail/linux-mtd/2018-July/082654.html
> > > It will be squashed with original commit introducing this driver.
> > > 
> > > Thanks,
> > > Miquèl
> > > 
> > > drivers/mtd/nand/raw/tegra_nand.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> > > index 22d6a7f9ff80..59ead8f41718 100644
> > > --- a/drivers/mtd/nand/raw/tegra_nand.c
> > > +++ b/drivers/mtd/nand/raw/tegra_nand.c
> > > @@ -379,6 +379,9 @@ static int tegra_nand_cmd(struct nand_chip *chip,
> > >  		case NAND_OP_ADDR_INSTR:
> > >  			offset = nand_subop_get_addr_start_off(subop, op_id);
> > >  			naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
> > > +			if (offset < 0 || naddrs < 0)
> > > +				return -EINVAL;
> > > +
> > >  			addrs = &instr->ctx.addr.addrs[offset];
> > >  
> > >  			cmd |= COMMAND_ALE | COMMAND_ALE_SIZE(naddrs);
> > > @@ -395,6 +398,8 @@ static int tegra_nand_cmd(struct nand_chip *chip,
> > >  		case NAND_OP_DATA_IN_INSTR:
> > >  			size = nand_subop_get_data_len(subop, op_id);
> > >  			offset = nand_subop_get_data_start_off(subop, op_id);
> > > +			if (size < 0 || offset < 0)
> > > +				return -EINVAL;
> > >  
> > >  			cmd |= COMMAND_TRANS_SIZE(size) | COMMAND_PIO |
> > >  				COMMAND_RX | COMMAND_A_VALID;
> > > @@ -405,6 +410,8 @@ static int tegra_nand_cmd(struct nand_chip *chip,
> > >  		case NAND_OP_DATA_OUT_INSTR:
> > >  			size = nand_subop_get_data_len(subop, op_id);
> > >  			offset = nand_subop_get_data_start_off(subop, op_id);
> > > +			if (size < 0 || offset < 0)
> > > +				return -EINVAL;
> > >  
> > >  			cmd |= COMMAND_TRANS_SIZE(size) | COMMAND_PIO |
> > >  				COMMAND_TX | COMMAND_A_VALID;    
> >   
> 
> 
>