[PATCH v2 0/7] AVB: cosmetic adjustments/improvements

[PATCH v2 0/7] AVB: cosmetic adjustments/improvements

[Igor Opaniuk]

This is the first patch series for incoming major
improvements for AVB implementation, that include:
- Simplify and add more context for debug/error prints where it's needed.
- Move SPDX license identifiers to the first line, so it conforms
  to license placement rule.
- Use mmc_switch_part() only for eMMC.
- Rework do_avb_verify_part, take into account device lock state for setting correct
  androidboot.verifiedbootstate.
- Adjust AVB documentation, sync usage info with the one cmd/avb.c.

Igor Opaniuk (7):
  common: avb_verify: don't call mmc_switch_part for SD
  avb: move SPDX license identifiers to the first line
  common: avb_verify: rework error/debug prints
  cmd: avb: rework prints
  common: avb_verify: add str_avb_io_error/str_avb_slot_error
  cmd: avb: rework do_avb_verify_part
  doc: android: avb: sync usage details

 cmd/avb.c                              | 173 +++++++++++++------------
 common/avb_verify.c                    |  89 ++++++++++---
 doc/android/avb2.rst                   |  16 ++-
 include/avb_verify.h                   |   7 +-
 test/py/tests/test_android/test_avb.py |   3 +-
 5 files changed, 178 insertions(+), 110 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

mmc_switch_part() is used for switching between hw partitions
on eMMC (boot0, boot1, user, rpmb).
There is no need to do that for SD card.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Mattijs Korpershoek R-b tag applied

 common/avb_verify.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 48ba8db51e5..59f2c25e0de 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -358,9 +358,11 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
 		goto err;
 	}
 
-	ret = mmc_switch_part(part->mmc, part_num);
-	if (ret)
-		goto err;
+	if (IS_MMC(part->mmc)) {
+		ret = mmc_switch_part(part->mmc, part_num);
+		if (ret)
+			goto err;
+	}
 
 	mmc_blk = mmc_get_blk_desc(part->mmc);
 	if (!mmc_blk) {
-- 
2.34.1

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Dragan Simic]

On 2024-02-09 20:20, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@gmail.com>
> 
> mmc_switch_part() is used for switching between hw partitions
> on eMMC (boot0, boot1, user, rpmb).
> There is no need to do that for SD card.
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Looking good to me.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
> 
> Changes in v2:
> - Mattijs Korpershoek R-b tag applied
> 
>  common/avb_verify.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 48ba8db51e5..59f2c25e0de 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -358,9 +358,11 @@ static struct mmc_part *get_partition(AvbOps
> *ops, const char *partition)
>  		goto err;
>  	}
> 
> -	ret = mmc_switch_part(part->mmc, part_num);
> -	if (ret)
> -		goto err;
> +	if (IS_MMC(part->mmc)) {
> +		ret = mmc_switch_part(part->mmc, part_num);
> +		if (ret)
> +			goto err;
> +	}
> 
>  	mmc_blk = mmc_get_blk_desc(part->mmc);
>  	if (!mmc_blk) {

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Dan Carpenter]

On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@gmail.com>
> 
> mmc_switch_part() is used for switching between hw partitions
> on eMMC (boot0, boot1, user, rpmb).
> There is no need to do that for SD card.
> 

Is this a clean up or a bugfix?  What are the visible effects for the
user?

regards,
dan carpenter

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Igor Opaniuk]

Hi Dan,

On Mon, Feb 12, 2024 at 8:05 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote:
> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
> >
> > mmc_switch_part() is used for switching between hw partitions
> > on eMMC (boot0, boot1, user, rpmb).
> > There is no need to do that for SD card.
> >
>
> Is this a clean up or a bugfix?  What are the visible effects for the
> user?
avb cmd fails for SD cards, as mmc_switch_part() fails after trying to access
EXT_CSD register, which obviously is not available.
>
> regards,
> dan carpenter
>


-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
http://ua.linkedin.com/in/iopaniuk

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Mattijs Korpershoek]

Hi Igor,

On lun., févr. 12, 2024 at 09:05, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:

> Hi Dan,
>
> On Mon, Feb 12, 2024 at 8:05 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>
>> On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote:
>> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
>> >
>> > mmc_switch_part() is used for switching between hw partitions
>> > on eMMC (boot0, boot1, user, rpmb).
>> > There is no need to do that for SD card.
>> >
>>
>> Is this a clean up or a bugfix?  What are the visible effects for the
>> user?
> avb cmd fails for SD cards, as mmc_switch_part() fails after trying to access
> EXT_CSD register, which obviously is not available.

Does this means that we only need this patch to fix AVB commands when
booting from SD cards?

If yes, I propose adding the following note to the commit message:

"This fixes the avb command usage on on SD cards."

If you agree, I can do this while applying.

If not, we can keep the message as is.

>>
>> regards,
>> dan carpenter
>>
>
>
> -- 
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> http://ua.linkedin.com/in/iopaniuk

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Igor Opaniuk]

Hi Mattijs,

