lib: fwts_acpi_tables: re-work fwts_acpi_get_rsdp, less memory copies

Message ID 20170811184404.17806-1-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin King Aug. 11, 2017, 6:44 p.m.
From: Colin Ian King <colin.king@canonical.com>

Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_acpi_tables.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

Comments

Naresh Bhat Aug. 11, 2017, 6:53 p.m. | #1
On 12 August 2017 at 00:14, Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_acpi_tables.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_
> tables.c
> index f7547c43..9b2312e5 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -146,39 +146,37 @@ static void *fwts_acpi_find_rsdp_bios(void)
>  static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(fwts_framework *fw, void
> *addr, size_t *rsdp_len)
>  {
>         uint8_t *mem;
> -       fwts_acpi_table_rsdp *rsdp = NULL;
> +       fwts_acpi_table_rsdp *rsdp;
>         *rsdp_len = 0;
>
> +       /* Assume version 2.0 size, we don't care about a few bytes over
> allocation if it's version 1.0 */
> +       if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1,
> sizeof(fwts_acpi_table_rsdp))) == NULL) {
> +               fwts_log_error(fw, "Cannot allocate cached RSDP, out of
> low memory.");
> +               return NULL;
> +       }
> +
>         if ((mem = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_rsdp)))
> == FWTS_MAP_FAILED)
>                 return NULL;
>
> -       if (fwts_safe_memread(mem, sizeof(fwts_acpi_table_rsdp)) !=
> FWTS_OK) {
> -               fwts_log_error(fw, "Cannot safely read RSDP from address
> %p.", mem);
> -               goto out;
> +       if (fwts_safe_memcpy(rsdp, mem, sizeof(fwts_acpi_table_rsdp)) !=
> FWTS_OK) {
> +               fwts_log_error(fw, "Cannot safely copy RSDP from address
> %p.", mem);
> +               goto err;
>         }
>
> -       rsdp = (fwts_acpi_table_rsdp*)mem;
>         /* Determine original RSDP size from revision. */
>         *rsdp_len = (rsdp->revision < 1) ? 20 : 36;
>
>         /* Must have the correct signature */
>         if (strncmp(rsdp->signature, "RSD PTR ", 8)) {
>                 fwts_log_error(fw, "RSDP did not have expected
> signature.");
> -               rsdp = NULL;
> -               goto out;
> -       }
> -
> -       /* Assume version 2.0 size, we don't care about a few bytes over
> allocation if it's version 1.0 */
> -       if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1,
> sizeof(fwts_acpi_table_rsdp))) == NULL) {
> -               rsdp = NULL;
> -               goto out;
> +               goto err;
>         }
> +       return rsdp;
>
> -       memcpy(rsdp, mem, *rsdp_len);
> -out:
> +err:
>         (void)fwts_munmap(mem, sizeof(fwts_acpi_table_rsdp));
> -
> -       return rsdp;
> +       fwts_low_free(rsdp);
> +       return NULL;
>  }
>
>  /*
> --
> 2.11.0
>

Thanks Colin,

Reviewed-By/Tested-By: Naresh Bhat <naresh.bhat@linaro.org>


>
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/
> mailman/listinfo/fwts-devel
>
Jeffrey Hugo Aug. 11, 2017, 7:06 p.m. | #2
On 8/11/2017 12:44 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP

copioes -> copies
Colin King Aug. 11, 2017, 7:14 p.m. | #3
On 11/08/17 20:06, Jeffrey Hugo wrote:
> On 8/11/2017 12:44 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP
> 
> copioes -> copies
> 

Ivan/Alex, can you fix that typo up before applying the patch.
Alex Hung Aug. 14, 2017, 6:56 p.m. | #4
On 2017-08-11 11:44 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_acpi_tables.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index f7547c43..9b2312e5 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -146,39 +146,37 @@ static void *fwts_acpi_find_rsdp_bios(void)
>   static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(fwts_framework *fw, void *addr, size_t *rsdp_len)
>   {
>   	uint8_t *mem;
> -	fwts_acpi_table_rsdp *rsdp = NULL;
> +	fwts_acpi_table_rsdp *rsdp;
>   	*rsdp_len = 0;
>   
> +	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
> +	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
> +		fwts_log_error(fw, "Cannot allocate cached RSDP, out of low memory.");
> +		return NULL;
> +	}
> +
>   	if ((mem = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_rsdp))) == FWTS_MAP_FAILED)
>   		return NULL;
>   
> -	if (fwts_safe_memread(mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
> -		fwts_log_error(fw, "Cannot safely read RSDP from address %p.", mem);
> -		goto out;
> +	if (fwts_safe_memcpy(rsdp, mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
> +		fwts_log_error(fw, "Cannot safely copy RSDP from address %p.", mem);
> +		goto err;
>   	}
>   
> -	rsdp = (fwts_acpi_table_rsdp*)mem;
>   	/* Determine original RSDP size from revision. */
>   	*rsdp_len = (rsdp->revision < 1) ? 20 : 36;
>   
>   	/* Must have the correct signature */
>   	if (strncmp(rsdp->signature, "RSD PTR ", 8)) {
>   		fwts_log_error(fw, "RSDP did not have expected signature.");
> -		rsdp = NULL;
> -		goto out;
> -	}
> -
> -	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
> -	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
> -		rsdp = NULL;
> -		goto out;
> +		goto err;
>   	}
> +	return rsdp;
>   
> -	memcpy(rsdp, mem, *rsdp_len);
> -out:
> +err:
>   	(void)fwts_munmap(mem, sizeof(fwts_acpi_table_rsdp));
> -
> -	return rsdp;
> +	fwts_low_free(rsdp);
> +	return NULL;
>   }
>   
>   /*
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
ivanhu Aug. 21, 2017, 7:39 a.m. | #5
On 08/12/2017 02:44 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Re-work fwts_acpi_get_rsdp so there are less memory copioes of the RSDP
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_acpi_tables.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index f7547c43..9b2312e5 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -146,39 +146,37 @@ static void *fwts_acpi_find_rsdp_bios(void)
>   static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(fwts_framework *fw, void *addr, size_t *rsdp_len)
>   {
>   	uint8_t *mem;
> -	fwts_acpi_table_rsdp *rsdp = NULL;
> +	fwts_acpi_table_rsdp *rsdp;
>   	*rsdp_len = 0;
>   
> +	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
> +	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
> +		fwts_log_error(fw, "Cannot allocate cached RSDP, out of low memory.");
> +		return NULL;
> +	}
> +
>   	if ((mem = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_rsdp))) == FWTS_MAP_FAILED)
>   		return NULL;
>   
> -	if (fwts_safe_memread(mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
> -		fwts_log_error(fw, "Cannot safely read RSDP from address %p.", mem);
> -		goto out;
> +	if (fwts_safe_memcpy(rsdp, mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
> +		fwts_log_error(fw, "Cannot safely copy RSDP from address %p.", mem);
> +		goto err;
>   	}
>   
> -	rsdp = (fwts_acpi_table_rsdp*)mem;
>   	/* Determine original RSDP size from revision. */
>   	*rsdp_len = (rsdp->revision < 1) ? 20 : 36;
>   
>   	/* Must have the correct signature */
>   	if (strncmp(rsdp->signature, "RSD PTR ", 8)) {
>   		fwts_log_error(fw, "RSDP did not have expected signature.");
> -		rsdp = NULL;
> -		goto out;
> -	}
> -
> -	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
> -	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
> -		rsdp = NULL;
> -		goto out;
> +		goto err;
>   	}
> +	return rsdp;
>   
> -	memcpy(rsdp, mem, *rsdp_len);
> -out:
> +err:
>   	(void)fwts_munmap(mem, sizeof(fwts_acpi_table_rsdp));
> -
> -	return rsdp;
> +	fwts_low_free(rsdp);
> +	return NULL;
>   }
>   
>   /*
> 

Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
index f7547c43..9b2312e5 100644
--- a/src/lib/src/fwts_acpi_tables.c
+++ b/src/lib/src/fwts_acpi_tables.c
@@ -146,39 +146,37 @@  static void *fwts_acpi_find_rsdp_bios(void)
 static fwts_acpi_table_rsdp *fwts_acpi_get_rsdp(fwts_framework *fw, void *addr, size_t *rsdp_len)
 {
 	uint8_t *mem;
-	fwts_acpi_table_rsdp *rsdp = NULL;
+	fwts_acpi_table_rsdp *rsdp;
 	*rsdp_len = 0;
 
+	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
+	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
+		fwts_log_error(fw, "Cannot allocate cached RSDP, out of low memory.");
+		return NULL;
+	}
+
 	if ((mem = fwts_mmap((off_t)addr, sizeof(fwts_acpi_table_rsdp))) == FWTS_MAP_FAILED)
 		return NULL;
 
-	if (fwts_safe_memread(mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
-		fwts_log_error(fw, "Cannot safely read RSDP from address %p.", mem);
-		goto out;
+	if (fwts_safe_memcpy(rsdp, mem, sizeof(fwts_acpi_table_rsdp)) != FWTS_OK) {
+		fwts_log_error(fw, "Cannot safely copy RSDP from address %p.", mem);
+		goto err;
 	}
 
-	rsdp = (fwts_acpi_table_rsdp*)mem;
 	/* Determine original RSDP size from revision. */
 	*rsdp_len = (rsdp->revision < 1) ? 20 : 36;
 
 	/* Must have the correct signature */
 	if (strncmp(rsdp->signature, "RSD PTR ", 8)) {
 		fwts_log_error(fw, "RSDP did not have expected signature.");
-		rsdp = NULL;
-		goto out;
-	}
-
-	/* Assume version 2.0 size, we don't care about a few bytes over allocation if it's version 1.0 */
-	if ((rsdp = (fwts_acpi_table_rsdp*)fwts_low_calloc(1, sizeof(fwts_acpi_table_rsdp))) == NULL) {
-		rsdp = NULL;
-		goto out;
+		goto err;
 	}
+	return rsdp;
 
-	memcpy(rsdp, mem, *rsdp_len);
-out:
+err:
 	(void)fwts_munmap(mem, sizeof(fwts_acpi_table_rsdp));
-
-	return rsdp;
+	fwts_low_free(rsdp);
+	return NULL;
 }
 
 /*