[PATCH -v2 0/5] ACPI, APEI, BUG fixes for 2.6.36

[PATCH -v2 0/5] ACPI, APEI, BUG fixes for 2.6.36

[Huang Ying]

Hi, Len,

This patchset is version 2 of some BUG fixes about APEI for 2.6.36.
Can you merge it?

[PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking
[PATCH -v2 2/5] ACPI, APEI, Fix acpi_pre_map() return value
[PATCH -v2 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data
[PATCH -v2 4/5] ACPI, APEI, Fix error path for memory allocation
[PATCH -v2 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation

Best Regards,
Huang Ying

[PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Huang Ying]

On Huang Ying's machine:

erst_tab->header_length == sizeof(struct acpi_table_einj)

but Yinghai reported that on his machine,

erst_tab->header_length == sizeof(struct acpi_table_einj) -
sizeof(struct acpi_table_header)

To make erst table size checking code works on all systems, both
testing are treated as PASS.

Same situation applies to einj_tab->header_length, so corresponding
table size checking is changed in similar way too.

v2:

- Treat both table size as valid

Originally-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/einj.c |    4 +++-
 drivers/acpi/apei/erst.c |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -426,7 +426,9 @@ DEFINE_SIMPLE_ATTRIBUTE(error_inject_fop
 
 static int einj_check_table(struct acpi_table_einj *einj_tab)
 {
-	if (einj_tab->header_length != sizeof(struct acpi_table_einj))
+	if ((einj_tab->header_length !=
+	     (sizeof(struct acpi_table_einj) - sizeof(einj_tab->header)))
+	    && (einj_tab->header_length != sizeof(struct acpi_table_einj)))
 		return -EINVAL;
 	if (einj_tab->header.length < sizeof(struct acpi_table_einj))
 		return -EINVAL;
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -750,7 +750,9 @@ __setup("erst_disable", setup_erst_disab
 
 static int erst_check_table(struct acpi_table_erst *erst_tab)
 {
-	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+	if ((erst_tab->header_length !=
+	     (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))
+	    && (erst_tab->header_length != sizeof(struct acpi_table_einj)))
 		return -EINVAL;
 	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
 		return -EINVAL;

[PATCH -v2 2/5] ACPI, APEI, Fix acpi_pre_map() return value

[Huang Ying]

From: Jin Dongming <jin.dongming@np.css.fujitsu.com>

After we ioremap() a new region, we call __acpi_try_ioremap() to
see whether another thread has already mapped the same region.
This check clobbers "vaddr",  so compute the return value of
acpi_pre_map() using the ioremap() result "map->vaddr" instead.

v2:
    Modified the unsuitable description of patch.

v3:
    Removed unlikely() check and made description simpler.

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/atomicio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/acpi/atomicio.c
+++ b/drivers/acpi/atomicio.c
@@ -142,7 +142,7 @@ static void __iomem *acpi_pre_map(phys_a
 	list_add_tail_rcu(&map->list, &acpi_iomaps);
 	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
 
-	return vaddr + (paddr - pg_off);
+	return map->vaddr + (paddr - map->paddr);
 err_unmap:
 	iounmap(vaddr);
 	return NULL;

[PATCH -v2 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data

[Huang Ying]

From: Jin Dongming <jin.dongming@np.css.fujitsu.com>

platform_data in hest_parse_ghes() is used for saving the address of entry
information of erst_tab. When the device is failed to be added, platform_data
will be freed by platform_device_put(). But the value saved in platform_data
should not be freed here. If it is done, it will make system panic.

So I think platform_data should save the address of allocated memory
which saves entry information of erst_tab.

This patch fixed it and I confirmed it on x86_64 next-tree.

v2:
    Transport the pointer of hest_hdr to platform_data using
    platform_device_add_data()

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/ghes.c |    2 +-
 drivers/acpi/apei/hest.c |   11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -302,7 +302,7 @@ static int __devinit ghes_probe(struct p
 	struct ghes *ghes = NULL;
 	int rc = -EINVAL;
 
-	generic = ghes_dev->dev.platform_data;
+	generic = *(struct acpi_hest_generic **)ghes_dev->dev.platform_data;
 	if (!generic->enabled)
 		return -ENODEV;
 
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -137,20 +137,23 @@ static int hest_parse_ghes_count(struct
 
 static int hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
 {
-	struct acpi_hest_generic *generic;
 	struct platform_device *ghes_dev;
 	struct ghes_arr *ghes_arr = data;
 	int rc;
 
 	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR)
 		return 0;
-	generic = (struct acpi_hest_generic *)hest_hdr;
-	if (!generic->enabled)
+
+	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
 		return 0;
 	ghes_dev = platform_device_alloc("GHES", hest_hdr->source_id);
 	if (!ghes_dev)
 		return -ENOMEM;
-	ghes_dev->dev.platform_data = generic;
+
+	rc = platform_device_add_data(ghes_dev, &hest_hdr, sizeof(void *));
+	if (rc)
+		goto err;
+
 	rc = platform_device_add(ghes_dev);
 	if (rc)
 		goto err;

[PATCH -v2 4/5] ACPI, APEI, Fix error path for memory allocation

[Huang Ying]

In ERST debug/test support patch, a dynamic allocated buffer is
used. The may-failed memory allocation should be tried firstly before
free the previous buffer.

APEI resource management memory allocation related error path is fixed
too.

v2:

- Fix error messages for APEI resources management

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/apei-base.c |   21 ++++++++++++++++-----
 drivers/acpi/apei/erst-dbg.c  |   16 ++++++++++------
 2 files changed, 26 insertions(+), 11 deletions(-)

--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -445,11 +445,15 @@ EXPORT_SYMBOL_GPL(apei_resources_sub);
 int apei_resources_request(struct apei_resources *resources,
 			   const char *desc)
 {
-	struct apei_res *res, *res_bak;
+	struct apei_res *res, *res_bak = NULL;
 	struct resource *r;
+	int rc;
 
-	apei_resources_sub(resources, &apei_resources_all);
+	rc = apei_resources_sub(resources, &apei_resources_all);
+	if (rc)
+		return rc;
 
+	rc = -EINVAL;
 	list_for_each_entry(res, &resources->iomem, list) {
 		r = request_mem_region(res->start, res->end - res->start,
 				       desc);
@@ -475,7 +479,11 @@ int apei_resources_request(struct apei_r
 		}
 	}
 
-	apei_resources_merge(&apei_resources_all, resources);
+	rc = apei_resources_merge(&apei_resources_all, resources);
+	if (rc) {
+		pr_err(APEI_PFX "Fail to merge resources!\n");
+		goto err_unmap_ioport;
+	}
 
 	return 0;
 err_unmap_ioport:
@@ -491,12 +499,13 @@ err_unmap_iomem:
 			break;
 		release_mem_region(res->start, res->end - res->start);
 	}
-	return -EINVAL;
+	return rc;
 }
 EXPORT_SYMBOL_GPL(apei_resources_request);
 
 void apei_resources_release(struct apei_resources *resources)
 {
+	int rc;
 	struct apei_res *res;
 
 	list_for_each_entry(res, &resources->iomem, list)
@@ -504,7 +513,9 @@ void apei_resources_release(struct apei_
 	list_for_each_entry(res, &resources->ioport, list)
 		release_region(res->start, res->end - res->start);
 
-	apei_resources_sub(&apei_resources_all, resources);
+	rc = apei_resources_sub(&apei_resources_all, resources);
+	if (rc)
+		pr_err(APEI_PFX "Fail to sub resources!\n");
 }
 EXPORT_SYMBOL_GPL(apei_resources_release);
 
--- a/drivers/acpi/apei/erst-dbg.c
+++ b/drivers/acpi/apei/erst-dbg.c
@@ -111,11 +111,13 @@ retry:
 		goto out;
 	}
 	if (len > erst_dbg_buf_len) {
-		kfree(erst_dbg_buf);
+		void *p;
 		rc = -ENOMEM;
-		erst_dbg_buf = kmalloc(len, GFP_KERNEL);
-		if (!erst_dbg_buf)
+		p = kmalloc(len, GFP_KERNEL);
+		if (!p)
 			goto out;
+		kfree(erst_dbg_buf);
+		erst_dbg_buf = p;
 		erst_dbg_buf_len = len;
 		goto retry;
 	}
@@ -150,11 +152,13 @@ static ssize_t erst_dbg_write(struct fil
 	if (mutex_lock_interruptible(&erst_dbg_mutex))
 		return -EINTR;
 	if (usize > erst_dbg_buf_len) {
-		kfree(erst_dbg_buf);
+		void *p;
 		rc = -ENOMEM;
-		erst_dbg_buf = kmalloc(usize, GFP_KERNEL);
-		if (!erst_dbg_buf)
+		p = kmalloc(usize, GFP_KERNEL);
+		if (!p)
 			goto out;
+		kfree(erst_dbg_buf);
+		erst_dbg_buf = p;
 		erst_dbg_buf_len = usize;
 	}
 	rc = copy_from_user(erst_dbg_buf, ubuf, usize);

[PATCH -v2 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation

[Huang Ying]

The src_base and dst_base fields in apei_exec_context are physical
address, so they should be ioremaped before being used in ERST
MOVE_DATA instruction.

Reported-by: Javier Martinez Canillas <martinez.javier@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/erst.c |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -266,13 +266,30 @@ static int erst_exec_move_data(struct ap
 {
 	int rc;
 	u64 offset;
+	void *src, *dst;
+
+	/* ioremap does not work in interrupt context */
+	if (in_interrupt()) {
+		pr_warning(ERST_PFX
+			   "MOVE_DATA can not be used in interrupt context");
+		return -EBUSY;
+	}
 
 	rc = __apei_exec_read_register(entry, &offset);
 	if (rc)
 		return rc;
-	memmove((void *)(unsigned long)ctx->dst_base + offset,
-		(void *)(unsigned long)ctx->src_base + offset,
-		ctx->var2);
+
+	src = ioremap(ctx->src_base + offset, ctx->var2);
+	if (!src)
+		return -ENOMEM;
+	dst = ioremap(ctx->dst_base + offset, ctx->var2);
+	if (!dst)
+		return -ENOMEM;
+
+	memmove(dst, src, ctx->var2);
+
+	iounmap(src);
+	iounmap(dst);
 
 	return 0;
 }

Re: [PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Yinghai Lu]

On Wed, Sep 29, 2010 at 4:53 AM, Huang Ying <ying.huang@intel.com> wrote:
> On Huang Ying's machine:
>
> erst_tab->header_length == sizeof(struct acpi_table_einj)

So it seems your system firmware violates the 4.0 spec

Yinghai

Re: [PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Len Brown]

> Originally-by: Yinghai Lu <yinghai@kernel.org>

BTW.

If the patch is substantially the same as the original,
then the method to credit the original author is to
start the patch body with:

From: Yinghai Lu <yinghai@kernel.org>

and you can choose to note your changes in the signed-off part,
like akpm typically does, or not.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 2/5] ACPI, APEI, Fix acpi_pre_map() return value

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 4/5] ACPI, APEI, Fix error path for memory allocation

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Huang Ying]

Hi, Yinghai,

On Wed, 2010-09-29 at 23:47 +0800, Yinghai Lu wrote:
> On Wed, Sep 29, 2010 at 4:53 AM, Huang Ying <ying.huang@intel.com> wrote:
> > On Huang Ying's machine:
> >
> > erst_tab->header_length == sizeof(struct acpi_table_einj)
> 
> So it seems your system firmware violates the 4.0 spec

Perhaps. :)

But the machine firmware has been released, and we should support as
many machines as possible I think. Do you agree?

Best Regards,
Huang Ying

Re: [PATCH -v2 1/5] ACPI, APEI, Fix APEI related table size checking

[Huang Ying]

Hi, Len,

On Thu, 2010-09-30 at 02:01 +0800, Len Brown wrote:
> > Originally-by: Yinghai Lu <yinghai@kernel.org>
> 
> BTW.
> 
> If the patch is substantially the same as the original,
> then the method to credit the original author is to
> start the patch body with:
> 
> From: Yinghai Lu <yinghai@kernel.org>
> 
> and you can choose to note your changes in the signed-off part,
> like akpm typically does, or not.

Thanks for remaining.

I changes the logic in original patch, and don't know whether Yinghai
agree with me. So uses Originally-by: here. If I just do some minor
fixes, I will use From:

Best Regards,
Huang Ying