1. Patchew
  2. linux
  3. [PATCH 0/3] media: static-analyzers: Fix 6.12-rc1 cocci warnings
  4. View patch

[PATCH 3/3] media: atomisp: Use max() macros

Ricardo Ribalda posted 3 patches 2 months ago
There is a newer version of this series
[PATCH 3/3] media: atomisp: Use max() macros
Posted by Ricardo Ribalda 2 months ago 
The max() macro produce nicer code and also fixes the following cocci
errors:

drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max()
drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max()

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
index 8ba65161f7a9..9642506d2388 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
@@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b)
 	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
 
 	v >>= sSHIFT;
-	v >>= fit_shift > 0 ? fit_shift : 0;
+	v >>= max(fit_shift, 0);
 
 	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
 }
@@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
 	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
 
 	v >>= uSHIFT;
-	v >>= fit_shift > 0 ? fit_shift : 0;
+	v >>= max(fit_shift, 0);
 
 	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
 }

-- 
2.46.1.824.gd892dcdcdd-goog
RE: [PATCH 3/3] media: atomisp: Use max() macros
Posted by David Laight 1 month, 4 weeks ago 
From: Ricardo Ribalda
> Sent: 27 September 2024 10:42
> 
> The max() macro produce nicer code and also fixes the following cocci
> errors:
> 
> drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max()
> drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max()
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h
> b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> index 8ba65161f7a9..9642506d2388 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b)
>  	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
> 
>  	v >>= sSHIFT;

IIRC right shifts of signed values are undefined.
(C does not require a cpu to have a right shift that replicates the
sign bit.)

> -	v >>= fit_shift > 0 ? fit_shift : 0;
> +	v >>= max(fit_shift, 0);

If the shift isn't done the return value is garbage.
So the code better not let it happen.
In which case you might as well let the cpu generate a (different)
random value - so delete the conditional.

> 
>  	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);

all three values seem to be 'int' - so no need for the _t variant
and all the associated casts.

>  }
> @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
>  	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
> 
>  	v >>= uSHIFT;
> -	v >>= fit_shift > 0 ? fit_shift : 0;
> +	v >>= max(fit_shift, 0);
> 
>  	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);

as above, but it is just min(v, iISP_VAL_MAX)

	David

>  }
> 
> --
> 2.46.1.824.gd892dcdcdd-goog
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Re: [PATCH 3/3] media: atomisp: Use max() macros
Posted by Hans de Goede 1 month, 4 weeks ago 
Hi,

On 29-Sep-24 11:34 PM, David Laight wrote:
> From: Ricardo Ribalda
>> Sent: 27 September 2024 10:42
>>
>> The max() macro produce nicer code and also fixes the following cocci
>> errors:
>>
>> drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max()
>> drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max()
>>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h
>> b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>> index 8ba65161f7a9..9642506d2388 100644
>> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
>> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>> @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b)
>>  	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
>>
>>  	v >>= sSHIFT;
> 
> IIRC right shifts of signed values are undefined.
> (C does not require a cpu to have a right shift that replicates the
> sign bit.)
> 
>> -	v >>= fit_shift > 0 ? fit_shift : 0;
>> +	v >>= max(fit_shift, 0);
> 
> If the shift isn't done the return value is garbage.
> So the code better not let it happen.
> In which case you might as well let the cpu generate a (different)
> random value - so delete the conditional.

Given the history of this code I would no be surprised if some
weird corner case actually relies on the check, so NACK for
dropping the conditional.

> 
>>
>>  	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
> 
> all three values seem to be 'int' - so no need for the _t variant
> and all the associated casts.

sDIGIT_FITTING() originally was a macro with a bunch of max() + min()
calls nested leading to it expanding to a lot of code after running it
through the pre-processor. When converting this to a static online to
choice was made to with clamp_t() to avoid the overhead of the extra
type checks in regular clamp().

Regards,

Hans




> 
>>  }
>> @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
>>  	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
>>
>>  	v >>= uSHIFT;
>> -	v >>= fit_shift > 0 ? fit_shift : 0;
>> +	v >>= max(fit_shift, 0);
>>
>>  	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
> 
> as above, but it is just min(v, iISP_VAL_MAX)
> 
> 	David
> 
>>  }
>>
>> --
>> 2.46.1.824.gd892dcdcdd-goog
>>
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
Re: [PATCH 3/3] media: atomisp: Use max() macros
Posted by Hans Verkuil 2 months ago 
On 27/09/2024 11:42, Ricardo Ribalda wrote:
> The max() macro produce nicer code and also fixes the following cocci
> errors:
> 
> drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max()
> drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max()
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> index 8ba65161f7a9..9642506d2388 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b)
>  	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
>  
>  	v >>= sSHIFT;
> -	v >>= fit_shift > 0 ? fit_shift : 0;
> +	v >>= max(fit_shift, 0);

Does the warning go away if you change this to:

	if (fit_shift > 0)
		v >>= fit_shift;

Using 'max' for a shift is a bit weird in my opinion.
Also this change was done to reduce the min/max calls, so introducing
a new max call feels odd (although it should be fine).

Note that I think those cocci warnings should perhaps be ignored or
dropped. In part because of the huge macro expansion of min and max, but
also I often find the code that is not using min or max at least as readable,
if not more.

Regards,

	Hans

>  
>  	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
>  }
> @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
>  	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
>  
>  	v >>= uSHIFT;
> -	v >>= fit_shift > 0 ? fit_shift : 0;
> +	v >>= max(fit_shift, 0);
>  
>  	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
>  }
>
Re: [PATCH 3/3] media: atomisp: Use max() macros
Posted by Andy Shevchenko 2 months ago 
On Fri, Sep 27, 2024 at 09:42:15AM +0000, Ricardo Ribalda wrote:
> The max() macro produce nicer code and also fixes the following cocci
> errors:
> 
> drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max()
> drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max()

In some (rare) cases it's a bad advice.
NAK.

...

> -	v >>= fit_shift > 0 ? fit_shift : 0;
> +	v >>= max(fit_shift, 0);

max() with 0 is a bit unusual, esp. taking into account that the operator here
is a right shift. So, what we check here is the signedness of the value to
avoid not only potentially wrong calculations, but also UB. The original code
is clearer for all this.

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.