[U-Boot] [PATCH v1 1/1] usb: gadget: fastboot: Add fastboot erase

[Matt Reimer <mreimer@sdgsystems.com>]

On Tue, Feb 17, 2015 at 5:57 AM, Dileep Katta <dileep.katta@linaro.org>
wrote:

> On 17 February 2015 at 02:51, Steve Rae <srae@broadcom.com> wrote:
>
> >
> >
> > On 15-02-16 12:40 PM, Dileep Katta wrote:
> >
> >> Hi Steve,
> >>
> >> On 14 February 2015 at 02:15, Steve Rae <srae@broadcom.com> wrote:
> >>
> >>
> >>>
> >>> On 15-02-12 12:29 AM, Dileep Katta wrote:
> >>>
> >>>  Hi Steve,
> >>>>
> >>>> On 11 February 2015 at 05:25, Steve Rae <srae@broadcom.com> wrote:
> >>>>
> >>>>
> >>>>> Hi, Dileep
> >>>>>
> >>>>>
> >>>>> On 15-02-10 12:49 AM, Dileep Katta wrote:
> >>>>>
> >>>>>
> >>>>>> Adds the fastboot erase functionality, to erase a partition
> >>>>>> specified by name. The erase is performed based on erase group size,
> >>>>>> to avoid erasing other partitions. The start address and the size
> >>>>>> is aligned to the erase group size for this.
> >>>>>>
> >>>>>> Currently only supports erasing from eMMC.
> >>>>>>
> >>>>>> Signed-off-by: Dileep Katta <dileep.katta@linaro.org>
> >>>>>> ---
> >>>>>> Note: The changes are on top of oem command support added by
> >>>>>> robh at kernel.org
> >>>>>>
> >>>>>>     common/fb_mmc.c                 | 58
> >>>>>> ++++++++++++++++++++++++++++++
> >>>>>> +++++++++++
> >>>>>>     drivers/usb/gadget/f_fastboot.c | 23 ++++++++++++++++
> >>>>>>     include/fb_mmc.h                |  1 +
> >>>>>>     3 files changed, 82 insertions(+)
> >>>>>>
> >>>>>> diff --git a/common/fb_mmc.c b/common/fb_mmc.c
> >>>>>> index 6ea3938..3911989 100644
> >>>>>> --- a/common/fb_mmc.c
> >>>>>> +++ b/common/fb_mmc.c
> >>>>>> @@ -10,6 +10,7 @@
> >>>>>>     #include <part.h>
> >>>>>>     #include <aboot.h>
> >>>>>>     #include <sparse_format.h>
> >>>>>> +#include <mmc.h>
> >>>>>>
> >>>>>>     #ifndef CONFIG_FASTBOOT_GPT_NAME
> >>>>>>     #define CONFIG_FASTBOOT_GPT_NAME GPT_ENTRY_NAME
> >>>>>> @@ -110,3 +111,60 @@ void fb_mmc_flash_write(const char *cmd, void
> >>>>>> *download_buffer,
> >>>>>>                   write_raw_image(dev_desc, &info, cmd,
> >>>>>> download_buffer,
> >>>>>>                                   download_bytes);
> >>>>>>     }
> >>>>>> +
> >>>>>> +void fb_mmc_erase(const char *cmd, char *response)
> >>>>>> +{
> >>>>>> +       int ret;
> >>>>>> +       block_dev_desc_t *dev_desc;
> >>>>>> +       disk_partition_t info;
> >>>>>> +       lbaint_t blks, blks_start, blks_size, grp_size;
> >>>>>> +       struct mmc *mmc = find_mmc_device(CONFIG_
> >>>>>> FASTBOOT_FLASH_MMC_DEV);
> >>>>>> +
> >>>>>> +       if (mmc == NULL) {
> >>>>>> +               error("invalid mmc device\n");
> >>>>>>
> >>>>>>
> >>>>> no newline with error()
> >>>>>
> >>>>>  Will remove
> >>>>
> >>>>
> >>>>>   +               fastboot_fail("invalid mmc device");
> >>>>>
> >>>>>> +               return;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       /* initialize the response buffer */
> >>>>>> +       response_str = response;
> >>>>>> +
> >>>>>> +       dev_desc = get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
> >>>>>> +       if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) {
> >>>>>> +               error("invalid mmc device\n");
> >>>>>>
> >>>>>>
> >>>>> no newline with error()
> >>>>>
> >>>>>  Will remove
> >>>>
> >>>>
> >>>>>
> >>>>>   +               fastboot_fail("invalid mmc device");
> >>>>>
> >>>>>> +               return;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       ret = get_partition_info_efi_by_name(dev_desc, cmd, &info);
> >>>>>> +       if (ret) {
> >>>>>> +               error("cannot find partition: '%s'\n", cmd);
> >>>>>>
> >>>>>>
> >>>>> no newline with error()
> >>>>>
> >>>>>  Will remove
> >>>>
> >>>>
> >>>>>   +               fastboot_fail("cannot find partition");
> >>>>>
> >>>>>> +               return;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       puts("Erasing partition\n");
> >>>>>> +
> >>>>>> +       /* Align blocks to erase group size to avoid erasing other
> >>>>>> partitions */
> >>>>>> +       grp_size = mmc->erase_grp_size;
> >>>>>> +       blks_start = (info.start + grp_size - 1) & ~(grp_size - 1);
> >>>>>> +       if (info.size >= grp_size)
> >>>>>> +               blks_size = (info.size - (blks_start - info.start))
> &
> >>>>>> +                               (~(grp_size - 1));
> >>>>>> +       else
> >>>>>> +               blks_size = 0;
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> Is this logic correct??? Isn't the "erase_grp_size" in bytes? and the
> >>>>> info.start & info.size in LBA's?
> >>>>>
> >>>>>  Yes, the math will take care of it. Ref: mmc_berase()
> >>>>
> >>>>
> >>> So, I have a partition:
> >>>
> >>>    2     0x00000300      0x000003ff      "test"
> >>>          attrs:  0x0000000000000000
> >>>          type:   9e312af1-18fe-fa41-45f3-37b3bb1d1061
> >>>          guid:   18a5d0db-d23a-aac1-0d4c-692c7ba9ab1c
> >>>
> >>> and 'fastboot erase test' produces:
> >>>    Erasing blocks 1024 to 1024 due to alignment
> >>>    ........ erased 0 bytes from 'test'
> >>> which is not correct!
> >>>
> >>> Furthermore, doesn't the mmc_berase() already handle the
> >>> "erase_grp_size"....
> >>>
> >>>
> >> It does handle it, in a way, but the way it handles it is to erase more
> >> blocks than requested if the request isn't aligned. In your example, you
> >> requested the "test" partition to be erased (0x300 to 0x3ff), but what
> was
> >> actually erased (as printed in the "Caution" message from mmc_berase)
> was
> >> 0x0 to 0x7ff.
> >>
> >> If I remove your alignment logic, then it produces:
> >>
> >>>    Erasing blocks 768 to 1024 due to alignment
> >>>
> >>>    Caution! Your devices Erase group is 0x400
> >>>    The erase range would be change to 0x0~0x7ff
> >>>
> >>>    ........ erased 131072 bytes from 'test'
> >>> which looks correct (except for the "Caution" message)....
> >>>
> >>> Thanks, Steve
> >>>
> >>
> >>
> >> Except that this one has now erased 0x0 to 0x300 and 0x400 to 0x7ff
> also,
> >> which you did not want to erase, right?
> >> Aligning the start address is meant to protect this data from being
> erased
> >> unintentionally. The trade-off is that some small partitions won't get
> >> erased at all, if they are too small and not aligned.
> >>
> >> Regards, Dileep
> >>
> >>  I'm sorry, but I am really confused....
> > --  131072 bytes agrees with 0x300 to 0x3ff...
> > --  I'm suspicious that the Caution message and the actual erased bytes
> do
> > not match....
> > --  I'm not certain that the existing mmc_berase() code is correct...
> > --  If this "erase_grp_size" is a method to erase large sections
> > "quickly", then it would seem that it needs to be combined with erasing
> (by
> > writing "0" or "1" appropriately) to the smaller sections to erase the
> > "exact specified size". And this should be handled in the mmc_berase()
> code!
> >
>
> Thanks for your suggestions Steve. I will try to send a separate patch for
> mmc_berase() if required, after validating all the scenarios.
>
>
> > Additionally, because it is not necessary to "erase before writing", I
> > personally do not have any need for this "fastboot erase" command at all.
> > So I think that I am going to withdraw from this conversation....
> > Thanks, Steve
>
>
> We wish fastboot to be complete with all the commands of the
> protocol supported in u-boot.
>

Actually according to this comment from fastboot
(system/core/fastboot/fastboot.c), sometimes it is necessary to erase
before flashing:

/* Until we get lazy inode table init working in make_ext4fs, we need to
 * erase partitions of type ext4 before flashing a filesystem so no stale
 * inodes are left lying around.  Otherwise, e2fsck gets very upset.
 */

In order for fastboot to know that it needs to erase the partition first,
we'll also need to implement a fastboot var "partition-type" that will
return "ext4" for ext4 partitions. See
system/core/fastboot/engine.c:fb_format_supported().

Matt