* Re: [PATCH 3/3] media: atomisp: Use max() macros
2024-09-27 9:42 ` [PATCH 3/3] media: atomisp: Use max() macros Ricardo Ribalda
@ 2024-09-27 9:54 ` Andy Shevchenko
2024-09-27 10:00 ` Hans Verkuil
2024-09-29 21:34 ` David Laight
2 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2024-09-27 9:54 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Benoit Parrot, Mauro Carvalho Chehab, Sakari Ailus, Bingbu Cao,
Tianshu Qiu, Greg Kroah-Hartman, Hans de Goede, Hans Verkuil,
linux-media, linux-kernel, linux-staging
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Use max() macros
2024-09-27 9:42 ` [PATCH 3/3] media: atomisp: Use max() macros Ricardo Ribalda
2024-09-27 9:54 ` Andy Shevchenko
@ 2024-09-27 10:00 ` Hans Verkuil
2024-09-29 21:34 ` David Laight
2 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2024-09-27 10:00 UTC (permalink / raw)
To: Ricardo Ribalda, Benoit Parrot, Mauro Carvalho Chehab,
Sakari Ailus, Bingbu Cao, Tianshu Qiu, Greg Kroah-Hartman,
Hans de Goede, Andy Shevchenko
Cc: linux-media, linux-kernel, linux-staging
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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 3/3] media: atomisp: Use max() macros
2024-09-27 9:42 ` [PATCH 3/3] media: atomisp: Use max() macros Ricardo Ribalda
2024-09-27 9:54 ` Andy Shevchenko
2024-09-27 10:00 ` Hans Verkuil
@ 2024-09-29 21:34 ` David Laight
2024-09-30 8:27 ` Hans de Goede
2 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2024-09-29 21:34 UTC (permalink / raw)
To: 'Ricardo Ribalda', Benoit Parrot, Mauro Carvalho Chehab,
Sakari Ailus, Bingbu Cao, Tianshu Qiu, Greg Kroah-Hartman,
Hans de Goede, Andy Shevchenko, Hans Verkuil
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-staging@lists.linux.dev
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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Use max() macros
2024-09-29 21:34 ` David Laight
@ 2024-09-30 8:27 ` Hans de Goede
0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2024-09-30 8:27 UTC (permalink / raw)
To: David Laight, 'Ricardo Ribalda', Benoit Parrot,
Mauro Carvalho Chehab, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Greg Kroah-Hartman, Andy Shevchenko, Hans Verkuil
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-staging@lists.linux.dev
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)
^ permalink raw reply [flat|nested] 10+ messages in thread