[PATCH 0/3] media: static-analyzers: Fix 6.12-rc1 cocci warnings

[PATCH 0/3] media: static-analyzers: Fix 6.12-rc1 cocci warnings

[Ricardo Ribalda]

This patchset introduces fixes for all the new warnings introduced in
Linux 6.12-rc1

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
      media: ti: cal: Use str_up_down()
      staging: media: ipu3:Use str_down_up()
      media: atomisp: Use max() macros

 drivers/media/platform/ti/cal/cal-camerarx.c    | 2 +-
 drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++--
 drivers/staging/media/ipu3/ipu3-css.c           | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
---
base-commit: 075dbe9f6e3c21596c5245826a4ee1f1c1676eb8
change-id: 20240927-cocci-6-12-4c571bc8e9dd

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/3] media: ti: cal: Use str_up_down()

[Ricardo Ribalda]

The str_up_down() helper simplifies the code and fixes the following cocci
warning:

drivers/media/platform/ti/cal/cal-camerarx.c:194:3-9: opportunity for str_up_down(enable)

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/ti/cal/cal-camerarx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index 42dfe08b765f..ba8c4743f539 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -191,7 +191,7 @@ static void cal_camerarx_power(struct cal_camerarx *phy, bool enable)
 
 	if (i == 10)
 		phy_err(phy, "Failed to power %s complexio\n",
-			enable ? "up" : "down");
+			str_up_down(enable);
 }
 
 static void cal_camerarx_wait_reset(struct cal_camerarx *phy)

-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH 1/3] media: ti: cal: Use str_up_down()

[Andy Shevchenko]

On Fri, Sep 27, 2024 at 09:42:13AM +0000, Ricardo Ribalda wrote:
> The str_up_down() helper simplifies the code and fixes the following cocci
> warning:
> 
> drivers/media/platform/ti/cal/cal-camerarx.c:194:3-9: opportunity for str_up_down(enable)

...

>  	if (i == 10)
>  		phy_err(phy, "Failed to power %s complexio\n",
> -			enable ? "up" : "down");
> +			str_up_down(enable);

Now can fit one line

		phy_err(phy, "Failed to power %s complexio\n", str_up_down(enable));

But have you compiled it?

>  }

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] staging: media: ipu3:Use str_down_up()

[Ricardo Ribalda]

The str_down_up() helper simplifies the code and fixes the following cocci
warning:

drivers/staging/media/ipu3/ipu3-css.c:229:18-47: opportunity for str_down_up(state & IMGU_STATE_POWER_DOWN)

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/staging/media/ipu3/ipu3-css.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c
index 1b0a59b78949..bb22375481a0 100644
--- a/drivers/staging/media/ipu3/ipu3-css.c
+++ b/drivers/staging/media/ipu3/ipu3-css.c
@@ -226,7 +226,7 @@ int imgu_css_set_powerup(struct device *dev, void __iomem *base,
 	state = readl(base + IMGU_REG_STATE);
 
 	dev_dbg(dev, "CSS pm_ctrl 0x%x state 0x%x (power %s)\n",
-		pm_ctrl, state, state & IMGU_STATE_POWER_DOWN ? "down" : "up");
+		pm_ctrl, state, str_down_up(state & IMGU_STATE_POWER_DOWN));
 
 	/* Power up CSS using wrapper */
 	if (state & IMGU_STATE_POWER_DOWN) {

-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH 2/3] staging: media: ipu3:Use str_down_up()

[Andy Shevchenko]

On Fri, Sep 27, 2024 at 09:42:14AM +0000, Ricardo Ribalda wrote:
> The str_down_up() helper simplifies the code and fixes the following cocci
> warning:
> 
> drivers/staging/media/ipu3/ipu3-css.c:229:18-47: opportunity for str_down_up(state & IMGU_STATE_POWER_DOWN)

With a space after the colon in the subject it seems fine to me,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

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

[Ricardo Ribalda]

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

[Andy Shevchenko]

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

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

[Hans Verkuil]

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

[David Laight]

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

[Hans de Goede]

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)