Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Colin,

Thank you for the patch.

On mar., mars 12, 2024 at 07:57, Colin McAllister <colinmca242@gmail.com> wrote:

Sam also gave his review here:
https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p33Jw@mail.gmail.com/

Please include his review tag in the next submission.

I will add it at the appropriate place below:


> From: Colin McAllister <colin.mcallister@garmin.com>
>
> Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will
> not actually enable the #if protected code in android_ab.c. This is
> because "CONFIG_" should have been prepended to the config macro, or the
> macros defined in kconfig.h could have been used.
>
> The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no
> longer be conditionally compiled by preprocessor conditionals and
> instead use C conditionals. This better aligns with the Linux kernel
> style guide.
>
> Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message")
> Signed-off-by: Colin McAllister <colin.mcallister@garmin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Colin McAllister <colinmca242@gmail.com>
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> ---
> v2:
>   - Replaced #if conditionals with C if conditionals
>   - Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of
>     macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a
> 	boolean or tristate value and doesn't have different values when
> 	building SPL or TPL.
> v3:
>   - Added "Fixes:" tag
> v4:
>   - No changes
>
>  boot/android_ab.c | 97 ++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 52 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 9a3d15ec60..f547aa64e4 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  		   bool dec_tries)
>  {
>  	struct bootloader_control *abc = NULL;
> +	struct bootloader_control *backup_abc = NULL;
>  	u32 crc32_le;
>  	int slot, i, ret;
>  	bool store_needed = false;
> +	bool valid_backup = false;
>  	char slot_suffix[4];
> -#if ANDROID_AB_BACKUP_OFFSET
> -	struct bootloader_control *backup_abc = NULL;
> -#endif
>  
>  	ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0);
>  	if (ret < 0) {
> @@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  		return ret;
>  	}
>  
> -#if ANDROID_AB_BACKUP_OFFSET
> -	ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
> -					  ANDROID_AB_BACKUP_OFFSET);
> -	if (ret < 0) {
> -		free(abc);
> -		return ret;
> +	if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
> +		ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
> +						  CONFIG_ANDROID_AB_BACKUP_OFFSET);
> +		if (ret < 0) {
> +			free(abc);
> +			return ret;
> +		}
>  	}
> -#endif
>  
>  	crc32_le = ab_control_compute_crc(abc);
>  	if (abc->crc32_le != crc32_le) {
>  		log_err("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x),",
>  			crc32_le, abc->crc32_le);
> -#if ANDROID_AB_BACKUP_OFFSET
> -		crc32_le = ab_control_compute_crc(backup_abc);
> -		if (backup_abc->crc32_le != crc32_le) {
> -			log_err("ANDROID: Invalid backup CRC-32 ");
> -			log_err("expected %.8x, found %.8x),",
> -				crc32_le, backup_abc->crc32_le);
> -#endif
> -
> -			log_err("re-initializing A/B metadata.\n");
> +		if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
> +			crc32_le = ab_control_compute_crc(backup_abc);
> +			if (backup_abc->crc32_le != crc32_le) {
> +				log_err(" ANDROID: Invalid backup CRC-32 ");
> +				log_err("(expected %.8x, found %.8x),",
> +					crc32_le, backup_abc->crc32_le);
> +			} else {
> +				valid_backup = true;
> +				log_info(" copying A/B metadata from backup.\n");
> +				memcpy(abc, backup_abc, sizeof(*abc));
> +			}
> +		}
>  
> +		if (!valid_backup) {
> +			log_err(" re-initializing A/B metadata.\n");
>  			ret = ab_control_default(abc);
>  			if (ret < 0) {
> -#if ANDROID_AB_BACKUP_OFFSET
> -				free(backup_abc);
> -#endif
> +				if (CONFIG_ANDROID_AB_BACKUP_OFFSET)
> +					free(backup_abc);
>  				free(abc);
>  				return -ENODATA;
>  			}
> -#if ANDROID_AB_BACKUP_OFFSET
> -		} else {
> -			/*
> -			 * Backup is valid. Copy it to the primary
> -			 */
> -			memcpy(abc, backup_abc, sizeof(*abc));
>  		}
> -#endif
>  		store_needed = true;
>  	}
>  
>  	if (abc->magic != BOOT_CTRL_MAGIC) {
>  		log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic);
> -#if ANDROID_AB_BACKUP_OFFSET
> -		free(backup_abc);
> -#endif
> +		if (CONFIG_ANDROID_AB_BACKUP_OFFSET)
> +			free(backup_abc);
>  		free(abc);
>  		return -ENODATA;
>  	}
> @@ -259,9 +254,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  	if (abc->version > BOOT_CTRL_VERSION) {
>  		log_err("ANDROID: Unsupported A/B metadata version: %.8x\n",
>  			abc->version);
> -#if ANDROID_AB_BACKUP_OFFSET
> -		free(backup_abc);
> -#endif
> +		if (CONFIG_ANDROID_AB_BACKUP_OFFSET)
> +			free(backup_abc);
>  		free(abc);
>  		return -ENODATA;
>  	}
> @@ -338,30 +332,29 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  		abc->crc32_le = ab_control_compute_crc(abc);
>  		ret = ab_control_store(dev_desc, part_info, abc, 0);
>  		if (ret < 0) {
> -#if ANDROID_AB_BACKUP_OFFSET
> -			free(backup_abc);
> -#endif
> +			if (CONFIG_ANDROID_AB_BACKUP_OFFSET)
> +				free(backup_abc);
>  			free(abc);
>  			return ret;
>  		}
>  	}
>  
> -#if ANDROID_AB_BACKUP_OFFSET
> -	/*
> -	 * If the backup doesn't match the primary, write the primary
> -	 * to the backup offset
> -	 */
> -	if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
> -		ret = ab_control_store(dev_desc, part_info, abc,
> -				       ANDROID_AB_BACKUP_OFFSET);
> -		if (ret < 0) {
> -			free(backup_abc);
> -			free(abc);
> -			return ret;
> +	if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
> +		/*
> +		* If the backup doesn't match the primary, write the primary
> +		* to the backup offset
> +		*/

checkpatch.pl seems to complain about this comment block:

WARNING: Block comments should align the * on each line
#166: FILE: boot/android_ab.c:344:
+               /*
+               * If the backup doesn't match the primary, write the primary

To reproduce, run:

$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD

With the above addressed, please send a v5 including:

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

> +		if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
> +			ret = ab_control_store(dev_desc, part_info, abc,
> +					       CONFIG_ANDROID_AB_BACKUP_OFFSET);
> +			if (ret < 0) {
> +				free(backup_abc);
> +				free(abc);
> +				return ret;
> +			}
>  		}
> +		free(backup_abc);
>  	}
> -	free(backup_abc);
> -#endif
>  
>  	free(abc);
>  
> -- 
> 2.34.1