From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Agner Subject: Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver Date: Wed, 04 Jul 2018 09:43:44 +0200 Message-ID: <97dad372225961450773b9375920d806@agner.ch> References: <20180703141957.j27w6efnkhij5blz@kili.mountain> <20180703220424.52c3d8f2@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180703220424.52c3d8f2@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-mtd" Errors-To: linux-mtd-bounces+gldm-linux-mtd-36=gmane.org@lists.infradead.org To: Boris Brezillon Cc: linux-tegra@vger.kernel.org, Richard Weinberger , linux-mtd@lists.infradead.org, Dan Carpenter List-Id: linux-tegra@vger.kernel.org On 03.07.2018 22:04, Boris Brezillon wrote: > On Tue, 3 Jul 2018 17:19:57 +0300 > Dan Carpenter wrote: > >> Hello Stefan Agner, >> >> The patch d7d9f8ec77fe: "mtd: rawnand: add NVIDIA Tegra NAND Flash >> controller driver" from Jun 24, 2018, leads to the following static >> checker warning: >> >> drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() >> warn: array off by one? 'nand->cs[die_nr]' >> >> drivers/mtd/nand/raw/tegra_nand.c >> 465 static void tegra_nand_select_chip(struct mtd_info *mtd, int die_nr) >> 466 { >> 467 struct nand_chip *chip = mtd_to_nand(mtd); >> 468 struct tegra_nand_chip *nand = to_tegra_chip(chip); >> 469 struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller); >> 470 >> 471 if (die_nr < 0 || die_nr > 1) { >> 472 ctrl->cur_cs = -1; >> 473 return; >> 474 } >> 475 >> 476 ctrl->cur_cs = nand->cs[die_nr]; >> 477 } >> >> The story is that nand->cs[] is a one element array. Some people use >> one element arrays like this as variable size arrays. It's better to >> use a zero size array, but I think that might be a GCC feature and not >> everyone knows you can do that. Smatch treats this one as unknown size >> because apparently it can't tie it back to the kmalloc(). >> >> But it really is a one element array and the condition is off by one. > > I don't see where it's off by one? With the above test, die_nr is > guaranteed to be 0 when you reach the > "ctrl->cur_cs = nand->cs[die_nr];" statement, right? Am I missing > something? > Yeah I had to look twice too. But die_nr can be 1 according to this code... It should be: if (die_nr < 0 || die_nr >= 1) { >> >> But really one element arrays are super weird. Why not just use a >> pointer? > > The controller supports more than 1 CS, and I guess the plan was to > extend the array when the driver is ready to support this use case. I > guess we could make ->cs a single integer instead of an array of size > 1 if that helps. Exactly, and using the array also aligns with other drivers. I prefer to fix the issue above and leave it an array. I'll send a patch. -- Stefan ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kmu-office.ch ([2a02:418:6a02::a2]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1facSC-0000q1-KM for linux-mtd@lists.infradead.org; Wed, 04 Jul 2018 07:43:58 +0000 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Date: Wed, 04 Jul 2018 09:43:44 +0200 From: Stefan Agner To: Boris Brezillon Cc: Dan Carpenter , linux-tegra@vger.kernel.org, Richard Weinberger , linux-mtd@lists.infradead.org Subject: Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver In-Reply-To: <20180703220424.52c3d8f2@bbrezillon> References: <20180703141957.j27w6efnkhij5blz@kili.mountain> <20180703220424.52c3d8f2@bbrezillon> Message-ID: <97dad372225961450773b9375920d806@agner.ch> List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 03.07.2018 22:04, Boris Brezillon wrote: > On Tue, 3 Jul 2018 17:19:57 +0300 > Dan Carpenter wrote: > >> Hello Stefan Agner, >> >> The patch d7d9f8ec77fe: "mtd: rawnand: add NVIDIA Tegra NAND Flash >> controller driver" from Jun 24, 2018, leads to the following static >> checker warning: >> >> drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() >> warn: array off by one? 'nand->cs[die_nr]' >> >> drivers/mtd/nand/raw/tegra_nand.c >> 465 static void tegra_nand_select_chip(struct mtd_info *mtd, int die_nr) >> 466 { >> 467 struct nand_chip *chip = mtd_to_nand(mtd); >> 468 struct tegra_nand_chip *nand = to_tegra_chip(chip); >> 469 struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller); >> 470 >> 471 if (die_nr < 0 || die_nr > 1) { >> 472 ctrl->cur_cs = -1; >> 473 return; >> 474 } >> 475 >> 476 ctrl->cur_cs = nand->cs[die_nr]; >> 477 } >> >> The story is that nand->cs[] is a one element array. Some people use >> one element arrays like this as variable size arrays. It's better to >> use a zero size array, but I think that might be a GCC feature and not >> everyone knows you can do that. Smatch treats this one as unknown size >> because apparently it can't tie it back to the kmalloc(). >> >> But it really is a one element array and the condition is off by one. > > I don't see where it's off by one? With the above test, die_nr is > guaranteed to be 0 when you reach the > "ctrl->cur_cs = nand->cs[die_nr];" statement, right? Am I missing > something? > Yeah I had to look twice too. But die_nr can be 1 according to this code... It should be: if (die_nr < 0 || die_nr >= 1) { >> >> But really one element arrays are super weird. Why not just use a >> pointer? > > The controller supports more than 1 CS, and I guess the plan was to > extend the array when the driver is ready to support this use case. I > guess we could make ->cs a single integer instead of an array of size > 1 if that helps. Exactly, and using the array also aligns with other drivers. I prefer to fix the issue above and leave it an array. I'll send a patch. -- Stefan