diff mbox

[U-Boot,RFC] dfu: allow get_medium_size() to return bigger medium sizes than 2GiB

Message ID 1453991070-29369-1-git-send-email-hs@denx.de
State RFC
Delegated to: Przemyslaw Marczak
Headers show

Commit Message

Heiko Schocher Jan. 28, 2016, 2:24 p.m. UTC
change the get_medium_size() function from
-       long (*get_medium_size)(struct dfu_entity *dfu);
+       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);

so it can return bigger medium sizes than 2GiB, and the return
value is seperate from the size.

Signed-off-by: Heiko Schocher <hs@denx.de>

---
I just have a DDP nand with a size of 4GiB, and the
mtd partition layout is:
device nand2 <omap2-nand.0>, # parts = 9
 #: name                size            offset          mask_flags
 0: spl                 0x00080000      0x00000000      0
 1: spl.backup1         0x00080000      0x00080000      0
 2: spl.backup2         0x00080000      0x00100000      0
 3: spl.backup3         0x00080000      0x00180000      0
 4: u-boot              0x00780000      0x00200000      0
 5: u-boot.env0         0x00200000      0x00980000      0
 6: u-boot.env1         0x00200000      0x00b80000      0
 7: mtdoops             0x00200000      0x00d80000      0
 8: rootfs              0xff080000      0x00f80000      0

so the last partition is to big for returning the size in a long.

 drivers/dfu/dfu.c      | 8 ++++----
 drivers/dfu/dfu_mmc.c  | 8 +++++---
 drivers/dfu/dfu_nand.c | 5 +++--
 drivers/dfu/dfu_ram.c  | 5 +++--
 drivers/dfu/dfu_sf.c   | 5 +++--
 include/dfu.h          | 4 ++--
 6 files changed, 20 insertions(+), 15 deletions(-)

Comments

Łukasz Majewski Jan. 28, 2016, 2:56 p.m. UTC | #1
Hi Heiko,

> change the get_medium_size() function from
> -       long (*get_medium_size)(struct dfu_entity *dfu);
> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
> 
> so it can return bigger medium sizes than 2GiB, and the return
> value is seperate from the size.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> 
> ---
> I just have a DDP nand with a size of 4GiB, and the
> mtd partition layout is:
> device nand2 <omap2-nand.0>, # parts = 9
>  #: name                size            offset          mask_flags
>  0: spl                 0x00080000      0x00000000      0
>  1: spl.backup1         0x00080000      0x00080000      0
>  2: spl.backup2         0x00080000      0x00100000      0
>  3: spl.backup3         0x00080000      0x00180000      0
>  4: u-boot              0x00780000      0x00200000      0
>  5: u-boot.env0         0x00200000      0x00980000      0
>  6: u-boot.env1         0x00200000      0x00b80000      0
>  7: mtdoops             0x00200000      0x00d80000      0
>  8: rootfs              0xff080000      0x00f80000      0
> 
> so the last partition is to big for returning the size in a long.
> 
>  drivers/dfu/dfu.c      | 8 ++++----
>  drivers/dfu/dfu_mmc.c  | 8 +++++---
>  drivers/dfu/dfu_nand.c | 5 +++--
>  drivers/dfu/dfu_ram.c  | 5 +++--
>  drivers/dfu/dfu_sf.c   | 5 +++--
>  include/dfu.h          | 4 ++--
>  6 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 8f5915e..daa2eb9 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) if (dfu->i_buf_start == NULL)
>  			return -ENOMEM;
>  
> -		dfu->r_left = dfu->get_medium_size(dfu);
> -		if (dfu->r_left < 0)
> -			return dfu->r_left;
> +		ret = dfu->get_medium_size(dfu, &dfu->r_left);
> +		if (ret < 0)
> +			return ret;
>  
> -		debug("%s: %s %ld [B]\n", __func__, dfu->name,
> dfu->r_left);
> +		debug("%s: %s %lld [B]\n", __func__, dfu->name,
> dfu->r_left); 
>  		dfu->i_blk_seq_num = 0;
>  		dfu->crc = 0;
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 395d472..5c1c1d1 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>  	return ret;
>  }
>  
> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)

The idea to use the return value to get error code and separate pointer
for passing the size is the way to go in my opinion.

The problem is in details. Please find my comments below.

>  {
>  	int ret;
>  	long len;
>  
>  	switch (dfu->layout) {
>  	case DFU_RAW_ADDR:
> -		return dfu->data.mmc.lba_size *
> dfu->data.mmc.lba_blk_size;
> +		*size = dfu->data.mmc.lba_size *
> dfu->data.mmc.lba_blk_size;
> +		return 0;
>  	case DFU_FS_FAT:
>  	case DFU_FS_EXT4:
>  		dfu_file_buf_filled = -1;
> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity
> *dfu) return ret;
>  		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
>  			return -1;
> -		return len;
> +		*size = len;
> +		return 0;
>  	default:
>  		printf("%s: Layout (%s) not (yet) supported!\n",
> __func__, dfu_get_layout(dfu->layout));
> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> index a975492..4612e09 100644
> --- a/drivers/dfu/dfu_nand.c
> +++ b/drivers/dfu/dfu_nand.c
> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct
> dfu_entity *dfu, return ret;
>  }
>  
> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
>  {
> -	return dfu->data.nand.size;
> +	*size = dfu->data.nand.size;
> +	return 0;
>  }
>  
>  static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
> void *buf, diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
> index e094a94..466759d 100644
> --- a/drivers/dfu/dfu_ram.c
> +++ b/drivers/dfu/dfu_ram.c
> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct dfu_entity
> *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu,
> offset, buf, len); }
>  
> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
>  {
> -	return dfu->data.ram.size;
> +	*size = dfu->data.ram.size;
> +	return 0;
>  }
>  
>  static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
> index 9702eee..35c5fa1 100644
> --- a/drivers/dfu/dfu_sf.c
> +++ b/drivers/dfu/dfu_sf.c
> @@ -12,9 +12,10 @@
>  #include <spi.h>
>  #include <spi_flash.h>
>  
> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)

Originally the dfu_get_medium_size returns int. It is not suitable for
sizes > 2GiB.

I don't like the u64 for *size, since we use that size to perform some
arithmetic operation and comparison (>) in the dfu.c file. I would
prefer to have long long here.

>  {
> -	return dfu->data.sf.size;
> +	*size = dfu->data.sf.size;
> +	return 0;
>  }
>  
>  static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset,
> void *buf, diff --git a/include/dfu.h b/include/dfu.h
> index 6118dc2..323b032 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -110,7 +110,7 @@ struct dfu_entity {
>  		struct sf_internal_data sf;
>  	} data;
>  
> -	long (*get_medium_size)(struct dfu_entity *dfu);
> +	int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>  
>  	int (*read_medium)(struct dfu_entity *dfu,
>  			u64 offset, void *buf, long *len);
> @@ -132,7 +132,7 @@ struct dfu_entity {
>  	u8 *i_buf;
>  	u8 *i_buf_start;
>  	u8 *i_buf_end;
> -	long r_left;
> +	u64 r_left;

This patch changes r_left to be unsigned, but leaves b_left as signed
(long). 

I think that both b_left and r_left should be promoted to long long.

>  	long b_left;
>  
>  	u32 bad_skip;	/* for nand use */
Joe Hershberger Jan. 28, 2016, 3:03 p.m. UTC | #2
On Thu, Jan 28, 2016 at 8:24 AM, Heiko Schocher <hs@denx.de> wrote:
> change the get_medium_size() function from
> -       long (*get_medium_size)(struct dfu_entity *dfu);
> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>
> so it can return bigger medium sizes than 2GiB, and the return
> value is seperate from the size.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
>
> ---
> I just have a DDP nand with a size of 4GiB, and the
> mtd partition layout is:
> device nand2 <omap2-nand.0>, # parts = 9
>  #: name                size            offset          mask_flags
>  0: spl                 0x00080000      0x00000000      0
>  1: spl.backup1         0x00080000      0x00080000      0
>  2: spl.backup2         0x00080000      0x00100000      0
>  3: spl.backup3         0x00080000      0x00180000      0
>  4: u-boot              0x00780000      0x00200000      0
>  5: u-boot.env0         0x00200000      0x00980000      0
>  6: u-boot.env1         0x00200000      0x00b80000      0
>  7: mtdoops             0x00200000      0x00d80000      0
>  8: rootfs              0xff080000      0x00f80000      0
>
> so the last partition is to big for returning the size in a long.

to -> too

>
>  drivers/dfu/dfu.c      | 8 ++++----
>  drivers/dfu/dfu_mmc.c  | 8 +++++---
>  drivers/dfu/dfu_nand.c | 5 +++--
>  drivers/dfu/dfu_ram.c  | 5 +++--
>  drivers/dfu/dfu_sf.c   | 5 +++--
>  include/dfu.h          | 4 ++--
>  6 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 8f5915e..daa2eb9 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
>                 if (dfu->i_buf_start == NULL)
>                         return -ENOMEM;
>
> -               dfu->r_left = dfu->get_medium_size(dfu);
> -               if (dfu->r_left < 0)
> -                       return dfu->r_left;
> +               ret = dfu->get_medium_size(dfu, &dfu->r_left);
> +               if (ret < 0)
> +                       return ret;
>
> -               debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
> +               debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left);
>
>                 dfu->i_blk_seq_num = 0;
>                 dfu->crc = 0;
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 395d472..5c1c1d1 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>         return ret;
>  }
>
> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)

i64 size? Same for all other places.

