Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Colin King <colin.king@canonical.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	linux-mtd@lists.infradead.org, kernel-janitors@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH][next] mtd: rawnand: ensure return variable is initialized
Date: Tue, 1 Jun 2021 15:58:20 +0300	[thread overview]
Message-ID: <20210601125819.GZ1955@kadam> (raw)
In-Reply-To: <20210601121401.GY1955@kadam>

On Tue, Jun 01, 2021 at 03:14:02PM +0300, Dan Carpenter wrote:
> On Thu, May 27, 2021 at 05:03:09PM +0200, Miquel Raynal wrote:
> > Hi Colin,
> > 
> > Colin King <colin.king@canonical.com> wrote on Thu, 27 May 2021
> > 15:50:48 +0100:
> > 
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Currently there are corner cases where spec_times is NULL and
> > > chip->parameters.onfi or when best_mode is zero where ret is
> > 
> >                        ^
> > something is missing here, the sentence is not clear
> > 
> > > not assigned a value and an uninitialized return value can be
> > > returned. Fix this by ensuring ret is initialized to -EINVAL.
> > 
> > I don't see how this situation can happen.
> > 
> > In both cases, no matter the value of best_mode, the for loop will
> > always execute at least one time (mode 0) so ret will be populated.
> > 
> > Maybe the robot does not know that best_mode cannot be negative and
> > should be defined unsigned, but the current patch is invalid.
> >
> 
> People think list counter unsigned is a good idea, but it's a terrible
> idea and has caused hundreds of bugs for me to fix/report over the
> years.  *grumble*.

Imagine if best_mode were unsigned int (and the loop ended on > 0 so it
wasn't an endless loop).  Then instead of a no-op the loop would iterate
4 million times.  Each iteration would trigger the WARN_ON()
onfi_fill_sdr_interface_config().

I think people believe that the compiler will warn them something like:
"warning: Assigning a subtract operation to an unsigned!" but the
compiler is never going to do that.  Unsigned is just a declaration that
"I'm never going to be surprised so let's make this stuff more dangerous
and fun!"

There are times where it's appropriate, sure.  But it's mostly unsigned
long which is correct instead of unsigned int.  If you were to draw a
number line from 0-U64_MAX then the band between 2-4 million is quite
skinny and a long way from the zero.  There aren't that many thing which
fall into the band.  Most numbers are smaller, but once we start talking
about millions then 4 million is very limitting so we would want to use
a 64 bit type.

regards,
dan carpenter