[PATCH 1/2] FIT: refactor compression handling into separate function

[PATCH 1/2] FIT: refactor compression handling into separate function

[Ahmad Fatoum]

There are four conditions that need to be true for successful decompression
and the follow-up commit will add one more. Thus split off the compression
handling to aid readability.

No functional change.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/image-fit.c | 62 ++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index af9b2a94793c..9ceebde02931 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -564,6 +564,40 @@ static void fit_uncompress_error_fn(char *x)
 	pr_err("%s\n", x);
 }
 
+static int fit_handle_decompression(struct device_node *image,
+				    const void **data,
+				    int *data_len)
+{
+	const char *compression = NULL;
+	void *uc_data;
+	int ret;
+
+	of_property_read_string(image, "compression", &compression);
+	if (!compression || !strcmp(compression, "none"))
+		return 0;
+
+	if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
+		pr_err("image has compression = \"%s\", but support not compiled in\n",
+		       compression);
+		return -ENOSYS;
+	}
+
+	ret = uncompress_buf_to_buf(*data, *data_len, &uc_data,
+				    fit_uncompress_error_fn);
+	if (ret < 0) {
+		pr_err("data couldn't be decompressed\n");
+		return ret;
+	}
+
+	*data = uc_data;
+	*data_len = ret;
+
+	/* associate buffer with FIT, so it's not leaked */
+	__of_new_property(image, "uncompressed-data", uc_data, *data_len);
+
+	return 0;
+}
+
 /**
  * fit_open_image - Open an image in a FIT image
  * @handle: The FIT image handle
@@ -586,8 +620,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		   unsigned long *outsize)
 {
 	struct device_node *image;
-	const char *unit = name, *type = NULL, *compression = NULL,
-	      *desc= "(no description)";
+	const char *unit = name, *type = NULL, *desc= "(no description)";
 	const void *data;
 	int data_len;
 	int ret = 0;
@@ -619,28 +652,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 	if (ret < 0)
 		return ret;
 
-	of_property_read_string(image, "compression", &compression);
-	if (compression && strcmp(compression, "none") != 0) {
-		void *uc_data;
-
-		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
-			pr_err("image has compression = \"%s\", but support not compiled in\n",
-			       compression);
-			return -ENOSYS;
-		}
-
-		data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
-					    fit_uncompress_error_fn);
-		if (data_len < 0) {
-			pr_err("data couldn't be decompressed\n");
-			return data_len;
-		}
-
-		data = uc_data;
-
-		/* associate buffer with FIT, so it's not leaked */
-		__of_new_property(image, "uncompressed-data", uc_data, data_len);
-	}
+	ret = fit_handle_decompression(image, &data, &data_len);
+	if (ret)
+		return ret;
 
 	*outdata = data;
 	*outsize = data_len;
-- 
2.39.2

[PATCH 2/2] FIT: do not decompress ramdisks even if asked

[Ahmad Fatoum]

Linux will decompress its own ramdisk, so a well-formed ITS would
specify compression = "none", so the bootloader doesn't unpack the
ramdisk and the kernel takes care of it.

Some older versions of the Yocto kernel-fitimage.bbclass did populate
compression != "none" for ramdisks, so now barebox will fail to boot
the FIT images generated by them.

Fix this issue by not acting on the compression property when the image
in question is a ramdisk. We still print a warning, so users can fix
their ITS.

This aligns us with U-Boot's behavior[1].

[1]: https://git.yoctoproject.org/poky/commit/?h=kirkstone&id=2c58079222310
[2]: https://github.com/u-boot/u-boot/commit/bddd985734653c366c8da073650930