>  {
>         int ret;
>         long len;
>
>         switch (dfu->layout) {
>         case DFU_RAW_ADDR:
> -               return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
> +               *size = dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;

You need to check that size is not NULL before dereferencing it. Same
for all other places.

> +               return 0;
>         case DFU_FS_FAT:
>         case DFU_FS_EXT4:
>                 dfu_file_buf_filled = -1;
> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>                         return ret;
>                 if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
>                         return -1;
> -               return len;
> +               *size = len;
> +               return 0;
>         default:
>                 printf("%s: Layout (%s) not (yet) supported!\n", __func__,
>                        dfu_get_layout(dfu->layout));
> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> index a975492..4612e09 100644
> --- a/drivers/dfu/dfu_nand.c
> +++ b/drivers/dfu/dfu_nand.c
> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu,
>         return ret;
>  }
>
> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
>  {
> -       return dfu->data.nand.size;
> +       *size = dfu->data.nand.size;
> +       return 0;
>  }
>
>  static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf,
> diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
> index e094a94..466759d 100644
> --- a/drivers/dfu/dfu_ram.c
> +++ b/drivers/dfu/dfu_ram.c
> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset,
>         return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len);
>  }
>
> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
>  {
> -       return dfu->data.ram.size;
> +       *size = dfu->data.ram.size;
> +       return 0;
>  }
>
>  static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
> index 9702eee..35c5fa1 100644
> --- a/drivers/dfu/dfu_sf.c
> +++ b/drivers/dfu/dfu_sf.c
> @@ -12,9 +12,10 @@
>  #include <spi.h>
>  #include <spi_flash.h>
>
> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
>  {
> -       return dfu->data.sf.size;
> +       *size = dfu->data.sf.size;
> +       return 0;
>  }
>
>  static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void *buf,
> diff --git a/include/dfu.h b/include/dfu.h
> index 6118dc2..323b032 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -110,7 +110,7 @@ struct dfu_entity {
>                 struct sf_internal_data sf;
>         } data;
>
> -       long (*get_medium_size)(struct dfu_entity *dfu);
> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>
>         int (*read_medium)(struct dfu_entity *dfu,
>                         u64 offset, void *buf, long *len);
> @@ -132,7 +132,7 @@ struct dfu_entity {
>         u8 *i_buf;
>         u8 *i_buf_start;
>         u8 *i_buf_end;
> -       long r_left;
> +       u64 r_left;
>         long b_left;
>
>         u32 bad_skip;   /* for nand use */
> --
> 2.5.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Heiko Schocher Jan. 29, 2016, 5 a.m. UTC | #3
Hello Lukasz,

Am 28.01.2016 um 15:56 schrieb Lukasz Majewski:
> Hi Heiko,
>
>> change the get_medium_size() function from
>> -       long (*get_medium_size)(struct dfu_entity *dfu);
>> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>
>> so it can return bigger medium sizes than 2GiB, and the return
>> value is seperate from the size.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>
>> ---
>> I just have a DDP nand with a size of 4GiB, and the
>> mtd partition layout is:
>> device nand2 <omap2-nand.0>, # parts = 9
>>   #: name                size            offset          mask_flags
>>   0: spl                 0x00080000      0x00000000      0
>>   1: spl.backup1         0x00080000      0x00080000      0
>>   2: spl.backup2         0x00080000      0x00100000      0
>>   3: spl.backup3         0x00080000      0x00180000      0
>>   4: u-boot              0x00780000      0x00200000      0
>>   5: u-boot.env0         0x00200000      0x00980000      0
>>   6: u-boot.env1         0x00200000      0x00b80000      0
>>   7: mtdoops             0x00200000      0x00d80000      0
>>   8: rootfs              0xff080000      0x00f80000      0
>>
>> so the last partition is to big for returning the size in a long.
>>
>>   drivers/dfu/dfu.c      | 8 ++++----
>>   drivers/dfu/dfu_mmc.c  | 8 +++++---
>>   drivers/dfu/dfu_nand.c | 5 +++--
>>   drivers/dfu/dfu_ram.c  | 5 +++--
>>   drivers/dfu/dfu_sf.c   | 5 +++--
>>   include/dfu.h          | 4 ++--
>>   6 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>> index 8f5915e..daa2eb9 100644
>> --- a/drivers/dfu/dfu.c
>> +++ b/drivers/dfu/dfu.c
>> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
>> int size, int blk_seq_num) if (dfu->i_buf_start == NULL)
>>   			return -ENOMEM;
>>
>> -		dfu->r_left = dfu->get_medium_size(dfu);
>> -		if (dfu->r_left < 0)
>> -			return dfu->r_left;
>> +		ret = dfu->get_medium_size(dfu, &dfu->r_left);
>> +		if (ret < 0)
>> +			return ret;
>>
>> -		debug("%s: %s %ld [B]\n", __func__, dfu->name,
>> dfu->r_left);
>> +		debug("%s: %s %lld [B]\n", __func__, dfu->name,
>> dfu->r_left);
>>   		dfu->i_blk_seq_num = 0;
>>   		dfu->crc = 0;
>> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
>> index 395d472..5c1c1d1 100644
>> --- a/drivers/dfu/dfu_mmc.c
>> +++ b/drivers/dfu/dfu_mmc.c
>> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>>   	return ret;
>>   }
>>
>> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
>
> The idea to use the return value to get error code and separate pointer
> for passing the size is the way to go in my opinion.

Thats, why I posted it as an RFC, I was unsure too...

> The problem is in details. Please find my comments below.
>
>>   {
>>   	int ret;
>>   	long len;
>>
>>   	switch (dfu->layout) {
>>   	case DFU_RAW_ADDR:
>> -		return dfu->data.mmc.lba_size *
>> dfu->data.mmc.lba_blk_size;
>> +		*size = dfu->data.mmc.lba_size *
>> dfu->data.mmc.lba_blk_size;
>> +		return 0;
>>   	case DFU_FS_FAT:
>>   	case DFU_FS_EXT4:
>>   		dfu_file_buf_filled = -1;
>> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity
>> *dfu) return ret;
>>   		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
>>   			return -1;
>> -		return len;
>> +		*size = len;
>> +		return 0;
>>   	default:
>>   		printf("%s: Layout (%s) not (yet) supported!\n",
>> __func__, dfu_get_layout(dfu->layout));
>> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
>> index a975492..4612e09 100644
>> --- a/drivers/dfu/dfu_nand.c
>> +++ b/drivers/dfu/dfu_nand.c
>> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct
>> dfu_entity *dfu, return ret;
>>   }
>>
>> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
>>   {
>> -	return dfu->data.nand.size;
>> +	*size = dfu->data.nand.size;
>> +	return 0;
>>   }
>>
>>   static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
>> void *buf, diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
>> index e094a94..466759d 100644
>> --- a/drivers/dfu/dfu_ram.c
>> +++ b/drivers/dfu/dfu_ram.c
>> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct dfu_entity
>> *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu,
>> offset, buf, len); }
>>
>> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
>>   {
>> -	return dfu->data.ram.size;
>> +	*size = dfu->data.ram.size;
>> +	return 0;
>>   }
>>
>>   static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
>> index 9702eee..35c5fa1 100644
>> --- a/drivers/dfu/dfu_sf.c
>> +++ b/drivers/dfu/dfu_sf.c
>> @@ -12,9 +12,10 @@
>>   #include <spi.h>
>>   #include <spi_flash.h>
>>
>> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
>
> Originally the dfu_get_medium_size returns int. It is not suitable for
> sizes > 2GiB.
>
> I don't like the u64 for *size, since we use that size to perform some
> arithmetic operation and comparison (>) in the dfu.c file. I would
> prefer to have long long here.

