[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB
The motivation behind the 'bcb' command is largely explained in the second patch. The other patch performs polishing of https://patchwork.ozlabs.org/patch/1099689/, which is a hard prerequisite for this series.
v2: - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT. - Polished the code. Ensured no warnings returned by sparse, smatch, `cppcheck --force --enable=all --inconclusive`, make W=1. - Tested on R-Car-H3-ES20 ULCB-KF.
v1: - https://patchwork.ozlabs.org/cover/1080393/
Eugeniu Rosca (2): include: android_bootloader_message.h: Minimize the diff to AOSP cmd: Add 'bcb' command to read/modify/write BCB fields
cmd/Kconfig | 17 ++ cmd/Makefile | 1 + cmd/bcb.c | 330 +++++++++++++++++++++++++++ include/android_bootloader_message.h | 126 +++++----- 4 files changed, 416 insertions(+), 58 deletions(-) create mode 100644 cmd/bcb.c
Perform the following updates: - Relocate the commit id from the file to the description of U-Boot commit. The AOSP commit is c784ce50e8c10eaf70e1f97e24e8324aef45faf5. This is done to avoid stale references in the file itself. The reasoning is in https://patchwork.ozlabs.org/patch/1098056/#2170209. - Minimize the diff to AOSP, to decrease the effort of the next AOSP backports. The background can be found in: https://patchwork.ozlabs.org/patch/1080394/#2168454. - Guard the static_assert() calls by #ifndef __UBOOT__ ... #endif, to avoid compilation failures of files including the header.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- v2: - Newly pushed. No changes. --- include/android_bootloader_message.h | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-)
diff --git a/include/android_bootloader_message.h b/include/android_bootloader_message.h index b84789f02227..286d7ab0f31e 100644 --- a/include/android_bootloader_message.h +++ b/include/android_bootloader_message.h @@ -2,7 +2,7 @@ * This is from the Android Project, * Repository: https://android.googlesource.com/platform/bootable/recovery * File: bootloader_message/include/bootloader_message/bootloader_message.h - * Commit: c784ce50e8c10eaf70e1f97e24e8324aef45faf5 + * Commit: See U-Boot commit description * * Copyright (C) 2008 The Android Open Source Project * @@ -12,18 +12,24 @@ #ifndef __ANDROID_BOOTLOADER_MESSAGE_H #define __ANDROID_BOOTLOADER_MESSAGE_H
+#ifndef __UBOOT__ +#include <assert.h> +#include <stddef.h> +#include <stdint.h> +#else /* compiler.h defines the types that otherwise are included from stdint.h and * stddef.h */ #include <compiler.h> +#endif
-/* Spaces used by misc partition are as below: - * 0 - 2K For bootloader_message - * 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used - * as bootloader_message_ab struct) - * 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices - * Note that these offsets are admitted by bootloader,recovery and uncrypt, so they - * are not configurable without changing all of them. */ +// Spaces used by misc partition are as below: +// 0 - 2K For bootloader_message +// 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used +// as bootloader_message_ab struct) +// 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices +// Note that these offsets are admitted by bootloader,recovery and uncrypt, so they +// are not configurable without changing all of them. static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0; static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
@@ -61,17 +67,17 @@ struct bootloader_message { char status[32]; char recovery[768];
- /* The 'recovery' field used to be 1024 bytes. It has only ever - * been used to store the recovery command line, so 768 bytes - * should be plenty. We carve off the last 256 bytes to store the - * stage string (for multistage packages) and possible future - * expansion. */ + // The 'recovery' field used to be 1024 bytes. It has only ever + // been used to store the recovery command line, so 768 bytes + // should be plenty. We carve off the last 256 bytes to store the + // stage string (for multistage packages) and possible future + // expansion. char stage[32];
- /* The 'reserved' field used to be 224 bytes when it was initially - * carved off from the 1024-byte recovery field. Bump it up to - * 1184-byte so that the entire bootloader_message struct rounds up - * to 2048-byte. */ + // The 'reserved' field used to be 224 bytes when it was initially + // carved off from the 1024-byte recovery field. Bump it up to + // 1184-byte so that the entire bootloader_message struct rounds up + // to 2048-byte. char reserved[1184]; };
@@ -79,10 +85,12 @@ struct bootloader_message { * We must be cautious when changing the bootloader_message struct size, * because A/B-specific fields may end up with different offsets. */ +#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message) == 2048, "struct bootloader_message size changes, which may break A/B devices"); #endif +#endif /* __UBOOT__ */
/** * The A/B-specific bootloader message structure (4-KiB). @@ -108,7 +116,7 @@ struct bootloader_message_ab { char slot_suffix[32]; char update_channel[128];
- /* Round up the entire struct to 4096-byte. */ + // Round up the entire struct to 4096-byte. char reserved[1888]; };
@@ -116,26 +124,28 @@ struct bootloader_message_ab { * Be cautious about the struct size change, in case we put anything post * bootloader_message_ab struct (b/29159185). */ +#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message_ab) == 4096, "struct bootloader_message_ab size changes"); #endif +#endif /* __UBOOT__ */
#define BOOT_CTRL_MAGIC 0x42414342 /* Bootloader Control AB */ #define BOOT_CTRL_VERSION 1
struct slot_metadata { - /* Slot priority with 15 meaning highest priority, 1 lowest - * priority and 0 the slot is unbootable. */ + // Slot priority with 15 meaning highest priority, 1 lowest + // priority and 0 the slot is unbootable. uint8_t priority : 4; - /* Number of times left attempting to boot this slot. */ + // Number of times left attempting to boot this slot. uint8_t tries_remaining : 3; - /* 1 if this slot has booted successfully, 0 otherwise. */ + // 1 if this slot has booted successfully, 0 otherwise. uint8_t successful_boot : 1; - /* 1 if this slot is corrupted from a dm-verity corruption, 0 - * otherwise. */ + // 1 if this slot is corrupted from a dm-verity corruption, 0 + // otherwise. uint8_t verity_corrupted : 1; - /* Reserved for further use. */ + // Reserved for further use. uint8_t reserved : 7; } __attribute__((packed));
@@ -148,99 +158,99 @@ struct slot_metadata { * mandatory. */ struct bootloader_control { - /* NUL terminated active slot suffix. */ + // NUL terminated active slot suffix. char slot_suffix[4]; - /* Bootloader Control AB magic number (see BOOT_CTRL_MAGIC). */ + // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC). uint32_t magic; - /* Version of struct being used (see BOOT_CTRL_VERSION). */ + // Version of struct being used (see BOOT_CTRL_VERSION). uint8_t version; - /* Number of slots being managed. */ + // Number of slots being managed. uint8_t nb_slot : 3; - /* Number of times left attempting to boot recovery. */ + // Number of times left attempting to boot recovery. uint8_t recovery_tries_remaining : 3; - /* Ensure 4-bytes alignment for slot_info field. */ + // Ensure 4-bytes alignment for slot_info field. uint8_t reserved0[2]; - /* Per-slot information. Up to 4 slots. */ + // Per-slot information. Up to 4 slots. struct slot_metadata slot_info[4]; - /* Reserved for further use. */ + // Reserved for further use. uint8_t reserved1[8]; - /* CRC32 of all 28 bytes preceding this field (little endian - * format). */ + // CRC32 of all 28 bytes preceding this field (little endian + // format). uint32_t crc32_le; } __attribute__((packed));
+#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_control) == sizeof(((struct bootloader_message_ab *)0)->slot_suffix), "struct bootloader_control has wrong size"); #endif +#endif /* __UBOOT__ */
#ifndef __UBOOT__ - #ifdef __cplusplus
#include <string> #include <vector>
-/* Return the block device name for the bootloader message partition and waits - * for the device for up to 10 seconds. In case of error returns the empty - * string. */ +// Return the block device name for the bootloader message partition and waits +// for the device for up to 10 seconds. In case of error returns the empty +// string. std::string get_bootloader_message_blk_device(std::string* err);
-/* Read bootloader message into boot. Error message will be set in err. */ +// Read bootloader message into boot. Error message will be set in err. bool read_bootloader_message(bootloader_message* boot, std::string* err);
-/* Read bootloader message from the specified misc device into boot. */ +// Read bootloader message from the specified misc device into boot. bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device, std::string* err);
-/* Write bootloader message to BCB. */ +// Write bootloader message to BCB. bool write_bootloader_message(const bootloader_message& boot, std::string* err);
-/* Write bootloader message to the specified BCB device. */ +// Write bootloader message to the specified BCB device. bool write_bootloader_message_to(const bootloader_message& boot, const std::string& misc_blk_device, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to BCB. Will - * set the command and recovery fields, and reset the rest. */ +// Write bootloader message (boots into recovery with the options) to BCB. Will +// set the command and recovery fields, and reset the rest. bool write_bootloader_message(const std::vectorstd::string& options, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to the specific BCB device. Will - * set the command and recovery fields, and reset the rest. */ +// Write bootloader message (boots into recovery with the options) to the specific BCB device. Will +// set the command and recovery fields, and reset the rest. bool write_bootloader_message_to(const std::vectorstd::string& options, const std::string& misc_blk_device, std::string* err);
-/* Update bootloader message (boots into recovery with the options) to BCB. Will - * only update the command and recovery fields. */ +// Update bootloader message (boots into recovery with the options) to BCB. Will +// only update the command and recovery fields. bool update_bootloader_message(const std::vectorstd::string& options, std::string* err);
-/* Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update - * the command and recovery fields. */ +// Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update +// the command and recovery fields. bool update_bootloader_message_in_struct(bootloader_message* boot, const std::vectorstd::string& options);
-/* Clear BCB. */ +// Clear BCB. bool clear_bootloader_message(std::string* err);
-/* Writes the reboot-bootloader reboot reason to the bootloader_message. */ +// Writes the reboot-bootloader reboot reason to the bootloader_message. bool write_reboot_bootloader(std::string* err);
-/* Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC). */ +// Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC). bool read_wipe_package(std::string* package_data, size_t size, std::string* err);
-/* Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC). */ +// Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC). bool write_wipe_package(const std::string& package_data, std::string* err);
#else
#include <stdbool.h>
-/* C Interface. */ +// C Interface. bool write_bootloader_message(const char* options); bool write_reboot_bootloader(void);
-#endif /* ifdef __cplusplus */ - +#endif // ifdef __cplusplus #endif /* __UBOOT__ */
#endif /* __ANDROID_BOOTLOADER_MESSAGE_H */
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
On Fri, May 17, 2019 at 5:46 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Perform the following updates:
- Relocate the commit id from the file to the description of U-Boot commit. The AOSP commit is c784ce50e8c10eaf70e1f97e24e8324aef45faf5. This is done to avoid stale references in the file itself. The reasoning is in https://patchwork.ozlabs.org/patch/1098056/#2170209.
- Minimize the diff to AOSP, to decrease the effort of the next AOSP backports. The background can be found in: https://patchwork.ozlabs.org/patch/1080394/#2168454.
- Guard the static_assert() calls by #ifndef __UBOOT__ ... #endif, to avoid compilation failures of files including the header.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
v2:
- Newly pushed. No changes.
include/android_bootloader_message.h | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-)
diff --git a/include/android_bootloader_message.h b/include/android_bootloader_message.h index b84789f02227..286d7ab0f31e 100644 --- a/include/android_bootloader_message.h +++ b/include/android_bootloader_message.h @@ -2,7 +2,7 @@
- This is from the Android Project,
- Repository: https://android.googlesource.com/platform/bootable/recovery
- File: bootloader_message/include/bootloader_message/bootloader_message.h
- Commit: c784ce50e8c10eaf70e1f97e24e8324aef45faf5
- Commit: See U-Boot commit description
- Copyright (C) 2008 The Android Open Source Project
@@ -12,18 +12,24 @@ #ifndef __ANDROID_BOOTLOADER_MESSAGE_H #define __ANDROID_BOOTLOADER_MESSAGE_H
+#ifndef __UBOOT__ +#include <assert.h> +#include <stddef.h> +#include <stdint.h> +#else /* compiler.h defines the types that otherwise are included from stdint.h and
- stddef.h
*/ #include <compiler.h> +#endif
-/* Spaces used by misc partition are as below:
- 0 - 2K For bootloader_message
- 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used
as bootloader_message_ab struct)
- 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices
- Note that these offsets are admitted by bootloader,recovery and uncrypt, so they
- are not configurable without changing all of them. */
+// Spaces used by misc partition are as below: +// 0 - 2K For bootloader_message +// 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used +// as bootloader_message_ab struct) +// 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices +// Note that these offsets are admitted by bootloader,recovery and uncrypt, so they +// are not configurable without changing all of them. static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0; static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
@@ -61,17 +67,17 @@ struct bootloader_message { char status[32]; char recovery[768];
- /* The 'recovery' field used to be 1024 bytes. It has only ever
* been used to store the recovery command line, so 768 bytes* should be plenty. We carve off the last 256 bytes to store the* stage string (for multistage packages) and possible future* expansion. */
- // The 'recovery' field used to be 1024 bytes. It has only ever
- // been used to store the recovery command line, so 768 bytes
- // should be plenty. We carve off the last 256 bytes to store the
- // stage string (for multistage packages) and possible future
- // expansion. char stage[32];
- /* The 'reserved' field used to be 224 bytes when it was initially
* carved off from the 1024-byte recovery field. Bump it up to* 1184-byte so that the entire bootloader_message struct rounds up* to 2048-byte. */
- // The 'reserved' field used to be 224 bytes when it was initially
- // carved off from the 1024-byte recovery field. Bump it up to
- // 1184-byte so that the entire bootloader_message struct rounds up
- // to 2048-byte. char reserved[1184];
};
@@ -79,10 +85,12 @@ struct bootloader_message {
- We must be cautious when changing the bootloader_message struct size,
- because A/B-specific fields may end up with different offsets.
*/ +#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message) == 2048, "struct bootloader_message size changes, which may break A/B devices"); #endif +#endif /* __UBOOT__ */
/**
- The A/B-specific bootloader message structure (4-KiB).
@@ -108,7 +116,7 @@ struct bootloader_message_ab { char slot_suffix[32]; char update_channel[128];
- /* Round up the entire struct to 4096-byte. */
- // Round up the entire struct to 4096-byte. char reserved[1888];
};
@@ -116,26 +124,28 @@ struct bootloader_message_ab {
- Be cautious about the struct size change, in case we put anything post
- bootloader_message_ab struct (b/29159185).
*/ +#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message_ab) == 4096, "struct bootloader_message_ab size changes"); #endif +#endif /* __UBOOT__ */
#define BOOT_CTRL_MAGIC 0x42414342 /* Bootloader Control AB */ #define BOOT_CTRL_VERSION 1
struct slot_metadata {
- /* Slot priority with 15 meaning highest priority, 1 lowest
* priority and 0 the slot is unbootable. */
- // Slot priority with 15 meaning highest priority, 1 lowest
- // priority and 0 the slot is unbootable. uint8_t priority : 4;
- /* Number of times left attempting to boot this slot. */
- // Number of times left attempting to boot this slot. uint8_t tries_remaining : 3;
- /* 1 if this slot has booted successfully, 0 otherwise. */
- // 1 if this slot has booted successfully, 0 otherwise. uint8_t successful_boot : 1;
- /* 1 if this slot is corrupted from a dm-verity corruption, 0
* otherwise. */
- // 1 if this slot is corrupted from a dm-verity corruption, 0
- // otherwise. uint8_t verity_corrupted : 1;
- /* Reserved for further use. */
- // Reserved for further use. uint8_t reserved : 7;
} __attribute__((packed));
@@ -148,99 +158,99 @@ struct slot_metadata {
- mandatory.
*/ struct bootloader_control {
- /* NUL terminated active slot suffix. */
- // NUL terminated active slot suffix. char slot_suffix[4];
- /* Bootloader Control AB magic number (see BOOT_CTRL_MAGIC). */
- // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC). uint32_t magic;
- /* Version of struct being used (see BOOT_CTRL_VERSION). */
- // Version of struct being used (see BOOT_CTRL_VERSION). uint8_t version;
- /* Number of slots being managed. */
- // Number of slots being managed. uint8_t nb_slot : 3;
- /* Number of times left attempting to boot recovery. */
- // Number of times left attempting to boot recovery. uint8_t recovery_tries_remaining : 3;
- /* Ensure 4-bytes alignment for slot_info field. */
- // Ensure 4-bytes alignment for slot_info field. uint8_t reserved0[2];
- /* Per-slot information. Up to 4 slots. */
- // Per-slot information. Up to 4 slots. struct slot_metadata slot_info[4];
- /* Reserved for further use. */
- // Reserved for further use. uint8_t reserved1[8];
- /* CRC32 of all 28 bytes preceding this field (little endian
* format). */
- // CRC32 of all 28 bytes preceding this field (little endian
- // format). uint32_t crc32_le;
} __attribute__((packed));
+#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_control) == sizeof(((struct bootloader_message_ab *)0)->slot_suffix), "struct bootloader_control has wrong size"); #endif +#endif /* __UBOOT__ */
#ifndef __UBOOT__
#ifdef __cplusplus
#include <string> #include <vector>
-/* Return the block device name for the bootloader message partition and waits
- for the device for up to 10 seconds. In case of error returns the empty
- string. */
+// Return the block device name for the bootloader message partition and waits +// for the device for up to 10 seconds. In case of error returns the empty +// string. std::string get_bootloader_message_blk_device(std::string* err);
-/* Read bootloader message into boot. Error message will be set in err. */ +// Read bootloader message into boot. Error message will be set in err. bool read_bootloader_message(bootloader_message* boot, std::string* err);
-/* Read bootloader message from the specified misc device into boot. */ +// Read bootloader message from the specified misc device into boot. bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device, std::string* err);
-/* Write bootloader message to BCB. */ +// Write bootloader message to BCB. bool write_bootloader_message(const bootloader_message& boot, std::string* err);
-/* Write bootloader message to the specified BCB device. */ +// Write bootloader message to the specified BCB device. bool write_bootloader_message_to(const bootloader_message& boot, const std::string& misc_blk_device, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to BCB. Will
- set the command and recovery fields, and reset the rest. */
+// Write bootloader message (boots into recovery with the options) to BCB. Will +// set the command and recovery fields, and reset the rest. bool write_bootloader_message(const std::vectorstd::string& options, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to the specific BCB device. Will
- set the command and recovery fields, and reset the rest. */
+// Write bootloader message (boots into recovery with the options) to the specific BCB device. Will +// set the command and recovery fields, and reset the rest. bool write_bootloader_message_to(const std::vectorstd::string& options, const std::string& misc_blk_device, std::string* err);
-/* Update bootloader message (boots into recovery with the options) to BCB. Will
- only update the command and recovery fields. */
+// Update bootloader message (boots into recovery with the options) to BCB. Will +// only update the command and recovery fields. bool update_bootloader_message(const std::vectorstd::string& options, std::string* err);
-/* Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update
- the command and recovery fields. */
+// Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update +// the command and recovery fields. bool update_bootloader_message_in_struct(bootloader_message* boot, const std::vectorstd::string& options);
-/* Clear BCB. */ +// Clear BCB. bool clear_bootloader_message(std::string* err);
-/* Writes the reboot-bootloader reboot reason to the bootloader_message. */ +// Writes the reboot-bootloader reboot reason to the bootloader_message. bool write_reboot_bootloader(std::string* err);
-/* Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC). */ +// Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC). bool read_wipe_package(std::string* package_data, size_t size, std::string* err);
-/* Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC). */ +// Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC). bool write_wipe_package(const std::string& package_data, std::string* err);
#else
#include <stdbool.h>
-/* C Interface. */ +// C Interface. bool write_bootloader_message(const char* options); bool write_reboot_bootloader(void);
-#endif /* ifdef __cplusplus */
+#endif // ifdef __cplusplus #endif /* __UBOOT__ */
#endif /* __ANDROID_BOOTLOADER_MESSAGE_H */
2.21.0
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
On higher level, this allows implementing a subset of Android Bootloader Requirements [2], amongst which is the Android-specific bootloader flow [3]. Regardless how the latter is implemented in U-Boot ([3] being the most memorable example), reading/writing/dumping the BCB fields in the development process from inside the U-Boot is a convenient feature. Hence, make it available to the users.
Some usage examples of the new command recorded on R-Car H3ULCB-KF ('>>>' is an overlay on top of the original console output):
=> bcb bcb - Load/set/clear/test/dump/store Android BCB fields
Usage: bcb load <dev> <part> - load BCB from mmc <dev>:<part> bcb set <field> <val> - set BCB <field> to <val> bcb clear [<field>] - clear BCB <field> or all fields bcb test <field> <op> <val> - test BCB <field> against <val> bcb dump <field> - dump BCB <field> bcb store - store BCB back to mmc
Legend: <dev> - MMC device index containing the BCB partition <part> - MMC partition index or name containing the BCB <field> - one of {command,status,recovery,stage,reserved} <op> - the binary operator used in 'bcb test': '=' returns true if <val> matches the string stored in <field> '~' returns true if <val> matches a subset of <field>'s string <val> - string/text provided as input to bcb {set,test} NOTE: any ':' character in <val> will be replaced by line feed during 'bcb set' and used as separator by upper layers
=> bcb dump command Error: BCB not loaded!
Users must specify mmc device and partition before any other call
=> bcb load 1 misc => bcb load 1 1
The two calls are equivalent (assuming "misc" has index 1)
=> bcb dump command 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The output is in binary/string format for convenience The output size matches the size of inspected BCB field (32 bytes in case of 'command')
=> bcb test command = bootonce-shell && echo true true => bcb test command = bootonce-shell- && echo true => bcb test command = bootonce-shel && echo true
The '=' operator returns 'true' on perfect match
=> bcb test command ~ bootonce-shel && echo true true => bcb test command ~ bootonce-shell && echo true true
The '~' operator returns 'true' on substring match
=> bcb set command recovery => bcb dump command 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The new value is NULL-terminated and stored in the BCB field
=> bcb set recovery "msg1:msg2:msg3" => bcb dump recovery 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
--- snip --- Every ':' is replaced by line-feed '\n' (0xA). The latter is used as separator between individual commands by Android userspace
=> bcb store
Flush/store the BCB structure to MMC
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- v2: - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT. - Polished the code. Ensured no warnings returned by sparse, smatch, `cppcheck --force --enable=all --inconclusive`, make W=1. - Tested on R-Car-H3-ES20 ULCB-KF.
v1: - https://patchwork.ozlabs.org/patch/1080395/ --- cmd/Kconfig | 17 +++ cmd/Makefile | 1 + cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 cmd/bcb.c
diff --git a/cmd/Kconfig b/cmd/Kconfig index 0d36da2a5c43..495bc1052f50 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -631,6 +631,23 @@ config CMD_ADC Shows ADC device info and permit printing one-shot analog converted data from a named Analog to Digital Converter.
+config CMD_BCB + bool "bcb" + depends on MMC + depends on PARTITIONS + help + Read/modify/write the fields of Bootloader Control Block, usually + stored on the flash "misc" partition with its structure defined in: + https://android.googlesource.com/platform/bootable/recovery/+/master/ + bootloader_message/include/bootloader_message/bootloader_message.h + + Some real-life use-cases include (but are not limited to): + - Determine the "boot reason" (and act accordingly): + https://source.android.com/devices/bootloader/boot-reason + - Get/pass a list of commands from/to recovery: + https://android.googlesource.com/platform/bootable/recovery + - Inspect/dump the contents of the BCB fields + config CMD_BIND bool "bind/unbind - Bind or unbind a device to/from a driver" depends on DM diff --git a/cmd/Makefile b/cmd/Makefile index 7864fcf95c36..8f31b478eb1b 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o obj-$(CONFIG_CMD_ARMFLASH) += armflash.o obj-y += blk_common.o obj-$(CONFIG_CMD_SOURCE) += source.o +obj-$(CONFIG_CMD_BCB) += bcb.o obj-$(CONFIG_CMD_BDI) += bdinfo.o obj-$(CONFIG_CMD_BEDBUG) += bedbug.o obj-$(CONFIG_CMD_BIND) += bind.o diff --git a/cmd/bcb.c b/cmd/bcb.c new file mode 100644 index 000000000000..5d8486684542 --- /dev/null +++ b/cmd/bcb.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2019 Eugeniu Rosca rosca.eugeniu@gmail.com + * + * Command to read/modify/write Android BCB fields + */ + +#include <android_bootloader_message.h> +#include <command.h> +#include <common.h> +#include <malloc.h> + +enum bcb_cmd { + BCB_CMD_LOAD, + BCB_CMD_FIELD_SET, + BCB_CMD_FIELD_CLEAR, + BCB_CMD_FIELD_TEST, + BCB_CMD_FIELD_DUMP, + BCB_CMD_STORE, +}; + +static int bcb_dev = -1; +static int bcb_part = -1; +static struct bootloader_message bcb = { { 0 } }; + +static int bcb_cmd_get(char *cmd) +{ + if (!strncmp(cmd, "load", sizeof("load"))) + return BCB_CMD_LOAD; + if (!strncmp(cmd, "set", sizeof("set"))) + return BCB_CMD_FIELD_SET; + if (!strncmp(cmd, "clear", sizeof("clear"))) + return BCB_CMD_FIELD_CLEAR; + if (!strncmp(cmd, "test", sizeof("test"))) + return BCB_CMD_FIELD_TEST; + if (!strncmp(cmd, "store", sizeof("store"))) + return BCB_CMD_STORE; + if (!strncmp(cmd, "dump", sizeof("dump"))) + return BCB_CMD_FIELD_DUMP; + else + return -1; +} + +static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct blk_desc *desc; + disk_partition_t info; + u64 cnt; + char *endp; + int part; + + if (argc != 3) + return CMD_RET_USAGE; + + if (blk_get_device_by_str("mmc", argv[1], &desc) < 0) + goto err_1; + + part = simple_strtoul(argv[2], &endp, 0); + if (*endp == '\0') { + if (part_get_info(desc, part, &info)) + goto err_1; + } else { + part = part_get_info_by_name(desc, argv[2], &info); + if (part < 0) + goto err_1; + } + + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); + if (cnt > info.size) + goto err_2; + + if (blk_dread(desc, info.start, cnt, &bcb) != cnt) + goto err_1; + + bcb_dev = desc->devnum; + bcb_part = part; + debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part); + + return CMD_RET_SUCCESS; +err_1: + printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]); + goto err; +err_2: + printf("Error: mmc %s:%s too small!", argv[1], argv[2]); + goto err; +err: + bcb_dev = -1; + bcb_part = -1; + return CMD_RET_FAILURE; +} + +static int bcb_is_misused(int argc, char *const argv[]) +{ + if (bcb_dev < 0 || bcb_part < 0) { + printf("Error: BCB not loaded!\n"); + return -1; + } + switch (bcb_cmd_get(argv[0])) { + case BCB_CMD_LOAD: + /* Dedicated arg handling */ + break; + case BCB_CMD_FIELD_SET: + if (argc != 3) + goto err; + break; + case BCB_CMD_FIELD_TEST: + if (argc != 4) + goto err; + break; + case BCB_CMD_FIELD_CLEAR: + if (argc != 1 && argc != 2) + goto err; + break; + case BCB_CMD_STORE: + if (argc != 1) + goto err; + break; + case BCB_CMD_FIELD_DUMP: + if (argc != 2) + goto err; + break; + default: + debug("%s: Error: Unexpected BCB command\n", __func__); + return -1; + } + + return 0; +err: + printf("Error: Bad usage of 'bcb %s'\n", argv[0]); + return -1; +} + +static int +bcb_field_get(char *name, char **field, int *size) +{ + if (!strncmp(name, "command", sizeof("command"))) { + *field = bcb.command; + *size = sizeof(bcb.command); + } else if (!strncmp(name, "status", sizeof("status"))) { + *field = bcb.status; + *size = sizeof(bcb.status); + } else if (!strncmp(name, "recovery", sizeof("recovery"))) { + *field = bcb.recovery; + *size = sizeof(bcb.recovery); + } else if (!strncmp(name, "stage", sizeof("stage"))) { + *field = bcb.stage; + *size = sizeof(bcb.stage); + } else if (!strncmp(name, "reserved", sizeof("reserved"))) { + *field = bcb.reserved; + *size = sizeof(bcb.reserved); + } else { + printf("Error: Unknown field '%s'\n", name); + return -1; + } + return 0; +} + +static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size, len; + char *field, *str, *found, *keep; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + len = strlen(argv[2]); + if (len >= size) { + printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n", + argv[2], len, size, argv[1]); + return CMD_RET_FAILURE; + } + str = strdup(argv[2]); + keep = str; + + field[0] = '\0'; + while ((found = strsep(&str, ":"))) { + if (field[0] != '\0') + strcat(field, "\n"); + strcat(field, found); + } + + free(keep); + return CMD_RET_SUCCESS; +} + +static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (argc == 1) { + memset(&bcb, 0, sizeof(bcb)); + return CMD_RET_SUCCESS; + } + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + memset(field, 0, size); + return CMD_RET_SUCCESS; +} + +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16); + return CMD_RET_SUCCESS; +} + +static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + if (!strncmp(argv[2], "=", sizeof("="))) { + if (!strncmp(argv[3], field, size)) + return CMD_RET_SUCCESS; + else + return CMD_RET_FAILURE; + } else if (!strncmp(argv[2], "~", sizeof("~"))) { + if (!strstr(field, argv[3])) + return CMD_RET_FAILURE; + else + return CMD_RET_SUCCESS; + } else { + printf("Error: Unknown operator '%s'\n", argv[2]); + } + return CMD_RET_FAILURE; +} + +static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct blk_desc *desc; + disk_partition_t info; + u64 cnt; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev); + if (!desc) + goto err; + + if (part_get_info(desc, bcb_part, &info)) + goto err; + + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); + + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) + goto err; + + return CMD_RET_SUCCESS; +err: + printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part); + return CMD_RET_FAILURE; +} + +static cmd_tbl_t cmd_bcb_sub[] = { + U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""), + U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""), + U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""), + U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""), + U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""), + U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""), +}; + +static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +{ + cmd_tbl_t *c; + + if (argc < 2) + return CMD_RET_USAGE; + + argc--; + argv++; + + c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub)); + if (c) + return c->cmd(cmdtp, flag, argc, argv); + + return CMD_RET_USAGE; +} + +U_BOOT_CMD( + bcb, CONFIG_SYS_MAXARGS, 1, do_bcb, + "Load/set/clear/test/dump/store Android BCB fields", + "load <dev> <part> - load BCB from mmc <dev>:<part>\n" + "bcb set <field> <val> - set BCB <field> to <val>\n" + "bcb clear [<field>] - clear BCB <field> or all fields\n" + "bcb test <field> <op> <val> - test BCB <field> against <val>\n" + "bcb dump <field> - dump BCB <field>\n" + "bcb store - store BCB back to mmc\n" + "\n" + "Legend:\n" + "<dev> - MMC device index containing the BCB partition\n" + "<part> - MMC partition index or name containing the BCB\n" + "<field> - one of {command,status,recovery,stage,reserved}\n" + "<op> - the binary operator used in 'bcb test':\n" + " '=' returns true if <val> matches the string stored in <field>\n" + " '~' returns true if <val> matches a subset of <field>'s string\n" + "<val> - string/text provided as input to bcb {set,test}\n" + " NOTE: any ':' character in <val> will be replaced by line feed\n" + " during 'bcb set' and used as separator by upper layers\n" +);
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
On higher level, this allows implementing a subset of Android Bootloader Requirements [2], amongst which is the Android-specific bootloader flow [3]. Regardless how the latter is implemented in U-Boot ([3] being the most memorable example), reading/writing/dumping the BCB fields in the development process from inside the U-Boot is a convenient feature. Hence, make it available to the users.
Some usage examples of the new command recorded on R-Car H3ULCB-KF ('>>>' is an overlay on top of the original console output):
=> bcb bcb - Load/set/clear/test/dump/store Android BCB fields
Usage: bcb load <dev> <part> - load BCB from mmc <dev>:<part> bcb set <field> <val> - set BCB <field> to <val> bcb clear [<field>] - clear BCB <field> or all fields bcb test <field> <op> <val> - test BCB <field> against <val> bcb dump <field> - dump BCB <field> bcb store - store BCB back to mmc
Legend: <dev> - MMC device index containing the BCB partition <part> - MMC partition index or name containing the BCB <field> - one of {command,status,recovery,stage,reserved} <op> - the binary operator used in 'bcb test': '=' returns true if <val> matches the string stored in <field> '~' returns true if <val> matches a subset of <field>'s string <val> - string/text provided as input to bcb {set,test} NOTE: any ':' character in <val> will be replaced by line feed during 'bcb set' and used as separator by upper layers
=> bcb dump command Error: BCB not loaded!
Users must specify mmc device and partition before any other call
=> bcb load 1 misc => bcb load 1 1
The two calls are equivalent (assuming "misc" has index 1)
=> bcb dump command 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The output is in binary/string format for convenience The output size matches the size of inspected BCB field (32 bytes in case of 'command')
=> bcb test command = bootonce-shell && echo true true => bcb test command = bootonce-shell- && echo true => bcb test command = bootonce-shel && echo true
The '=' operator returns 'true' on perfect match
=> bcb test command ~ bootonce-shel && echo true true => bcb test command ~ bootonce-shell && echo true true
The '~' operator returns 'true' on substring match
=> bcb set command recovery => bcb dump command 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The new value is NULL-terminated and stored in the BCB field
=> bcb set recovery "msg1:msg2:msg3" => bcb dump recovery 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
--- snip --- Every ':' is replaced by line-feed '\n' (0xA). The latter is used as separator between individual commands by Android userspace
=> bcb store
Flush/store the BCB structure to MMC
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
v2:
- [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT.
- Polished the code. Ensured no warnings returned by sparse, smatch, `cppcheck --force --enable=all --inconclusive`, make W=1.
- Tested on R-Car-H3-ES20 ULCB-KF.
v1:
cmd/Kconfig | 17 +++ cmd/Makefile | 1 + cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 cmd/bcb.c
Where is this documented? Perhaps it should go in README.avb2?
Should it default to enabled if avb is used?
Regards, Simon
Hi Simon cc: Sam, Igor, feel free to correct/augment anything of below
On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
[..]
Where is this documented? Perhaps it should go in README.avb2?
README.avb2 is solely about the "verified/secure" booting of Android, while the 'bcb' command proposed in this patch can be useful both in verified boot scenarios (e.g. full-featured U-Boot builds), as well as in non-avb ones (e.g. development, PoC, demos, custom configurations). Thus, I think that README.avb2 is not the best place for 'bcb'.
More generally, as somebody who uses git as an extension of himself, I am quite happy with commit-only documentation. OTOH, for those who receive U-Boot in tarballs or expect source-level docs/tutorials, I agree that having the 'bcb' described in a README might be helpful.
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command: - doc/README.android-fastboot - doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery, but this looks like something that might well give me a better interface.
Hi Alex,
On Mon, May 20, 2019 at 08:32:28AM +0100, Alex Kiernan wrote:
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery, but this looks like something that might well give me a better interface.
Thanks for this piece of feedback. It means 'bcb' concept and implementation might span beyond the Android use-cases/needs. On small scale, this seems to approve my choice of not inserting the "android" keyword into the command or Kconfig symbol names.
Hi Alex,
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery,
I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space?
Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed.
but this looks like something that might well give me a better interface.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery,
I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space?
Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed.
It's a long story... I'm constrained by historic choices, which makes using the environment problematic. But you're right.
Hi Lukasz,
On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery,
I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space?
Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed.
That's a truly constructive suggestion. Nevertheless, I believe this would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot is built and used by the developers in our organization.
It's a long story... I'm constrained by historic choices, which makes using the environment problematic. But you're right.
-- Alex Kiernan
On Tue, 21 May 2019 11:13:52 +0200 Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Lukasz,
On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery,
I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space?
Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed.
That's a truly constructive suggestion. Nevertheless, I believe this would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot is built and used by the developers in our organization.
I don't mind to see Android's "bcd" supported in U-Boot (I'm even happy for it).
And yes - the CONFIG_ENV_IS_NOWHERE means that one loads the default envs (created at build time) to RAM for booting.
Just one note (maybe you will find it useful) - it is possible to specify the default envs from external file: https://lists.denx.de/pipermail/u-boot/2018-March/323347.html
As we don't have memory to store envs we cannot adjust or pass data via it.
It's a long story... I'm constrained by historic choices, which makes using the environment problematic. But you're right.
-- Alex Kiernan
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Lukasz,
On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski lukma@denx.de wrote:
Hi Alex,
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery,
I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space?
Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed.
That's a truly constructive suggestion. Nevertheless, I believe this would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot is built and used by the developers in our organization.
That's how we build/run too, but with with hackery like this in a boot script to pick pieces out of the legacy world:
mmc dev ${mmcdev} mmc read ${loadaddr} 1300 100 env import -c ${loadaddr} 20000 ethaddr
Yes, it's ugly...
On Tue, May 21, 2019 at 10:24:10AM +0100, Alex Kiernan wrote:
On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
That's a truly constructive suggestion. Nevertheless, I believe this would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot is built and used by the developers in our organization.
That's how we build/run too, but with with hackery like this in a boot script to pick pieces out of the legacy world:
mmc dev ${mmcdev} mmc read ${loadaddr} 1300 100 env import -c ${loadaddr} 20000 ethaddr
FWIW, we have a cascaded load of boot scripts combined with `env import -t` and/or `source`, which allows to control the boot media by changing one line in a text file. This might be super ugly in terms of vanilla standards, but it works in the development environment and that's what matters the most.
Hi Eugeniu,
On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon cc: Sam, Igor, feel free to correct/augment anything of below
On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
[..]
Where is this documented? Perhaps it should go in README.avb2?
README.avb2 is solely about the "verified/secure" booting of Android, while the 'bcb' command proposed in this patch can be useful both in verified boot scenarios (e.g. full-featured U-Boot builds), as well as in non-avb ones (e.g. development, PoC, demos, custom configurations). Thus, I think that README.avb2 is not the best place for 'bcb'.
More generally, as somebody who uses git as an extension of himself, I am quite happy with commit-only documentation. OTOH, for those who receive U-Boot in tarballs or expect source-level docs/tutorials, I agree that having the 'bcb' described in a README might be helpful.
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command:
- doc/README.android-fastboot
- doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc. Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation.
All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course.
Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels: - file/directories: we may want to move all Android related commands to sub-directory - commands: we may want to add main command called "android" for all Android-related commands, or maybe just a prefix - Kconfig: we may want to have some generic naming scheme for all Android-related commands - Documentation, tests: the same here
So at some point we will probably need to discuss and fix that somehow. Not here, of course :)
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
-- Best Regards, Eugeniu.
Anyway, from my side:
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
On Mon, May 20, 2019 at 6:16 PM Sam Protsenko semen.protsenko@linaro.org wrote:
Hi Eugeniu,
On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon cc: Sam, Igor, feel free to correct/augment anything of below
On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
[..]
Where is this documented? Perhaps it should go in README.avb2?
README.avb2 is solely about the "verified/secure" booting of Android, while the 'bcb' command proposed in this patch can be useful both in verified boot scenarios (e.g. full-featured U-Boot builds), as well as in non-avb ones (e.g. development, PoC, demos, custom configurations). Thus, I think that README.avb2 is not the best place for 'bcb'.
More generally, as somebody who uses git as an extension of himself, I am quite happy with commit-only documentation. OTOH, for those who receive U-Boot in tarballs or expect source-level docs/tutorials, I agree that having the 'bcb' described in a README might be helpful.
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command:
- doc/README.android-fastboot
- doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc. Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation.
All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course.
Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels:
- file/directories: we may want to move all Android related commands
to sub-directory
- commands: we may want to add main command called "android" for all
Android-related commands, or maybe just a prefix
- Kconfig: we may want to have some generic naming scheme for all
Android-related commands
- Documentation, tests: the same here
So at some point we will probably need to discuss and fix that somehow. Not here, of course :)
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
-- Best Regards, Eugeniu.
Hi Sam,
On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
Hi Eugeniu,
On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc.
Agreed. In my queue.
Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation.
Well, a subset of the Android bootloader requirements is publicly available via https://source.android.com/devices/bootloader, but I saw traces of the "Android Boot/Bootloader Requirements" document name mentioned in the descriptions of U-Boot commits received from our suppliers. I _do not_ have access to the latter. If anybody from Google sees this message and can share the document or a subset of it, that would be great.
To be clear, the background behind the 'bcb' command is not because I was assigned the task to implement any of the Android bootloader requirements, but because I saw improper implementations of some of those requirements in the patches received from our supplier and wanted to showcase a better/U-Boot-compliant way to accomplish those.
So, I don't give any commitment to support the Android-related parts in U-Boot, but will signal and express my opinion whenever any features backported from AOSP don't follow the patterns established in community.
All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course.
Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels:
- file/directories: we may want to move all Android related commands
to sub-directory
- commands: we may want to add main command called "android" for all
Android-related commands, or maybe just a prefix
- Kconfig: we may want to have some generic naming scheme for all
Android-related commands
- Documentation, tests: the same here
So at some point we will probably need to discuss and fix that somehow. Not here, of course :)
Agree with the most of the bullets. However, WRT merging all the available Android commands into a single command, I see room for more discussion. Here is some valuable feedback from Ruslan Trofymenko:
--- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 --- On Thu, 6 Dec 2018 at 03:31, Simon Glass sjg@chromium.org wrote:
Perhaps we need a new 'android' command with an 'ab_select' subcommand? Then the automatica abbreviation will work.
Agree with all your previous comments, will send v2 shortly. But this one I'd like to leave as is (I will drop android_ prefix though). I think adding "android" command may lead to unneeded architecture complexity in future, e.g.: - we will need to re-work next commands as sub-commands of "android" command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select command file has BSD license and other commands have GPL license) - we will need to implement sub-sub-commands (e.g. for "android dtimg dump ..." etc.) - having "android" command can be inconvenient for users and may break existing API (e.g. it will force users to use "android fastboot" instead of just "fastboot" command) - actually I don't see any namespace issues that can be solved by adding "android" command
Also, in subsequent patch, I will add the dedicated menu option for Android-related commands and will pull all existing Android commands (along with ab_select) to that menu entry.
So I hope it's fine with you if I leave this command as "ab_select"? --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
In addition, based on the feedback from Alex Kiernan, 'bcb' might be useful in non-Android contexts. If so, then embedding it as sub-command into a higher level command (e.g. 'android') does not make much sense.
Hi Eugeniu,
On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Sam,
On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
Hi Eugeniu,
On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc.
Agreed. In my queue.
Just to be clear: can we expect it to be sent in v3, or it will be separate patch-set?
Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation.
Well, a subset of the Android bootloader requirements is publicly available via https://source.android.com/devices/bootloader, but I saw traces of the "Android Boot/Bootloader Requirements" document name mentioned in the descriptions of U-Boot commits received from our suppliers. I _do not_ have access to the latter. If anybody from Google sees this message and can share the document or a subset of it, that would be great.
To be clear, the background behind the 'bcb' command is not because I was assigned the task to implement any of the Android bootloader requirements, but because I saw improper implementations of some of those requirements in the patches received from our supplier and wanted to showcase a better/U-Boot-compliant way to accomplish those.
So, I don't give any commitment to support the Android-related parts in U-Boot, but will signal and express my opinion whenever any features backported from AOSP don't follow the patterns established in community.
Sure, I just meant that it could be nice to mention it in the documentation, that 'bcb' command can help in those Google requirements implementation.
All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course.
Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels:
- file/directories: we may want to move all Android related commands
to sub-directory
- commands: we may want to add main command called "android" for all
Android-related commands, or maybe just a prefix
- Kconfig: we may want to have some generic naming scheme for all
Android-related commands
- Documentation, tests: the same here
So at some point we will probably need to discuss and fix that somehow. Not here, of course :)
Agree with the most of the bullets. However, WRT merging all the available Android commands into a single command, I see room for more discussion. Here is some valuable feedback from Ruslan Trofymenko:
--- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 --- On Thu, 6 Dec 2018 at 03:31, Simon Glass sjg@chromium.org wrote:
Perhaps we need a new 'android' command with an 'ab_select' subcommand? Then the automatica abbreviation will work.
Agree with all your previous comments, will send v2 shortly. But this one I'd like to leave as is (I will drop android_ prefix though). I think adding "android" command may lead to unneeded architecture complexity in future, e.g.:
- we will need to re-work next commands as sub-commands of "android"
command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select command file has BSD license and other commands have GPL license)
- we will need to implement sub-sub-commands (e.g. for "android dtimg
dump ..." etc.)
- having "android" command can be inconvenient for users and may
break existing API (e.g. it will force users to use "android fastboot" instead of just "fastboot" command)
- actually I don't see any namespace issues that can be solved by
adding "android" command
Also, in subsequent patch, I will add the dedicated menu option for Android-related commands and will pull all existing Android commands (along with ab_select) to that menu entry.
So I hope it's fine with you if I leave this command as "ab_select"? --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
In addition, based on the feedback from Alex Kiernan, 'bcb' might be useful in non-Android contexts. If so, then embedding it as sub-command into a higher level command (e.g. 'android') does not make much sense.
Yeah, let's keep it aside of this thread, I realize it's off-topic :) We probably just need to make sure that all Android-related commands are located in corresponding menu in menuconfig.
-- Best Regards, Eugeniu.
Hi Sam,
On Tue, May 21, 2019 at 07:46:22PM +0300, Sam Protsenko wrote:
On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
Agreed. In my queue.
Just to be clear: can we expect it to be sent in v3, or it will be separate patch-set?
We'll have a v3 for fixing Simon's review comments. I will append the docs if I have time.
Hi Eugeniu,
On Mon, 20 May 2019 at 01:23, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon cc: Sam, Igor, feel free to correct/augment anything of below
On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
[..]
Where is this documented? Perhaps it should go in README.avb2?
README.avb2 is solely about the "verified/secure" booting of Android, while the 'bcb' command proposed in this patch can be useful both in verified boot scenarios (e.g. full-featured U-Boot builds), as well as in non-avb ones (e.g. development, PoC, demos, custom configurations). Thus, I think that README.avb2 is not the best place for 'bcb'.
More generally, as somebody who uses git as an extension of himself, I am quite happy with commit-only documentation. OTOH, for those who receive U-Boot in tarballs or expect source-level docs/tutorials, I agree that having the 'bcb' described in a README might be helpful.
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command:
- doc/README.android-fastboot
- doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
How about a new README.android which links to the other two and adds your new info?
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
OK. Also is there a sandbox driver for this? We should have a test.
Regards, Simon
Hi Simon,
On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote:
On Mon, 20 May 2019 at 01:23, Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command:
- doc/README.android-fastboot
- doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
How about a new README.android which links to the other two and adds your new info?
How about below directory structure:
u-boot $ tree doc/android doc/android ├── avb2.txt ├── bcb.txt └── fastboot.txt
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
OK. Also is there a sandbox driver for this? We should have a test.
Emulating and exposing MMC devices in sandbox should be the only prerequisite for sandbox testing and this seems to be already supported via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful bringing up the MMC devices on sandbox in the past. Particularly, booting the latest sandbox U-Boot (CMD_MMC=y) I get:
=> mmc list No MMC device available
I think there is something elementary which I am missing?
Regardless, I need some more days to implement the test and repartition the README files. I think Sam would appreciate if you can provide your Ack to the series as-is (it was extensively statically and dynamically tested on R-Car H3ULCB) and I submit the doc/test updates separately. Otherwise, I will push the next revision hopefully in a week or so.
Regards, Simon
Hi Eugeniu,
On Tue, 21 May 2019 at 11:32, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon,
On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote:
On Mon, 20 May 2019 at 01:23, Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command:
- doc/README.android-fastboot
- doc/README.avb2
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
How about a new README.android which links to the other two and adds your new info?
How about below directory structure:
u-boot $ tree doc/android doc/android ├── avb2.txt ├── bcb.txt └── fastboot.txt
Sounds good
Should it default to enabled if avb is used?
I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
OK. Also is there a sandbox driver for this? We should have a test.
Emulating and exposing MMC devices in sandbox should be the only prerequisite for sandbox testing and this seems to be already supported via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful bringing up the MMC devices on sandbox in the past. Particularly, booting the latest sandbox U-Boot (CMD_MMC=y) I get:
=> mmc list No MMC device available
I think there is something elementary which I am missing?
Probably need to copy an mmc node over from test.dts to sandbox.dts
Regardless, I need some more days to implement the test and repartition the README files. I think Sam would appreciate if you can provide your Ack to the series as-is (it was extensively statically and dynamically tested on R-Car H3ULCB) and I submit the doc/test updates separately. Otherwise, I will push the next revision hopefully in a week or so.
That's OK with me, but let me review it first.
Regards, Simon
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader.
On higher level, this allows implementing a subset of Android Bootloader Requirements [2], amongst which is the Android-specific bootloader flow [3]. Regardless how the latter is implemented in U-Boot ([3] being the most memorable example), reading/writing/dumping the BCB fields in the development process from inside the U-Boot is a convenient feature. Hence, make it available to the users.
Some usage examples of the new command recorded on R-Car H3ULCB-KF ('>>>' is an overlay on top of the original console output):
=> bcb bcb - Load/set/clear/test/dump/store Android BCB fields
Usage: bcb load <dev> <part> - load BCB from mmc <dev>:<part> bcb set <field> <val> - set BCB <field> to <val> bcb clear [<field>] - clear BCB <field> or all fields bcb test <field> <op> <val> - test BCB <field> against <val> bcb dump <field> - dump BCB <field> bcb store - store BCB back to mmc
Legend: <dev> - MMC device index containing the BCB partition <part> - MMC partition index or name containing the BCB <field> - one of {command,status,recovery,stage,reserved} <op> - the binary operator used in 'bcb test': '=' returns true if <val> matches the string stored in <field> '~' returns true if <val> matches a subset of <field>'s string <val> - string/text provided as input to bcb {set,test} NOTE: any ':' character in <val> will be replaced by line feed during 'bcb set' and used as separator by upper layers
=> bcb dump command Error: BCB not loaded!
Users must specify mmc device and partition before any other call
=> bcb load 1 misc => bcb load 1 1
The two calls are equivalent (assuming "misc" has index 1)
=> bcb dump command 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The output is in binary/string format for convenience The output size matches the size of inspected BCB field (32 bytes in case of 'command')
=> bcb test command = bootonce-shell && echo true true => bcb test command = bootonce-shell- && echo true => bcb test command = bootonce-shel && echo true
The '=' operator returns 'true' on perfect match
=> bcb test command ~ bootonce-shel && echo true true => bcb test command ~ bootonce-shell && echo true true
The '~' operator returns 'true' on substring match
=> bcb set command recovery => bcb dump command 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r.............
The new value is NULL-terminated and stored in the BCB field
=> bcb set recovery "msg1:msg2:msg3" => bcb dump recovery 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
--- snip --- Every ':' is replaced by line-feed '\n' (0xA). The latter is used as separator between individual commands by Android userspace
=> bcb store
Flush/store the BCB structure to MMC
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
As discussed, please add these docs somewhere in the U-Boot tree (can be in a separate patch)
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
v2:
- [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT.
- Polished the code. Ensured no warnings returned by sparse, smatch, `cppcheck --force --enable=all --inconclusive`, make W=1.
- Tested on R-Car-H3-ES20 ULCB-KF.
v1:
cmd/Kconfig | 17 +++ cmd/Makefile | 1 + cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 cmd/bcb.c
diff --git a/cmd/Kconfig b/cmd/Kconfig index 0d36da2a5c43..495bc1052f50 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -631,6 +631,23 @@ config CMD_ADC Shows ADC device info and permit printing one-shot analog converted data from a named Analog to Digital Converter.
+config CMD_BCB
bool "bcb"depends on MMCdepends on PARTITIONShelpRead/modify/write the fields of Bootloader Control Block, usuallystored on the flash "misc" partition with its structure defined in:https://android.googlesource.com/platform/bootable/recovery/+/master/bootloader_message/include/bootloader_message/bootloader_message.hSome real-life use-cases include (but are not limited to):- Determine the "boot reason" (and act accordingly):https://source.android.com/devices/bootloader/boot-reason- Get/pass a list of commands from/to recovery:https://android.googlesource.com/platform/bootable/recovery- Inspect/dump the contents of the BCB fieldsconfig CMD_BIND bool "bind/unbind - Bind or unbind a device to/from a driver" depends on DM diff --git a/cmd/Makefile b/cmd/Makefile index 7864fcf95c36..8f31b478eb1b 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o obj-$(CONFIG_CMD_ARMFLASH) += armflash.o obj-y += blk_common.o obj-$(CONFIG_CMD_SOURCE) += source.o +obj-$(CONFIG_CMD_BCB) += bcb.o obj-$(CONFIG_CMD_BDI) += bdinfo.o obj-$(CONFIG_CMD_BEDBUG) += bedbug.o obj-$(CONFIG_CMD_BIND) += bind.o diff --git a/cmd/bcb.c b/cmd/bcb.c new file mode 100644 index 000000000000..5d8486684542 --- /dev/null +++ b/cmd/bcb.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2019 Eugeniu Rosca rosca.eugeniu@gmail.com
- Command to read/modify/write Android BCB fields
- */
+#include <android_bootloader_message.h> +#include <command.h> +#include <common.h> +#include <malloc.h>
+enum bcb_cmd {
BCB_CMD_LOAD,BCB_CMD_FIELD_SET,BCB_CMD_FIELD_CLEAR,BCB_CMD_FIELD_TEST,BCB_CMD_FIELD_DUMP,BCB_CMD_STORE,+};
+static int bcb_dev = -1; +static int bcb_part = -1; +static struct bootloader_message bcb = { { 0 } };
+static int bcb_cmd_get(char *cmd) +{
if (!strncmp(cmd, "load", sizeof("load")))return BCB_CMD_LOAD;if (!strncmp(cmd, "set", sizeof("set")))return BCB_CMD_FIELD_SET;if (!strncmp(cmd, "clear", sizeof("clear")))return BCB_CMD_FIELD_CLEAR;if (!strncmp(cmd, "test", sizeof("test")))return BCB_CMD_FIELD_TEST;if (!strncmp(cmd, "store", sizeof("store")))return BCB_CMD_STORE;if (!strncmp(cmd, "dump", sizeof("dump")))return BCB_CMD_FIELD_DUMP;elsereturn -1;+}
+static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
struct blk_desc *desc;disk_partition_t info;u64 cnt;char *endp;int part;if (argc != 3)return CMD_RET_USAGE;if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)
Error codes should be reported.
goto err_1;part = simple_strtoul(argv[2], &endp, 0);if (*endp == '\0') {if (part_get_info(desc, part, &info))goto err_1;} else {part = part_get_info_by_name(desc, argv[2], &info);if (part < 0)goto err_1;}cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);if (cnt > info.size)goto err_2;if (blk_dread(desc, info.start, cnt, &bcb) != cnt)goto err_1;bcb_dev = desc->devnum;bcb_part = part;debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part);return CMD_RET_SUCCESS;+err_1:
printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);
Add error code here.
goto err;+err_2:
printf("Error: mmc %s:%s too small!", argv[1], argv[2]);goto err;+err:
bcb_dev = -1;bcb_part = -1;return CMD_RET_FAILURE;+}
+static int bcb_is_misused(int argc, char *const argv[]) +{
if (bcb_dev < 0 || bcb_part < 0) {printf("Error: BCB not loaded!\n");return -1;}switch (bcb_cmd_get(argv[0])) {
Why have a switch() here, when you could just put the below code in each function? Or put the call to this function from the main do_bcb_set() function.
case BCB_CMD_LOAD:/* Dedicated arg handling */break;case BCB_CMD_FIELD_SET:if (argc != 3)goto err;break;case BCB_CMD_FIELD_TEST:if (argc != 4)goto err;break;case BCB_CMD_FIELD_CLEAR:if (argc != 1 && argc != 2)goto err;break;case BCB_CMD_STORE:if (argc != 1)goto err;break;case BCB_CMD_FIELD_DUMP:if (argc != 2)goto err;break;default:debug("%s: Error: Unexpected BCB command\n", __func__);return -1;}return 0;+err:
printf("Error: Bad usage of 'bcb %s'\n", argv[0]);return -1;+}
+static int +bcb_field_get(char *name, char **field, int *size) +{
if (!strncmp(name, "command", sizeof("command"))) {*field = bcb.command;*size = sizeof(bcb.command);} else if (!strncmp(name, "status", sizeof("status"))) {*field = bcb.status;*size = sizeof(bcb.status);} else if (!strncmp(name, "recovery", sizeof("recovery"))) {*field = bcb.recovery;*size = sizeof(bcb.recovery);} else if (!strncmp(name, "stage", sizeof("stage"))) {*field = bcb.stage;*size = sizeof(bcb.stage);} else if (!strncmp(name, "reserved", sizeof("reserved"))) {*field = bcb.reserved;*size = sizeof(bcb.reserved);} else {printf("Error: Unknown field '%s'\n", name);return -1;}return 0;+}
+static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
int size, len;char *field, *str, *found, *keep;if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;len = strlen(argv[2]);if (len >= size) {printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n",argv[2], len, size, argv[1]);return CMD_RET_FAILURE;}str = strdup(argv[2]);
It is OK to change the args if you like.
keep = str;field[0] = '\0';while ((found = strsep(&str, ":"))) {if (field[0] != '\0')strcat(field, "\n");strcat(field, found);}free(keep);return CMD_RET_SUCCESS;+}
+static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
int size;char *field;if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;if (argc == 1) {memset(&bcb, 0, sizeof(bcb));return CMD_RET_SUCCESS;}if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;memset(field, 0, size);return CMD_RET_SUCCESS;+}
+static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
int size;char *field;if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;
You should return CMD_RET_USAGE if there is a usage problem.
if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);
Please add newline before return
return CMD_RET_SUCCESS;+}
+static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
int size;char *field;if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;if (!strncmp(argv[2], "=", sizeof("="))) {
Think about code size:
if (*argv[2] == '=')
if (!strncmp(argv[3], field, size))return CMD_RET_SUCCESS;elsereturn CMD_RET_FAILURE;} else if (!strncmp(argv[2], "~", sizeof("~"))) {if (!strstr(field, argv[3]))return CMD_RET_FAILURE;elsereturn CMD_RET_SUCCESS;} else {printf("Error: Unknown operator '%s'\n", argv[2]);}return CMD_RET_FAILURE;+}
+static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
struct blk_desc *desc;disk_partition_t info;u64 cnt;if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev);if (!desc)goto err;if (part_get_info(desc, bcb_part, &info))goto err;
Again, error numbers need to be printed.
cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt)goto err;return CMD_RET_SUCCESS;+err:
printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part);return CMD_RET_FAILURE;+}
+static cmd_tbl_t cmd_bcb_sub[] = {
U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),+};
+static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +{
cmd_tbl_t *c;if (argc < 2)return CMD_RET_USAGE;argc--;argv++;c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));if (c)return c->cmd(cmdtp, flag, argc, argv);return CMD_RET_USAGE;+}
+U_BOOT_CMD(
bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,"Load/set/clear/test/dump/store Android BCB fields","load <dev> <part> - load BCB from mmc <dev>:<part>\n""bcb set <field> <val> - set BCB <field> to <val>\n""bcb clear [<field>] - clear BCB <field> or all fields\n""bcb test <field> <op> <val> - test BCB <field> against <val>\n""bcb dump <field> - dump BCB <field>\n""bcb store - store BCB back to mmc\n""\n""Legend:\n""<dev> - MMC device index containing the BCB partition\n""<part> - MMC partition index or name containing the BCB\n""<field> - one of {command,status,recovery,stage,reserved}\n""<op> - the binary operator used in 'bcb test':\n"" '=' returns true if <val> matches the string stored in <field>\n"" '~' returns true if <val> matches a subset of <field>'s string\n""<val> - string/text provided as input to bcb {set,test}\n"" NOTE: any ':' character in <val> will be replaced by line feed\n"" during 'bcb set' and used as separator by upper layers\n"+);
2.21.0
Regards, Simon
Hi Simon,
Thanks for the review. Would you please reply to my questions below?
On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
As discussed, please add these docs somewhere in the U-Boot tree (can be in a separate patch)
Sure.
if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)Error codes should be reported.
printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);Add error code here.
Will address.
switch (bcb_cmd_get(argv[0])) {Why have a switch() here, when you could just put the below code in each function? Or put the call to this function from the main do_bcb_set() function.
s/do_bcb_set/do_bcb/ ?
The switch() is there to tell the user that he misused specifically {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means printing full-blown help text) on _any_ kind of command misuse , we don't help the user _at all_, IMHO. I would consider being user-friendly as the higher and more important policy. However, if you prioritize the code size over user experience, then I am open to rewrite the function. Would you please clarify which policy takes precedence between the two?
str = strdup(argv[2]);It is OK to change the args if you like.
I will try getting rid of strdup.
if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;You should return CMD_RET_USAGE if there is a usage problem.
This again connects with my previous statement on the user-experience. I would like to tell the user where exactly he did the mistake in using the 'bcb' rather than throwing a full-blown help message.
if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);Please add newline before return
Fine.
if (!strncmp(argv[2], "=", sizeof("="))) {Think about code size:
if (*argv[2] == '=')
Checking a single character will not detect the difference between '=' and '=anything' So, I have to validate, in addition, that the NULL terminator is there. But I agree with the comment in general.
if (part_get_info(desc, bcb_part, &info))goto err;Again, error numbers need to be printed.
Will address.
Regards, Simon
Thanks!
Hi Eugeniu,
On Wed, 22 May 2019 at 01:11, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon,
Thanks for the review. Would you please reply to my questions below?
On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
Hi Eugeniu,
On Fri, 17 May 2019 at 08:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
[1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
As discussed, please add these docs somewhere in the U-Boot tree (can be in a separate patch)
Sure.
if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)Error codes should be reported.
printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);Add error code here.
Will address.
switch (bcb_cmd_get(argv[0])) {Why have a switch() here, when you could just put the below code in each function? Or put the call to this function from the main do_bcb_set() function.
s/do_bcb_set/do_bcb/ ?
Yes
The switch() is there to tell the user that he misused specifically {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means printing full-blown help text) on _any_ kind of command misuse , we don't help the user _at all_, IMHO. I would consider being user-friendly as the higher and more important policy. However, if you prioritize the code size over user experience, then I am open to rewrite the function. Would you please clarify which policy takes precedence between the two?
I think code size in commands is not the major priority, although we do tend to try to keep it moderate.
Here I wasn't suggesting removing code. I was just suggesting that the error handling could be in each specific command's function. So take the code out of each case statement and put into the function that it relates to.
Or continue to use the generic error handler, but call it from the generic function. It seems like we have:
do_bcb() { switch (cmd) { case CMD_FRED do_bcb_fred(); ... } ... }
do_bcb_fred() { check_args(CMD_FRED); ... }
check_args(int cmd) { switch (cmd) { case CMD_FRED: print error } }
I mean, put 'print error' inside do_bcb_fred()
or, call check_args() from do_bcb()
str = strdup(argv[2]);It is OK to change the args if you like.
I will try getting rid of strdup.
if (bcb_is_misused(argc, argv))return CMD_RET_FAILURE;You should return CMD_RET_USAGE if there is a usage problem.
This again connects with my previous statement on the user-experience. I would like to tell the user where exactly he did the mistake in using the 'bcb' rather than throwing a full-blown help message.
Yes this is good. See above where I tried to explain what I mean.
if (bcb_field_get(argv[1], &field, &size))return CMD_RET_FAILURE;print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);Please add newline before return
Fine.
if (!strncmp(argv[2], "=", sizeof("="))) {Think about code size:
if (*argv[2] == '=')
Checking a single character will not detect the difference between '=' and '=anything' So, I have to validate, in addition, that the NULL terminator is there. But I agree with the comment in general.
Yes, understood (I would calll this 'nul', BTW)
[..]
Regards, Simon
Hello Simon,
I am really grateful for your review comments.
I think I tackled all of them in https://patchwork.ozlabs.org/cover/1104242/ ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
I would appreciate if you can have one more/final look.
Hi All,
cc: Stephen, Jeremy
FWIW/jFYI, the patchwork frontend appears to mangle/skip spaces in the patch subjects. Examples: - https://patchwork.ozlabs.org/cover/1101106/ - https://patchwork.ozlabs.org/patch/1101108/ - https://patchwork.ozlabs.org/patch/1101107/
The same patches look fine on https://marc.info: - https://marc.info/?l=u-boot&m=155810440206070&w=2 - https://marc.info/?l=u-boot&m=155810448306089&w=2 - https://marc.info/?l=u-boot&m=155810444306080&w=2
On Fri, 2019-05-17 at 17:24 +0200, Eugeniu Rosca wrote:
Hi All,
cc: Stephen, Jeremy
FWIW/jFYI, the patchwork frontend appears to mangle/skip spaces in the patch subjects. Examples:
- https://patchwork.ozlabs.org/cover/1101106/
- https://patchwork.ozlabs.org/patch/1101108/
- https://patchwork.ozlabs.org/patch/1101107/
The same patches look fine on https://marc.info:
http://patchwork.ozlabs.org/patch/1099264/
We're just waiting for ozlabs to apply that patch.
Stephen
On Fri, May 17, 2019 at 04:26:23PM +0100, Stephen Finucane wrote: [..]
http://patchwork.ozlabs.org/patch/1099264/
We're just waiting for ozlabs to apply that patch.
Stephen
Thanks!
Hello Stephen,
On Fri, May 17, 2019 at 05:28:25PM +0200, Eugeniu Rosca wrote:
On Fri, May 17, 2019 at 04:26:23PM +0100, Stephen Finucane wrote: [..]
http://patchwork.ozlabs.org/patch/1099264/
We're just waiting for ozlabs to apply that patch.
Stephen
Thanks!
jFYI/FWIW, a recent patch series no longer suffers from this problem: https://patchwork.ozlabs.org/cover/1104242/ ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
However, the old series still exhibits the issue, as if the old series is rendered by the old patchwork version: https://patchwork.ozlabs.org/cover/1101106/ ("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")
Hi Eugeniu,
jFYI/FWIW, a recent patch series no longer suffers from this problem: https://patchwork.ozlabs.org/cover/1104242/ ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
OK, super. I'd applied the fix for that issue on patchwork.ozlabs.org last week.
However, the old series still exhibits the issue, as if the old series is rendered by the old patchwork version: https://patchwork.ozlabs.org/cover/1101106/ ("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")
The issue was with the parser. Since the patch was parsed before the fix was applied, it'll remain like that.
Cheers,
Jeremy
Hi Jeremy,
On Sat, May 25, 2019 at 07:37:06AM +0800, Jeremy Kerr wrote:
Hi Eugeniu,
jFYI/FWIW, a recent patch series no longer suffers from this problem: https://patchwork.ozlabs.org/cover/1104242/ ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
OK, super. I'd applied the fix for that issue on patchwork.ozlabs.org last week.
Awesome. Thanks!
However, the old series still exhibits the issue, as if the old series is rendered by the old patchwork version: https://patchwork.ozlabs.org/cover/1101106/ ("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")
The issue was with the parser. Since the patch was parsed before the fix was applied, it'll remain like that.
Good to know :)
Superseded by https://patchwork.ozlabs.org/cover/1104242/ ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
participants (8)
-
Alex Kiernan -
Eugeniu Rosca -
Eugeniu Rosca -
Jeremy Kerr -
Lukasz Majewski -
Sam Protsenko -
Simon Glass -
Stephen Finucane