From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1ffPFv-00014d-2r for linux-mtd@lists.infradead.org; Tue, 17 Jul 2018 12:39:04 +0000 Date: Tue, 17 Jul 2018 14:38:41 +0200 From: Boris Brezillon To: Miquel Raynal Cc: Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , linux-mtd@lists.infradead.org Subject: Re: [PATCH] mtd: rawnand: tegra: fix error handling of subop helpers Message-ID: <20180717143841.3e8cbd88@bbrezillon> In-Reply-To: <20180717142254.61998c98@xps13> References: <20180714163251.7773-1-miquel.raynal@bootlin.com> <20180717141733.61bc9c71@bbrezillon> <20180717142254.61998c98@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 17 Jul 2018 14:22:54 +0200 Miquel Raynal wrote: > Hi Boris, >=20 > Boris Brezillon wrote on Tue, 17 Jul 2018 > 14:17:33 +0200: >=20 > > On Sat, 14 Jul 2018 18:32:51 +0200 > > Miquel Raynal wrote: > > =20 > > > 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. > > >=20 > > > Fix the Tegra NAND controller driver implementation by checking > > > potential negative error values coming from these helpers. =20 > >=20 > > Hm, ok. I thought you were opting for a return 0 + WARN_ON() approach, > > what made you change your mind? =20 >=20 > 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... >=20 > 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. >=20 > > =20 > > >=20 > > > Signed-off-by: Miquel Raynal > > > --- > > >=20 > > > 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. > > >=20 > > > Thanks, > > > Miqu=C3=A8l > > >=20 > > > drivers/mtd/nand/raw/tegra_nand.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > >=20 > > > 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 =3D nand_subop_get_addr_start_off(subop, op_id); > > > naddrs =3D nand_subop_get_num_addr_cyc(subop, op_id); > > > + if (offset < 0 || naddrs < 0) > > > + return -EINVAL; > > > + > > > addrs =3D &instr->ctx.addr.addrs[offset]; > > > =20 > > > cmd |=3D 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 =3D nand_subop_get_data_len(subop, op_id); > > > offset =3D nand_subop_get_data_start_off(subop, op_id); > > > + if (size < 0 || offset < 0) > > > + return -EINVAL; > > > =20 > > > cmd |=3D 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 =3D nand_subop_get_data_len(subop, op_id); > > > offset =3D nand_subop_get_data_start_off(subop, op_id); > > > + if (size < 0 || offset < 0) > > > + return -EINVAL; > > > =20 > > > cmd |=3D COMMAND_TRANS_SIZE(size) | COMMAND_PIO | > > > COMMAND_TX | COMMAND_A_VALID; =20 > > =20 >=20 >=20 >=20