Fixes: 2ab6780b80e3 ("FIT: add first support for compressed images")
Reported-by: Christian Eggers <ceggers@arri.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/image-fit.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index 9ceebde02931..0352dc5cbd0c 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -565,6 +565,7 @@ static void fit_uncompress_error_fn(char *x)
 }
 
 static int fit_handle_decompression(struct device_node *image,
+				    const char *type,
 				    const void **data,
 				    int *data_len)
 {
@@ -576,6 +577,12 @@ static int fit_handle_decompression(struct device_node *image,
 	if (!compression || !strcmp(compression, "none"))
 		return 0;
 
+	if (!strcmp(type, "ramdisk")) {
+		pr_warn("compression != \"none\" for ramdisks is deprecated,"
+			" please fix your .its file!\n");
+		return 0;
+	}
+
 	if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
 		pr_err("image has compression = \"%s\", but support not compiled in\n",
 		       compression);
@@ -652,7 +659,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 	if (ret < 0)
 		return ret;
 
-	ret = fit_handle_decompression(image, &data, &data_len);
+	ret = fit_handle_decompression(image, type, &data, &data_len);
 	if (ret)
 		return ret;
 
-- 
2.39.2

Re: [PATCH 2/2] FIT: do not decompress ramdisks even if asked

[Christian Eggers]

On Friday, 25 August 2023, 12:22:46 CEST, Ahmad Fatoum wrote:
> Linux will decompress its own ramdisk, so a well-formed ITS would
> specify compression = "none", so the bootloader doesn't unpack the
> ramdisk and the kernel takes care of it.
> 
> Some older versions of the Yocto kernel-fitimage.bbclass did populate
> compression != "none" for ramdisks, so now barebox will fail to boot
> the FIT images generated by them.
> 
> Fix this issue by not acting on the compression property when the image
> in question is a ramdisk. We still print a warning, so users can fix
> their ITS.
> 
> This aligns us with U-Boot's behavior[1].
> 
> [1]: https://git.yoctoproject.org/poky/commit/?h=kirkstone&id=2c58079222310
> [2]: https://github.com/u-boot/u-boot/commit/bddd985734653c366c8da073650930
> 
> Fixes: 2ab6780b80e3 ("FIT: add first support for compressed images")
> Reported-by: Christian Eggers <ceggers@arri.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/image-fit.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 9ceebde02931..0352dc5cbd0c 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -565,6 +565,7 @@ static void fit_uncompress_error_fn(char *x)
>  }
>  
>  static int fit_handle_decompression(struct device_node *image,
> +				    const char *type,
>  				    const void **data,
>  				    int *data_len)
>  {
> @@ -576,6 +577,12 @@ static int fit_handle_decompression(struct device_node *image,
>  	if (!compression || !strcmp(compression, "none"))
>  		return 0;
>  
> +	if (!strcmp(type, "ramdisk")) {
> +		pr_warn("compression != \"none\" for ramdisks is deprecated,"
> +			" please fix your .its file!\n");
> +		return 0;
> +	}
> +
>  	if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
>  		pr_err("image has compression = \"%s\", but support not compiled in\n",
>  		       compression);
> @@ -652,7 +659,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = fit_handle_decompression(image, &data, &data_len);
> +	ret = fit_handle_decompression(image, type, &data, &data_len);
>  	if (ret)
>  		return ret;
>  
> 

Hi Ahmad,

thanks for the fast solution!

Tested-by: Christian Eggers <ceggers@arri.de>
[Tested both patches, as 1/2 is also required ]

Re: [PATCH 1/2] FIT: refactor compression handling into separate function

[Sascha Hauer]

On Fri, Aug 25, 2023 at 12:22:45PM +0200, Ahmad Fatoum wrote:
> There are four conditions that need to be true for successful decompression
> and the follow-up commit will add one more. Thus split off the compression
> handling to aid readability.
> 
> No functional change.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/image-fit.c | 62 ++++++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 24 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index af9b2a94793c..9ceebde02931 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -564,6 +564,40 @@ static void fit_uncompress_error_fn(char *x)
>  	pr_err("%s\n", x);
>  }
>  
> +static int fit_handle_decompression(struct device_node *image,
> +				    const void **data,
> +				    int *data_len)
> +{
> +	const char *compression = NULL;
> +	void *uc_data;
> +	int ret;
> +
> +	of_property_read_string(image, "compression", &compression);
> +	if (!compression || !strcmp(compression, "none"))
> +		return 0;
> +
> +	if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
> +		pr_err("image has compression = \"%s\", but support not compiled in\n",
> +		       compression);
> +		return -ENOSYS;
> +	}
> +
> +	ret = uncompress_buf_to_buf(*data, *data_len, &uc_data,
> +				    fit_uncompress_error_fn);
> +	if (ret < 0) {
> +		pr_err("data couldn't be decompressed\n");
> +		return ret;
> +	}
> +
> +	*data = uc_data;
> +	*data_len = ret;
> +
> +	/* associate buffer with FIT, so it's not leaked */
> +	__of_new_property(image, "uncompressed-data", uc_data, *data_len);
> +
> +	return 0;
> +}
> +
>  /**
>   * fit_open_image - Open an image in a FIT image
>   * @handle: The FIT image handle
> @@ -586,8 +620,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   unsigned long *outsize)
>  {
>  	struct device_node *image;
> -	const char *unit = name, *type = NULL, *compression = NULL,
> -	      *desc= "(no description)";
> +	const char *unit = name, *type = NULL, *desc= "(no description)";
>  	const void *data;
>  	int data_len;
>  	int ret = 0;
> @@ -619,28 +652,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  	if (ret < 0)
>  		return ret;
>  
> -	of_property_read_string(image, "compression", &compression);
> -	if (compression && strcmp(compression, "none") != 0) {
> -		void *uc_data;
> -
> -		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
> -			pr_err("image has compression = \"%s\", but support not compiled in\n",
> -			       compression);
> -			return -ENOSYS;
> -		}
> -
> -		data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
> -					    fit_uncompress_error_fn);
> -		if (data_len < 0) {
> -			pr_err("data couldn't be decompressed\n");
> -			return data_len;
> -		}
> -
> -		data = uc_data;
> -
> -		/* associate buffer with FIT, so it's not leaked */
> -		__of_new_property(image, "uncompressed-data", uc_data, data_len);
> -	}
> +	ret = fit_handle_decompression(image, &data, &data_len);
> +	if (ret)
> +		return ret;
>  
>  	*outdata = data;
>  	*outsize = data_len;
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |