ACPI, APEI: Cleanup alignment related codes for APEI

PATCH Superseded Chen, Gong 2 participants 15 messages Nov 11, 2013 – Dec 18, 2013

Subsystems (3)

ACPI ACPI APEI Pstore filesystem

lore

Share & tools

lore

Chen, Gong

author

2013-11-11 02:24 UTC #0 lore raw

PATCH v2 1/1 ACPI, APEI: Cleanup alignment related codes for APEI

We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.

v2->v1: Make description information clearer.

Signed-off-by Chen, Gong <[email protected]>

---
 drivers/acpi/apei/apei-base.c |  4 ++--
 drivers/acpi/apei/einj.c      | 19 +++++++++----------
 drivers/acpi/apei/erst.c      |  2 +-
 3 files changed, 12 insertions(+), 13 deletions(-)

drivers/acpi/apei/apei-base.c +2 -2

index 46f80e2..c481adf 100644
@@ -41,6 +41,7 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -567,8 +568,7 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	bit_offset = reg->bit_offset;
 	access_size_code = reg->access_width;
 	space_id = reg->space_id;
-	/* Handle possible alignment issues */
-	memcpy(paddr, &reg->address, sizeof(*paddr));
+	paddr = get_unaligned(&reg->address);
 	if (!*paddr) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid physical address in GAR [0x%llx/%u/%u/%u/%u]\n",

drivers/acpi/apei/einj.c +9 ~~-10~~

index fb57d03..361177a 100644
@@ -34,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/mm.h>
 #include <acpi/acpi.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -216,7 +217,7 @@ static void check_vendor_extension(u64 paddr,
 static void *einj_get_parameter_address(void)
 {
 	int i;
-	u64 paddrv4 = 0, paddrv5 = 0;
+	u64 pa_v4 = 0, pa_v5 = 0;
 	struct acpi_whea_header *entry;
 
 	entry = EINJ_TAB_ENTRY(einj_tab);

@@ -225,30 +226,28 @@ static void *einj_get_parameter_address(void)
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv4, &entry->register_region.address,
-			       sizeof(paddrv4));
+			pa_v4 = get_unaligned(&entry->register_region.address);
 		if (entry->action == ACPI_EINJ_SET_ERROR_TYPE_WITH_ADDRESS &&
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv5, &entry->register_region.address,
-			       sizeof(paddrv5));
+			pa_v5 = get_unaligned(&entry->register_region.address);
 		entry++;
 	}
