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, 30 Apr 2021 16:33:29 +0300	[thread overview]
Message-ID: <20210430133329.GH1981@kadam> (raw)
In-Reply-To: <YIv92DBnaVotWd9Y@smile.fi.intel.com>

On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote:
> > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote:
> > > > On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote:
> > > > > > This loop ends on -1 so the error message will never be printed.
> > > > > > 
> > > > > > Fixes: 4bcf59a5dea0 ("serial: 8250: 8250_omap: Account for data in flight during DMA teardown")
> > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > 
> > > > > ...
> > > > > 
> > > > > >  			       poll_count--)
> > > > > >  				cpu_relax();
> > > > > >  
> > > > > > -			if (!poll_count)
> > > > > > +			if (poll_count == -1)
> > > > > 
> > > > > Why not to change poll_count-- to --poll_count?
> > > > >
> > > > 
> > > > Either one is fine.  I considered several different ways and wrote the
> > > > patch twice.  The downside of --poll_count is that it's an off by one
> > > > in that the author clearly intended to loop 25 times.  It doesn't really
> > > > matter if we only loop 24 but off by ones are aesthetically unpleasant.
> > > 
> > > I didn't get. If you use --poll_count you get exactly 25 times and moreover,
> > > you may convert variable to unsigned type.
> > > 
> > 
> > Here is a small test to show that it loops 24 times.
> > 
> > #include <stdio.h>
> > 
> > int main(void)
> > {
> >         int i = 25;
> > 
> >         while (--i)
> >                 printf("%d\n", i);
> > 
> >         return 0;
> > }
> > 
> > gcc test.c
> > ./a.out | tac
> > 
> > Why would I make it unsigned?  As a static analysis developer,
> > pointlessly unsigned variables are one of the leading causes for the
> > bugs I see.
> > 
> > There are times where a iterator counter needs to be unsigned long, or
> > u64 but I have never seen a case where changing an iterator from
> > "int i;" to "unsigned int i;" solves a real life kernel bug.  It only
> > introduces bugs.
> 
> See my followup to that, I meant
> 
> unsigned int count;
> 
> do {
> 	...
> } while (--count);
> 
> It doesn't solve bug, but prevents the code be read incorrectly like what you
> are fixing can be avoided with do {} while (); along with unsigned type.
> 

Why would you use an unsigned int for this???

regards,
dan carpenter