On Tue, Feb 13, 2024 at 9:13 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Igor,
>
> On lun., févr. 12, 2024 at 09:05, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> > Hi Dan,
> >
> > On Mon, Feb 12, 2024 at 8:05 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >>
> >> On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote:
> >> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
> >> >
> >> > mmc_switch_part() is used for switching between hw partitions
> >> > on eMMC (boot0, boot1, user, rpmb).
> >> > There is no need to do that for SD card.
> >> >
> >>
> >> Is this a clean up or a bugfix?  What are the visible effects for the
> >> user?
> > avb cmd fails for SD cards, as mmc_switch_part() fails after trying to access
> > EXT_CSD register, which obviously is not available.
>
> Does this means that we only need this patch to fix AVB commands when
> booting from SD cards?
>
> If yes, I propose adding the following note to the commit message:
>
> "This fixes the avb command usage on on SD cards."
>
> If you agree, I can do this while applying.
Yes, could you please add to the commit message so I don't
send v3 for that (if there are no any additional objections/comments)

Thanks
>
> If not, we can keep the message as is.
>
> >>
> >> regards,
> >> dan carpenter
> >>
> >
> >
> > --
> > Best regards - Atentamente - Meilleures salutations
> >
> > Igor Opaniuk
> >
> > mailto: igor.opaniuk@gmail.com
> > skype: igor.opanyuk
> > http://ua.linkedin.com/in/iopaniuk



-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
http://ua.linkedin.com/in/iopaniuk

Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD

[Mattijs Korpershoek]

Hi Igor,

On mar., févr. 13, 2024 at 12:19, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:

> Hi Mattijs,
>
> On Tue, Feb 13, 2024 at 9:13 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Igor,
>>
>> On lun., févr. 12, 2024 at 09:05, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>>
>> > Hi Dan,
>> >
>> > On Mon, Feb 12, 2024 at 8:05 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>> >>
>> >> On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote:
>> >> > From: Igor Opaniuk <igor.opaniuk@gmail.com>
>> >> >
>> >> > mmc_switch_part() is used for switching between hw partitions
>> >> > on eMMC (boot0, boot1, user, rpmb).
>> >> > There is no need to do that for SD card.
>> >> >
>> >>
>> >> Is this a clean up or a bugfix?  What are the visible effects for the
>> >> user?
>> > avb cmd fails for SD cards, as mmc_switch_part() fails after trying to access
>> > EXT_CSD register, which obviously is not available.
>>
>> Does this means that we only need this patch to fix AVB commands when
>> booting from SD cards?
>>
>> If yes, I propose adding the following note to the commit message:
>>
>> "This fixes the avb command usage on on SD cards."
>>
>> If you agree, I can do this while applying.
> Yes, could you please add to the commit message so I don't
> send v3 for that (if there are no any additional objections/comments)

There are no additional objections, comments on my end.

I will add this to the commit message when applying.

>
> Thanks
>>
>> If not, we can keep the message as is.
>>
>> >>
>> >> regards,
>> >> dan carpenter
>> >>
>> >
>> >
>> > --
>> > Best regards - Atentamente - Meilleures salutations
>> >
>> > Igor Opaniuk
>> >
>> > mailto: igor.opaniuk@gmail.com
>> > skype: igor.opanyuk
>> > http://ua.linkedin.com/in/iopaniuk
>
>
>
> -- 
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> http://ua.linkedin.com/in/iopaniuk

[PATCH v2 2/7] avb: move SPDX license identifiers to the first line

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Move SPDX license identifiers to the first line, so it conforms
to license placement rule [1]:

Placement:
The SPDX license identifier in kernel files shall be added at the first
possible line in a file which can contain a comment.  For the majority
of files this is the first line, except for scripts which require the
'#!PATH_TO_INTERPRETER' in the first line.  For those scripts the SPDX
identifier goes into the second line.

[1] https://www.kernel.org/doc/Documentation/process/license-rules.rst
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Fix typo in commit message
- Mattijs Korpershoek R-b tag applied

 cmd/avb.c                              | 4 +---
 common/avb_verify.c                    | 3 +--
 include/avb_verify.h                   | 4 +---
 test/py/tests/test_android/test_avb.py | 3 +--
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index 783f51b8169..ce8b63873f2 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -1,8 +1,6 @@
-
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #include <avb_verify.h>
diff --git a/common/avb_verify.c b/common/avb_verify.c
index 59f2c25e0de..938a5383b5d 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -1,7 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #include <avb_verify.h>
diff --git a/include/avb_verify.h b/include/avb_verify.h
index 1e787ba6668..2fb850044d9 100644
--- a/include/avb_verify.h
+++ b/include/avb_verify.h
@@ -1,8 +1,6 @@
-
+/* SPDX-License-Identifier: GPL-2.0+ */
 /*
  * (C) Copyright 2018, Linaro Limited
- *
- * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #ifndef	_AVB_VERIFY_H
diff --git a/test/py/tests/test_android/test_avb.py b/test/py/tests/test_android/test_avb.py
index 238b48c90fa..865efbca4de 100644
--- a/test/py/tests/test_android/test_avb.py
+++ b/test/py/tests/test_android/test_avb.py
@@ -1,6 +1,5 @@
-# Copyright (c) 2018, Linaro Limited
-#
 # SPDX-License-Identifier:  GPL-2.0+
+# Copyright (c) 2018, Linaro Limited
 #
 # Android Verified Boot 2.0 Test
 
-- 
2.34.1

[PATCH v2 3/7] common: avb_verify: rework error/debug prints

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Make error prints more verbose with additional context.
Also s/print/debug/g for prints, which might be relevant only
for debugging purposes.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Mattijs Korpershoek R-b tag applied

 common/avb_verify.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 938a5383b5d..ed58239cf8a 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -279,9 +279,9 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part,
 	 * Reading fails on unaligned buffers, so we have to
 	 * use aligned temporary buffer and then copy to destination
 	 */
