[PATCH 0/2] drm/amd/display: fix MIN_I64 overflow on bw_fixed.c

[PATCH 0/2] drm/amd/display: fix MIN_I64 overflow on bw_fixed.c

[Tales Aparecida]

Hi,

This series fixes an error accused by GCC and some checkpatch warnings.

The first patch of this series fixes an error reported and fixed by
David Gow (thanks!) at [1], where the macro MIN_I64 was used for the
first time and warned about an integer overflow.
The fix uses a similar syntax from include/vdso/limits.h

To cause the error you can either apply that series or, for example,
define a global variable:

    int64_t min_i64 = MIN_I64;

The second patch fix lesser warnings in the same file.

Thanks for the review!

[1] https://lore.kernel.org/amd-gfx/CABVgOSmkeybnR2sGEEgn1Cb0cR2eKxW=vhXkHjC5xCuhaxsqVg@mail.gmail.com/

David Gow (1):
  drm/amd/display: fix overflow on MIN_I64 definition

Tales Aparecida (1):
  drm/amd/display: fix minor codestyle problems

 .../gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c    | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)


base-commit: 1a60172c97fd2b51151cf17483f372bb61246c0b
-- 
2.37.0

[PATCH 1/2] drm/amd/display: fix overflow on MIN_I64 definition

[Tales Aparecida]

From: David Gow <davidgow@google.com>

The definition of MIN_I64 in bw_fixed.c can cause gcc to whinge about
integer overflow, because it is treated as a positive value, which is
then negated. The temporary positive value is not necessarily
representable.

This causes the following warning:
../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/calcs/bw_fixed.c:30:19:
warning: integer overflow in expression ‘-9223372036854775808’ of type
‘long long int’ results in ‘-9223372036854775808’ [-Woverflow]
  30 |         (int64_t)(-(1LL << 63))
     |                   ^

Writing out (-MAX_I64 - 1) works instead.

Signed-off-by: David Gow <davidgow@google.com>
Signed-off-by: Tales Aparecida <tales.aparecida@gmail.com>
---
 drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
index 6ca288fb5fb9..2d46bc527b21 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
@@ -26,12 +26,12 @@
 #include "bw_fixed.h"
 
 
-#define MIN_I64 \
-	(int64_t)(-(1LL << 63))
-
 #define MAX_I64 \
 	(int64_t)((1ULL << 63) - 1)
 
+#define MIN_I64 \
+	(-MAX_I64 - 1)
+
 #define FRACTIONAL_PART_MASK \
 	((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)
 
-- 
2.37.0

[PATCH 2/2] drm/amd/display: fix minor codestyle problems

[Tales Aparecida]

Fixes five checkpatch warnings:

CHECK: Please don't use multiple blank lines
+
+

ERROR: Macros with complex values should be enclosed in parentheses
+#define MAX_I64 \
+       (int64_t)((1ULL << 63) - 1)

WARNING: Missing a blank line after declarations
+       struct bw_fixed res;
+       ASSERT(value < BW_FIXED_MAX_I32 && value > BW_FIXED_MIN_I32);

ERROR: that open brace { should be on the previous line
+               do
+               {

ERROR: that open brace { should be on the previous line
+                       if (remainder >= arg2_value)
+                       {

Signed-off-by: Tales Aparecida <tales.aparecida@gmail.com>
---
 drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
index 2d46bc527b21..3aa8dd0acd5e 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
@@ -25,9 +25,8 @@
 #include "dm_services.h"
 #include "bw_fixed.h"
 
-
 #define MAX_I64 \
-	(int64_t)((1ULL << 63) - 1)
+	((int64_t)((1ULL << 63) - 1))
 
 #define MIN_I64 \
 	(-MAX_I64 - 1)
@@ -49,6 +48,7 @@ static uint64_t abs_i64(int64_t arg)
 struct bw_fixed bw_int_to_fixed_nonconst(int64_t value)
 {
 	struct bw_fixed res;
+
 	ASSERT(value < BW_FIXED_MAX_I32 && value > BW_FIXED_MIN_I32);
 	res.value = value << BW_FIXED_BITS_PER_FRACTIONAL_PART;
 	return res;
@@ -78,14 +78,12 @@ struct bw_fixed bw_frc_to_fixed(int64_t numerator, int64_t denominator)
 	{
 		uint32_t i = BW_FIXED_BITS_PER_FRACTIONAL_PART;
 
-		do
-		{
+		do {
 			remainder <<= 1;
 
 			res_value <<= 1;
 
-			if (remainder >= arg2_value)
-			{
+			if (remainder >= arg2_value) {
 				res_value |= 1;
 				remainder -= arg2_value;
 			}
-- 
2.37.0

Re: [PATCH 2/2] drm/amd/display: fix minor codestyle problems

[Alex Deucher]

Applied the series.  Thanks!

Alex

On Thu, Aug 11, 2022 at 4:44 PM Tales Aparecida
<tales.aparecida@gmail.com> wrote:
>
> Fixes five checkpatch warnings:
>
> CHECK: Please don't use multiple blank lines
> +
> +
>
> ERROR: Macros with complex values should be enclosed in parentheses
> +#define MAX_I64 \
> +       (int64_t)((1ULL << 63) - 1)
>
> WARNING: Missing a blank line after declarations
> +       struct bw_fixed res;
> +       ASSERT(value < BW_FIXED_MAX_I32 && value > BW_FIXED_MIN_I32);
>
> ERROR: that open brace { should be on the previous line
> +               do
> +               {
>
> ERROR: that open brace { should be on the previous line
> +                       if (remainder >= arg2_value)
> +                       {
>
> Signed-off-by: Tales Aparecida <tales.aparecida@gmail.com>
> ---
>  drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
> index 2d46bc527b21..3aa8dd0acd5e 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
> +++ b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
> @@ -25,9 +25,8 @@
>  #include "dm_services.h"
>  #include "bw_fixed.h"
>
> -
>  #define MAX_I64 \
> -       (int64_t)((1ULL << 63) - 1)
> +       ((int64_t)((1ULL << 63) - 1))
>
>  #define MIN_I64 \
>         (-MAX_I64 - 1)
> @@ -49,6 +48,7 @@ static uint64_t abs_i64(int64_t arg)
>  struct bw_fixed bw_int_to_fixed_nonconst(int64_t value)
>  {
>         struct bw_fixed res;
> +
>         ASSERT(value < BW_FIXED_MAX_I32 && value > BW_FIXED_MIN_I32);
>         res.value = value << BW_FIXED_BITS_PER_FRACTIONAL_PART;
>         return res;
> @@ -78,14 +78,12 @@ struct bw_fixed bw_frc_to_fixed(int64_t numerator, int64_t denominator)
>         {
>                 uint32_t i = BW_FIXED_BITS_PER_FRACTIONAL_PART;
>
> -               do
> -               {
> +               do {
>                         remainder <<= 1;
>
>                         res_value <<= 1;
>
> -                       if (remainder >= arg2_value)
> -                       {
> +                       if (remainder >= arg2_value) {
>                                 res_value |= 1;
>                                 remainder -= arg2_value;
>                         }
> --
> 2.37.0
>