Re: [U-Boot] [PATCH v3 10/17] fs: Convert fs_read/write to take buffer instead of address

[Alexander Graf]

On 15.06.18 16:24, Simon Glass wrote:
> Hi Alex,
> 
> On 15 June 2018 at 06:42, Alexander Graf  wrote:
>> The fs_read() and fs_write() functions are internal interfaces that
>> naturally want to get pointers as arguments. Most users so far even
>> have pointers and explicitly cast them into integers just to be able
>> to pass them into the function.
>>
>> Convert them over to instead take a pointer argument for the buffer.
>> That way any sandbox mapping gets greatly simplified and users of
>> the API intuitively know what to do.
>>
>> Signed-off-by: Alexander Graf 
>> ---
>>  board/BuR/common/common.c |  2 +-
>>  board/gdsys/p1022/controlcenterd-id.c | 10 +-
>>  cmd/mvebu/bubt.c  |  4 ++--
>>  common/splash_source.c|  4 +++-
>>  drivers/bootcount/bootcount_ext.c | 12 ++--
>>  drivers/fpga/zynqpl.c |  8 +---
>>  fs/fs.c   | 20 ++--
>>  include/fs.h  | 12 ++--
>>  lib/efi_loader/efi_file.c |  6 ++
>>  9 files changed, 40 insertions(+), 38 deletions(-)
> 
> U-Boot uses addresses for loading and managing images. I don't see a
> good reason to change that. We expect all logging to emit an address
> rather than a pointer, for example. See for example all the FIT and
> legacy image stuff, bootm, all the commands, etc.

Yes, usually the point of transition from address is when you go from
command line to internal infrastructure.

If you for example take a look at struct fstype_info in fs.c, you can
see that the read and write callbacks already use pointers.

The block equivalent of fs_read() is blk_dread() which also takes a buffer.

fs_read()/fs_write() really are the odd ones out in the API. And if you
take a look at all users of the API, I'd say 90% of them already get it
"wrong" (read: they assume a 1:1 map and just randomly cast pointers to
addresses).

So IMHO this patch really is just streamlining the API to adjust it to
the rest of the U-Boot infrastructure.


Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 10/17] fs: Convert fs_read/write to take buffer instead of address

[Simon Glass]

Hi Alex,

On 15 June 2018 at 06:42, Alexander Graf  wrote:
> The fs_read() and fs_write() functions are internal interfaces that
> naturally want to get pointers as arguments. Most users so far even
> have pointers and explicitly cast them into integers just to be able
> to pass them into the function.
>
> Convert them over to instead take a pointer argument for the buffer.
> That way any sandbox mapping gets greatly simplified and users of
> the API intuitively know what to do.
>
> Signed-off-by: Alexander Graf 
> ---
>  board/BuR/common/common.c |  2 +-
>  board/gdsys/p1022/controlcenterd-id.c | 10 +-
>  cmd/mvebu/bubt.c  |  4 ++--
>  common/splash_source.c|  4 +++-
>  drivers/bootcount/bootcount_ext.c | 12 ++--
>  drivers/fpga/zynqpl.c |  8 +---
>  fs/fs.c   | 20 ++--
>  include/fs.h  | 12 ++--
>  lib/efi_loader/efi_file.c |  6 ++
>  9 files changed, 40 insertions(+), 38 deletions(-)

U-Boot uses addresses for loading and managing images. I don't see a
good reason to change that. We expect all logging to emit an address
rather than a pointer, for example. See for example all the FIT and
legacy image stuff, bootm, all the commands, etc.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 10/17] fs: Convert fs_read/write to take buffer instead of address

[Alexander Graf]

The fs_read() and fs_write() functions are internal interfaces that
naturally want to get pointers as arguments. Most users so far even
have pointers and explicitly cast them into integers just to be able
to pass them into the function.

Convert them over to instead take a pointer argument for the buffer.
That way any sandbox mapping gets greatly simplified and users of
the API intuitively know what to do.

Signed-off-by: Alexander Graf 
---
 board/BuR/common/common.c |  2 +-
 board/gdsys/p1022/controlcenterd-id.c | 10 +-
 cmd/mvebu/bubt.c  |  4 ++--
 common/splash_source.c|  4 +++-
 drivers/bootcount/bootcount_ext.c | 12 ++--
 drivers/fpga/zynqpl.c |  8 +---
 fs/fs.c   | 20 ++--
 include/fs.h  | 12 ++--
 lib/efi_loader/efi_file.c |  6 ++
 9 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/board/BuR/common/common.c b/board/BuR/common/common.c