-	if (paddrv5) {
+	if (pa_v5) {
 		struct set_error_type_with_address *v5param;
 
-		v5param = acpi_os_map_memory(paddrv5, sizeof(*v5param));
+		v5param = acpi_os_map_memory(pa_v5, sizeof(*v5param));
 		if (v5param) {
 			acpi5 = 1;
-			check_vendor_extension(paddrv5, v5param);
+			check_vendor_extension(pa_v5, v5param);
 			return v5param;
 		}
 	}
-	if (param_extension && paddrv4) {
+	if (param_extension && pa_v4) {
 		struct einj_parameter *v4param;
 
-		v4param = acpi_os_map_memory(paddrv4, sizeof(*v4param));
+		v4param = acpi_os_map_memory(pa_v4, sizeof(*v4param));
 		if (!v4param)
 			return NULL;
 		if (v4param->reserved1 || v4param->reserved2) {

drivers/acpi/apei/erst.c +1 -1

index 26311f2..bf30a12 100644
@@ -611,7 +611,7 @@ static void __erst_record_id_cache_compact(void)
 		if (entries[i] == APEI_ERST_INVALID_RECORD_ID)
 			continue;
 		if (wpos != i)
-			memcpy(&entries[wpos], &entries[i], sizeof(entries[i]));
+			entries[wpos] = entries[i];
 		wpos++;
 	}
 	erst_record_id_cache.len = wpos;

Signature · 1 line

1.8.4.rc3

Chen, Gong

author

2013-11-14 02:06 UTC #1 lore raw

Quoted 7 lines · Gong

Date: Sun, 10 Nov 2013 21:07:46 -0500
From: "Chen, Gong" <[email protected]>
To: [email protected], [email protected]
Cc: [email protected], "Chen, Gong" <[email protected]>
Subject: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI
X-Mailer: git-send-email 1.8.4.rc3

Quoted 8 lines

We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.
v2->v1: Make description information clearer.

Any comments? Boris/Tony?

Borislav Petkov

reviewer

2013-11-14 12:30 UTC #2 lore raw

Quoted 14 lines · Gong

Quoted 13 lines · Gong
Date: Sun, 10 Nov 2013 21:07:46 -0500
From: "Chen, Gong" <[email protected]>
To: [email protected], [email protected]
Cc: [email protected], "Chen, Gong" <[email protected]>
Subject: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI
X-Mailer: git-send-email 1.8.4.rc3
We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.

Quoted 3 lines

Quoted 2 lines
v2->v1: Make description information clearer.
Any comments? Boris/Tony?

I get this when building here:

drivers/acpi/apei/apei-base.c: In function ‘apei_check_gar’:
drivers/acpi/apei/apei-base.c:571:8: warning: assignment makes pointer from integer without a cast [enabled by default]

  paddr = get_unaligned(&reg->address);
        ^

drivers/acpi/apei/apei-base.c: In function ‘collect_res_callback’:
drivers/acpi/apei/apei-base.c:716:3: warning: ‘paddr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return apei_res_add(&resources->iomem, paddr,
^
drivers/acpi/apei/apei-base.c: In function ‘apei_read’:
drivers/acpi/apei/apei-base.c:645:10: warning: ‘address’ may be used uninitialized in this function [-Wmaybe-uninitialized]
status = acpi_os_read_memory((acpi_physical_address) address,

drivers/acpi/apei/apei-base.c: In function ‘apei_write’:
drivers/acpi/apei/apei-base.c:678:10: warning: ‘address’ may be used uninitialized in this function [-Wmaybe-uninitialized]
status = acpi_os_write_memory((acpi_physical_address) address,

Signature · 9 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chen, Gong

author

2013-11-15 05:55 UTC #3 lore raw

Quoted 24 lines · Borislav Petkov

Date: Thu, 14 Nov 2013 13:29:56 +0100
From: Borislav Petkov <[email protected]>
To: "Chen, Gong" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI
User-Agent: Mutt/1.5.21 (2010-09-15)
Quoted 16 lines · Gong
Quoted 15 lines · Gong
Date: Sun, 10 Nov 2013 21:07:46 -0500
From: "Chen, Gong" <[email protected]>
To: [email protected], [email protected]
Cc: [email protected], "Chen, Gong" <[email protected]>
Subject: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI
X-Mailer: git-send-email 1.8.4.rc3
We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.
v2->v1: Make description information clearer.

Quoted 8 lines

Quoted 1 line
Any comments? Boris/Tony?
I get this when building here:
drivers/acpi/apei/apei-base.c: In function ‘apei_check_gar’:
drivers/acpi/apei/apei-base.c:571:8: warning: assignment makes pointer from integer without a cast [enabled by default]
  paddr = get_unaligned(&reg->address);
        ^

Gee, it is really really a stupid error. I thought I checked the patch throughly but I'm wrong :-(.

It should be

    *paddr = get_unaligned(&reg->address);

Quoted 18 lines

drivers/acpi/apei/apei-base.c: In function ‘collect_res_callback’:
drivers/acpi/apei/apei-base.c:716:3: warning: ‘paddr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return apei_res_add(&resources->iomem, paddr,
^
drivers/acpi/apei/apei-base.c: In function ‘apei_read’:
drivers/acpi/apei/apei-base.c:645:10: warning: ‘address’ may be used uninitialized in this function [-Wmaybe-uninitialized]
status = acpi_os_read_memory((acpi_physical_address) address,
          ^
drivers/acpi/apei/apei-base.c: In function ‘apei_write’:
drivers/acpi/apei/apei-base.c:678:10: warning: ‘address’ may be used uninitialized in this function [-Wmaybe-uninitialized]
status = acpi_os_write_memory((acpi_physical_address) address,
Signature · 5 lines
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

author

2013-11-15 06:02 UTC #4 lore raw

PATCH v3 1/1 ACPI, APEI: Cleanup alignment related codes for APEI

In reply to Borislav Petkov

v3->v2: Fix an evaluation error.
v2->v1: Make description information clearer.

Signed-off-by Chen, Gong <[email protected]>

---
 drivers/acpi/apei/apei-base.c |  4 ++--
 drivers/acpi/apei/einj.c      | 19 +++++++++----------
 drivers/acpi/apei/erst.c      |  2 +-
 3 files changed, 12 insertions(+), 13 deletions(-)

drivers/acpi/apei/apei-base.c +2 -2

index 6d2c49b..e55584a 100644
@@ -41,6 +41,7 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -567,8 +568,7 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	bit_offset = reg->bit_offset;
 	access_size_code = reg->access_width;
 	space_id = reg->space_id;
-	/* Handle possible alignment issues */
-	memcpy(paddr, &reg->address, sizeof(*paddr));
+	*paddr = get_unaligned(&reg->address);
 	if (!*paddr) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid physical address in GAR [0x%llx/%u/%u/%u/%u]\n",

drivers/acpi/apei/einj.c +9 ~~-10~~

index fb57d03..361177a 100644
@@ -34,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/mm.h>
 #include <acpi/acpi.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -216,7 +217,7 @@ static void check_vendor_extension(u64 paddr,
 static void *einj_get_parameter_address(void)
 {
 	int i;
-	u64 paddrv4 = 0, paddrv5 = 0;
+	u64 pa_v4 = 0, pa_v5 = 0;
 	struct acpi_whea_header *entry;
 
 	entry = EINJ_TAB_ENTRY(einj_tab);

@@ -225,30 +226,28 @@ static void *einj_get_parameter_address(void)
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv4, &entry->register_region.address,
-			       sizeof(paddrv4));
+			pa_v4 = get_unaligned(&entry->register_region.address);
 		if (entry->action == ACPI_EINJ_SET_ERROR_TYPE_WITH_ADDRESS &&
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv5, &entry->register_region.address,
-			       sizeof(paddrv5));
+			pa_v5 = get_unaligned(&entry->register_region.address);
 		entry++;
 	}
-	if (paddrv5) {
+	if (pa_v5) {
 		struct set_error_type_with_address *v5param;
 
-		v5param = acpi_os_map_memory(paddrv5, sizeof(*v5param));
+		v5param = acpi_os_map_memory(pa_v5, sizeof(*v5param));
 		if (v5param) {
 			acpi5 = 1;
-			check_vendor_extension(paddrv5, v5param);
+			check_vendor_extension(pa_v5, v5param);
 			return v5param;
 		}
 	}
-	if (param_extension && paddrv4) {
+	if (param_extension && pa_v4) {
 		struct einj_parameter *v4param;
 
-		v4param = acpi_os_map_memory(paddrv4, sizeof(*v4param));
+		v4param = acpi_os_map_memory(pa_v4, sizeof(*v4param));
 		if (!v4param)
 			return NULL;
 		if (v4param->reserved1 || v4param->reserved2) {

drivers/acpi/apei/erst.c +1 -1

index 26311f2..bf30a12 100644
@@ -611,7 +611,7 @@ static void __erst_record_id_cache_compact(void)
 		if (entries[i] == APEI_ERST_INVALID_RECORD_ID)
 			continue;
 		if (wpos != i)
-			memcpy(&entries[wpos], &entries[i], sizeof(entries[i]));
+			entries[wpos] = entries[i];
 		wpos++;
 	}
 	erst_record_id_cache.len = wpos;

Signature · 1 line

1.8.4.rc3

Borislav Petkov

reviewer Acked-by on v3 1/1

2013-11-16 11:48 UTC #5 lore raw

Quoted 10 lines · Gong

We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.
v3->v2: Fix an evaluation error.
v2->v1: Make description information clearer.
Signed-off-by Chen, Gong <[email protected]>

Acked-by Borislav Petkov <[email protected]>

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

author

2013-12-14 14:00 UTC #6 lore raw

Quoted 14 lines · Borislav Petkov

Date: Sat, 16 Nov 2013 12:48:03 +0100
From: Borislav Petkov <[email protected]>
To: "Chen, Gong" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
User-Agent: Mutt/1.5.21 (2010-09-15)
Quoted 6 lines · Gong
We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue. In the meanwhile, simplify some variable names to avoid the limit of 80 characters per line and use structure assignment instead of unnecessary memcpy. No functional changes.

Quoted 11 lines

Quoted 4 lines
v3->v2: Fix an evaluation error.
v2->v1: Make description information clearer.
Signed-off-by Chen, Gong <[email protected]>
Acked-by Borislav Petkov <[email protected]>
Signature · 2 lines
Regards/Gruss,
    Boris.

Hi, Boris

I notice that you sent a new RAS pull request. So will you pick up this patch or miss it?

Borislav Petkov

reviewer

2013-12-14 16:46 UTC #7 lore raw

Quoted 2 lines · Gong

I notice that you sent a new RAS pull request. So will you pick up this patch or miss it?

I missed it, sorry. :-\ It got buried in my mbox. I'll queue it for the next pull - it is not critical stuff to have to redo the current pull request.

Thanks for reminding me.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

reviewer

2013-12-16 14:19 UTC #8 lore raw

In reply to Chen, Gong

drivers/acpi/apei/erst.c static void __erst_record_id_cache_compact(void) · line 614

Quoted 10 lines · Gong

drivers/acpi/apei/erst.c +1 -1

index 26311f2..bf30a12 100644
@@ -611,7 +611,7 @@ static void __erst_record_id_cache_compact(void)
 		if (entries[i] == APEI_ERST_INVALID_RECORD_ID)
 			continue;
 		if (wpos != i)
-			memcpy(&entries[wpos], &entries[i], sizeof(entries[i]));
+			entries[wpos] = entries[i];

Why is it ok to drop the memcpy here and do a normal access?

__erst_record_id_cache_add_one still has a memcpy-like access.

What is the difference with all those accesses to erst_record_id_cache and why doesn't it need the unaligned helpers?

This all needs to be explained in detail in the commit message.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

author

2013-12-16 14:57 UTC #9 lore raw

Quoted 21 lines · Borislav Petkov

Date: Mon, 16 Dec 2013 15:19:06 +0100
From: Borislav Petkov <[email protected]>
To: "Chen, Gong" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
User-Agent: Mutt/1.5.21 (2010-09-15)
drivers/acpi/apei/erst.c static void __erst_record_id_cache_compact(void) · line 614
Quoted 10 lines · Gong
drivers/acpi/apei/erst.c +1 -1
index 26311f2..bf30a12 100644
@@ -611,7 +611,7 @@ static void __erst_record_id_cache_compact(void)
 		if (entries[i] == APEI_ERST_INVALID_RECORD_ID)
 			continue;
 		if (wpos != i)
-			memcpy(&entries[wpos], &entries[i], sizeof(entries[i]));
+			entries[wpos] = entries[i];
Why is it ok to drop the memcpy here and do a normal access?

Quoted 9 lines

__erst_record_id_cache_add_one still has a memcpy-like access.
What is the difference with all those accesses to erst_record_id_cache and why doesn't it need the unaligned helpers?
This all needs to be explained in detail in the commit message.
Thanks.

erst record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access.

Borislav Petkov

reviewer

2013-12-16 17:55 UTC #10 lore raw

Quoted 3 lines · Gong

erst record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access.

So what's with the memcpy in __erst_record_id_cache_add_one()?

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

author

2013-12-17 05:53 UTC #11 lore raw

Quoted 7 lines · Borislav Petkov

Date: Mon, 16 Dec 2013 18:55:51 +0100
From: Borislav Petkov <[email protected]>
To: [email protected], [email protected], "Chen, Gong"
<[email protected]>
Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
User-Agent: Mutt/1.5.21 (2010-09-15)

Quoted 7 lines

Quoted 3 lines · Gong
erst record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access.
So what's with the memcpy in __erst_record_id_cache_add_one()?

I think you mean
...

                memcpy(new_entries, entries,
                       erst_record_id_cache.len *
                       sizeof(entries[0]));

...

It is a copy from one integer array to another one(from old ID array to new bigger ID array), so I can't use a simple evaluation to get this goal.

Borislav Petkov

reviewer

2013-12-17 15:17 UTC #12 lore raw

Quoted 3 lines · Gong

It is a copy from one integer array to another one(from old ID array to new bigger ID array), so I can't use a simple evaluation to get this goal.

Ok, I see.

Please add this information I spent a couple of days asking you about to the commit message. Something along the lines of this one:

"erst record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access."

which would explain your last hunk changing the memcpy to a simple assignment. It is obvious that it is not that trivial information so documenting it for the future is very important.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Chen, Gong

author

2013-12-18 06:49 UTC #13 lore raw

PATCH v4 1/1 ACPI, APEI: Cleanup alignment related codes for APEI

In reply to Chen, Gong

We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue.

Because ERST record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access. BTW, simplify some variable names to avoid the limit of 80 characters per line.

v4->v3: Add more detail description.
v3->v2: Fix an evaluation error.
v2->v1: Make description information clearer.

Signed-off-by Chen, Gong <[email protected]>

Acked-by Borislav Petkov <[email protected]>

---
 drivers/acpi/apei/apei-base.c |  4 ++--
 drivers/acpi/apei/einj.c      | 19 +++++++++----------
 drivers/acpi/apei/erst.c      |  2 +-
 3 files changed, 12 insertions(+), 13 deletions(-)

drivers/acpi/apei/apei-base.c +2 -2

index 6d2c49b..e55584a 100644
@@ -41,6 +41,7 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -567,8 +568,7 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	bit_offset = reg->bit_offset;
 	access_size_code = reg->access_width;
 	space_id = reg->space_id;
-	/* Handle possible alignment issues */
-	memcpy(paddr, &reg->address, sizeof(*paddr));
+	*paddr = get_unaligned(&reg->address);
 	if (!*paddr) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid physical address in GAR [0x%llx/%u/%u/%u/%u]\n",

drivers/acpi/apei/einj.c +9 ~~-10~~

index fb57d03..361177a 100644
@@ -34,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/mm.h>
 #include <acpi/acpi.h>
+#include <asm/unaligned.h>
 
 #include "apei-internal.h"

@@ -216,7 +217,7 @@ static void check_vendor_extension(u64 paddr,
 static void *einj_get_parameter_address(void)
 {
 	int i;
-	u64 paddrv4 = 0, paddrv5 = 0;
+	u64 pa_v4 = 0, pa_v5 = 0;
 	struct acpi_whea_header *entry;
 
 	entry = EINJ_TAB_ENTRY(einj_tab);

@@ -225,30 +226,28 @@ static void *einj_get_parameter_address(void)
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv4, &entry->register_region.address,
-			       sizeof(paddrv4));
+			pa_v4 = get_unaligned(&entry->register_region.address);
 		if (entry->action == ACPI_EINJ_SET_ERROR_TYPE_WITH_ADDRESS &&
 		    entry->instruction == ACPI_EINJ_WRITE_REGISTER &&
 		    entry->register_region.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			memcpy(&paddrv5, &entry->register_region.address,
-			       sizeof(paddrv5));
+			pa_v5 = get_unaligned(&entry->register_region.address);
 		entry++;
 	}
-	if (paddrv5) {
+	if (pa_v5) {
 		struct set_error_type_with_address *v5param;
 
-		v5param = acpi_os_map_memory(paddrv5, sizeof(*v5param));
+		v5param = acpi_os_map_memory(pa_v5, sizeof(*v5param));
 		if (v5param) {
 			acpi5 = 1;
-			check_vendor_extension(paddrv5, v5param);
+			check_vendor_extension(pa_v5, v5param);
 			return v5param;
 		}
 	}
-	if (param_extension && paddrv4) {
+	if (param_extension && pa_v4) {
 		struct einj_parameter *v4param;
 
-		v4param = acpi_os_map_memory(paddrv4, sizeof(*v4param));
+		v4param = acpi_os_map_memory(pa_v4, sizeof(*v4param));
 		if (!v4param)
 			return NULL;
 		if (v4param->reserved1 || v4param->reserved2) {

drivers/acpi/apei/erst.c +1 -1

index 26311f2..bf30a12 100644
@@ -611,7 +611,7 @@ static void __erst_record_id_cache_compact(void)
 		if (entries[i] == APEI_ERST_INVALID_RECORD_ID)
 			continue;
 		if (wpos != i)
-			memcpy(&entries[wpos], &entries[i], sizeof(entries[i]));
+			entries[wpos] = entries[i];
 		wpos++;
 	}
 	erst_record_id_cache.len = wpos;

Signature · 1 line

1.8.4.3

Borislav Petkov

reviewer Applied

2013-12-18 21:13 UTC #14 lore raw

Quoted 9 lines · Gong

We ever used *memcpy* to avoid access alignment issue between firmware and OS. Now we can use a better and standard way to avoid this issue.
Because ERST record id cache is implemented in the memory to increase the access speed via caching ERST content, so it doesn't matter with firmware access. BTW, simplify some variable names to avoid the limit of 80 characters per line.

Quoted 6 lines

v4->v3: Add more detail description.
v3->v2: Fix an evaluation error.
v2->v1: Make description information clearer.
Signed-off-by Chen, Gong <[email protected]>
Acked-by Borislav Petkov <[email protected]>

Applied, thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--