[U-Boot] [PATCH v4 4/5] usb/gadget: add the fastboot gadget

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Rob,

> On Wed, Apr 23, 2014 at 6:02 AM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi Rob,
> >
> >> From: Sebastian Siewior <bigeasy@linutronix.de>
> >>
> >> This patch contains an implementation of the fastboot protocol on
> >> the device side and documentation. This is based on USB download
> >> gadget infrastructure. The fastboot function implements the
> >> getvar, reboot, download and reboot commands. What is missing is
> >> the flash handling i.e. writting the image to media.
> >>
> 
> [...]
> 
> >> +     f_fb->out_ep = usb_ep_autoconfig(gadget, &fs_ep_out);
> >> +     if (!f_fb->out_ep)
> >> +             goto err;
> >> +     f_fb->out_ep->driver_data = c->cdev;
> >> +
> >> +     hs_ep_out.bEndpointAddress = fs_ep_out.bEndpointAddress;
> >> +
> >> +     return 0;
> >> +err:
> >> +     return -1;
> >
> >                 Maybe -ENODEV or -EINVAL?
> 
> Okay. Actually, we can remove the goto and just return the original
> error codes.
> 
> 
> >> +     if (buffer_size < transfer_size)
> >> +             transfer_size = buffer_size;
> >> +
> >> +     memcpy((void *)CONFIG_USB_FASTBOOT_BUF_ADDR + download_bytes,
> >
> >         Please consider using dfu_get_buf() from dfu.c to provide
> >         dynamically allocated and controlled buffer instead of the
> >         CONFIG_USB_FASTBOOT_BUF_ADDR and _SIZE.
> >
> >         Another advantage of this code is the ability to set
> >         "dfu_bufsiz" env variable with size of the buffer.
> 
> I considered this already. I certainly don't like reinventing things
> which was why I originally used loadaddr and added loadsize to provide
> a defined load buffer size.
> 
> The problem is fastboot needs enough RAM to download an entire sparse
> filesystem. I have no idea what size exactly is typical or required,
> but it seems that we want to be able to use nearly all free RAM. We
> can talk all we want about how this is a crappy design, but it is what
> it is. This is how the protocol works.

I understand you :-). The same situation was with DFU on the beginning.
Large buffer with starting address defined per board.

Then, after some discussion, we come to conclusion that it would be
better to increase malloc pool and dynamically allocate buffer. 

Am I correct, that you don't know beforehand what would be the size of
downloaded file - maybe 5 MiB or maybe 512 MiB? Also from your
descriptor it seems like fastboot protocol don't want to impose any
restrictions about the size. Is it user's responsibility to send data
smaller than RAM size?

In the DFU/THOR we store data in buffer size packets (32 MiB). It also
has some drawbacks - with large raw data images we cannot download the
whole (e.g. rootfs) image and beforehand flashing check integrity.

One question - when your board has e.g. 768 MiB of "available" RAM, then
is the size of large rootfs restricted to this size?


> 
> The problem with the DFU buffer is it is allocated from the malloc
> region. If we just increase the malloc region to be close to total RAM
> size then we will start to break other commands like tftp and fsload
> which typically just use the RAM u-boot is not using (i.e. all but the
> end of memory). The only platforms which have more than a few MB for
> malloc are the ones that enable DFU.

Correct. On the other hand when we want to allocate too large buffer
we receive error from malloc and flashing is aborted. No harm is done.

> 
> Rob


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group