[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Timur Tabi]

In print_size(), the math that calculates the fractional remainder of a number
used the same integer size as a physical address.  However, the "10 *" factor
of the algorithm means that a large number (e.g. 1.5GB) can overflow the
integer if we're running on a 32-bit system.  Therefore, we need to
disassociate this function from the size of a physical address.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 include/common.h              |    2 +-
 lib_generic/display_options.c |    5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/common.h b/include/common.h
index a133e34..4e77727 100644
--- a/include/common.h
+++ b/include/common.h
@@ -218,7 +218,7 @@ void	hang		(void) __attribute__ ((noreturn));
 /* */
 phys_size_t initdram (int);
 int	display_options (void);
-void	print_size (phys_size_t, const char *);
+void	print_size(unsigned long long, const char *);
 int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
 
 /* common/main.c */
diff --git a/lib_generic/display_options.c b/lib_generic/display_options.c
index 2dc2567..cafb603 100644
--- a/lib_generic/display_options.c
+++ b/lib_generic/display_options.c
@@ -43,10 +43,9 @@ int display_options (void)
  * xxx GB, or xxx.y GB as needed; allow for optional trailing string
  * (like "\n")
  */
-void print_size (phys_size_t size, const char *s)
+void print_size(unsigned long long size, const char *s)
 {
-	ulong m = 0, n;
-	phys_size_t d = 1 << 30;		/* 1 GB */
+	unsigned long m = 0, n, d = 1 << 30 /* 1 GB */;
 	char  c = 'G';
 
 	if (size < d) {			/* try MB */
-- 
1.6.5

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Scott Wood]

Timur Tabi wrote:
> In print_size(), the math that calculates the fractional remainder of a number
> used the same integer size as a physical address.  However, the "10 *" factor
> of the algorithm means that a large number (e.g. 1.5GB) can overflow the
> integer if we're running on a 32-bit system.  Therefore, we need to
> disassociate this function from the size of a physical address.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  include/common.h              |    2 +-
>  lib_generic/display_options.c |    5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/include/common.h b/include/common.h
> index a133e34..4e77727 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -218,7 +218,7 @@ void	hang		(void) __attribute__ ((noreturn));
>  /* */
>  phys_size_t initdram (int);
>  int	display_options (void);
> -void	print_size (phys_size_t, const char *);
> +void	print_size(unsigned long long, const char *);
>  int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
>  
>  /* common/main.c */
> diff --git a/lib_generic/display_options.c b/lib_generic/display_options.c
> index 2dc2567..cafb603 100644
> --- a/lib_generic/display_options.c
> +++ b/lib_generic/display_options.c
> @@ -43,10 +43,9 @@ int display_options (void)
>   * xxx GB, or xxx.y GB as needed; allow for optional trailing string
>   * (like "\n")
>   */
> -void print_size (phys_size_t size, const char *s)
> +void print_size(unsigned long long size, const char *s)
>  {
> -	ulong m = 0, n;
> -	phys_size_t d = 1 << 30;		/* 1 GB */
> +	unsigned long m = 0, n, d = 1 << 30 /* 1 GB */;
>  	char  c = 'G';

In changing "d" from phys_size_t to unsigned long, I think you're 
introducing overflow in "n * d" (consider 5.5G rather than 1.5G).

Wouldn't a more straightforward fix, that doesn't affect the function 
signature, be to just change "10 *" to "10ULL *"?

-Scott

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Timur Tabi]

Scott Wood wrote:
> In changing "d" from phys_size_t to unsigned long, I think you're 
> introducing overflow in "n * d" (consider 5.5G rather than 1.5G).
> 
> Wouldn't a more straightforward fix, that doesn't affect the function 
> signature, be to just change "10 *" to "10ULL *"?

I don't see how that suggestion would make the code any different.  Here's the expression:

	(10 * (size - (n * d)) + (d / 2) ) / d;

I made 'size' into a u64, and I assume that the compiler will evaluate every other subexpression as a u64.  That may be wrong.

However, changing 10 to 10ULL makes the same assumption.

I'll test 5.5GB and see what it does.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
>> In changing "d" from phys_size_t to unsigned long, I think you're 
>> introducing overflow in "n * d" (consider 5.5G rather than 1.5G).
>>
>> Wouldn't a more straightforward fix, that doesn't affect the function 
>> signature, be to just change "10 *" to "10ULL *"?
> 
> I don't see how that suggestion would make the code any different.

It would make the 10 * (...) product 64-bit regardless of phys_size_t, 
without changing the function signature (overflow is an internal 
implementation detail).

>  Here's the expression:
> 
> 	(10 * (size - (n * d)) + (d / 2) ) / d;
> 
> I made 'size' into a u64, and I assume that the compiler will evaluate every other subexpression as a u64.  That may be wrong.

The "n * d" subexpression does not involve size, and thus will not be 
64-bit.

> However, changing 10 to 10ULL makes the same assumption.

The difference is that "d" remains phys_size_t, rather than being 
converted to unsigned long.

-Scott

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Timur Tabi]

Scott Wood wrote:

> It would make the 10 * (...) product 64-bit regardless of phys_size_t, 
> without changing the function signature (overflow is an internal 
> implementation detail).

You are right that (n * d) is evaluated as a 32-bit integer:

	print_size(5905580032)= 6.35 GB

However, changing "10" to "10ULL" does not fix this.  I think this is because we are both expecting integer sizes to commute across arithmetic operations.  That is, I assumed that:

	u64 - (u32 * u32) 

would be treated as 

	u64 - ((u64)u32 * u32) 

And you assumed that 

	u64 * (u32 - u32)

would be treated as

	u64 * (u32 - (u64) u32)

Both appear to be wrong.  If we want to treat (n * d) as a u64, we need to be explicit.  Casting "(n * d)" to a u64 just doesn't work.  We have to cast (or make) either 'n' or 'd' to a u64.

I think the simplest approach is make 'd' into a u64.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] fix print_size printing fractional gigabyte numbers on 32-bit platforms

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> It would make the 10 * (...) product 64-bit regardless of phys_size_t, 
>> without changing the function signature (overflow is an internal 
>> implementation detail).
> 
> You are right that (n * d) is evaluated as a 32-bit integer:
> 
> 	print_size(5905580032)= 6.35 GB
> 
> However, changing "10" to "10ULL" does not fix this. 

It's not supposed to.

There are two different overflows, 10*expr and n*d.

Today, 10*expr overflows and n*d doesn't.  With your patch, n*d 
overflows and 10*expr doesn't.

I was suggesting a simple way to fix 10*expr without other changes.

> I think this is because we are both expecting integer sizes to commute across arithmetic operations.  That is, I assumed that:
> 
> 	u64 - (u32 * u32) 
> 
> would be treated as 
> 
> 	u64 - ((u64)u32 * u32) 
> 
> And you assumed that 
> 
> 	u64 * (u32 - u32)
> 
> would be treated as
> 
> 	u64 * (u32 - (u64) u32)

I assumed no such thing.

What I assumed was that the n*d overflow only matters if phys_size_t is 
64-bit, because it should always be less than size.

-Scott