Ok.

>>   {
>> -	return dfu->data.sf.size;
>> +	*size = dfu->data.sf.size;
>> +	return 0;
>>   }
>>
>>   static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset,
>> void *buf, diff --git a/include/dfu.h b/include/dfu.h
>> index 6118dc2..323b032 100644
>> --- a/include/dfu.h
>> +++ b/include/dfu.h
>> @@ -110,7 +110,7 @@ struct dfu_entity {
>>   		struct sf_internal_data sf;
>>   	} data;
>>
>> -	long (*get_medium_size)(struct dfu_entity *dfu);
>> +	int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>
>>   	int (*read_medium)(struct dfu_entity *dfu,
>>   			u64 offset, void *buf, long *len);
>> @@ -132,7 +132,7 @@ struct dfu_entity {
>>   	u8 *i_buf;
>>   	u8 *i_buf_start;
>>   	u8 *i_buf_end;
>> -	long r_left;
>> +	u64 r_left;
>
> This patch changes r_left to be unsigned, but leaves b_left as signed
> (long).
>
> I think that both b_left and r_left should be promoted to long long.

Ok, I change it to long long ... but, I have detected another problem:

drivers/dfu/dfu_nand.c:

static int nand_block_op(enum dfu_op op, struct dfu_entity *dfu,
                         u64 offset, void *buf, long *len)
{
         loff_t start, lim;
         size_t count, actual;
         int ret;
         nand_info_t *nand;

         /* if buf == NULL return total size of the area */
         if (buf == NULL) {
                 *len = dfu->data.nand.size;
                 return 0;
         }

returns also a long only. nand_block_op gets called in the end from

static int dfu_write_medium_nand(struct dfu_entity *dfu,
                 u64 offset, void *buf, long *len)

and

static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf,
                 long *len)
{

So I have to change also this functions ... Why returns the read/write
operation  the length of the complete area, if buf is NULL ? Is this
a DFU feature?

bye,
Heiko
>
>>   	long b_left;
>>
>>   	u32 bad_skip;	/* for nand use */
>
>
>
Heiko Schocher Jan. 29, 2016, 6:06 a.m. UTC | #4
Hello Joe,

Am 28.01.2016 um 16:03 schrieb Joe Hershberger:
> On Thu, Jan 28, 2016 at 8:24 AM, Heiko Schocher <hs@denx.de> wrote:
>> change the get_medium_size() function from
>> -       long (*get_medium_size)(struct dfu_entity *dfu);
>> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>
>> so it can return bigger medium sizes than 2GiB, and the return
>> value is seperate from the size.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>
>> ---
>> I just have a DDP nand with a size of 4GiB, and the
>> mtd partition layout is:
>> device nand2 <omap2-nand.0>, # parts = 9
>>   #: name                size            offset          mask_flags
>>   0: spl                 0x00080000      0x00000000      0
>>   1: spl.backup1         0x00080000      0x00080000      0
>>   2: spl.backup2         0x00080000      0x00100000      0
>>   3: spl.backup3         0x00080000      0x00180000      0
>>   4: u-boot              0x00780000      0x00200000      0
>>   5: u-boot.env0         0x00200000      0x00980000      0
>>   6: u-boot.env1         0x00200000      0x00b80000      0
>>   7: mtdoops             0x00200000      0x00d80000      0
>>   8: rootfs              0xff080000      0x00f80000      0
>>
>> so the last partition is to big for returning the size in a long.
>
> to -> too
>
>>
>>   drivers/dfu/dfu.c      | 8 ++++----
>>   drivers/dfu/dfu_mmc.c  | 8 +++++---
>>   drivers/dfu/dfu_nand.c | 5 +++--
>>   drivers/dfu/dfu_ram.c  | 5 +++--
>>   drivers/dfu/dfu_sf.c   | 5 +++--
>>   include/dfu.h          | 4 ++--
>>   6 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>> index 8f5915e..daa2eb9 100644
>> --- a/drivers/dfu/dfu.c
>> +++ b/drivers/dfu/dfu.c
>> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
>>                  if (dfu->i_buf_start == NULL)
>>                          return -ENOMEM;
>>
>> -               dfu->r_left = dfu->get_medium_size(dfu);
>> -               if (dfu->r_left < 0)
>> -                       return dfu->r_left;
>> +               ret = dfu->get_medium_size(dfu, &dfu->r_left);
>> +               if (ret < 0)
>> +                       return ret;
>>
>> -               debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
>> +               debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left);
>>
>>                  dfu->i_blk_seq_num = 0;
>>                  dfu->crc = 0;
>> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
>> index 395d472..5c1c1d1 100644
>> --- a/drivers/dfu/dfu_mmc.c
>> +++ b/drivers/dfu/dfu_mmc.c
>> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>>          return ret;
>>   }
>>
>> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
>
> i64 size? Same for all other places.

