diff mbox series

mtd: rawnand: tegra: fix error handling of subop helpers

Message ID 20180714163251.7773-1-miquel.raynal@bootlin.com
State Changes Requested
Headers show
Series mtd: rawnand: tegra: fix error handling of subop helpers | expand

Commit Message

Miquel Raynal July 14, 2018, 4:32 p.m. UTC 
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.

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(+)

Comments

Boris Brezillon July 17, 2018, 12:17 p.m. UTC | #1 
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?

> 
> 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 July 17, 2018, 12:22 p.m. UTC | #2 
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;  
>
Boris Brezillon July 17, 2018, 12:38 p.m. UTC | #3 
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;    
> >   
> 
> 
>
Miquel Raynal July 17, 2018, 12:41 p.m. UTC | #4 
Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 17 Jul 2018
14:38:41 +0200:

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

I've never been a good listener :)

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

I'm fine with this approach, I'll send a v2.

Thanks,
Miquèl
diff mbox series

Patch

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;