[RFC 1/2] ACPI, APEI, Add apei_exec_run_optional

[RFC 1/2] ACPI, APEI, Add apei_exec_run_optional

[Huang Ying]

Some actions in APEI ERST and EINJ tables are optional, for example,
ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
error injection, and firmware may choose to do nothing here.  While
some other actions are mandatory, for example, firmware must provide
ACPI_EINJ_GET_ERROR_TYPE implementation.

Original implementation treats all actions as optional (that is, can
have no instructions), that may cause issue if firmware does not
provide some mandatory actions.  To fix this, this patch adds
apei_exec_run_optional, which should be used for optional actions.
The original apei_exec_run should be used for mandatory actions.

Cc: Thomas Renninger <trenn@novell.com>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/apei-base.c     |    9 +++++----
 drivers/acpi/apei/apei-internal.h |   13 ++++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -157,9 +157,10 @@ EXPORT_SYMBOL_GPL(apei_exec_noop);
  * Interpret the specified action. Go through whole action table,
  * execute all instructions belong to the action.
  */
-int apei_exec_run(struct apei_exec_context *ctx, u8 action)
+int __apei_exec_run(struct apei_exec_context *ctx, u8 action,
+		    bool optional)
 {
-	int rc;
+	int rc = -ENOENT;
 	u32 i, ip;
 	struct acpi_whea_header *entry;
 	apei_exec_ins_func_t run;
@@ -198,9 +199,9 @@ rewind:
 			goto rewind;
 	}
 
-	return 0;
+	return !optional && rc < 0 ? rc : 0;
 }
-EXPORT_SYMBOL_GPL(apei_exec_run);
+EXPORT_SYMBOL_GPL(__apei_exec_run);
 
 typedef int (*apei_exec_entry_func_t)(struct apei_exec_context *ctx,
 				      struct acpi_whea_header *entry,
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -50,7 +50,18 @@ static inline u64 apei_exec_ctx_get_outp
 	return ctx->value;
 }
 
-int apei_exec_run(struct apei_exec_context *ctx, u8 action);
+int __apei_exec_run(struct apei_exec_context *ctx, u8 action, bool optional);
+
+static inline int apei_exec_run(struct apei_exec_context *ctx, u8 action)
+{
+	return __apei_exec_run(ctx, action, 0);
+}
+
+/* It is optional whether the firmware provides the action */
+static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 action)
+{
+	return __apei_exec_run(ctx, action, 1);
+}
 
 /* Common instruction implementation */
 

[RFC 2/2] ACPI, APEI, Use apei_exec_run_optional in APEI EINJ and ERST

[Huang Ying]

This patch changes APEI EINJ and ERST to use apei_exec_run for
mandatory actions, and apei_exec_run_optional for optional actions.

