Re: [PATCH 06/14] fastboot: Change fastboot_buf_addr to an address

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

On Tue, Dec 05, 2023 at 20:54, Simon Glass <sjg@chromium.org> wrote:

> Hi Mattijs,
>
> On Tue, 5 Dec 2023 at 02:16, Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Simon,
>>
>> Thank you for your patch.
>>
>> On dim., déc. 03, 2023 at 17:31, Simon Glass <sjg@chromium.org> wrote:
>>
>> > Given the name of this variable, it should be an address, not a
>> > pointer. Update this, to make it easier to use with sandbox.
>> >
>> > Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> Tested that I could reflash AOSP on the Khadas VIM 3 board.
>>
>> Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3
>>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>
>> Some small nit/question below.
>>
>> > ---
>> >
>> >  cmd/fastboot.c                |  2 +-
>> >  drivers/fastboot/fb_command.c | 13 ++++++++-----
>> >  drivers/fastboot/fb_common.c  | 15 ++++-----------
>> >  include/fastboot-internal.h   |  2 +-
>> >  include/fastboot.h            |  6 +++---
>> >  5 files changed, 17 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/cmd/fastboot.c b/cmd/fastboot.c
>> > index c3c19231c988..792e83d372c3 100644
>> > --- a/cmd/fastboot.c
>> > +++ b/cmd/fastboot.c
>> > @@ -159,7 +159,7 @@ NXTARG:
>> >               return CMD_RET_USAGE;
>> >       }
>> >
>> > -     fastboot_init((void *)buf_addr, buf_size);
>> > +     fastboot_init(buf_addr, buf_size);
>> >
>> >       if (!strcmp(argv[1], "udp"))
>> >               return do_fastboot_udp(argc, argv, buf_addr, buf_size);
>> > diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
>> > index 5fcadcdf503d..ec030886dbb8 100644
>> > --- a/drivers/fastboot/fb_command.c
>> > +++ b/drivers/fastboot/fb_command.c
>> > @@ -10,6 +10,7 @@
>> >  #include <fastboot-internal.h>
>> >  #include <fb_mmc.h>
>> >  #include <fb_nand.h>
>> > +#include <mapmem.h>
>> >  #include <part.h>
>> >  #include <stdlib.h>
>> >  #include <linux/printk.h>
>> > @@ -252,7 +253,7 @@ void fastboot_data_download(const void *fastboot_data,
>> >               return;
>> >       }
>> >       /* Download data to fastboot_buf_addr */
>> > -     memcpy(fastboot_buf_addr + fastboot_bytes_received,
>> > +     memcpy(map_sysmem(fastboot_buf_addr, 0) + fastboot_bytes_received,
>> >              fastboot_data, fastboot_data_len);
>>
>> I'm a little confused on why there is no call of unmap_sysmem() here but
>> that seems to be the case for plenty other callers of map_sysmem().
>>
>> From what I've seen, in the sandbox unmap_sysmem() is a no-oop, but what
>> about other architectures ?
>
> Yes it is always a nop. It is designed such that address 0 can be used
> as RAM in sandbox.
>
> For PCI the mapping can in fact do something special, so that
> memory-mapped peripherals can expose their registers. See
> drivers/misc/swap_case.c for an example of that. But we have not ended
> up with lots of tests.

Understood, thank you for clarifying !

>
>>
>> >
>> >       pre_dot_num = fastboot_bytes_received / BYTES_PER_DOT;
>> > @@ -296,13 +297,15 @@ void fastboot_data_complete(char *response)
>> >   */
>> >  static void __maybe_unused flash(char *cmd_parameter, char *response)
>> >  {
>> > +     void *buf = map_sysmem(fastboot_buf_addr, 0);
>> > +
>> >       if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
>> > -             fastboot_mmc_flash_write(cmd_parameter, fastboot_buf_addr,
>> > -                                      image_size, response);
>> > +             fastboot_mmc_flash_write(cmd_parameter, buf, image_size,
>> > +                                      response);
>> >
>> >       if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND))
>> > -             fastboot_nand_flash_write(cmd_parameter, fastboot_buf_addr,
>> > -                                       image_size, response);
>> > +             fastboot_nand_flash_write(cmd_parameter, buf, image_size,
>> > +                                       response);
>>
>> Same here.
>>
>> >  }
>> >
>
> [..]
>
> Regards,
> Simon