public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [PATCH] fastboot: mmc: switch to user hwpart after erase/flash
@ 2022-10-10  8:05 Mattijs Korpershoek
  2022-10-10 15:38 ` Sean Anderson
  0 siblings, 1 reply; 5+ messages in thread
From: Mattijs Korpershoek @ 2022-10-10  8:05 UTC (permalink / raw)
  To: Patrick Delaunay, Sean Anderson
  Cc: u-boot, Simon Glass, Jaehoon Chung, Matthias Schiffer,
	Mattijs Korpershoek

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/+/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))
+		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,
-- 
Mattijs Korpershoek <mkorpershoek@baylibre.com>

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash
  2022-10-10  8:05 [PATCH] fastboot: mmc: switch to user hwpart after erase/flash Mattijs Korpershoek
@ 2022-10-10 15:38 ` Sean Anderson
  2022-10-11 12:07   ` Mattijs Korpershoek
  0 siblings, 1 reply; 5+ messages in thread
From: Sean Anderson @ 2022-10-10 15:38 UTC (permalink / raw)
  To: Mattijs Korpershoek, Patrick Delaunay
  Cc: u-boot, Simon Glass, Jaehoon Chung, Matthias Schiffer



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash
  2022-10-10 15:38 ` Sean Anderson
@ 2022-10-11 12:07   ` Mattijs Korpershoek
  2022-10-11 15:52     ` Sean Anderson
  0 siblings, 1 reply; 5+ messages in thread
From: Mattijs Korpershoek @ 2022-10-11 12:07 UTC (permalink / raw)
  To: Sean Anderson, Patrick Delaunay
  Cc: u-boot, Simon Glass, Jaehoon Chung, Matthias Schiffer

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/+/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.

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash
  2022-10-11 12:07   ` Mattijs Korpershoek
@ 2022-10-11 15:52     ` Sean Anderson
  2022-10-13  7:40       ` Mattijs Korpershoek
  0 siblings, 1 reply; 5+ messages in thread
From: Sean Anderson @ 2022-10-11 15:52 UTC (permalink / raw)
  To: Mattijs Korpershoek, Patrick Delaunay
  Cc: u-boot, Simon Glass, Jaehoon Chung, Matthias Schiffer



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/+/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.
> 
> 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
>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash
  2022-10-11 15:52     ` Sean Anderson
@ 2022-10-13  7:40       ` Mattijs Korpershoek
  0 siblings, 0 replies; 5+ messages in thread
From: Mattijs Korpershoek @ 2022-10-13  7:40 UTC (permalink / raw)
  To: Sean Anderson, Patrick Delaunay
  Cc: u-boot, Simon Glass, Jaehoon Chung, Matthias Schiffer

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/+/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.
>> 
>> 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
>>>
>> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-10-13  7:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-10  8:05 [PATCH] fastboot: mmc: switch to user hwpart after erase/flash Mattijs Korpershoek
2022-10-10 15:38 ` Sean Anderson
2022-10-11 12:07   ` Mattijs Korpershoek
2022-10-11 15:52     ` Sean Anderson
2022-10-13  7:40       ` Mattijs Korpershoek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox