* Parameter check for EINJ error injection
@ 2013-05-16 2:29 Chen Gong
2013-05-16 2:29 ` [PATCH 1/2] ACPI/APEI: Add parameter check before " Chen Gong
2013-05-16 2:29 ` [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2 Chen Gong
0 siblings, 2 replies; 9+ messages in thread
From: Chen Gong @ 2013-05-16 2:29 UTC (permalink / raw)
To: tony.luck, bp; +Cc: linux-acpi
[PATCH 1/2] ACPI/APEI: Add parameter check before error injection
[PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2
There two patches are employed to enhance the check for param1/param2
when einj is used. Param1/param2 are very subtle for the guy
who lacks of experience. These two patches add some extra checks to
avoid happening further errors to confuse users.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] ACPI/APEI: Add parameter check before error injection
2013-05-16 2:29 Parameter check for EINJ error injection Chen Gong
@ 2013-05-16 2:29 ` Chen Gong
2013-05-16 11:05 ` Borislav Petkov
2013-05-16 2:29 ` [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2 Chen Gong
1 sibling, 1 reply; 9+ messages in thread
From: Chen Gong @ 2013-05-16 2:29 UTC (permalink / raw)
To: tony.luck, bp; +Cc: linux-acpi, Chen Gong
When param1 is enabled in EINJ but not assigned with a valid
value, sometimes it will cause the error like below:
APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger
registers
It is because some firmware will access target address specified in
param1 to trigger the error when injecting memory error. This will
cause resource conflict with regular memory. So It must be removed
from trigger table resources, but uncorrected param1/param2
combination will stop this action. Add extra check to avoid
happening this error.
Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
drivers/acpi/apei/einj.c | 29 +++++++++++++++++++++++++----
kernel/resource.c | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 8d457b5..015546e 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -32,6 +32,7 @@
#include <linux/seq_file.h>
#include <linux/nmi.h>
#include <linux/delay.h>
+#include <linux/mm.h>
#include <acpi/acpi.h>
#include "apei-internal.h"
@@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
/* Inject the specified hardware error */
static int einj_error_inject(u32 type, u64 param1, u64 param2)
{
- int rc;
+ int rc = 0;
+ unsigned long pfn;
+ bool checkparam = false;
+ u64 param2_mask = 0xfffffffffffff000;
- mutex_lock(&einj_mutex);
- rc = __einj_error_inject(type, param1, param2);
- mutex_unlock(&einj_mutex);
+ /* ensure param1/param2 existed & injection is memory related */
+ if (param_extension || acpi5) {
+ if (type & 0x80000000) {
+ if (vendor_flags == SETWA_FLAGS_MEM)
+ checkparam = true;
+ } else if (type & 0x38)
+ checkparam = true;
+ }
+ if (checkparam) {
+ pfn = PFN_DOWN(param1 & param2);
+ if (!page_is_ram(pfn) ||
+ ((param2 & param2_mask) != param2_mask))
+ rc = -EINVAL;
+ }
+
+ if (rc == 0) {
+ mutex_lock(&einj_mutex);
+ rc = __einj_error_inject(type, param1, param2);
+ mutex_unlock(&einj_mutex);
+ }
return rc;
}
diff --git a/kernel/resource.c b/kernel/resource.c
index d738698..77bf11a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -409,6 +409,7 @@ int __weak page_is_ram(unsigned long pfn)
{
return walk_system_ram_range(pfn, 1, NULL, __is_ram) == 1;
}
+EXPORT_SYMBOL_GPL(page_is_ram);
void __weak arch_remove_reservations(struct resource *avail)
{
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2
2013-05-16 2:29 Parameter check for EINJ error injection Chen Gong
2013-05-16 2:29 ` [PATCH 1/2] ACPI/APEI: Add parameter check before " Chen Gong
@ 2013-05-16 2:29 ` Chen Gong
2013-05-16 11:07 ` Borislav Petkov
1 sibling, 1 reply; 9+ messages in thread
From: Chen Gong @ 2013-05-16 2:29 UTC (permalink / raw)
To: tony.luck, bp; +Cc: linux-acpi, Chen Gong
To ensure EINJ working well when injecting errors via EINJ
table, add some attentions for param1/param2.
Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
Documentation/acpi/apei/einj.txt | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/acpi/apei/einj.txt b/Documentation/acpi/apei/einj.txt
index e20b6da..80c4ab2 100644
--- a/Documentation/acpi/apei/einj.txt
+++ b/Documentation/acpi/apei/einj.txt
@@ -47,11 +47,15 @@ directory apei/einj. The following files are provided.
- param1
This file is used to set the first error parameter value. Effect of
- parameter depends on error_type specified.
+ parameter depends on error_type specified. For exmaple, if error
+ type is memory relatd type, the param1 should be a valid physical
+ memory address.
- param2
This file is used to set the second error parameter value. Effect of
- parameter depends on error_type specified.
+ parameter depends on error_type specified. For example, if error
+ type is memory relatd type, the param2 should be a valid physical
+ memory address mask, say, 0xfffffffffffff000.
- notrigger
The EINJ mechanism is a two step process. First inject the error, then
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error injection
2013-05-16 2:29 ` [PATCH 1/2] ACPI/APEI: Add parameter check before " Chen Gong
@ 2013-05-16 11:05 ` Borislav Petkov
2013-05-17 5:09 ` Chen Gong
0 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2013-05-16 11:05 UTC (permalink / raw)
To: Chen Gong; +Cc: tony.luck, linux-acpi
On Wed, May 15, 2013 at 10:29:06PM -0400, Chen Gong wrote:
> When param1 is enabled in EINJ but not assigned with a valid
> value, sometimes it will cause the error like below:
>
> APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger
> registers
>
> It is because some firmware will access target address specified in
> param1 to trigger the error when injecting memory error. This will
> cause resource conflict with regular memory. So It must be removed
> from trigger table resources, but uncorrected param1/param2
> combination will stop this action. Add extra check to avoid
> happening this error.
>
> Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
> ---
> drivers/acpi/apei/einj.c | 29 +++++++++++++++++++++++++----
> kernel/resource.c | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 8d457b5..015546e 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -32,6 +32,7 @@
> #include <linux/seq_file.h>
> #include <linux/nmi.h>
> #include <linux/delay.h>
> +#include <linux/mm.h>
> #include <acpi/acpi.h>
>
> #include "apei-internal.h"
> @@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
> /* Inject the specified hardware error */
> static int einj_error_inject(u32 type, u64 param1, u64 param2)
> {
> - int rc;
> + int rc = 0;
> + unsigned long pfn;
> + bool checkparam = false;
> + u64 param2_mask = 0xfffffffffffff000;
>
> - mutex_lock(&einj_mutex);
> - rc = __einj_error_inject(type, param1, param2);
> - mutex_unlock(&einj_mutex);
> + /* ensure param1/param2 existed & injection is memory related */
> + if (param_extension || acpi5) {
> + if (type & 0x80000000) {
if (type & BIT(31)) {
better readable.
even better if you define what bit 31 is
#define INJ_TYPE BIT(31)
> + if (vendor_flags == SETWA_FLAGS_MEM)
> + checkparam = true;
> + } else if (type & 0x38)
what is 0x38? Also a macro define with readable name pls.
> + checkparam = true;
> + }
> + if (checkparam) {
> + pfn = PFN_DOWN(param1 & param2);
> + if (!page_is_ram(pfn) ||
> + ((param2 & param2_mask) != param2_mask))
Hmm, shouldn't this be:
(param2 & param2_mask) != param2
?
> + rc = -EINVAL;
> + }
This checkparam variable looks unneeded to me:
if (type & INJ_TYPE) {
if (vendor_flags == SETWA_FLAGS_MEM || type & 0x38) {
...
Ok, I went and rewrote the function to show you what I mean. I've
also reversed the logic to flatten the indentation level (completely
untested, of course):
static int einj_error_inject(u32 type, u64 param1, u64 param2)
{
int rc;
unsigned long pfn;
/* ensure param1/param2 exist and we inject to real memory */
if (!(param_extension || acpi5))
goto inject;
if (type & INJ_TYPE)
if (vendor_flags != SETWA_FLAGS_MEM)
goto inject;
else if (!(type & THREE_BIT_MASK))
goto inject;
pfn = PFN_DOWN(param1 & param2);
if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != param2))
return -EINVAL;
inject:
mutex_lock(&einj_mutex);
rc = __einj_error_inject(type, param1, param2);
mutex_unlock(&einj_mutex);
return rc;
}
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2
2013-05-16 2:29 ` [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2 Chen Gong
@ 2013-05-16 11:07 ` Borislav Petkov
2013-05-17 5:10 ` Chen Gong
0 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2013-05-16 11:07 UTC (permalink / raw)
To: Chen Gong; +Cc: tony.luck, linux-acpi
On Wed, May 15, 2013 at 10:29:07PM -0400, Chen Gong wrote:
> To ensure EINJ working well when injecting errors via EINJ
> table, add some attentions for param1/param2.
>
> Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
> ---
> Documentation/acpi/apei/einj.txt | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/acpi/apei/einj.txt b/Documentation/acpi/apei/einj.txt
> index e20b6da..80c4ab2 100644
> --- a/Documentation/acpi/apei/einj.txt
> +++ b/Documentation/acpi/apei/einj.txt
> @@ -47,11 +47,15 @@ directory apei/einj. The following files are provided.
>
> - param1
> This file is used to set the first error parameter value. Effect of
> - parameter depends on error_type specified.
> + parameter depends on error_type specified. For exmaple, if error
> + type is memory relatd type, the param1 should be a valid physical
> + memory address.
>
> - param2
> This file is used to set the second error parameter value. Effect of
> - parameter depends on error_type specified.
> + parameter depends on error_type specified. For example, if error
> + type is memory relatd type, the param2 should be a valid physical
> + memory address mask, say, 0xfffffffffffff000.
>
> - notrigger
> The EINJ mechanism is a two step process. First inject the error, then
Please run those through the spell checker.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error injection
2013-05-16 11:05 ` Borislav Petkov
@ 2013-05-17 5:09 ` Chen Gong
2013-05-17 9:07 ` Borislav Petkov
0 siblings, 1 reply; 9+ messages in thread
From: Chen Gong @ 2013-05-17 5:09 UTC (permalink / raw)
To: Borislav Petkov; +Cc: tony.luck, linux-acpi
[-- Attachment #1: Type: text/plain, Size: 8135 bytes --]
On Thu, May 16, 2013 at 01:05:03PM +0200, Borislav Petkov wrote:
> Date: Thu, 16 May 2013 13:05:03 +0200
> 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 1/2] ACPI/APEI: Add parameter check before error
> injection
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Wed, May 15, 2013 at 10:29:06PM -0400, Chen Gong wrote:
> > When param1 is enabled in EINJ but not assigned with a valid
> > value, sometimes it will cause the error like below:
> >
> > APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger
> > registers
> >
> > It is because some firmware will access target address specified in
> > param1 to trigger the error when injecting memory error. This will
> > cause resource conflict with regular memory. So It must be removed
> > from trigger table resources, but uncorrected param1/param2
> > combination will stop this action. Add extra check to avoid
> > happening this error.
> >
> > Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
> > ---
> > drivers/acpi/apei/einj.c | 29 +++++++++++++++++++++++++----
> > kernel/resource.c | 1 +
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> > index 8d457b5..015546e 100644
> > --- a/drivers/acpi/apei/einj.c
> > +++ b/drivers/acpi/apei/einj.c
> > @@ -32,6 +32,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/nmi.h>
> > #include <linux/delay.h>
> > +#include <linux/mm.h>
> > #include <acpi/acpi.h>
> >
> > #include "apei-internal.h"
> > @@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
> > /* Inject the specified hardware error */
> > static int einj_error_inject(u32 type, u64 param1, u64 param2)
> > {
> > - int rc;
> > + int rc = 0;
> > + unsigned long pfn;
> > + bool checkparam = false;
> > + u64 param2_mask = 0xfffffffffffff000;
> >
> > - mutex_lock(&einj_mutex);
> > - rc = __einj_error_inject(type, param1, param2);
> > - mutex_unlock(&einj_mutex);
> > + /* ensure param1/param2 existed & injection is memory related */
> > + if (param_extension || acpi5) {
> > + if (type & 0x80000000) {
>
> if (type & BIT(31)) {
>
> better readable.
>
> even better if you define what bit 31 is
>
> #define INJ_TYPE BIT(31)
>
> > + if (vendor_flags == SETWA_FLAGS_MEM)
> > + checkparam = true;
> > + } else if (type & 0x38)
>
> what is 0x38? Also a macro define with readable name pls.
>
> > + checkparam = true;
> > + }
> > + if (checkparam) {
> > + pfn = PFN_DOWN(param1 & param2);
> > + if (!page_is_ram(pfn) ||
> > + ((param2 & param2_mask) != param2_mask))
>
> Hmm, shouldn't this be:
>
> (param2 & param2_mask) != param2
>
> ?
>
> > + rc = -EINVAL;
> > + }
>
> This checkparam variable looks unneeded to me:
>
> if (type & INJ_TYPE) {
> if (vendor_flags == SETWA_FLAGS_MEM || type & 0x38) {
> ...
>
> Ok, I went and rewrote the function to show you what I mean. I've
> also reversed the logic to flatten the indentation level (completely
> untested, of course):
>
> static int einj_error_inject(u32 type, u64 param1, u64 param2)
> {
> int rc;
> unsigned long pfn;
>
> /* ensure param1/param2 exist and we inject to real memory */
> if (!(param_extension || acpi5))
> goto inject;
>
> if (type & INJ_TYPE)
> if (vendor_flags != SETWA_FLAGS_MEM)
> goto inject;
> else if (!(type & THREE_BIT_MASK))
> goto inject;
>
> pfn = PFN_DOWN(param1 & param2);
> if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != param2))
> return -EINVAL;
>
> inject:
> mutex_lock(&einj_mutex);
> rc = __einj_error_inject(type, param1, param2);
> mutex_unlock(&einj_mutex);
>
> return rc;
> }
>
> --
> Regards/Gruss,
> Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --
Hi, Boris
Thanks for your review. I agree with you about readability enhancement.
IMHO, I don't think your attached patch has obvious improvment. I paste
my updated patch here and please continue to review. I will send updated
patch based on your further review.
In this patch, I update previous confused definition at the same time.
They are very little and no ill-efect so I don't write a new patch
for it.
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 8d457b5..dba59be 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -32,6 +32,7 @@
#include <linux/seq_file.h>
#include <linux/nmi.h>
#include <linux/delay.h>
+#include <linux/mm.h>
#include <acpi/acpi.h>
#include "apei-internal.h"
@@ -41,6 +42,11 @@
#define SPIN_UNIT 100 /* 100ns */
/* Firmware should respond within 1 milliseconds */
#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC)
+#define ACPI5_VENDOR_BIT BIT(31)
+#define PAGE_MASK 0xfffffffffffff000
+#define MEM_ERROR_MASK (ACPI_EINJ_MEMORY_CORRECTABLE | \
+ ACPI_EINJ_MEMORY_UNCORRECTABLE | \
+ ACPI_EINJ_MEMORY_FATAL)
/*
* ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
@@ -367,7 +373,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
* This will cause resource conflict with regular memory. So
* remove it from trigger table resources.
*/
- if ((param_extension || acpi5) && (type & 0x0038) && param2) {
+ if ((param_extension || acpi5) && (type & MEM_ERROR_MASK) && param2) {
struct apei_resources addr_resources;
apei_resources_init(&addr_resources);
trigger_param_region = einj_get_trigger_parameter_region(
@@ -427,7 +433,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
struct set_error_type_with_address *v5param = einj_param;
tototototoram->type = type;
- if (type & 0x80000000) {
+ if (type & ACPI5_VENDOR_BIT) {
switch (vendor_flags) {
case SETWA_FLAGS_APICID:
v5param->apicid = param1;
@@ -512,6 +518,25 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
static int einj_error_inject(u32 type, u64 param1, u64 param2)
{
int rc;
+ unsigned long pfn;
+ bool checkparam;
+
+ /* ensure param1/param2 existed & injection is memory related */
+ if (param_extension || acpi5) {
+ checkparam = false;
+ if (type & ACPI5_VENDOR_BIT) {
+ if (vendor_flags == SETWA_FLAGS_MEM)
+ checkparam = true;
+ } else if (type & MEM_ERROR_MASK)
+ checkparam = true;
+
+ if (checkparam) {
+ pfn = PFN_DOWN(param1 & param2);
+ if (!page_is_ram(pfn) ||
+ ((param2 & PAGE_MASK) != PAGE_MASK))
+ return -EINVAL;
+ }
+ }
mutex_lock(&einj_mutex);
rc = __einj_error_inject(type, param1, param2);
@@ -590,7 +615,7 @@ static int error_type_set(void *data, u64 val)
* Vendor defined types have 0x80000000 bit set, and
* are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
*/
- vendor = val & 0x80000000;
+ vendor = val & ACPI5_VENDOR_BIT;
tval = val & 0x7fffffff;
/* Only one error type can be specified */
diff --git a/kernel/resource.c b/kernel/resource.c
index d738698..77bf11a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -409,6 +409,7 @@ int __weak page_is_ram(unsigned long pfn)
{
return walk_system_ram_range(pfn, 1, NULL, __is_ram) == 1;
}
+EXPORT_SYMBOL_GPL(page_is_ram);
void __weak arch_remove_reservations(struct resource *avail)
{
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2
2013-05-16 11:07 ` Borislav Petkov
@ 2013-05-17 5:10 ` Chen Gong
0 siblings, 0 replies; 9+ messages in thread
From: Chen Gong @ 2013-05-17 5:10 UTC (permalink / raw)
To: Borislav Petkov; +Cc: tony.luck, linux-acpi
[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]
On Thu, May 16, 2013 at 01:07:05PM +0200, Borislav Petkov wrote:
> Date: Thu, 16 May 2013 13:07:05 +0200
> 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 2/2] ACPI/APEI: Update einj documentation for
> param1/param2
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Wed, May 15, 2013 at 10:29:07PM -0400, Chen Gong wrote:
> > To ensure EINJ working well when injecting errors via EINJ
> > table, add some attentions for param1/param2.
> >
> > Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
> > ---
> > Documentation/acpi/apei/einj.txt | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/acpi/apei/einj.txt b/Documentation/acpi/apei/einj.txt
> > index e20b6da..80c4ab2 100644
> > --- a/Documentation/acpi/apei/einj.txt
> > +++ b/Documentation/acpi/apei/einj.txt
> > @@ -47,11 +47,15 @@ directory apei/einj. The following files are provided.
> >
> > - param1
> > This file is used to set the first error parameter value. Effect of
> > - parameter depends on error_type specified.
> > + parameter depends on error_type specified. For exmaple, if error
> > + type is memory relatd type, the param1 should be a valid physical
> > + memory address.
> >
> > - param2
> > This file is used to set the second error parameter value. Effect of
> > - parameter depends on error_type specified.
> > + parameter depends on error_type specified. For example, if error
> > + type is memory relatd type, the param2 should be a valid physical
> > + memory address mask, say, 0xfffffffffffff000.
> >
> > - notrigger
> > The EINJ mechanism is a two step process. First inject the error, then
>
> Please run those through the spell checker.
>
OK, I will(exmaple & relatd).
> Thanks.
>
> --
> 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error injection
2013-05-17 5:09 ` Chen Gong
@ 2013-05-17 9:07 ` Borislav Petkov
2013-05-20 2:13 ` Chen Gong
0 siblings, 1 reply; 9+ messages in thread
From: Borislav Petkov @ 2013-05-17 9:07 UTC (permalink / raw)
To: tony.luck, linux-acpi
On Fri, May 17, 2013 at 01:09:32AM -0400, Chen Gong wrote:
> IMHO, I don't think your attached patch has obvious improvment. I
> paste my updated patch here and please continue to review. I will send
> updated patch based on your further review.
I already did my review. Having a bool checkparam and querying it
straight afterwards is clumsy code. Also you're adding a redundant
definiton of PAGE_MASK even though it already exists in the kernel.
Maybe you should've grepped the sources first...
Not to mention that this check seems wrong:
((param2 & PAGE_MASK) != PAGE_MASK)
But hey, I have better things to do instead of wasting time with this.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error injection
2013-05-17 9:07 ` Borislav Petkov
@ 2013-05-20 2:13 ` Chen Gong
0 siblings, 0 replies; 9+ messages in thread
From: Chen Gong @ 2013-05-20 2:13 UTC (permalink / raw)
To: Borislav Petkov; +Cc: tony.luck, linux-acpi
[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]
On Fri, May 17, 2013 at 11:07:16AM +0200, Borislav Petkov wrote:
> Date: Fri, 17 May 2013 11:07:16 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: tony.luck@intel.com, linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error
> injection
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Fri, May 17, 2013 at 01:09:32AM -0400, Chen Gong wrote:
> > IMHO, I don't think your attached patch has obvious improvment. I
> > paste my updated patch here and please continue to review. I will send
> > updated patch based on your further review.
>
> I already did my review. Having a bool checkparam and querying it
> straight afterwards is clumsy code. Also you're adding a redundant
> definiton of PAGE_MASK even though it already exists in the kernel.
> Maybe you should've grepped the sources first...
>
> Not to mention that this check seems wrong:
>
> ((param2 & PAGE_MASK) != PAGE_MASK)
Oops, it's my fault. I missed this stupid error. One reason is I like
to use Ctrl + P in VIM to match words, meanwhile, I often set ignorecase
in VIM so that I hitted this fault.
I accept your suggestion and resend it soon.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-20 2:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-16 2:29 Parameter check for EINJ error injection Chen Gong
2013-05-16 2:29 ` [PATCH 1/2] ACPI/APEI: Add parameter check before " Chen Gong
2013-05-16 11:05 ` Borislav Petkov
2013-05-17 5:09 ` Chen Gong
2013-05-17 9:07 ` Borislav Petkov
2013-05-20 2:13 ` Chen Gong
2013-05-16 2:29 ` [PATCH 2/2] ACPI/APEI: Update einj documentation for param1/param2 Chen Gong
2013-05-16 11:07 ` Borislav Petkov
2013-05-17 5:10 ` Chen Gong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).