[PATCH] fastboot: mmc: switch to user hwpart after erase/flash
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)
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/+/c008ddaec0943adda39... --- 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)) + 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,
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/+/c008ddaec0943adda39...
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,
Hi Sean,
Thank you for your review.
On Mon, Oct 10, 2022 at 11:38, Sean Anderson sean.anderson@seco.com wrote:
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.
I don't think I can fix this in my boot script. My boot script runs => fastboot usb 0 which will start a loop to receive fastboot commands from the host.
The full u-boot env i'm using is in include/configs/meson64_android.h
This loop will go on until "fastboot reboot <reboot_reason>" is called.
I do can "work around" this by using the following fastboot sequence: $ fastboot erase mmc2boot1 $ fastboot oem format # switch backs to hwpart 0 (USER) $ fastboot reboot bootloader
But that's not a good option to me. From a user's perspective, it should not matter in which order we issue fastboot commands to the board.
Maybe I should fix bcb.c instead to make sure that it selects hwpart0 before reading/writing into the bcb partition ?
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/+/c008ddaec0943adda39...
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.
ACK. This patch just replaces one broken behaviour by another one. I will see if I can save the hardware partition state instead or patch the bcb command.
--Sean
On 10/11/22 8:07 AM, Mattijs Korpershoek wrote:
Hi Sean,
Thank you for your review.
On Mon, Oct 10, 2022 at 11:38, Sean Anderson sean.anderson@seco.com wrote:
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.
I don't think I can fix this in my boot script. My boot script runs => fastboot usb 0 which will start a loop to receive fastboot commands from the host.
The full u-boot env i'm using is in include/configs/meson64_android.h
This loop will go on until "fastboot reboot <reboot_reason>" is called.
I do can "work around" this by using the following fastboot sequence: $ fastboot erase mmc2boot1 $ fastboot oem format # switch backs to hwpart 0 (USER) $ fastboot reboot bootloader
But that's not a good option to me. From a user's perspective, it should not matter in which order we issue fastboot commands to the board.
Maybe I should fix bcb.c instead to make sure that it selects hwpart0 before reading/writing into the bcb partition ?
Yes. The bcb command should be using something like part_get_info_by_dev_and_name_or_num instead of using its own bespoke parsing. Unfortunately, command syntax is part of the API, but at the very least you can fall back to something sane if argv[2] ("devnum") doesn't parse as an int (aka it's the ifname).
--Sean
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/+/c008ddaec0943adda39...
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.
ACK. This patch just replaces one broken behaviour by another one. I will see if I can save the hardware partition state instead or patch the bcb command.
--Sean
On Tue, Oct 11, 2022 at 11:52, Sean Anderson sean.anderson@seco.com wrote:
On 10/11/22 8:07 AM, Mattijs Korpershoek wrote:
Hi Sean,
Thank you for your review.
On Mon, Oct 10, 2022 at 11:38, Sean Anderson sean.anderson@seco.com wrote:
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.
I don't think I can fix this in my boot script. My boot script runs => fastboot usb 0 which will start a loop to receive fastboot commands from the host.
The full u-boot env i'm using is in include/configs/meson64_android.h
This loop will go on until "fastboot reboot <reboot_reason>" is called.
I do can "work around" this by using the following fastboot sequence: $ fastboot erase mmc2boot1 $ fastboot oem format # switch backs to hwpart 0 (USER) $ fastboot reboot bootloader
But that's not a good option to me. From a user's perspective, it should not matter in which order we issue fastboot commands to the board.
Maybe I should fix bcb.c instead to make sure that it selects hwpart0 before reading/writing into the bcb partition ?
Yes. The bcb command should be using something like part_get_info_by_dev_and_name_or_num instead of using its own bespoke parsing. Unfortunately, command syntax is part of the API, but at the very least you can fall back to something sane if argv[2] ("devnum") doesn't parse as an int (aka it's the ifname).
Thank you for the suggestion. I will look into patching cmd/bcb.c instead.
--Sean
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/+/c008ddaec0943adda39...
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.
ACK. This patch just replaces one broken behaviour by another one. I will see if I can save the hardware partition state instead or patch the bcb command.
--Sean
participants (2)
-
Mattijs Korpershoek -
Sean Anderson