Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash

Sean Anderson
2022-10-10T15:38:36Z

On 10/10/22 4:05 AM, Mattijs Korpershoek wrote:
> Before erasing (or flashing) an mmc hardware partition, such as
> mmc0boot0 or mmc0boot1, we select it using blk_dselect_hwpart().
>
> However, we don't select back the normal (userdata) hwpartition
> afterwards.
>
> For example, the following sequence is broken:
>
> $ fastboot erase mmc2boot1 # switch to hwpart 1
> $ fastboot reboot bootloader # attempts to read GPT from hwpart 1
>
>> writing 128 blocks starting at 8064...
>> ........ wrote 65536 bytes to 'mmc0boot1'
>> GUID Partition Table Header signature is wrong: 0xFB4EC30FC5B7E5B2 != 0x5452415020494645
>> find_valid_gpt: *** ERROR: Invalid GPT ***
>> GUID Partition Table Header signature is wrong: 0x0 != 0x5452415020494645
>> find_valid_gpt: *** ERROR: Invalid Backup GPT ***
>> Error: mmc 2:misc read failed (-2)
>
> The GPT is stored in hwpart0 (userdata), not hwpart1 (mmc0boot1)

This seems like a problem with your boot script. You should run `mmc dev
${mmcdev}` (and `mmc rescan`) before trying to access any MMC
partitions. This is especially important after fastbooting, since the
partition layout (or active hardware partition) may have changed.

> As the blk documentation states:
>> Once selected only the region of the device
>> covered by that partition is accessible.
>
> Add a cleanup function, fastboot_mmc_select_default_hwpart() which
> selects back the default hwpartition (userdata).
>
> Note: this is more visible since
> commit a362ce214f03 ("fastboot: Implement generic fastboot_set_reboot_flag")
> because "fastboot reboot bootloader" will access the "misc" partition.
>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> This fix has been used for over a year in the AOSP tree at [1]
>
> [1] https://android.googlesource.com/device/amlogic/yukawa/+/c008ddaec0943adda394b229e83e147f6165773c
> ---
> drivers/fastboot/fb_mmc.c | 20 +++++++++++++++++---
> include/mmc.h | 1 +
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c
> index 033c510bc096..dedef7c4deb1 100644
> --- a/drivers/fastboot/fb_mmc.c
> +++ b/drivers/fastboot/fb_mmc.c
> @@ -199,6 +199,12 @@ static void write_raw_image(struct blk_desc *dev_desc,
> fastboot_okay(NULL, response);
> }
>
> +static void fastboot_mmc_select_default_hwpart(struct blk_desc *dev_desc)
> +{
> + if (blk_dselect_hwpart(dev_desc, MMC_PART_USERDATA))

If you really want to do this, we shoud save the current partition and
restore if after fastbooting. Always switching to hardware partition 0
is just as broken as the current behavior.

--Sean

> + pr_err("Failed to restore hwpart to userdata\n");
> +}
> +
> #if defined(CONFIG_FASTBOOT_MMC_BOOT_SUPPORT) || \
> defined(CONFIG_FASTBOOT_MMC_USER_SUPPORT)
> static int fb_mmc_erase_mmc_hwpart(struct blk_desc *dev_desc)
> @@ -246,7 +252,7 @@ static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer,
> if (blkcnt > dev_desc->lba) {
> pr_err("Image size too large\n");
> fastboot_fail("Image size too large", response);
> - return;
> + goto fail_restore_hwpart;
> }
>
> debug("Start Flashing Image to EMMC_BOOT%d...\n", hwpart);
> @@ -257,7 +263,7 @@ static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer,
> pr_err("Failed to write EMMC_BOOT%d\n", hwpart);
> fastboot_fail("Failed to write EMMC_BOOT part",
> response);
> - return;
> + goto fail_restore_hwpart;
> }
>
> printf("........ wrote %lu bytes to EMMC_BOOT%d\n",
> @@ -267,11 +273,15 @@ static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer,
> pr_err("Failed to erase EMMC_BOOT%d\n", hwpart);
> fastboot_fail("Failed to erase EMMC_BOOT part",
> response);
> - return;
> + goto fail_restore_hwpart;
> }
> }
>
> fastboot_okay(NULL, response);
> + return;
> +
> +fail_restore_hwpart:
> + fastboot_mmc_select_default_hwpart(dev_desc);
> }
> #endif
>
> @@ -630,6 +640,8 @@ void fastboot_mmc_flash_write(const char *cmd, void *download_buffer,
> write_raw_image(dev_desc, &info, cmd, download_buffer,
> download_bytes, response);
> }
> +
> + fastboot_mmc_select_default_hwpart(dev_desc);
> }
>
> /**
> @@ -694,6 +706,8 @@ void fastboot_mmc_erase(const char *cmd, char *response)
>
> blks = fb_mmc_blk_write(dev_desc, blks_start, blks_size, NULL);
>
> + fastboot_mmc_select_default_hwpart(dev_desc);
> +
> if (blks != blks_size) {
> pr_err("failed erasing from device %d\n", dev_desc->devnum);
> fastboot_fail("failed erasing from device", response);
> diff --git a/include/mmc.h b/include/mmc.h
> index 027e8bcc73a6..cdd457a60a5b 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -362,6 +362,7 @@ enum mmc_voltage {
> /* The number of MMC physical partitions. These consist of:
> * boot partitions (2), general purpose partitions (4) in MMC v4.4.
> */
> +#define MMC_PART_USERDATA 0
> #define MMC_NUM_BOOT_PARTITION 2
> #define MMC_PART_RPMB 3 /* RPMB partition number */
>
>
> ---
> base-commit: f5717231abad983b4d8f87db612ae60dec86cb1b
> change-id: 20221010-switch-hwpart-7fed7746db59
>
> Best regards,
>