index 9df19791c2..ab9d9c51cf 100644
--- a/board/BuR/common/common.c
+++ b/board/BuR/common/common.c
@@ -269,7 +269,7 @@ static int load_devicetree(void)
puts("load_devicetree: set_blk_dev failed.\n");
return -1;
}
-   rc = fs_read(dtbname, (u32)dtbaddr, 0, 0, &dtbsize);
+   rc = fs_read(dtbname, (u_char *)dtbaddr, 0, 0, &dtbsize);
 #endif
if (rc == 0) {
gd->fdt_blob = (void *)dtbaddr;
diff --git a/board/gdsys/p1022/controlcenterd-id.c 
b/board/gdsys/p1022/controlcenterd-id.c
index 7e082dff05..2f01f7b7eb 100644
--- a/board/gdsys/p1022/controlcenterd-id.c
+++ b/board/gdsys/p1022/controlcenterd-id.c
@@ -874,7 +874,7 @@ static struct key_program *load_key_chunk(const char 
*ifname,
 
if (fs_set_blk_dev(ifname, dev_part_str, fs_type))
goto failure;
-   if (fs_read(path, (ulong)buf, 0, 12, &i) < 0)
+   if (fs_read(path, buf, 0, 12, &i) < 0)
goto failure;
if (i < 12)
goto failure;
@@ -890,7 +890,7 @@ static struct key_program *load_key_chunk(const char 
*ifname,
goto failure;
if (fs_set_blk_dev(ifname, dev_part_str, fs_type))
goto failure;
-   if (fs_read(path, (ulong)result, 0,
+   if (fs_read(path, result, 0,
sizeof(struct key_program) + header.code_size, &i) < 0)
goto failure;
if (i <= 0)
@@ -1019,7 +1019,7 @@ static int second_stage_init(void)
struct key_program *hmac_blob = NULL;
const char *image_path = "/ccdm.itb";
char *mac_path = NULL;
-   ulong image_addr;
+   u8 *image_addr;
loff_t image_size;
uint32_t err;
 
@@ -1059,7 +1059,7 @@ static int second_stage_init(void)
strcat(mac_path, mac_suffix);
 
/* read image from mmcdev (ccdm.itb) */
-   image_addr = (ulong)get_image_location();
+   image_addr = get_image_location();
if (fs_set_blk_dev("mmc", mmcdev, FS_TYPE_EXT))
goto failure;
if (fs_read(image_path, image_addr, 0, 0, &image_size) < 0)
@@ -1077,7 +1077,7 @@ static int second_stage_init(void)
puts("corrupted mac file\n");
goto failure;
}
-   if (check_hmac(hmac_blob, (u8 *)image_addr, image_size)) {
+   if (check_hmac(hmac_blob, image_addr, image_size)) {
puts("image integrity could not be verified\n");
goto failure;
}
diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index b4d371f305..29fff898fa 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -209,7 +209,7 @@ static size_t mmc_read_file(const char *file_name)
}
 
/* Perfrom file read */
-   rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
+   rc = fs_read(file_name, (void *)get_load_addr(), 0, 0, &act_read);
if (rc)
return 0;
 
@@ -392,7 +392,7 @@ static size_t usb_read_file(const char *file_name)
}
 
/* Perfrom file read */
-   rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
+   rc = fs_read(file_name, (void *)get_load_addr(), 0, 0, &act_read);
if (rc)
return 0;
 
diff --git a/common/splash_source.c b/common/splash_source.c
index 62763b9ebd..79dbea12fc 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -252,7 +253,8 @@ static int splash_load_fs(struct splash_location *location, 
u32 bmp_load_addr)
}
 
splash_select_fs_dev(location);
-   res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
+   res = fs_read(splash_file, map_sysmem(bmp_load_addr, bmp_size),
+ 0, 0, &actread);
 
 out:
if (location->ubivol != NULL)
diff --git a/drivers/bootcount/bootcount_ext.c 
b/drivers/bootcount/bootcount_ext.c
index 075e590896..