All of lore.kernel.org
 help / color / mirror / Atom feed
From: Boris Brezillon <boris.brezillon@bootlin.com>
To: Stefan Agner <stefan@agner.ch>
Cc: linux-tegra@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Date: Wed, 4 Jul 2018 09:52:08 +0200	[thread overview]
Message-ID: <20180704095208.090b8091@bbrezillon> (raw)
In-Reply-To: <97dad372225961450773b9375920d806@agner.ch>

On Wed, 04 Jul 2018 09:43:44 +0200
Stefan Agner <stefan@agner.ch> wrote:

> 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) {

Oh, brain fart on my end. Indeed, now that I see the fix it's
obvious :-).

You should probably add a WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)),
because that would clearly be a bug in the core if you're passed a CS
that is not 0 or -1 since you pass max_chipselect = 1 to nand_scan().

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <boris.brezillon@bootlin.com>
To: Stefan Agner <stefan@agner.ch>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
	linux-tegra@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org
Subject: Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Date: Wed, 4 Jul 2018 09:52:08 +0200	[thread overview]
Message-ID: <20180704095208.090b8091@bbrezillon> (raw)
In-Reply-To: <97dad372225961450773b9375920d806@agner.ch>

On Wed, 04 Jul 2018 09:43:44 +0200
Stefan Agner <stefan@agner.ch> wrote:

> 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) {

Oh, brain fart on my end. Indeed, now that I see the fix it's
obvious :-).

You should probably add a WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)),
because that would clearly be a bug in the core if you're passed a CS
that is not 0 or -1 since you pass max_chipselect = 1 to nand_scan().

  reply	other threads:[~2018-07-04  7:52 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-03 14:19 [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver Dan Carpenter
2018-07-03 14:19 ` Dan Carpenter
2018-07-03 20:04 ` Boris Brezillon
2018-07-03 20:04   ` Boris Brezillon
2018-07-04  7:43   ` Stefan Agner
2018-07-04  7:43     ` Stefan Agner
2018-07-04  7:52     ` Boris Brezillon [this message]
2018-07-04  7:52       ` Boris Brezillon
2018-07-04  8:14       ` Stefan Agner
2018-07-04  8:14         ` Stefan Agner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180704095208.090b8091@bbrezillon \
    --to=boris.brezillon@bootlin.com \
    --cc=dan.carpenter@oracle.com \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=stefan@agner.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.