-
 	if (unaligned) {
-		printf("Handling unaligned read buffer..\n");
+		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
+		      __func__, buffer);
 		tmp_buf = get_sector_buf();
 		buf_size = get_sector_buf_size();
 		if (sectors > buf_size / part->info.blksz)
@@ -320,7 +320,8 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start,
 	if (unaligned) {
 		tmp_buf = get_sector_buf();
 		buf_size = get_sector_buf_size();
-		printf("Handling unaligned wrire buffer..\n");
+		debug("%s: handling unaligned read buffer, addr = 0x%p\n",
+		      __func__, buffer);
 		if (sectors > buf_size / part->info.blksz)
 			sectors = buf_size / part->info.blksz;
 
@@ -348,30 +349,35 @@ static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
 	dev_num = get_boot_device(ops);
 	part->mmc = find_mmc_device(dev_num);
 	if (!part->mmc) {
-		printf("No MMC device at slot %x\n", dev_num);
+		printf("%s: no MMC device at slot %x\n", __func__, dev_num);
 		goto err;
 	}
 
-	if (mmc_init(part->mmc)) {
-		printf("MMC initialization failed\n");
+	ret = mmc_init(part->mmc);
+	if (ret) {
+		printf("%s: MMC initialization failed, err = %d\n",
+		       __func__, ret);
 		goto err;
 	}
 
 	if (IS_MMC(part->mmc)) {
 		ret = mmc_switch_part(part->mmc, part_num);
-		if (ret)
+		if (ret) {
+			printf("%s: MMC part switch failed, err = %d\n",
+			       __func__, ret);
 			goto err;
+		}
 	}
 
 	mmc_blk = mmc_get_blk_desc(part->mmc);
 	if (!mmc_blk) {
-		printf("Error - failed to obtain block descriptor\n");
+		printf("%s: failed to obtain block descriptor\n", __func__);
 		goto err;
 	}
 
 	ret = part_get_info_by_name(mmc_blk, partition, &part->info);
 	if (ret < 0) {
-		printf("Can't find partition '%s'\n", partition);
+		printf("%s: can't find partition '%s'\n", __func__, partition);
 		goto err;
 	}
 
@@ -684,7 +690,7 @@ static AvbIOResult read_rollback_index(AvbOps *ops,
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now we always return 0 as the stored rollback index. */
-	printf("%s not supported yet\n", __func__);
+	debug("%s: rollback protection is not implemented\n", __func__);
 
 	if (out_rollback_index)
 		*out_rollback_index = 0;
@@ -730,7 +736,7 @@ static AvbIOResult write_rollback_index(AvbOps *ops,
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now this is a no-op. */
-	printf("%s not supported yet\n", __func__);
+	debug("%s: rollback protection is not implemented\n", __func__);
 
 	return AVB_IO_RESULT_OK;
 #else
@@ -766,8 +772,7 @@ static AvbIOResult read_is_device_unlocked(AvbOps *ops, bool *out_is_unlocked)
 {
 #ifndef CONFIG_OPTEE_TA_AVB
 	/* For now we always return that the device is unlocked. */
-
-	printf("%s not supported yet\n", __func__);
+	debug("%s: device locking is not implemented\n", __func__);
 
 	*out_is_unlocked = true;
 
-- 
2.34.1

[PATCH v2 4/7] cmd: avb: rework prints

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Simplify and add more context for prints where it's needed.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Drop AVB_OPS_CHECK macro and leave previous check

 cmd/avb.c | 123 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 48 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index ce8b63873f2..62a3ee18e7f 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -11,6 +11,7 @@
 #include <mmc.h>
 
 #define AVB_BOOTARGS	"avb_bootargs"
+
 static struct AvbOps *avb_ops;
 
 int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
@@ -28,8 +29,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	avb_ops = avb_ops_alloc(mmc_dev);
 	if (avb_ops)
 		return CMD_RET_SUCCESS;
+	else
+		printf("Can't allocate AvbOps");
 
-	printf("Failed to initialize avb2\n");
+	printf("Failed to initialize AVB\n");
 
 	return CMD_RET_FAILURE;
 }
@@ -41,10 +44,11 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes, bytes_read = 0;
 	void *buffer;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
-		return CMD_RET_USAGE;
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
+		return CMD_RET_FAILURE;
 	}
 
 	if (argc != 5)
@@ -55,14 +59,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	bytes = hextoul(argv[3], NULL);
 	buffer = (void *)hextoul(argv[4], NULL);
 
-	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
-					 buffer, &bytes_read) ==
-					 AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_from_partition(avb_ops, part, offset,
+					   bytes, buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Read %zu bytes\n", bytes_read);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read from partition\n");
+	printf("Failed to read from partition '%s', err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -74,10 +79,11 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes, bytes_read = 0;
 	char *buffer;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
-		return CMD_RET_USAGE;
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
+		return CMD_RET_FAILURE;
 	}
 
 	if (argc != 4)
@@ -94,8 +100,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 	}
 	memset(buffer, 0, bytes);
 
-	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
-					 &bytes_read) == AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_from_partition(avb_ops, part, offset,
+					   bytes, buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
 		printf("Data: ");
 		for (int i = 0; i < bytes_read; i++)
@@ -107,7 +114,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read from partition\n");
+	printf("Failed to read from partition '%s', err = %d\n",
+	       part, ret);
 
 	free(buffer);
 	return CMD_RET_FAILURE;
@@ -120,9 +128,10 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	s64 offset;
 	size_t bytes;
 	void *buffer;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -134,13 +143,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
 	bytes = hextoul(argv[3], NULL);
 	buffer = (void *)hextoul(argv[4], NULL);
 
-	if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->write_to_partition(avb_ops, part, offset,
+					  bytes, buffer);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Wrote %zu bytes\n", bytes);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to write in partition\n");
+	printf("Failed to write in partition '%s', err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -150,9 +161,10 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	size_t index;
 	u64 rb_idx;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -161,13 +173,14 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	index = (size_t)hextoul(argv[1], NULL);
 
-	if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Rollback index: %llx\n", rb_idx);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read rollback index\n");
+	printf("Failed to read rollback index id = %zu, err = %d\n",
+	       index, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -177,9 +190,10 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	size_t index;
 	u64 rb_idx;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -189,11 +203,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
 	index = (size_t)hextoul(argv[1], NULL);
 	rb_idx = hextoul(argv[2], NULL);
 
-	if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
-	    AVB_IO_RESULT_OK)
+	ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx);
+	if (ret == AVB_IO_RESULT_OK)
 		return CMD_RET_SUCCESS;
 
