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@xxxxxxxxxxxxxxx>
> ---
> 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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
