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@xxxxxxxxxx>
> > > >
> > > > ...
> > > >
> > > > > 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.
--
With Best Regards,
Andy Shevchenko
