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:

quoted

On Thu, Apr 29, 2021 at 04:02:15PM +0300, Dan Carpenter wrote:

quoted

On Thu, Apr 29, 2021 at 02:08:45PM +0300, Andy Shevchenko wrote:

quoted

On Thu, Apr 29, 2021 at 10:19:22AM +0300, Dan Carpenter wrote:

quoted

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 <redacted>

...

quoted

 			       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