[PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Colin Ian King]

There are incorrect %u format specifiers being used to for signed integers,
fix this by using %d instead.

Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
 drivers/staging/media/atomisp/pci/runtime/bufq/src/bufq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/bufq/src/bufq.c b/drivers/staging/media/atomisp/pci/runtime/bufq/src/bufq.c
index bda35614c862..0f0d16f4ce7c 100644
--- a/drivers/staging/media/atomisp/pci/runtime/bufq/src/bufq.c
+++ b/drivers/staging/media/atomisp/pci/runtime/bufq/src/bufq.c
@@ -497,7 +497,7 @@ void ia_css_bufq_dump_queue_info(void)
 	for (i = 0; i < SH_CSS_MAX_SP_THREADS; i++) {
 		for (j = 0; j < SH_CSS_MAX_NUM_QUEUES; j++) {
 			snprintf(prefix, BUFQ_DUMP_FILE_NAME_PREFIX_SIZE,
-				 "host2sp_buffer_queue[%u][%u]", i, j);
+				 "host2sp_buffer_queue[%d][%d]", i, j);
 			bufq_dump_queue_info(prefix,
 					     &css_queues.host2sp_buffer_queue_handles[i][j]);
 		}
@@ -505,7 +505,7 @@ void ia_css_bufq_dump_queue_info(void)
 
 	for (i = 0; i < SH_CSS_MAX_NUM_QUEUES; i++) {
 		snprintf(prefix, BUFQ_DUMP_FILE_NAME_PREFIX_SIZE,
-			 "sp2host_buffer_queue[%u]", i);
+			 "sp2host_buffer_queue[%d]", i);
 		bufq_dump_queue_info(prefix,
 				     &css_queues.sp2host_buffer_queue_handles[i]);
 	}
-- 
2.50.0

Re: [PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Andy Shevchenko]

On Fri, Aug 1, 2025 at 6:01 PM Colin Ian King <colin.i.king@gmail.com> wrote:
>
> There are incorrect %u format specifiers being used to for signed integers,
> fix this by using %d instead.

Both of them sound to me like the fix of the symptom and not the
cause. Can we simply make types of the iterators to be unsigned
instead?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Dan Carpenter]

On Fri, Aug 01, 2025 at 11:57:43PM +0200, Andy Shevchenko wrote:
> On Fri, Aug 1, 2025 at 6:01 PM Colin Ian King <colin.i.king@gmail.com> wrote:
> >
> > There are incorrect %u format specifiers being used to for signed integers,
> > fix this by using %d instead.
> 
> Both of them sound to me like the fix of the symptom and not the
> cause. Can we simply make types of the iterators to be unsigned
> instead?

Making iterator unsigned by default only increases the rate of bugs.

(Although, my stats might be biased because I'm only looking at bugs I
can detect through static analysis).

regards,
dan carpenter

Re: [PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Andy Shevchenko]

On Sat, Aug 2, 2025 at 9:32 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Fri, Aug 01, 2025 at 11:57:43PM +0200, Andy Shevchenko wrote:
> > On Fri, Aug 1, 2025 at 6:01 PM Colin Ian King <colin.i.king@gmail.com> wrote:
> > >
> > > There are incorrect %u format specifiers being used to for signed integers,
> > > fix this by using %d instead.
> >
> > Both of them sound to me like the fix of the symptom and not the
> > cause. Can we simply make types of the iterators to be unsigned
> > instead?
>
> Making iterator unsigned by default only increases the rate of bugs.

How? Please, make sure this is relevant to this case.

> (Although, my stats might be biased because I'm only looking at bugs I
> can detect through static analysis).

In general making a variable to be signed that is never negative at
bare minimum is illogical.

P.S.
FWIW, it is a common approach in the media subsystem to require
iterators to be unsigned when they are truly unsigned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Dan Carpenter]

On Sat, Aug 02, 2025 at 10:45:49AM +0200, Andy Shevchenko wrote:
> On Sat, Aug 2, 2025 at 9:32 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > On Fri, Aug 01, 2025 at 11:57:43PM +0200, Andy Shevchenko wrote:
> > > On Fri, Aug 1, 2025 at 6:01 PM Colin Ian King <colin.i.king@gmail.com> wrote:
> > > >
> > > > There are incorrect %u format specifiers being used to for signed integers,
> > > > fix this by using %d instead.
> > >
> > > Both of them sound to me like the fix of the symptom and not the
> > > cause. Can we simply make types of the iterators to be unsigned
> > > instead?
> >
> > Making iterator unsigned by default only increases the rate of bugs.
> 
> How? Please, make sure this is relevant to this case.

You're suggesting that he should change:

-	int i, j;
+	unsigned int i, j;

It's just bad advice.  Making iterators unsigned makes the code less
safe.  It leads underflow bugs when we do subtraction:

	for (i = num - 1; i < limit; i++) {

Now i starts at UINT_MAX.  Which I guess is fine in this example...

But it also leads to endless loops in the error handling:

	while (i-- >= 0) {

Making iterators unsigned is a bad habbit and it's bad advice in terms
of the data that we have with regards to bugs.

regards,
dan carpenter

Re: [PATCH][next] media: atomisp: Fix incorrect snprintf format specifiers for signed integers

[Andy Shevchenko]

On Sat, Aug 2, 2025 at 11:02 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Sat, Aug 02, 2025 at 10:45:49AM +0200, Andy Shevchenko wrote:
> > On Sat, Aug 2, 2025 at 9:32 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > On Fri, Aug 01, 2025 at 11:57:43PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Aug 1, 2025 at 6:01 PM Colin Ian King <colin.i.king@gmail.com> wrote:
> > > > >
> > > > > There are incorrect %u format specifiers being used to for signed integers,
> > > > > fix this by using %d instead.
> > > >
> > > > Both of them sound to me like the fix of the symptom and not the
> > > > cause. Can we simply make types of the iterators to be unsigned
> > > > instead?
> > >
> > > Making iterator unsigned by default only increases the rate of bugs.
> >
> > How? Please, make sure this is relevant to this case.
>
> You're suggesting that he should change:
>
> -       int i, j;
> +       unsigned int i, j;
>
> It's just bad advice.

I disagree with this statement. The code varies and in some cases it
should be negative, but those cases are not these one, or you are
talking about _this_ case? If you are talking in general, again I
fully disagree with your statement. One needs to use a common sense.

>  Making iterators unsigned makes the code less
> safe.  It leads underflow bugs when we do subtraction:
>
>         for (i = num - 1; i < limit; i++) {
>
> Now i starts at UINT_MAX.  Which I guess is fine in this example...

Depends on the num semantics. The main what one needs is a common sense.

> But it also leads to endless loops in the error handling:
>
>         while (i-- >= 0) {

How? Error handling usually takes i > 0. Bad example, try harder.

>
> Making iterators unsigned is a bad habbit

True when use in conjunction with the same statement for signed cases:
"Making iterators signed is a bad habit"

> and it's bad advice in terms
> of the data that we have with regards to bugs.

Disagree. Bugs are common because people do not understand the C
language and its integer rules, wrap-arounds, etc. I believe in many
cases using signed iterators "fix" the bugs due to other variables
also being signed instead of both being unsigned.

-- 
With Best Regards,
Andy Shevchenko