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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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