Re: [PATCH] serial: 8250_omap: fix a timeout loop condition

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Alexander Sverdlin <alexander.sverdlin@gmail.com>,
	Johan Hovold <johan@kernel.org>,
	linux-serial@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] serial: 8250_omap: fix a timeout loop condition
Date: Fri, 14 May 2021 11:00:33 +0300	[thread overview]
Message-ID: <20210514080033.GG1922@kadam> (raw)
In-Reply-To: <YI/HqpJN+OvYduMn@smile.fi.intel.com>

On Mon, May 03, 2021 at 12:51:38PM +0300, Andy Shevchenko wrote:
> > Signed ints
> > are safer.
> 
> Any research article about it? What about wrong integral promotions which
> I consider is a root cause of many bugs? People should learn that, or the
> C (standard) should be fixed to make it easier to get.
> 
> > Unsigned ints are a *leading* cause of bugs in the kernel.
> 
> Again, where this statistics comes from? Maybe it's a simple answer to the
> question that review in kernel is not good enough?
> 

When I say that unsigned ints are a leading cause of bug I'm talking
about things like commit 95b079d8215b ("swiotlb: Fix the type of index")
which does this:

-       unsigned int index, i;
+       unsigned int i;
+       int index;

This is perhaps a poor example, because the patch doesn't really fix a
bug.  I don't think that the author thought about how type promotion
works or else the commit would have said "this commit does not change
runtime behavior".

The other reason it's a bad example, is that although a "int i;" would
work here in real life, the correct type is unsigned long i.  The size
of the allocation is specified by the user and allocated at boot with
memblock_alloc() so it might actually be possible to allocate gigabytes
of memory.  I said that earlier, that if you really need an unsigned
list counter then it's pretty likely it should be "unsigned long"
instead of "unsigned int".

But what I get from this example is that people are just declaring
things "unsigned int" for no reason.  I expect that unsigned comparisons
with zero will drop off now that GCC has a compile time warning for
that.  I've been developing the kernel for a decade now and I've
probably had to deal with one of these bugs every ten days on average
over that period.

Meanwhile GCC urges people to declare the list counter as
"unsigned int i;" but I have never seen that fix even a single real
life kernel bug.  I'm not an academic but I have looked for these kinds
of bugs.  GCC warns about it even when it can see at compile time that
the type makes no difference.

There are performance reasons for unsigned types.  Also inside structs
then unsigned ints can make sense.  Also it's nice to declare array
indexes as unsigned because it means that every time you check against
the max, there is a hidden check against negatives because of type
promotion.  But for list counters which start at zero and increment up
then none of that applies.

High level languages like Python don't have signed types.  Pascal had
unsigned types but Niklaus Wirth got rid of them in later programming
languages like Oberon.  Unsigned types and type promotion can help with
performance but they make things more complicated.

regards,
dan carpenter