From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AD232C433FE for ; Thu, 13 Oct 2022 07:40:42 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id EEE6984CDB; Thu, 13 Oct 2022 09:40:39 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20210112.gappssmtp.com header.i=@baylibre-com.20210112.gappssmtp.com header.b="Rj/lT5Ve"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C685684EA8; Thu, 13 Oct 2022 09:40:38 +0200 (CEST) Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id BCD2183914 for ; Thu, 13 Oct 2022 09:40:35 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@baylibre.com Received: by mail-wm1-x336.google.com with SMTP id iv17so623629wmb.4 for ; Thu, 13 Oct 2022 00:40:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=WUPSkFPdJW1CfGW18DSDDFSPX4g8i2LAoEV1lmlzyqc=; b=Rj/lT5VeTcLC2sayeckQr3QgRxOjhaFZWzbnFJ67eSjFHaOFI9sHzOt1sbvUgtlq43 0pbbDDRp9/BG+YuooqJYR4Gw8VtO2M/kvPurpKf6+t/HMRjwtz4yMYXe6Ebxzd7OMLvw ytuNawxks4mKvkF6QEZnpng0RPoyFfEwmBpeKIzkF7YP+owIfZodhOl+z+LArRwn4ziu CrJDgZMi8QGipC6RsCeMXUmtMJVlA+Jw6hrvGk+MvcKtS7RbtgSnyFY8k3uraqGoA5y8 g/d6OboxzexNNB6oBbDIYh8RY3Xb6CXN1nvL04pzT4e7s6h7NWzRJekWQEnEhq66w4Pc Gxlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WUPSkFPdJW1CfGW18DSDDFSPX4g8i2LAoEV1lmlzyqc=; b=o5iW3MzvjWz+OAIMZyD45t2cFakdmwiB0vD9orH8n4cWm1eQjUpiFNy9W/MumE/8UV wWnSPSbyElBcAbfFNTAnWRenJwjr1Vfgy94Vo8Qo1GcTu+uq8cjWXsjYqqo1L2U1BkWU PmpFlhx73lS+1hc5o10q9sRaK9dp9TkBK56hyb/XwfzCBPkCtKmiRp+4pO+RldNy/uof 6oB0VW2mNTNB5CHKu6esOEUiOLckQnz9OTr1Ct9oubo89PweHvpQkPDniHZ8pZ4d+QoE jgMrZo4lkxYIbzY/zqF5aV2QtcbI8xP9b5Y8hdZPfcW6ffOETLcXFcVokonkYOyPK9Ur /e2A== X-Gm-Message-State: ACrzQf1v2stQ3p3Qp28N7PNk9h6kOzEWYRhC6kdlWrD6cm3q5urLS9X1 f5kTVA6JG5A7FCKctGWtfoMCCw== X-Google-Smtp-Source: AMsMyM4BLsIBK3n8zJreXbEX0F+HdxDDGdQU/0dAOGTEDSbAz+3Mhz5OoLkF4oFq2qGAEaCAo5uo3g== X-Received: by 2002:a05:600c:1c88:b0:3c6:d9a5:a083 with SMTP id k8-20020a05600c1c8800b003c6d9a5a083mr2789650wms.54.1665646835228; Thu, 13 Oct 2022 00:40:35 -0700 (PDT) Received: from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id b6-20020adff246000000b00228a6ce17b4sm1376828wrp.37.2022.10.13.00.40.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Oct 2022 00:40:34 -0700 (PDT) From: Mattijs Korpershoek To: Sean Anderson , Patrick Delaunay Cc: u-boot@lists.denx.de, Simon Glass , Jaehoon Chung , Matthias Schiffer Subject: Re: [PATCH] fastboot: mmc: switch to user hwpart after erase/flash In-Reply-To: <7554ecac-11a7-c58b-950e-ad6a627318cf@seco.com> References: <20221010-switch-hwpart-v1-0-52cbd34c72b7@baylibre.com> <87k05636o7.fsf@baylibre.com> <7554ecac-11a7-c58b-950e-ad6a627318cf@seco.com> Date: Thu, 13 Oct 2022 09:40:34 +0200 Message-ID: <87tu485fyl.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Tue, Oct 11, 2022 at 11:52, Sean Anderson 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 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 " 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 >>>> --- >>>> 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 >>> >>