Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Stefan Agner <stefan@agner.ch>]

On 03.07.2018 22:04, Boris Brezillon wrote:
> On Tue, 3 Jul 2018 17:19:57 +0300
> Dan Carpenter <dan.carpenter@oracle.com> 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/