[PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI

[PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI

[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. 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 <gong.chen@linux.intel.com>
---
 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(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 46f80e2..c481adf 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -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",
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index fb57d03..361177a 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -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) {
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 26311f2..bf30a12 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -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;
-- 
1.8.4.rc3

Re: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Sun, Nov 10, 2013 at 09:07:46PM -0500, Chen, Gong wrote:
> Date: Sun, 10 Nov 2013 21:07:46 -0500
> From: "Chen, Gong" <gong.chen@linux.intel.com>
> To: tony.luck@intel.com, bp@alien8.de
> Cc: linux-acpi@vger.kernel.org, "Chen, Gong" <gong.chen@linux.intel.com>
> 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.
> 
Any comments? Boris/Tony?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Wed, Nov 13, 2013 at 08:49:24PM -0500, Chen, Gong wrote:
> On Sun, Nov 10, 2013 at 09:07:46PM -0500, Chen, Gong wrote:
> > Date: Sun, 10 Nov 2013 21:07:46 -0500
> > From: "Chen, Gong" <gong.chen@linux.intel.com>
> > To: tony.luck@intel.com, bp@alien8.de
> > Cc: linux-acpi@vger.kernel.org, "Chen, Gong" <gong.chen@linux.intel.com>
> > 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.
> > 
> 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,

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Thu, Nov 14, 2013 at 01:29:56PM +0100, Borislav Petkov wrote:
> Date: Thu, 14 Nov 2013 13:29:56 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org
> Subject: Re: [PATCH V2] ACPI, APEI: Cleanup alignment related codes for APEI
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Wed, Nov 13, 2013 at 08:49:24PM -0500, Chen, Gong wrote:
> > On Sun, Nov 10, 2013 at 09:07:46PM -0500, Chen, Gong wrote:
> > > Date: Sun, 10 Nov 2013 21:07:46 -0500
> > > From: "Chen, Gong" <gong.chen@linux.intel.com>
> > > To: tony.luck@intel.com, bp@alien8.de
> > > Cc: linux-acpi@vger.kernel.org, "Chen, Gong" <gong.chen@linux.intel.com>
> > > 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.
> > > 
> > 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);

> 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,
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[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. 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 <gong.chen@linux.intel.com>
---
 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(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 6d2c49b..e55584a 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -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",
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index fb57d03..361177a 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -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) {
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 26311f2..bf30a12 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -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;
-- 
1.8.4.rc3

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Fri, Nov 15, 2013 at 12:45:56AM -0500, Chen, Gong wrote:
> 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 <gong.chen@linux.intel.com>

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Sat, Nov 16, 2013 at 12:48:03PM +0100, Borislav Petkov wrote:
> Date: Sat, 16 Nov 2013 12:48:03 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org
> Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Fri, Nov 15, 2013 at 12:45:56AM -0500, Chen, Gong wrote:
> > 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 <gong.chen@linux.intel.com>
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> 
> -- 
> 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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Sat, Dec 14, 2013 at 08:41:44AM -0500, Chen, Gong wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Fri, Nov 15, 2013 at 12:45:56AM -0500, Chen, Gong wrote:
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 26311f2..bf30a12 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

On Mon, Dec 16, 2013 at 03:19:06PM +0100, Borislav Petkov wrote:
> Date: Mon, 16 Dec 2013 15:19:06 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org
> Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Fri, Nov 15, 2013 at 12:45:56AM -0500, Chen, Gong wrote:
> > diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> > index 26311f2..bf30a12 100644
> > --- a/drivers/acpi/apei/erst.c
> > +++ b/drivers/acpi/apei/erst.c
> > @@ -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.
> 
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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Mon, Dec 16, 2013 at 09:39:34AM -0500, Chen, Gong wrote:
> 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()?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Mon, Dec 16, 2013 at 06:55:51PM +0100, Borislav Petkov wrote:
> Date: Mon, 16 Dec 2013 18:55:51 +0100
> From: Borislav Petkov <bp@alien8.de>
> To: tony.luck@intel.com, linux-acpi@vger.kernel.org, "Chen, Gong"
>  <gong.chen@linux.intel.com>
> Subject: Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Mon, Dec 16, 2013 at 09:39:34AM -0500, Chen, Gong wrote:
> > 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Tue, Dec 17, 2013 at 12:34:52AM -0500, Chen, Gong wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4] ACPI, APEI: Cleanup alignment related codes for APEI

[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 <gong.chen@linux.intel.com>
Acked-by: Borislav Petkov <bp@suse.de>
---
 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(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 6d2c49b..e55584a 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -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",
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index fb57d03..361177a 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -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) {
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 26311f2..bf30a12 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -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;
-- 
1.8.4.3

Re: [PATCH v4] ACPI, APEI: Cleanup alignment related codes for APEI

[Borislav Petkov]

On Wed, Dec 18, 2013 at 01:30:49AM -0500, Chen, Gong wrote:
> 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 <gong.chen@linux.intel.com>
> Acked-by: Borislav Petkov <bp@suse.de>

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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