-	printf("Failed to write rollback index\n");
+	printf("Failed to write rollback index id = %zu, err = %d\n",
+	       index, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -203,9 +218,10 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
 {
 	const char *part;
 	char buffer[UUID_STR_LEN + 1];
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -214,14 +230,16 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
 
 	part = argv[1];
 
-	if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
-						   UUID_STR_LEN + 1) ==
-						   AVB_IO_RESULT_OK) {
+	ret = avb_ops->get_unique_guid_for_partition(avb_ops, part,
+						     buffer,
+						     UUID_STR_LEN + 1);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("'%s' UUID: %s\n", part, buffer);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read UUID\n");
+	printf("Failed to read partition '%s' UUID, err = %d\n",
+	       part, ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -235,12 +253,13 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	char *cmdline;
 	char *extra_args;
 	char *slot_suffix = "";
+	int ret;
 
 	bool unlocked = false;
 	int res = CMD_RET_FAILURE;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -253,9 +272,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	printf("## Android Verified Boot 2.0 version %s\n",
 	       avb_version_string());
 
-	if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
-	    AVB_IO_RESULT_OK) {
-		printf("Can't determine device lock state.\n");
+	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
+	if (ret != AVB_IO_RESULT_OK) {
+		printf("Can't determine device lock state, err = %d\n",
+		       ret);
 		return CMD_RET_FAILURE;
 	}
 
@@ -302,10 +322,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 		printf("Corrupted dm-verity metadata detected\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
-		printf("Unsupported version avbtool was used\n");
+		printf("Unsupported version of avbtool was used\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
-		printf("Checking rollback index failed\n");
+		printf("Rollback index check failed\n");
 		break;
 	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
 		printf("Public key was rejected\n");
@@ -324,9 +344,10 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
 		       int argc, char *const argv[])
 {
 	bool unlock;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -335,13 +356,14 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
 		return CMD_RET_USAGE;
 	}
 
-	if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Unlocked = %d\n", unlock);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Can't determine device lock state.\n");
+	printf("Can't determine device lock state, err = %d\n",
+	       ret);
 
 	return CMD_RET_FAILURE;
 }
@@ -354,9 +376,10 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	size_t bytes_read;
 	void *buffer;
 	char *endp;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -372,15 +395,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!buffer)
 		return CMD_RET_FAILURE;
 
-	if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
-					   &bytes_read) == AVB_IO_RESULT_OK) {
+	ret = avb_ops->read_persistent_value(avb_ops, name, bytes,
+					     buffer, &bytes_read);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Read %zu bytes, value = %s\n", bytes_read,
 		       (char *)buffer);
 		free(buffer);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to read persistent value\n");
