* [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking
2010-09-19 3:00 [PATCH 0/5] ACPI, APEI, BUG fixes for 2.6.36 Huang Ying
@ 2010-09-19 3:00 ` Huang Ying
2010-09-21 12:39 ` Andi Kleen
2010-09-19 3:00 ` [PATCH 2/5] ACPI, APEI, Fix acpi_pre_map() return value Huang Ying
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2010-09-19 3:00 UTC (permalink / raw)
To: Len Brown; +Cc: linux-kernel, Andi Kleen, linux-acpi, Yinghai Lu
From: Yinghai Lu <yinghai@kernel.org>
Both ERST and EINJ table size checking is fixed.
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/apei/einj.c | 3 ++-
drivers/acpi/apei/erst.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 465c885..b184baa 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -426,7 +426,8 @@ DEFINE_SIMPLE_ATTRIBUTE(error_inject_fops, NULL,
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)))
return -EINVAL;
if (einj_tab->header.length < sizeof(struct acpi_table_einj))
return -EINVAL;
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index a4904f1..40b01c3 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -750,7 +750,8 @@ __setup("erst_disable", setup_erst_disable);
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)))
return -EINVAL;
if (erst_tab->header.length < sizeof(struct acpi_table_erst))
return -EINVAL;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking
2010-09-19 3:00 ` [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking Huang Ying
@ 2010-09-21 12:39 ` Andi Kleen
2010-09-26 5:30 ` Huang Ying
0 siblings, 1 reply; 17+ messages in thread
From: Andi Kleen @ 2010-09-21 12:39 UTC (permalink / raw)
To: Huang Ying; +Cc: Len Brown, linux-kernel, Andi Kleen, linux-acpi, Yinghai Lu
On Sun, Sep 19, 2010 at 11:00:31AM +0800, Huang Ying wrote:
> From: Yinghai Lu <yinghai@kernel.org>
>
> Both ERST and EINJ table size checking is fixed.
Needs a better description.
> ---
> drivers/acpi/apei/einj.c | 3 ++-
> drivers/acpi/apei/erst.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 465c885..b184baa 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -426,7 +426,8 @@ DEFINE_SIMPLE_ATTRIBUTE(error_inject_fops, NULL,
>
> 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)))
I don't understand these changes. So on any system where the old check worked
before it won't work anymore? Since the code has been presumably tested
before this would break systems, won't it?
Same with the other changes.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking
2010-09-21 12:39 ` Andi Kleen
@ 2010-09-26 5:30 ` Huang Ying
0 siblings, 0 replies; 17+ messages in thread
From: Huang Ying @ 2010-09-26 5:30 UTC (permalink / raw)
To: Andi Kleen, Yinghai Lu; +Cc: Len Brown, linux-kernel, linux-acpi
On Tue, 2010-09-21 at 20:39 +0800, Andi Kleen wrote:
> On Sun, Sep 19, 2010 at 11:00:31AM +0800, Huang Ying wrote:
> > From: Yinghai Lu <yinghai@kernel.org>
> >
> > Both ERST and EINJ table size checking is fixed.
>
> Needs a better description.
> > ---
> > drivers/acpi/apei/einj.c | 3 ++-
> > drivers/acpi/apei/erst.c | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> > index 465c885..b184baa 100644
> > --- a/drivers/acpi/apei/einj.c
> > +++ b/drivers/acpi/apei/einj.c
> > @@ -426,7 +426,8 @@ DEFINE_SIMPLE_ATTRIBUTE(error_inject_fops, NULL,
> >
> > 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)))
>
> I don't understand these changes. So on any system where the old check worked
> before it won't work anymore? Since the code has been presumably tested
> before this would break systems, won't it?
>
> Same with the other changes.
Yes. It seems that old check works on my testing machine but not on
Yinghai's machine, while the new check works on Yinghai's machine and
not on my testing machine.
So I think it is better to remove the checks. Apparently, Windows does
not check that.
Best Regards,
Huang Ying
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] ACPI, APEI, Fix acpi_pre_map() return value
2010-09-19 3:00 [PATCH 0/5] ACPI, APEI, BUG fixes for 2.6.36 Huang Ying
2010-09-19 3:00 ` [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking Huang Ying
@ 2010-09-19 3:00 ` Huang Ying
2010-09-21 12:47 ` Andi Kleen
2010-09-19 3:00 ` [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data Huang Ying
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2010-09-19 3:00 UTC (permalink / raw)
To: Len Brown; +Cc: linux-kernel, Andi Kleen, linux-acpi, Jin Dongming
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>
Acked-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/atomicio.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
index 8f8bd73..542e539 100644
--- a/drivers/acpi/atomicio.c
+++ b/drivers/acpi/atomicio.c
@@ -142,7 +142,7 @@ static void __iomem *acpi_pre_map(phys_addr_t paddr,
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;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] ACPI, APEI, Fix acpi_pre_map() return value
2010-09-19 3:00 ` [PATCH 2/5] ACPI, APEI, Fix acpi_pre_map() return value Huang Ying
@ 2010-09-21 12:47 ` Andi Kleen
0 siblings, 0 replies; 17+ messages in thread
From: Andi Kleen @ 2010-09-21 12:47 UTC (permalink / raw)
To: Huang Ying; +Cc: Len Brown, linux-kernel, Andi Kleen, linux-acpi, Jin Dongming
On Sun, Sep 19, 2010 at 11:00:32AM +0800, Huang Ying wrote:
> 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.
Reviewed-by: Andi Kleen <ak@linux.intel.com>
-Andi
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data
2010-09-19 3:00 [PATCH 0/5] ACPI, APEI, BUG fixes for 2.6.36 Huang Ying
2010-09-19 3:00 ` [PATCH 1/5] ACPI, APEI, Fix APEI related table size checking Huang Ying
2010-09-19 3:00 ` [PATCH 2/5] ACPI, APEI, Fix acpi_pre_map() return value Huang Ying
@ 2010-09-19 3:00 ` Huang Ying
2010-09-21 12:43 ` Andi Kleen
2010-09-19 3:00 ` [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation Huang Ying
2010-09-19 3:00 ` [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation Huang Ying
4 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2010-09-19 3:00 UTC (permalink / raw)
To: Len Brown; +Cc: linux-kernel, Andi Kleen, linux-acpi, Jin Dongming
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>
Acked-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(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 385a605..0d505e5 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -302,7 +302,7 @@ static int __devinit ghes_probe(struct platform_device *ghes_dev)
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;
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 343168d..1a3508a 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -137,20 +137,23 @@ static int hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void *data)
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;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data
2010-09-19 3:00 ` [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data Huang Ying
@ 2010-09-21 12:43 ` Andi Kleen
2010-09-22 1:05 ` Jin Dongming
2010-09-23 9:06 ` huang ying
0 siblings, 2 replies; 17+ messages in thread
From: Andi Kleen @ 2010-09-21 12:43 UTC (permalink / raw)
To: Huang Ying; +Cc: Len Brown, linux-kernel, Andi Kleen, linux-acpi, Jin Dongming
On Sun, Sep 19, 2010 at 11:00:33AM +0800, Huang Ying wrote:
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 343168d..1a3508a 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -137,20 +137,23 @@ static int hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void *data)
>
> static int hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
hest_hdr is a local variable on the stack.
> +
> + rc = platform_device_add_data(ghes_dev, &hest_hdr, sizeof(void *));
> + if (rc)
> + goto err;
Now you put the address of that local variable in some global data structure.
And then you likely return and later the access accesses random stack space?
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data
2010-09-21 12:43 ` Andi Kleen
@ 2010-09-22 1:05 ` Jin Dongming
2010-09-23 9:06 ` huang ying
1 sibling, 0 replies; 17+ messages in thread
From: Jin Dongming @ 2010-09-22 1:05 UTC (permalink / raw)
To: Andi Kleen; +Cc: Huang Ying, Len Brown, linux-kernel, linux-acpi
Hi, Andi
(2010/09/21 21:43), Andi Kleen wrote:
> On Sun, Sep 19, 2010 at 11:00:33AM +0800, Huang Ying wrote:
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index 343168d..1a3508a 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -137,20 +137,23 @@ static int hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void *data)
>>
>> static int hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
>
> hest_hdr is a local variable on the stack.
>
Yes, you are right. Exactly what hest_hdr pointed is ghes address of
global static variable hest_tab.
>
>> +
>> + rc = platform_device_add_data(ghes_dev, &hest_hdr, sizeof(void *));
>> + if (rc)
>> + goto err;
>
> Now you put the address of that local variable in some global data structure.
>
> And then you likely return and later the access accesses random stack space?
>
No, I don't think so.
Here the value(ghes address) of hest_hdr is saved into the private area of ghes_dev,
not the address of hest_hdr itself. So what you described will not happen.
Best Regards,
Jin Dongming
> -Andi
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data
2010-09-21 12:43 ` Andi Kleen
2010-09-22 1:05 ` Jin Dongming
@ 2010-09-23 9:06 ` huang ying
1 sibling, 0 replies; 17+ messages in thread
From: huang ying @ 2010-09-23 9:06 UTC (permalink / raw)
To: Andi Kleen; +Cc: Huang Ying, Len Brown, linux-kernel, linux-acpi, Jin Dongming
Hi, Andi,
On Tue, Sep 21, 2010 at 8:43 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sun, Sep 19, 2010 at 11:00:33AM +0800, Huang Ying wrote:
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index 343168d..1a3508a 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -137,20 +137,23 @@ static int hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void *data)
>>
>> static int hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
>
> hest_hdr is a local variable on the stack.
>
>
>> +
>> + rc = platform_device_add_data(ghes_dev, &hest_hdr, sizeof(void *));
>> + if (rc)
>> + goto err;
>
> Now you put the address of that local variable in some global data structure.
>
> And then you likely return and later the access accesses random stack space?
platform_device_add_data will allocate some new memory, and copy
hest_hdr to there, so this is safe.
Best Regards,
Huang Ying
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation
2010-09-19 3:00 [PATCH 0/5] ACPI, APEI, BUG fixes for 2.6.36 Huang Ying
` (2 preceding siblings ...)
2010-09-19 3:00 ` [PATCH 3/5] ACPI, APEI, HEST Fix the unsuitable usage of platform_data Huang Ying
@ 2010-09-19 3:00 ` Huang Ying
2010-09-21 12:45 ` Andi Kleen
2010-09-19 3:00 ` [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation Huang Ying
4 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2010-09-19 3:00 UTC (permalink / raw)
To: Len Brown; +Cc: linux-kernel, Andi Kleen, linux-acpi, 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.
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/apei/apei-base.c | 25 ++++++++++++++++++++-----
drivers/acpi/apei/erst-dbg.c | 16 ++++++++++------
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 73fd0c7..6bf11f9 100644
--- 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,13 @@ int apei_resources_request(struct apei_resources *resources,
}
}
- apei_resources_merge(&apei_resources_all, resources);
+ rc = apei_resources_merge(&apei_resources_all, resources);
+ if (rc) {
+ pr_err(APEI_PFX
+"Error in APEI internal resource management, there may be inconsistent "
+"between APEI internal and system resource management.");
+ goto err_unmap_ioport;
+ }
return 0;
err_unmap_ioport:
@@ -491,12 +501,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 +515,11 @@ void apei_resources_release(struct apei_resources *resources)
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
+"Error in APEI internal resource management, there may be inconsistent "
+"between APEI internal and system resource management.");
}
EXPORT_SYMBOL_GPL(apei_resources_release);
diff --git a/drivers/acpi/apei/erst-dbg.c b/drivers/acpi/apei/erst-dbg.c
index 98ffa29..da1228a 100644
--- 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 file *filp, const char __user *ubuf,
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);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation
2010-09-19 3:00 ` [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation Huang Ying
@ 2010-09-21 12:45 ` Andi Kleen
2010-09-23 8:53 ` huang ying
0 siblings, 1 reply; 17+ messages in thread
From: Andi Kleen @ 2010-09-21 12:45 UTC (permalink / raw)
To: Huang Ying; +Cc: Len Brown, linux-kernel, Andi Kleen, linux-acpi
On Sun, Sep 19, 2010 at 11:00:34AM +0800, Huang Ying wrote:
> @@ -475,7 +479,13 @@ int apei_resources_request(struct apei_resources *resources,
> }
> }
>
> - apei_resources_merge(&apei_resources_all, resources);
> + rc = apei_resources_merge(&apei_resources_all, resources);
> + if (rc) {
> + pr_err(APEI_PFX
> +"Error in APEI internal resource management, there may be inconsistent "
> +"between APEI internal and system resource management.");
Please fix the English. This will wrap in the syslog without \ns no?
Kernel messges should be not wider than 80 characters.
> + rc = apei_resources_sub(&apei_resources_all, resources);
> + if (rc)
> + pr_err(APEI_PFX
> +"Error in APEI internal resource management, there may be inconsistent "
> +"between APEI internal and system resource management.");
Same.
Also instead of Error it would be better just say what is wrong and don't
try to analyze the problem.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation
2010-09-21 12:45 ` Andi Kleen
@ 2010-09-23 8:53 ` huang ying
0 siblings, 0 replies; 17+ messages in thread
From: huang ying @ 2010-09-23 8:53 UTC (permalink / raw)
To: Andi Kleen; +Cc: Huang Ying, Len Brown, linux-kernel, linux-acpi
Hi, Andi,
On Tue, Sep 21, 2010 at 8:45 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sun, Sep 19, 2010 at 11:00:34AM +0800, Huang Ying wrote:
>> @@ -475,7 +479,13 @@ int apei_resources_request(struct apei_resources *resources,
>> }
>> }
>>
>> - apei_resources_merge(&apei_resources_all, resources);
>> + rc = apei_resources_merge(&apei_resources_all, resources);
>> + if (rc) {
>> + pr_err(APEI_PFX
>> +"Error in APEI internal resource management, there may be inconsistent "
>> +"between APEI internal and system resource management.");
>
> Please fix the English. This will wrap in the syslog without \ns no?
> Kernel messges should be not wider than 80 characters.
>
>> + rc = apei_resources_sub(&apei_resources_all, resources);
>> + if (rc)
>> + pr_err(APEI_PFX
>> +"Error in APEI internal resource management, there may be inconsistent "
>> +"between APEI internal and system resource management.");
>
> Same.
>
> Also instead of Error it would be better just say what is wrong and don't
> try to analyze the problem.
OK, I will do it.
Best Regards,
Huang Ying
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation
2010-09-19 3:00 [PATCH 0/5] ACPI, APEI, BUG fixes for 2.6.36 Huang Ying
` (3 preceding siblings ...)
2010-09-19 3:00 ` [PATCH 4/5] ACPI, APEI, Fix error path for memory allocation Huang Ying
@ 2010-09-19 3:00 ` Huang Ying
2010-09-21 12:47 ` Andi Kleen
4 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2010-09-19 3:00 UTC (permalink / raw)
To: Len Brown; +Cc: linux-kernel, Andi Kleen, linux-acpi, 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 | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 40b01c3..a103963 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -266,13 +266,27 @@ static int erst_exec_move_data(struct apei_exec_context *ctx,
{
int rc;
u64 offset;
+ void *src, *dst;
+
+ /* ioremap does not work in interrupt context */
+ if (in_interrupt())
+ return -EBUSY;
rc = __apei_exec_read_register(entry, &offset);
if (rc)
return rc;
- memmove((void *)ctx->dst_base + offset,
- (void *)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;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation
2010-09-19 3:00 ` [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation Huang Ying
@ 2010-09-21 12:47 ` Andi Kleen
2010-09-23 9:04 ` huang ying
0 siblings, 1 reply; 17+ messages in thread
From: Andi Kleen @ 2010-09-21 12:47 UTC (permalink / raw)
To: Huang Ying; +Cc: Len Brown, linux-kernel, Andi Kleen, linux-acpi
On Sun, Sep 19, 2010 at 11:00:35AM +0800, Huang Ying wrote:
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 40b01c3..a103963 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -266,13 +266,27 @@ static int erst_exec_move_data(struct apei_exec_context *ctx,
> {
> int rc;
> u64 offset;
> + void *src, *dst;
> +
> + /* ioremap does not work in interrupt context */
> + if (in_interrupt())
> + return -EBUSY;
That breaks serialization of machine checks no? If the BIOS uses MOVE_DATA
How about using kmap_atomic instead?
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation
2010-09-21 12:47 ` Andi Kleen
@ 2010-09-23 9:04 ` huang ying
2010-09-27 5:31 ` Andi Kleen
0 siblings, 1 reply; 17+ messages in thread
From: huang ying @ 2010-09-23 9:04 UTC (permalink / raw)
To: Andi Kleen; +Cc: Huang Ying, Len Brown, linux-kernel, linux-acpi
Hi,
On Tue, Sep 21, 2010 at 8:47 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sun, Sep 19, 2010 at 11:00:35AM +0800, Huang Ying wrote:
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 40b01c3..a103963 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -266,13 +266,27 @@ static int erst_exec_move_data(struct apei_exec_context *ctx,
>> {
>> int rc;
>> u64 offset;
>> + void *src, *dst;
>> +
>> + /* ioremap does not work in interrupt context */
>> + if (in_interrupt())
>> + return -EBUSY;
>
> That breaks serialization of machine checks no? If the BIOS uses MOVE_DATA
> How about using kmap_atomic instead?
In my test machine, MOVE_DATA is not used. So this does not breaks
serialization of machine checks.
We can not use kmap_atomic or kmap_atomic_pfn here. Because
kmap_atomic_xxx needs struct page, but the address provided by BIOS
may be in E820_RESERVED area, that is, the corresponding page may have
no struct page. Maybe we need another kmap_atomic implementation for
E820_RESERVED area.
Best Regards,
Huang Ying
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] ACPI, APEI, Fix ERST MOVE_DATA instruction implementation
2010-09-23 9:04 ` huang ying
@ 2010-09-27 5:31 ` Andi Kleen
0 siblings, 0 replies; 17+ messages in thread
From: Andi Kleen @ 2010-09-27 5:31 UTC (permalink / raw)
To: huang ying; +Cc: Andi Kleen, Huang Ying, Len Brown, linux-kernel, linux-acpi
> In my test machine, MOVE_DATA is not used. So this does not breaks
> serialization of machine checks.
Ok. But we don't know what other systems are doing.
>
> We can not use kmap_atomic or kmap_atomic_pfn here. Because
> kmap_atomic_xxx needs struct page, but the address provided by BIOS
> may be in E820_RESERVED area, that is, the corresponding page may have
> no struct page. Maybe we need another kmap_atomic implementation for
> E820_RESERVED area.
Yes, at some point that may be needed.
It would be good to have some kind of warning that gets reported
back to us when this happens. This should happen on panicing situations
anyways, so we can printk.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 17+ messages in thread