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

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

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.
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 :)

> 
> > 
> > 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;  
> 



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com