+	printf("Failed to read persistent value, err = %d\n", ret);
 
 	free(buffer);
 
@@ -392,9 +416,10 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	const char *name;
 	const char *value;
+	int ret;
 
 	if (!avb_ops) {
-		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
+		printf("AVB is not initialized, please run 'avb init <id>'\n");
 		return CMD_RET_FAILURE;
 	}
 
@@ -404,14 +429,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
 	name = argv[1];
 	value = argv[2];
 
-	if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
-					    (const uint8_t *)value) ==
-	    AVB_IO_RESULT_OK) {
+	ret = avb_ops->write_persistent_value(avb_ops, name,
+					      strlen(value) + 1,
+					      (const uint8_t *)value);
+	if (ret == AVB_IO_RESULT_OK) {
 		printf("Wrote %zu bytes\n", strlen(value) + 1);
 		return CMD_RET_SUCCESS;
 	}
 
-	printf("Failed to write persistent value\n");
+	printf("Failed to write persistent value `%s` = `%s`, err = %d\n",
+	       name, value, ret);
 
 	return CMD_RET_FAILURE;
 }
-- 
2.34.1

Re: [PATCH v2 4/7] cmd: avb: rework prints

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On ven., févr. 09, 2024 at 20:20, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Simplify and add more context for prints where it's needed.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
> Changes in v2:
> - Drop AVB_OPS_CHECK macro and leave previous check
>
>  cmd/avb.c | 123 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 75 insertions(+), 48 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index ce8b63873f2..62a3ee18e7f 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -11,6 +11,7 @@
>  #include <mmc.h>
>  
>  #define AVB_BOOTARGS	"avb_bootargs"
> +
>  static struct AvbOps *avb_ops;
>  
>  int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> @@ -28,8 +29,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	avb_ops = avb_ops_alloc(mmc_dev);
>  	if (avb_ops)
>  		return CMD_RET_SUCCESS;
> +	else
> +		printf("Can't allocate AvbOps");
>  
> -	printf("Failed to initialize avb2\n");
> +	printf("Failed to initialize AVB\n");
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -41,10 +44,11 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes, bytes_read = 0;
>  	void *buffer;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> -		return CMD_RET_USAGE;
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
> +		return CMD_RET_FAILURE;
>  	}
>  
>  	if (argc != 5)
> @@ -55,14 +59,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	bytes = hextoul(argv[3], NULL);
>  	buffer = (void *)hextoul(argv[4], NULL);
>  
> -	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
> -					 buffer, &bytes_read) ==
> -					 AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_from_partition(avb_ops, part, offset,
> +					   bytes, buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Read %zu bytes\n", bytes_read);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read from partition\n");
> +	printf("Failed to read from partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -74,10 +79,11 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes, bytes_read = 0;
>  	char *buffer;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> -		return CMD_RET_USAGE;
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
> +		return CMD_RET_FAILURE;
>  	}
>  
>  	if (argc != 4)
> @@ -94,8 +100,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  	}
>  	memset(buffer, 0, bytes);
>  
> -	if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
> -					 &bytes_read) == AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_from_partition(avb_ops, part, offset,
> +					   bytes, buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
>  		printf("Data: ");
>  		for (int i = 0; i < bytes_read; i++)
> @@ -107,7 +114,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read from partition\n");
> +	printf("Failed to read from partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	free(buffer);
>  	return CMD_RET_FAILURE;
> @@ -120,9 +128,10 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	s64 offset;
>  	size_t bytes;
>  	void *buffer;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -134,13 +143,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  	bytes = hextoul(argv[3], NULL);
>  	buffer = (void *)hextoul(argv[4], NULL);
>  
> -	if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->write_to_partition(avb_ops, part, offset,
> +					  bytes, buffer);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Wrote %zu bytes\n", bytes);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to write in partition\n");
> +	printf("Failed to write in partition '%s', err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -150,9 +161,10 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	size_t index;
>  	u64 rb_idx;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -161,13 +173,14 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  
>  	index = (size_t)hextoul(argv[1], NULL);
>  
> -	if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Rollback index: %llx\n", rb_idx);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read rollback index\n");
> +	printf("Failed to read rollback index id = %zu, err = %d\n",
> +	       index, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -177,9 +190,10 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	size_t index;
>  	u64 rb_idx;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -189,11 +203,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, int argc,
>  	index = (size_t)hextoul(argv[1], NULL);
>  	rb_idx = hextoul(argv[2], NULL);
>  
> -	if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
> -	    AVB_IO_RESULT_OK)
> +	ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx);
> +	if (ret == AVB_IO_RESULT_OK)
>  		return CMD_RET_SUCCESS;
>  
> -	printf("Failed to write rollback index\n");
> +	printf("Failed to write rollback index id = %zu, err = %d\n",
> +	       index, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -203,9 +218,10 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
>  {
>  	const char *part;
>  	char buffer[UUID_STR_LEN + 1];
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -214,14 +230,16 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
>  
>  	part = argv[1];
>  
> -	if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
> -						   UUID_STR_LEN + 1) ==
> -						   AVB_IO_RESULT_OK) {
> +	ret = avb_ops->get_unique_guid_for_partition(avb_ops, part,
> +						     buffer,
> +						     UUID_STR_LEN + 1);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("'%s' UUID: %s\n", part, buffer);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read UUID\n");
> +	printf("Failed to read partition '%s' UUID, err = %d\n",
> +	       part, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -235,12 +253,13 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  	char *cmdline;
>  	char *extra_args;
>  	char *slot_suffix = "";
> +	int ret;
>  
>  	bool unlocked = false;
>  	int res = CMD_RET_FAILURE;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -253,9 +272,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  	printf("## Android Verified Boot 2.0 version %s\n",
>  	       avb_version_string());
>  
> -	if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
> -	    AVB_IO_RESULT_OK) {
> -		printf("Can't determine device lock state.\n");
> +	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
> +	if (ret != AVB_IO_RESULT_OK) {
> +		printf("Can't determine device lock state, err = %d\n",
> +		       ret);
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -302,10 +322,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
>  		printf("Corrupted dm-verity metadata detected\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> -		printf("Unsupported version avbtool was used\n");
> +		printf("Unsupported version of avbtool was used\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> -		printf("Checking rollback index failed\n");
> +		printf("Rollback index check failed\n");
>  		break;
>  	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
>  		printf("Public key was rejected\n");
> @@ -324,9 +344,10 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
>  		       int argc, char *const argv[])
>  {
>  	bool unlock;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -335,13 +356,14 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
>  		return CMD_RET_USAGE;
>  	}
>  
> -	if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Unlocked = %d\n", unlock);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Can't determine device lock state.\n");
> +	printf("Can't determine device lock state, err = %d\n",
> +	       ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> @@ -354,9 +376,10 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	size_t bytes_read;
>  	void *buffer;
>  	char *endp;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -372,15 +395,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	if (!buffer)
>  		return CMD_RET_FAILURE;
>  
> -	if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer,
> -					   &bytes_read) == AVB_IO_RESULT_OK) {
> +	ret = avb_ops->read_persistent_value(avb_ops, name, bytes,
> +					     buffer, &bytes_read);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Read %zu bytes, value = %s\n", bytes_read,
>  		       (char *)buffer);
>  		free(buffer);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to read persistent value\n");
> +	printf("Failed to read persistent value, err = %d\n", ret);
>  
>  	free(buffer);
>  
> @@ -392,9 +416,10 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  {
>  	const char *name;
>  	const char *value;
> +	int ret;
>  
>  	if (!avb_ops) {
> -		printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +		printf("AVB is not initialized, please run 'avb init <id>'\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> @@ -404,14 +429,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc,
>  	name = argv[1];
>  	value = argv[2];
>  
> -	if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1,
> -					    (const uint8_t *)value) ==
> -	    AVB_IO_RESULT_OK) {
> +	ret = avb_ops->write_persistent_value(avb_ops, name,
> +					      strlen(value) + 1,
> +					      (const uint8_t *)value);
> +	if (ret == AVB_IO_RESULT_OK) {
>  		printf("Wrote %zu bytes\n", strlen(value) + 1);
>  		return CMD_RET_SUCCESS;
>  	}
>  
> -	printf("Failed to write persistent value\n");
> +	printf("Failed to write persistent value `%s` = `%s`, err = %d\n",
> +	       name, value, ret);
>  
>  	return CMD_RET_FAILURE;
>  }
> -- 
> 2.34.1

[PATCH v2 5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Introduce str_avb_io_error() and str_avb_slot_error() functions,
that provide a pointer to AVB runtime error message.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Mattijs Korpershoek R-b tag applied

 common/avb_verify.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/avb_verify.h |  3 ++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index ed58239cf8a..cff9117d92f 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -119,6 +119,55 @@ static const unsigned char avb_root_pub[1032] = {
 	0xd8, 0x7e,
 };
 
+const char *str_avb_io_error(AvbIOResult res)
+{
+	switch (res) {
+	case AVB_IO_RESULT_OK:
+		return "Requested operation was successful";
+	case AVB_IO_RESULT_ERROR_IO:
+		return "Underlying hardware encountered an I/O error";
+	case AVB_IO_RESULT_ERROR_OOM:
+		return "Unable to allocate memory";
+	case AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION:
+		return "Requested partition does not exist";
+	case AVB_IO_RESULT_ERROR_RANGE_OUTSIDE_PARTITION:
+		return "Bytes requested is outside the range of partition";
+	case AVB_IO_RESULT_ERROR_NO_SUCH_VALUE:
+		return "Named persistent value does not exist";
+	case AVB_IO_RESULT_ERROR_INVALID_VALUE_SIZE:
+		return "Named persistent value size is not supported";
+	case AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE:
+		return "Buffer is too small for the requested operation";
+	default:
+		return "Unknown AVB error";
+	}
+}
+
+const char *str_avb_slot_error(AvbSlotVerifyResult res)
+{
+	switch (res) {
+	case AVB_SLOT_VERIFY_RESULT_OK:
+		return "Verification passed successfully";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
+		return "Allocation of memory failed";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
+		return "I/O error occurred while trying to load data";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
+		return "Digest didn't match or signature checks failed";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
+		return "Rollback index is less than its stored value";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
+		return "Public keys are not accepted";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
+		return "Metadata is invalid or inconsistent";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
+		return "Metadata requires a newer version of libavb";
+	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT:
+		return "Invalid arguments are used";
+	default:
+		return "Unknown AVB slot verification error";
+	}
+}
 /**
  * ============================================================================
  * Boot states support (GREEN, YELLOW, ORANGE, RED) and dm_verity
diff --git a/include/avb_verify.h b/include/avb_verify.h
index 2fb850044d9..5d998b5a302 100644
--- a/include/avb_verify.h
+++ b/include/avb_verify.h
@@ -52,7 +52,8 @@ char *avb_set_enforce_verity(const char *cmdline);
 char *avb_set_ignore_corruption(const char *cmdline);
 
 char *append_cmd_line(char *cmdline_orig, char *cmdline_new);
-
+const char *str_avb_io_error(AvbIOResult res);
+const char *str_avb_slot_error(AvbSlotVerifyResult res);
 /**
  * ============================================================================
  * I/O helper inline functions
-- 
2.34.1

[PATCH v2 6/7] cmd: avb: rework do_avb_verify_part

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Use existing str_avb_slot_error() function for obtaining
verification fail reason details.
Take into account device lock state for setting correct
androidboot.verifiedbootstate kernel cmdline parameter.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Mattijs Korpershoek R-b tag applied

 cmd/avb.c | 50 +++++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

diff --git a/cmd/avb.c b/cmd/avb.c
index 62a3ee18e7f..8fbd48ee5a2 100644
--- a/cmd/avb.c
+++ b/cmd/avb.c
@@ -250,6 +250,7 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 	const char * const requested_partitions[] = {"boot", NULL};
 	AvbSlotVerifyResult slot_result;
 	AvbSlotVerifyData *out_data;
+	enum avb_boot_state boot_state;
 	char *cmdline;
 	char *extra_args;
 	char *slot_suffix = "";
@@ -287,18 +288,23 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 				AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE,
 				&out_data);
 
-	switch (slot_result) {
-	case AVB_SLOT_VERIFY_RESULT_OK:
-		/* Until we don't have support of changing unlock states, we
-		 * assume that we are by default in locked state.
-		 * So in this case we can boot only when verification is
-		 * successful; we also supply in cmdline GREEN boot state
-		 */
+	/*
+	 * LOCKED devices with custom root of trust setup is not supported (YELLOW)
+	 */
+	if (slot_result == AVB_SLOT_VERIFY_RESULT_OK) {
 		printf("Verification passed successfully\n");
 
-		/* export additional bootargs to AVB_BOOTARGS env var */
+		/*
+		 * ORANGE state indicates that device may be freely modified.
+		 * Device integrity is left to the user to verify out-of-band.
+		 */
+		if (unlocked)
+			boot_state = AVB_ORANGE;
+		else
+			boot_state = AVB_GREEN;
 
-		extra_args = avb_set_state(avb_ops, AVB_GREEN);
+		/* export boot state to AVB_BOOTARGS env var */
+		extra_args = avb_set_state(avb_ops, boot_state);
 		if (extra_args)
 			cmdline = append_cmd_line(out_data->cmdline,
 						  extra_args);
@@ -308,30 +314,8 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag,
 		env_set(AVB_BOOTARGS, cmdline);
 
 		res = CMD_RET_SUCCESS;
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
-		printf("Verification failed\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
-		printf("I/O error occurred during verification\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
-		printf("OOM error occurred during verification\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
-		printf("Corrupted dm-verity metadata detected\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
-		printf("Unsupported version of avbtool was used\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
-		printf("Rollback index check failed\n");
-		break;
-	case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
-		printf("Public key was rejected\n");
-		break;
-	default:
-		printf("Unknown error occurred\n");
+	} else {
+		printf("Verification failed, reason: %s\n", str_avb_slot_error(slot_result));
 	}
 
 	if (out_data)
-- 
2.34.1

[PATCH v2 7/7] doc: android: avb: sync usage details

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@gmail.com>

Sync usage info with the one cmd/avb.c.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

Changes in v2:
- Address Mattijs comment about usage info, sync with cmd/avb.c

 doc/android/avb2.rst | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst
index a072119574f..4aca7a5c660 100644
--- a/doc/android/avb2.rst
+++ b/doc/android/avb2.rst
@@ -38,16 +38,22 @@ AVB 2.0 U-Boot shell commands
 Provides CLI interface to invoke AVB 2.0 verification + misc. commands for
 different testing purposes::
 
-    avb init <dev> - initialize avb 2.0 for <dev>
-    avb verify - run verification process using hash data from vbmeta structure
+    avb init <dev> - initialize avb 2 for <dev>
     avb read_rb <num> - read rollback index at location <num>
     avb write_rb <num> <rb> - write rollback index <rb> to <num>
     avb is_unlocked - returns unlock status of the device
-    avb get_uuid <partname> - read and print uuid of partition <partname>
+    avb get_uuid <partname> - read and print uuid of partition <part>
     avb read_part <partname> <offset> <num> <addr> - read <num> bytes from
-    partition <partname> to buffer <addr>
+        partition <partname> to buffer <addr>
+    avb read_part_hex <partname> <offset> <num> - read <num> bytes from
+        partition <partname> and print to stdout
     avb write_part <partname> <offset> <num> <addr> - write <num> bytes to
-    <partname> by <offset> using data from <addr>
+        <partname> by <offset> using data from <addr>
+    avb read_pvalue <name> <bytes> - read a persistent value <name>
+    avb write_pvalue <name> <value> - write a persistent value <name>
+    avb verify [slot_suffix] - run verification process using hash data
+        from vbmeta structure
+        [slot_suffix] - _a, _b, etc (if vbmeta partition is slotted)
 
 Partitions tampering (example)
 ------------------------------
-- 
2.34.1

Re: [PATCH v2 7/7] doc: android: avb: sync usage details

[Mattijs Korpershoek]

Hi Igor,

Thank you for the patch.

On ven., févr. 09, 2024 at 20:20, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:

> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Sync usage info with the one cmd/avb.c.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
> Changes in v2:
> - Address Mattijs comment about usage info, sync with cmd/avb.c
>
>  doc/android/avb2.rst | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst
> index a072119574f..4aca7a5c660 100644
> --- a/doc/android/avb2.rst
> +++ b/doc/android/avb2.rst
> @@ -38,16 +38,22 @@ AVB 2.0 U-Boot shell commands
>  Provides CLI interface to invoke AVB 2.0 verification + misc. commands for
>  different testing purposes::
>  
> -    avb init <dev> - initialize avb 2.0 for <dev>
> -    avb verify - run verification process using hash data from vbmeta structure
> +    avb init <dev> - initialize avb 2 for <dev>
>      avb read_rb <num> - read rollback index at location <num>
>      avb write_rb <num> <rb> - write rollback index <rb> to <num>
>      avb is_unlocked - returns unlock status of the device
> -    avb get_uuid <partname> - read and print uuid of partition <partname>
> +    avb get_uuid <partname> - read and print uuid of partition <part>
>      avb read_part <partname> <offset> <num> <addr> - read <num> bytes from
> -    partition <partname> to buffer <addr>
> +        partition <partname> to buffer <addr>
> +    avb read_part_hex <partname> <offset> <num> - read <num> bytes from
> +        partition <partname> and print to stdout
>      avb write_part <partname> <offset> <num> <addr> - write <num> bytes to
> -    <partname> by <offset> using data from <addr>
> +        <partname> by <offset> using data from <addr>
> +    avb read_pvalue <name> <bytes> - read a persistent value <name>
> +    avb write_pvalue <name> <value> - write a persistent value <name>
> +    avb verify [slot_suffix] - run verification process using hash data
> +        from vbmeta structure
> +        [slot_suffix] - _a, _b, etc (if vbmeta partition is slotted)
>  
>  Partitions tampering (example)
>  ------------------------------
> -- 
> 2.34.1

Re: [PATCH v2 0/7] AVB: cosmetic adjustments/improvements

[Mattijs Korpershoek]

Hi,

On Fri, 09 Feb 2024 20:20:38 +0100, Igor Opaniuk wrote:
> This is the first patch series for incoming major
> improvements for AVB implementation, that include:
> - Simplify and add more context for debug/error prints where it's needed.
> - Move SPDX license identifiers to the first line, so it conforms
>   to license placement rule.
> - Use mmc_switch_part() only for eMMC.
> - Rework do_avb_verify_part, take into account device lock state for setting correct
>   androidboot.verifiedbootstate.
> - Adjust AVB documentation, sync usage info with the one cmd/avb.c.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/7] common: avb_verify: don't call mmc_switch_part for SD
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/dbde93c12616bd6ba22e6bcb76363a6a3f253087
[2/7] avb: move SPDX license identifiers to the first line
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/bf7d47b4274568927adadeff8c9d1aaf2bd45ca6
[3/7] common: avb_verify: rework error/debug prints
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/bdcc8f8c8fd3dfb053cea7a0f62b34ef062411e0
[4/7] cmd: avb: rework prints
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/055462005fcc48a12a86edf2cbd3f7aa8712788b
[5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/63736ddd4fc357768e31a6f10c10b1a28a10ccbc
[6/7] cmd: avb: rework do_avb_verify_part
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/07caf5febcfcfb5f2cdaade124ed8d11735a6029
[7/7] doc: android: avb: sync usage details
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/85af871dcca7e9cae074e3a635706052a57629a4

--
Mattijs