Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: devel@driverdev.osuosl.org, robh@kernel.org, mchehab@kernel.org,
	jorhand@linux.microsoft.com, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, kieran.bingham@ideasonboard.com,
	Daniel Scally <djrscally@gmail.com>,
	yong.zhi@intel.com, Sakari Ailus <sakari.ailus@linux.intel.com>,
	bingbu.cao@intel.com, kitakar@gmail.com, davem@davemloft.net,
	tian.shu.qiu@intel.com, linux-media@vger.kernel.org
Subject: Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Date: Thu, 17 Sep 2020 16:15:39 +0300	[thread overview]
Message-ID: <20200917131539.GR4282@kadam> (raw)
In-Reply-To: <20200917122529.GJ3956970@smile.fi.intel.com>

On Thu, Sep 17, 2020 at 03:25:29PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> 
> > > > +	int i, ret;
> > > 
> > > unsigned int i
> > > 
> > 
> > Why?
> > 
> > For list iterators then "int i;" is best...  For sizes then unsigned is
> > sometimes best.  Or if it's part of the hardware spec or network spec
> > unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> > They're not as intuitive as signed variables.  Imagine if there is an
> > error in this loop and you want to unwind.  With a signed variable you
> > can do:
> > 
> > 	while (--i >= 0)
> > 		cleanup(&bridge.sensors[i]);
> 
> Ha-ha. It's actually a counter argument to your stuff because above is the same as
> 
> 	while (i--)
> 		cleanup(&bridge.sensors[i]);
> 
> with pretty much unsigned int i.

With vanilla "int i;" you can't go wrong because both styles work as
expected.  I was just giving examples of real life bugs that I have seen
involving unsigned iterators.

54313503f9a3 ("drm/amdgpu: signedness bug in amdgpu_cs_parser_init()")

Here are a couple more bugs involving unsigned iterators...

d72cf01f410a ("drm/mipi-dbi: fix a loop in debugfs code")
ce6c1cd2c324 ("pinctrl: nsp-gpio: forever loop in nsp_gpio_get_strength()")

I've change a lot of variables unsigned as well.  For min_t() then it
should *always* be an unsigned type.

It's pretty rare to iterate over 2 billion times in the kernel, but
there are times when you might want to do that.  Normally you would
want to declare the iterator as an unsigned ong in that case.  But most
of the time iterators should just be "int i;" to prevent bugs.

regards,
dan carpenter