Cc: Thomas Renninger <trenn@novell.com>
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/einj.c |    4 ++--
 drivers/acpi/apei/erst.c |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -307,7 +307,7 @@ static int __einj_error_inject(u32 type,
 
 	einj_exec_ctx_init(&ctx);
 
-	rc = apei_exec_run(&ctx, ACPI_EINJ_BEGIN_OPERATION);
+	rc = apei_exec_run_optional(&ctx, ACPI_EINJ_BEGIN_OPERATION);
 	if (rc)
 		return rc;
 	apei_exec_ctx_set_input(&ctx, type);
@@ -345,7 +345,7 @@ static int __einj_error_inject(u32 type,
 	rc = __einj_error_trigger(trigger_paddr);
 	if (rc)
 		return rc;
-	rc = apei_exec_run(&ctx, ACPI_EINJ_END_OPERATION);
+	rc = apei_exec_run_optional(&ctx, ACPI_EINJ_END_OPERATION);
 
 	return rc;
 }
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -642,7 +642,7 @@ static int __erst_write_to_storage(u64 o
 	int rc;
 
 	erst_exec_ctx_init(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_BEGIN_WRITE);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_BEGIN_WRITE);
 	if (rc)
 		return rc;
 	apei_exec_ctx_set_input(&ctx, offset);
@@ -666,7 +666,7 @@ static int __erst_write_to_storage(u64 o
 	if (rc)
 		return rc;
 	val = apei_exec_ctx_get_output(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_END);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_END);
 	if (rc)
 		return rc;
 
@@ -681,7 +681,7 @@ static int __erst_read_from_storage(u64
 	int rc;
 
 	erst_exec_ctx_init(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_BEGIN_READ);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_BEGIN_READ);
 	if (rc)
 		return rc;
 	apei_exec_ctx_set_input(&ctx, offset);
@@ -709,7 +709,7 @@ static int __erst_read_from_storage(u64
 	if (rc)
 		return rc;
 	val = apei_exec_ctx_get_output(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_END);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_END);
 	if (rc)
 		return rc;
 
@@ -724,7 +724,7 @@ static int __erst_clear_from_storage(u64
 	int rc;
 
 	erst_exec_ctx_init(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_BEGIN_CLEAR);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_BEGIN_CLEAR);
 	if (rc)
 		return rc;
 	apei_exec_ctx_set_input(&ctx, record_id);
@@ -748,7 +748,7 @@ static int __erst_clear_from_storage(u64
 	if (rc)
 		return rc;
 	val = apei_exec_ctx_get_output(&ctx);
-	rc = apei_exec_run(&ctx, ACPI_ERST_END);
+	rc = apei_exec_run_optional(&ctx, ACPI_ERST_END);
 	if (rc)
 		return rc;
 

Re: [RFC 1/2] ACPI, APEI, Add apei_exec_run_optional

[Gong Chen]

于 2011/7/5 14:07, Huang Ying 写道:
> Some actions in APEI ERST and EINJ tables are optional, for example,
> ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
> error injection, and firmware may choose to do nothing here.  While
> some other actions are mandatory, for example, firmware must provide
> ACPI_EINJ_GET_ERROR_TYPE implementation.
>
> Original implementation treats all actions as optional (that is, can
> have no instructions), that may cause issue if firmware does not
> provide some mandatory actions.  To fix this, this patch adds
> apei_exec_run_optional, which should be used for optional actions.
> The original apei_exec_run should be used for mandatory actions.
>
> Cc: Thomas Renninger<trenn@novell.com>
> Signed-off-by: Huang Ying<ying.huang@intel.com>
> ---
>   drivers/acpi/apei/apei-base.c     |    9 +++++----
>   drivers/acpi/apei/apei-internal.h |   13 ++++++++++++-
>   2 files changed, 17 insertions(+), 5 deletions(-)
>
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -157,9 +157,10 @@ EXPORT_SYMBOL_GPL(apei_exec_noop);
>    * Interpret the specified action. Go through whole action table,
>    * execute all instructions belong to the action.
>    */
> -int apei_exec_run(struct apei_exec_context *ctx, u8 action)
> +int __apei_exec_run(struct apei_exec_context *ctx, u8 action,
> +		    bool optional)
>   {
> -	int rc;
> +	int rc = -ENOENT;
>   	u32 i, ip;
>   	struct acpi_whea_header *entry;
>   	apei_exec_ins_func_t run;
> @@ -198,9 +199,9 @@ rewind:
>   			goto rewind;
>   	}
>
> -	return 0;
> +	return !optional&&  rc<  0 ? rc : 0;

if one operation is optional but running into errors when executing this 
kind of command,
here just ignoring it. Is it reasonable ?

--
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: [RFC 1/2] ACPI, APEI, Add apei_exec_run_optional

[Huang Ying]

Hi, Gong,

Thanks for review.

On 07/05/2011 09:53 PM, Gong Chen wrote:
> 于 2011/7/5 14:07, Huang Ying 写道:
>> Some actions in APEI ERST and EINJ tables are optional, for example,
>> ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
>> error injection, and firmware may choose to do nothing here.  While
>> some other actions are mandatory, for example, firmware must provide
>> ACPI_EINJ_GET_ERROR_TYPE implementation.
>>
>> Original implementation treats all actions as optional (that is, can
>> have no instructions), that may cause issue if firmware does not
>> provide some mandatory actions.  To fix this, this patch adds
>> apei_exec_run_optional, which should be used for optional actions.
>> The original apei_exec_run should be used for mandatory actions.
>>
>> Cc: Thomas Renninger<trenn@novell.com>
>> Signed-off-by: Huang Ying<ying.huang@intel.com>
>> ---
>>   drivers/acpi/apei/apei-base.c     |    9 +++++----
>>   drivers/acpi/apei/apei-internal.h |   13 ++++++++++++-
>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> --- a/drivers/acpi/apei/apei-base.c
>> +++ b/drivers/acpi/apei/apei-base.c
>> @@ -157,9 +157,10 @@ EXPORT_SYMBOL_GPL(apei_exec_noop);
>>    * Interpret the specified action. Go through whole action table,
>>    * execute all instructions belong to the action.
>>    */
>> -int apei_exec_run(struct apei_exec_context *ctx, u8 action)
>> +int __apei_exec_run(struct apei_exec_context *ctx, u8 action,
>> +		    bool optional)
>>   {
>> -	int rc;
>> +	int rc = -ENOENT;
>>   	u32 i, ip;
>>   	struct acpi_whea_header *entry;
>>   	apei_exec_ins_func_t run;
>> @@ -198,9 +199,9 @@ rewind:
>>   			goto rewind;
>>   	}
>>
>> -	return 0;
>> +	return !optional&&  rc<  0 ? rc : 0;
> 
> if one operation is optional but running into errors when executing this 
> kind of command,
> here just ignoring it. Is it reasonable ?

If we running into errors except there is no instructions for the
action, we will return the error code before this.  Please take a look
at the whole function.

Best Regards,
Huang Ying
--
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