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

[Simon Glass]

Hi Mattijs,

On Tue, 5 Dec 2023 at 02:16, Mattijs Korpershoek
<mkorpershoek at baylibre.com> wrote:
>
> Hi Simon,
>
> Thank you for your patch.
>
> On dim., déc. 03, 2023 at 17:31, Simon Glass <sjg at 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 at chromium.org>
>
> Tested that I could reflash AOSP on the Khadas VIM 3 board.
>
> Tested-by: Mattijs Korpershoek <mkorpershoek at baylibre.com> # on vim3
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek at 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.

>
> >
> >       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