Lukasz mentioned "long long" ...

>>   {
>>          int ret;
>>          long len;
>>
>>          switch (dfu->layout) {
>>          case DFU_RAW_ADDR:
>> -               return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
>> +               *size = dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
>
> You need to check that size is not NULL before dereferencing it. Same
> for all other places.

Correct, add it.

>> +               return 0;
>>          case DFU_FS_FAT:
>>          case DFU_FS_EXT4:
>>                  dfu_file_buf_filled = -1;
>> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>>                          return ret;
>>                  if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
>>                          return -1;
>> -               return len;
>> +               *size = len;
>> +               return 0;
>>          default:
>>                  printf("%s: Layout (%s) not (yet) supported!\n", __func__,
>>                         dfu_get_layout(dfu->layout));
>> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
>> index a975492..4612e09 100644
>> --- a/drivers/dfu/dfu_nand.c
>> +++ b/drivers/dfu/dfu_nand.c
>> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu,
>>          return ret;
>>   }
>>
>> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
>>   {
>> -       return dfu->data.nand.size;
>> +       *size = dfu->data.nand.size;
>> +       return 0;
>>   }
>>
>>   static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf,
>> diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
>> index e094a94..466759d 100644
>> --- a/drivers/dfu/dfu_ram.c
>> +++ b/drivers/dfu/dfu_ram.c
>> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset,
>>          return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len);
>>   }
>>
>> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
>>   {
>> -       return dfu->data.ram.size;
>> +       *size = dfu->data.ram.size;
>> +       return 0;
>>   }
>>
>>   static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
>> index 9702eee..35c5fa1 100644
>> --- a/drivers/dfu/dfu_sf.c
>> +++ b/drivers/dfu/dfu_sf.c
>> @@ -12,9 +12,10 @@
>>   #include <spi.h>
>>   #include <spi_flash.h>
>>
>> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
>> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
>>   {
>> -       return dfu->data.sf.size;
>> +       *size = dfu->data.sf.size;
>> +       return 0;
>>   }
>>
>>   static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void *buf,
>> diff --git a/include/dfu.h b/include/dfu.h
>> index 6118dc2..323b032 100644
>> --- a/include/dfu.h
>> +++ b/include/dfu.h
>> @@ -110,7 +110,7 @@ struct dfu_entity {
>>                  struct sf_internal_data sf;
>>          } data;
>>
>> -       long (*get_medium_size)(struct dfu_entity *dfu);
>> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>
>>          int (*read_medium)(struct dfu_entity *dfu,
>>                          u64 offset, void *buf, long *len);
>> @@ -132,7 +132,7 @@ struct dfu_entity {
>>          u8 *i_buf;
>>          u8 *i_buf_start;
>>          u8 *i_buf_end;
>> -       long r_left;
>> +       u64 r_left;
>>          long b_left;
>>
>>          u32 bad_skip;   /* for nand use */
>> --
>> 2.5.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
Łukasz Majewski Jan. 29, 2016, 2:13 p.m. UTC | #5
Hi Heiko,

> Hello Lukasz,
> 
> Am 28.01.2016 um 15:56 schrieb Lukasz Majewski:
> > Hi Heiko,
> >
> >> change the get_medium_size() function from
> >> -       long (*get_medium_size)(struct dfu_entity *dfu);
> >> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
> >>
> >> so it can return bigger medium sizes than 2GiB, and the return
> >> value is seperate from the size.
> >>
> >> Signed-off-by: Heiko Schocher <hs@denx.de>
> >>
> >> ---
> >> I just have a DDP nand with a size of 4GiB, and the
> >> mtd partition layout is:
> >> device nand2 <omap2-nand.0>, # parts = 9
> >>   #: name                size            offset          mask_flags
> >>   0: spl                 0x00080000      0x00000000      0
> >>   1: spl.backup1         0x00080000      0x00080000      0
> >>   2: spl.backup2         0x00080000      0x00100000      0
> >>   3: spl.backup3         0x00080000      0x00180000      0
> >>   4: u-boot              0x00780000      0x00200000      0
> >>   5: u-boot.env0         0x00200000      0x00980000      0
> >>   6: u-boot.env1         0x00200000      0x00b80000      0
> >>   7: mtdoops             0x00200000      0x00d80000      0
> >>   8: rootfs              0xff080000      0x00f80000      0
> >>
> >> so the last partition is to big for returning the size in a long.
> >>
> >>   drivers/dfu/dfu.c      | 8 ++++----
> >>   drivers/dfu/dfu_mmc.c  | 8 +++++---
> >>   drivers/dfu/dfu_nand.c | 5 +++--
> >>   drivers/dfu/dfu_ram.c  | 5 +++--
> >>   drivers/dfu/dfu_sf.c   | 5 +++--
> >>   include/dfu.h          | 4 ++--
> >>   6 files changed, 20 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> >> index 8f5915e..daa2eb9 100644
> >> --- a/drivers/dfu/dfu.c
> >> +++ b/drivers/dfu/dfu.c
> >> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void
> >> *buf, int size, int blk_seq_num) if (dfu->i_buf_start == NULL)
> >>   			return -ENOMEM;
> >>
> >> -		dfu->r_left = dfu->get_medium_size(dfu);
> >> -		if (dfu->r_left < 0)
> >> -			return dfu->r_left;
> >> +		ret = dfu->get_medium_size(dfu, &dfu->r_left);
> >> +		if (ret < 0)
> >> +			return ret;
> >>
> >> -		debug("%s: %s %ld [B]\n", __func__, dfu->name,
> >> dfu->r_left);
> >> +		debug("%s: %s %lld [B]\n", __func__, dfu->name,
> >> dfu->r_left);
> >>   		dfu->i_blk_seq_num = 0;
> >>   		dfu->crc = 0;
> >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> >> index 395d472..5c1c1d1 100644
> >> --- a/drivers/dfu/dfu_mmc.c
> >> +++ b/drivers/dfu/dfu_mmc.c
> >> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity
> >> *dfu) return ret;
> >>   }
> >>
> >> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
> >> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
> >
> > The idea to use the return value to get error code and separate
> > pointer for passing the size is the way to go in my opinion.
> 
> Thats, why I posted it as an RFC, I was unsure too...
> 
> > The problem is in details. Please find my comments below.
> >
> >>   {
> >>   	int ret;
> >>   	long len;
> >>
> >>   	switch (dfu->layout) {
> >>   	case DFU_RAW_ADDR:
> >> -		return dfu->data.mmc.lba_size *
> >> dfu->data.mmc.lba_blk_size;
> >> +		*size = dfu->data.mmc.lba_size *
> >> dfu->data.mmc.lba_blk_size;
> >> +		return 0;
> >>   	case DFU_FS_FAT:
> >>   	case DFU_FS_EXT4:
> >>   		dfu_file_buf_filled = -1;
> >> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity
> >> *dfu) return ret;
> >>   		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
> >>   			return -1;
> >> -		return len;
> >> +		*size = len;
> >> +		return 0;
> >>   	default:
> >>   		printf("%s: Layout (%s) not (yet) supported!\n",
> >> __func__, dfu_get_layout(dfu->layout));
> >> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> >> index a975492..4612e09 100644
> >> --- a/drivers/dfu/dfu_nand.c
> >> +++ b/drivers/dfu/dfu_nand.c
> >> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct
> >> dfu_entity *dfu, return ret;
> >>   }
> >>
> >> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
> >> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
> >>   {
> >> -	return dfu->data.nand.size;
> >> +	*size = dfu->data.nand.size;
> >> +	return 0;
> >>   }
> >>
> >>   static int dfu_read_medium_nand(struct dfu_entity *dfu, u64
> >> offset, void *buf, diff --git a/drivers/dfu/dfu_ram.c
> >> b/drivers/dfu/dfu_ram.c index e094a94..466759d 100644
> >> --- a/drivers/dfu/dfu_ram.c
> >> +++ b/drivers/dfu/dfu_ram.c
> >> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct
> >> dfu_entity *dfu, u64 offset, return
> >> dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len); }
> >>
> >> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
> >> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
> >>   {
> >> -	return dfu->data.ram.size;
> >> +	*size = dfu->data.ram.size;
> >> +	return 0;
> >>   }
> >>
> >>   static int dfu_read_medium_ram(struct dfu_entity *dfu, u64
> >> offset, diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
> >> index 9702eee..35c5fa1 100644
> >> --- a/drivers/dfu/dfu_sf.c
> >> +++ b/drivers/dfu/dfu_sf.c
> >> @@ -12,9 +12,10 @@
> >>   #include <spi.h>
> >>   #include <spi_flash.h>
> >>
> >> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
> >> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
> >
> > Originally the dfu_get_medium_size returns int. It is not suitable
> > for sizes > 2GiB.
> >
> > I don't like the u64 for *size, since we use that size to perform
> > some arithmetic operation and comparison (>) in the dfu.c file. I
> > would prefer to have long long here.
> 
> Ok.
> 
> >>   {
> >> -	return dfu->data.sf.size;
> >> +	*size = dfu->data.sf.size;
> >> +	return 0;
> >>   }
> >>
> >>   static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset,
> >> void *buf, diff --git a/include/dfu.h b/include/dfu.h
> >> index 6118dc2..323b032 100644
> >> --- a/include/dfu.h
> >> +++ b/include/dfu.h
> >> @@ -110,7 +110,7 @@ struct dfu_entity {
> >>   		struct sf_internal_data sf;
> >>   	} data;
> >>
> >> -	long (*get_medium_size)(struct dfu_entity *dfu);
> >> +	int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
> >>
> >>   	int (*read_medium)(struct dfu_entity *dfu,
> >>   			u64 offset, void *buf, long *len);
> >> @@ -132,7 +132,7 @@ struct dfu_entity {
> >>   	u8 *i_buf;
> >>   	u8 *i_buf_start;
> >>   	u8 *i_buf_end;
> >> -	long r_left;
> >> +	u64 r_left;
> >
> > This patch changes r_left to be unsigned, but leaves b_left as
> > signed (long).
> >
> > I think that both b_left and r_left should be promoted to long long.
> 
> Ok, I change it to long long ... but, I have detected another problem:
> 
> drivers/dfu/dfu_nand.c:
> 
> static int nand_block_op(enum dfu_op op, struct dfu_entity *dfu,
>                          u64 offset, void *buf, long *len)
> {
>          loff_t start, lim;
>          size_t count, actual;
>          int ret;
>          nand_info_t *nand;
> 
>          /* if buf == NULL return total size of the area */
>          if (buf == NULL) {
>                  *len = dfu->data.nand.size;
>                  return 0;
>          }
> 
> returns also a long only. nand_block_op gets called in the end from
> 
> static int dfu_write_medium_nand(struct dfu_entity *dfu,
>                  u64 offset, void *buf, long *len)
> 
> and
> 
> static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
> void *buf, long *len)
> {
> 
> So I have to change also this functions ... Why returns the read/write
> operation  the length of the complete area, if buf is NULL ? Is this
> a DFU feature?

Git blame hints that this code is a reminiscent of first code, which
added NAND support to dfu (SHA1: c6631764, year 2013).

It seems like in the past the nand_block_op() has been called with buf
= NULL parameter to read the NAND area size.

However, Stephen Warren has added the get_medium_size_nand() function a
year later (in 2014) to read the medium size.

Hence it seems to me, that the buf == NULL code in nand_block_op could
be safely removed. 

As I've check - in contemporary u-boot - there is no call to this
function with buf NULL parameter.


> 
> bye,
> Heiko
> >
> >>   	long b_left;
> >>
> >>   	u32 bad_skip;	/* for nand use */
> >
> >
> >
>
Joe Hershberger Jan. 29, 2016, 4:37 p.m. UTC | #6
On Fri, Jan 29, 2016 at 12:06 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Joe,
>
>
> Am 28.01.2016 um 16:03 schrieb Joe Hershberger:
>>
>> On Thu, Jan 28, 2016 at 8:24 AM, Heiko Schocher <hs@denx.de> wrote:
>>>
>>> change the get_medium_size() function from
>>> -       long (*get_medium_size)(struct dfu_entity *dfu);
>>> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>>
>>> so it can return bigger medium sizes than 2GiB, and the return
>>> value is seperate from the size.
>>>
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>
>>> ---
>>> I just have a DDP nand with a size of 4GiB, and the
>>> mtd partition layout is:
>>> device nand2 <omap2-nand.0>, # parts = 9
>>>   #: name                size            offset          mask_flags
>>>   0: spl                 0x00080000      0x00000000      0
>>>   1: spl.backup1         0x00080000      0x00080000      0
>>>   2: spl.backup2         0x00080000      0x00100000      0
>>>   3: spl.backup3         0x00080000      0x00180000      0
>>>   4: u-boot              0x00780000      0x00200000      0
>>>   5: u-boot.env0         0x00200000      0x00980000      0
>>>   6: u-boot.env1         0x00200000      0x00b80000      0
>>>   7: mtdoops             0x00200000      0x00d80000      0
>>>   8: rootfs              0xff080000      0x00f80000      0
>>>
>>> so the last partition is to big for returning the size in a long.
>>
>>
>> to -> too
>>
>>>
>>>   drivers/dfu/dfu.c      | 8 ++++----
>>>   drivers/dfu/dfu_mmc.c  | 8 +++++---
>>>   drivers/dfu/dfu_nand.c | 5 +++--
>>>   drivers/dfu/dfu_ram.c  | 5 +++--
>>>   drivers/dfu/dfu_sf.c   | 5 +++--
>>>   include/dfu.h          | 4 ++--
>>>   6 files changed, 20 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>>> index 8f5915e..daa2eb9 100644
>>> --- a/drivers/dfu/dfu.c
>>> +++ b/drivers/dfu/dfu.c
>>> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int
>>> size, int blk_seq_num)
>>>                  if (dfu->i_buf_start == NULL)
>>>                          return -ENOMEM;
>>>
>>> -               dfu->r_left = dfu->get_medium_size(dfu);
>>> -               if (dfu->r_left < 0)
>>> -                       return dfu->r_left;
>>> +               ret = dfu->get_medium_size(dfu, &dfu->r_left);
>>> +               if (ret < 0)
>>> +                       return ret;
>>>
>>> -               debug("%s: %s %ld [B]\n", __func__, dfu->name,
>>> dfu->r_left);
>>> +               debug("%s: %s %lld [B]\n", __func__, dfu->name,
>>> dfu->r_left);
>>>
>>>                  dfu->i_blk_seq_num = 0;
>>>                  dfu->crc = 0;
>>> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
>>> index 395d472..5c1c1d1 100644
>>> --- a/drivers/dfu/dfu_mmc.c
>>> +++ b/drivers/dfu/dfu_mmc.c
>>> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>>>          return ret;
>>>   }
>>>
>>> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>>> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
>>
>>
>> i64 size? Same for all other places.
>
>
> Lukasz mentioned "long long" ...

That's OK too.

>>>   {
>>>          int ret;
>>>          long len;
>>>
>>>          switch (dfu->layout) {
>>>          case DFU_RAW_ADDR:
>>> -               return dfu->data.mmc.lba_size *
>>> dfu->data.mmc.lba_blk_size;
>>> +               *size = dfu->data.mmc.lba_size *
>>> dfu->data.mmc.lba_blk_size;
>>
>>
>> You need to check that size is not NULL before dereferencing it. Same
>> for all other places.
>
>
> Correct, add it.
>
>
>>> +               return 0;
>>>          case DFU_FS_FAT:
>>>          case DFU_FS_EXT4:
>>>                  dfu_file_buf_filled = -1;
>>> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
>>>                          return ret;
>>>                  if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
>>>                          return -1;
>>> -               return len;
>>> +               *size = len;
>>> +               return 0;
>>>          default:
>>>                  printf("%s: Layout (%s) not (yet) supported!\n",
>>> __func__,
>>>                         dfu_get_layout(dfu->layout));
>>> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
>>> index a975492..4612e09 100644
>>> --- a/drivers/dfu/dfu_nand.c
>>> +++ b/drivers/dfu/dfu_nand.c
>>> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct dfu_entity
>>> *dfu,
>>>          return ret;
>>>   }
>>>
>>> -long dfu_get_medium_size_nand(struct dfu_entity *dfu)
>>> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
>>>   {
>>> -       return dfu->data.nand.size;
>>> +       *size = dfu->data.nand.size;
>>> +       return 0;
>>>   }
>>>
>>>   static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
>>> void *buf,
>>> diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
>>> index e094a94..466759d 100644
>>> --- a/drivers/dfu/dfu_ram.c
>>> +++ b/drivers/dfu/dfu_ram.c
>>> @@ -41,9 +41,10 @@ static int dfu_write_medium_ram(struct dfu_entity
>>> *dfu, u64 offset,
>>>          return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf,
>>> len);
>>>   }
>>>
>>> -long dfu_get_medium_size_ram(struct dfu_entity *dfu)
>>> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
>>>   {
>>> -       return dfu->data.ram.size;
>>> +       *size = dfu->data.ram.size;
>>> +       return 0;
>>>   }
>>>
>>>   static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
>>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
>>> index 9702eee..35c5fa1 100644
>>> --- a/drivers/dfu/dfu_sf.c
>>> +++ b/drivers/dfu/dfu_sf.c
>>> @@ -12,9 +12,10 @@
>>>   #include <spi.h>
>>>   #include <spi_flash.h>
>>>
>>> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
>>> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
>>>   {
>>> -       return dfu->data.sf.size;
>>> +       *size = dfu->data.sf.size;
>>> +       return 0;
>>>   }
>>>
>>>   static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void
>>> *buf,
>>> diff --git a/include/dfu.h b/include/dfu.h
>>> index 6118dc2..323b032 100644
>>> --- a/include/dfu.h
>>> +++ b/include/dfu.h
>>> @@ -110,7 +110,7 @@ struct dfu_entity {
>>>                  struct sf_internal_data sf;
>>>          } data;
>>>
>>> -       long (*get_medium_size)(struct dfu_entity *dfu);
>>> +       int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
>>>
>>>          int (*read_medium)(struct dfu_entity *dfu,
>>>                          u64 offset, void *buf, long *len);
>>> @@ -132,7 +132,7 @@ struct dfu_entity {
>>>          u8 *i_buf;
>>>          u8 *i_buf_start;
>>>          u8 *i_buf_end;
>>> -       long r_left;
>>> +       u64 r_left;
>>>          long b_left;
>>>
>>>          u32 bad_skip;   /* for nand use */
>>> --
>>> 2.5.0
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
diff mbox

Patch

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 8f5915e..daa2eb9 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -335,11 +335,11 @@  int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		if (dfu->i_buf_start == NULL)
 			return -ENOMEM;
 
-		dfu->r_left = dfu->get_medium_size(dfu);
-		if (dfu->r_left < 0)
-			return dfu->r_left;
+		ret = dfu->get_medium_size(dfu, &dfu->r_left);
+		if (ret < 0)
+			return ret;
 
-		debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
+		debug("%s: %s %lld [B]\n", __func__, dfu->name, dfu->r_left);
 
 		dfu->i_blk_seq_num = 0;
 		dfu->crc = 0;
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 395d472..5c1c1d1 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -205,14 +205,15 @@  int dfu_flush_medium_mmc(struct dfu_entity *dfu)
 	return ret;
 }
 
-long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
+int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size)
 {
 	int ret;
 	long len;
 
 	switch (dfu->layout) {
 	case DFU_RAW_ADDR:
-		return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
+		*size = dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
+		return 0;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
 		dfu_file_buf_filled = -1;
@@ -221,7 +222,8 @@  long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
 			return ret;
 		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
 			return -1;
-		return len;
+		*size = len;
+		return 0;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
 		       dfu_get_layout(dfu->layout));
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index a975492..4612e09 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -114,9 +114,10 @@  static int dfu_write_medium_nand(struct dfu_entity *dfu,
 	return ret;
 }
 
-long dfu_get_medium_size_nand(struct dfu_entity *dfu)
+int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size)
 {
-	return dfu->data.nand.size;
+	*size = dfu->data.nand.size;
+	return 0;
 }
 
 static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf,
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
index e094a94..466759d 100644
--- a/drivers/dfu/dfu_ram.c
+++ b/drivers/dfu/dfu_ram.c
@@ -41,9 +41,10 @@  static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset,
 	return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len);
 }
 
-long dfu_get_medium_size_ram(struct dfu_entity *dfu)
+int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size)
 {
-	return dfu->data.ram.size;
+	*size = dfu->data.ram.size;
+	return 0;
 }
 
 static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index 9702eee..35c5fa1 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -12,9 +12,10 @@ 
 #include <spi.h>
 #include <spi_flash.h>
 
-static long dfu_get_medium_size_sf(struct dfu_entity *dfu)
+int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size)
 {
-	return dfu->data.sf.size;
+	*size = dfu->data.sf.size;
+	return 0;
 }
 
 static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, void *buf,
diff --git a/include/dfu.h b/include/dfu.h
index 6118dc2..323b032 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -110,7 +110,7 @@  struct dfu_entity {
 		struct sf_internal_data sf;
 	} data;
 
-	long (*get_medium_size)(struct dfu_entity *dfu);
+	int (*get_medium_size)(struct dfu_entity *dfu, u64 *size);
 
 	int (*read_medium)(struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len);
@@ -132,7 +132,7 @@  struct dfu_entity {
 	u8 *i_buf;
 	u8 *i_buf_start;
 	u8 *i_buf_end;
-	long r_left;
+	u64 r_left;
 	long b_left;
 
 	u32 bad_skip;	/* for nand use */