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 6A1E4D5AE71 for ; Thu, 7 Nov 2024 08:54:06 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id EDAE489220; Thu, 7 Nov 2024 09:54:04 +0100 (CET) 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.20230601.gappssmtp.com header.i=@baylibre-com.20230601.gappssmtp.com header.b="yC1u6zyf"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4A5388921C; Thu, 7 Nov 2024 09:54:04 +0100 (CET) Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) (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 A5C5A8904D for ; Thu, 7 Nov 2024 09:54:01 +0100 (CET) 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-x32b.google.com with SMTP id 5b1f17b1804b1-4315df7b43fso5933865e9.0 for ; Thu, 07 Nov 2024 00:54:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1730969641; x=1731574441; darn=lists.denx.de; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id :reply-to; bh=62NebAwLZ0tLPjRJh/RK1QUkF+esVUerAJcHq3e49Ss=; b=yC1u6zyfTHCOSpfUdv+NMT3A/3mLv3B2wAn07cQJZufuO7rI/3xf4aDWmuJONxAzpW 5O26VMJpNIFoEe2+4+q4Mme2LnwjZc5G8oWRSNhi7IJr/40E5VdKNdOjSNSFucGFwNeS EZZbLwmNehBxuQ77yaHOoUpt0BcQNiDlTgFnibcQeVXb8kYYkE76VOiUApinoVLii96W uuZ5OM4G72Kuym1ZdxVwWUNEzVecL3+AQL/Nly1XsA561TccKtrE9qIHTmNicukDqUdG cE8jj2IAWUPpqqOHpKLYi37Ladds599bMb5QwLYGTtriBRgpFGwsT+PdE1+A9sWIKnOm 87IQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730969641; x=1731574441; h=content-transfer-encoding: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=62NebAwLZ0tLPjRJh/RK1QUkF+esVUerAJcHq3e49Ss=; b=CUqfFSK4TH/prRh85V6W3q/aVUFp6E3v6sQs5d37kW6t2F6UiI7LIuTJXc6YR7bwJJ 0nzzjRlT1Wa+CZoYEKgUz8tkOofOqlbHDVnCSrdY7U6jAsfqKOT9AmSSFCHQ/4tRAYHX h9G7afVr/LHPP5fKU41Ad2XQfUBG1J/aIPdZNT6zNB8LRF56LKG7tGN5gH+k+e2To+er g3N+Y8lWKnkcx6NrP6MabVHfttOLrZwSgR4HqjnLoJgT/fIrXl0TYh+154uFmDTotzAO dFyR8rfLclW/UCURFwwUGR3RL58k1Jvw6bqsjzvJDsoLLslmsY7P7zJY7Ks06w2epFIq d9Gg== X-Forwarded-Encrypted: i=1; AJvYcCU+Brl71ThAmwVqI671Hx36gRvG4e62jWfG04hb3Kd0rfi6FCNWdkOe9d4QAqKUtkb7vla3KQc=@lists.denx.de X-Gm-Message-State: AOJu0Yw9Gkz6IoX4ZnkJ9ftZgluGBY7f5Jayk3Jq5Lx+9BmSocOnC7BI 9MwZf+uRLCfghPt36EZ1Lt8L+cqMEsJcrhBpeMEMqfgjtRBEUnKRI5lA2BjcE+k= X-Google-Smtp-Source: AGHT+IHBIqidVIVFQZbqwiQ+BAqoIlQ3ALIC3sLzxe+IfgIbyStfR4MUCcKvl1z7RFnIFQT7x5mk5A== X-Received: by 2002:a05:600c:4f15:b0:430:5654:45d0 with SMTP id 5b1f17b1804b1-4319acb104dmr396632535e9.14.1730969640485; Thu, 07 Nov 2024 00:54:00 -0800 (PST) Received: from localhost ([2a01:cb19:8f40:f900:f167:cb53:a707:1347]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381ed97cfefsm1107179f8f.26.2024.11.07.00.53.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Nov 2024 00:53:59 -0800 (PST) From: Mattijs Korpershoek To: Sam Protsenko Cc: Dmitry Rokosov , Igor Opaniuk , Tom Rini , "Andrew F. Davis" , Neil Armstrong , Simon Glass , Mario Six , u-boot@lists.denx.de, u-boot-amlogic@groups.io, rockosov@gmail.com, kernel@salutedevices.com, Guillaume La Roque Subject: Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block In-Reply-To: References: <20241017-android_ab_master-v5-0-43bfcc096d95@salutedevices.com> <20241017-android_ab_master-v5-6-43bfcc096d95@salutedevices.com> <87v7x1soto.fsf@baylibre.com> <875xp08ysu.fsf@baylibre.com> Date: Thu, 07 Nov 2024 09:53:56 +0100 Message-ID: <87v7wz4e6z.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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.8 at phobos.denx.de X-Virus-Status: Clean Hi Sam, On mer., nov. 06, 2024 at 21:17, Sam Protsenko = wrote: > On Wed, Nov 6, 2024 at 4:02=E2=80=AFAM Mattijs Korpershoek > wrote: >> >> Hi Sam, >> >> On mar., nov. 05, 2024 at 18:58, Sam Protsenko wrote: >> >> > On Tue, Nov 5, 2024 at 9:06=E2=80=AFAM Mattijs Korpershoek >> > wrote: >> >> >> >> Hi Sam, >> >> >> > >> > Hey Mattijs, >> > >> > [snip] >> > >> >> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, = struct disk_partition *part_info, >> >> >> * or the device tree. >> >> >> */ >> >> >> memset(slot_suffix, 0, sizeof(slot_suffix)); >> >> >> - slot_suffix[0] =3D BOOT_SLOT_NAME(slot); >> >> >> + slot_suffix[0] =3D '_'; >> >> >> + slot_suffix[1] =3D BOOT_SLOT_NAME(slot); >> >> > >> >> > AFAIU, this changes the behavior of two above functions, and >> >> > consequently of "bcb ab_select" command? If so, just to double chec= k: >> >> > were all users of those reworked correspondingly? I can see next >> >> > occurrences (there may be more): >> >> > >> >> > $ grep -sIrHn '"_' boot/bootmeth_android.c >> >> >> >> I thought the same when first reviewing the patch. >> >> Looking a bit closer... >> >> >> >> > >> >> > boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%= s", >> >> > priv->slot); >> >> > boot/bootmeth_android.c:111: sprintf(partname, >> >> > VENDOR_BOOT_PART_NAME "_%s", priv->slot); >> >> > boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->s= lot); >> >> > boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->s= lot); >> >> >> >> ... We can see that ab_select_slot() returns an integer >> >> That integer is used later on to initialize priv->slot: >> >> >> >> """ >> >> priv->slot[0] =3D BOOT_SLOT_NAME(ret); >> >> priv->slot[1] =3D '\0'; >> >> """ >> >> >> >> The change from Dmitry only changes what we **write** to the BCB (into >> >> the misc partition), not what is returned by ab_select_slot(). >> >> >> > >> > Sure. Just wanted to double check that the behavior is not changed in >> > any related parts, as the commit message doesn't mention that. Btw, >> > BCB is an interface between the bootloader and AOSP, so if this patch >> > changes what's being written into BCB, does it affect AOSP part of it >> > somehow? Especially for already existing devices with particular BCB >> > data, in case U-Boot gets updated there. >> >> Those are valid concerns. >> >> Per my understanding, on recent Android versions the slot suffix is not >> read from BCB, but from the ro.boot.slot_suffix property: >> > > That probably still leaves the possible combination of some devices > running new U-Boot versions (with this patch applied) together with > older Android versions. E.g. in case when U-Boot is updated but > Android isn't, may be especially relevant for some dev boards out > there. Yes, you are right. Boards with Android older than 2019 and U-Boot v2025.01+ My experience is that most of the time, it's the bootloader that stays out of date and the Android that gets updated. But that might not be the general case. > >> """ >> // Initialize the current_slot from the read-only property. If the pro= perty >> // was not set (from either the command line or the device tree), we c= an later >> // initialize it from the bootloader_control struct. > > So even in recent Android versions it's being initialized from BCB in > case the property is not set. Nope, the comment is wrong. If look at the lines below you can see that the function exits early when the property is not set. So in recent Android versions if the propery is not set, we just fail to in= itialize. > >> std::string suffix_prop =3D android::base::GetProperty("ro.boot.slot_s= uffix", ""); >> if (suffix_prop.empty()) { >> LOG(ERROR) << "Slot suffix property is not set"; >> return false; >> } >> current_slot_ =3D SlotSuffixToIndex(suffix_prop.c_str()); >> """ >> >> See: >> https://cs.android.com/android/platform/superproject/main/+/main:hardwar= e/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=3D185;drc= =3D86b8f575059a1799c760ca7012f540a528d68a9d;bpv=3D1;bpt=3D1 >> >> This has been the case since 2019. >> >> If we look at earlier implementations of libboot_control (which was in >> bootable/recovery) >> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1= 191517 >> >> So implementations before 2019 that do not have this patch: >> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1= 111899 >> >> Will get the slot suffix from the BCB (not from the commandline) >> >> For these older implementations, we will go through the following: >> BootControl::Init() >> LoadBootloaderControl(device.c_str(), &boot_ctrl) >> android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control) >> >> And struct bootloader_control has: >> >> struct bootloader_control { >> // NUL terminated active slot suffix. >> char slot_suffix[4]; >> >> And if we look at how the BCB is initialized from userspace in: >> https://cs.android.com/android/platform/superproject/main/+/main:hardwar= e/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=3D120;drc= =3D86b8f575059a1799c760ca7012f540a528d68a9d >> >> We can see that we copy _a, not a (for example, if slot =3D=3D 0). >> >> So I think this is fine. >> >> If needed, I can dig more for behaviour on older devices (<2019), let me= know! >> > > My point is, if it's possible to introduce this change but keep the > same old behavior (across both U-Boot and AOSP), it's usually better > to do that that way. If I'm not missing anything and that's a valid > concern, maybe a separate patch can be developed on top of the merged > patch series handling that. Anyways, I don't have enough time to work > on this right now, so just pointing out what I noticed, if it's useful > for anybody. I'll let the maintainers decide :) Yes, thanks a lot for noticing and taking the time to report your valid concerns :) > > Thanks! > >> > >> >> ab_select_slot() still returns an integer which needs to be converted >> >> via the BOOT_SLOT_NAME() macro. >> >> >> >> > >> >> > $ grep -sIrHn 'slot_suffix _' include/configs/ >> >> > include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${sl= ot_name};" >> >> > include/configs/meson64_android.h:65: "setenv slot_suffix >> >> > _${current_slot}; " \ >> >> >> >> Same goes for these 2 examples, we use: >> >> The "bcb ab_select current_slot" command to store the slot into the >> >> "current_slot" environment variable. >> >> Looking at cmd/bcb.c we can see: >> >> >> >> """ >> >> ret =3D ab_select_slot(dev_desc, &part_info, dec_tries); >> >> if (ret < 0) { >> >> printf("Android boot failed, error %d.\n", ret); >> >> return CMD_RET_FAILURE; >> >> } >> >> >> >> /* Android standard slot names are 'a', 'b', ... */ >> >> slot[0] =3D BOOT_SLOT_NAME(ret); >> >> slot[1] =3D '\0'; >> >> env_set(argv[1], slot); >> >> printf("ANDROID: Booting slot: %s\n", slot); >> >> """ >> >> >> >> So I think this is fine. >> >> >> > >> > [snip]