ACPI: APEI: EINJ: warn on invalid argument when explicitly indicated by platform

  Fix to return -EINVAL in the __einj_error_inject() error handling case
instead of -EBUSY, when explicitly indicated by the platform in the status
of the completed operation.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/acpi/apei/einj.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
  

-	if (val != EINJ_STATUS_SUCCESS)
+	if (val == EINJ_STATUS_FAIL)
 		return -EBUSY;
+	else if (val == EINJ_STATUS_INVAL)
+		return -EINVAL;

The ACPI Specification is really vague here. Documented error codes are

0 = Success (Linux #define EINJ_STATUS_SUCCESS)
1 = Unknown failure (Linux #define EINJ_STATUS_FAIL)
2 = Invalid Access (Linux #define EINJ_STATUS_INVAL)

I don't see how reporting -EBUSY for the "Unknown Failure" case is
actually better.

-Tony
  

On 2023/3/18 AM5:24, Luck, Tony wrote:
> -	if (val != EINJ_STATUS_SUCCESS)
> +	if (val == EINJ_STATUS_FAIL)
>  		return -EBUSY;
> +	else if (val == EINJ_STATUS_INVAL)
> +		return -EINVAL;
> 
> The ACPI Specification is really vague here. Documented error codes are
> 
> 0 = Success (Linux #define EINJ_STATUS_SUCCESS)
> 1 = Unknown failure (Linux #define EINJ_STATUS_FAIL)
> 2 = Invalid Access (Linux #define EINJ_STATUS_INVAL)

Absolutely right.

> 
> I don't see how reporting -EBUSY for the "Unknown Failure" case is
> actually better.

Tony, did you misunderstand this patch?

The original code report -EBUSY for both "Unknown Failure" and
"Invalid Access" cases.

This patch intends to report -EINVAL for "Invalid Access" case
and keeps reporting -EBUSY for "Unknown Failure" case unchanged.
Although -EBUSY for "Unknown Failure" case is not a good choice.
Will -EIO for "Unknown failure" case be better?

By the way, do you think -EIO for time out case is suitable.

	for (;;) {
		rc = apei_exec_run(&ctx, ACPI_EINJ_CHECK_BUSY_STATUS);
		if (rc)
			return rc;
		val = apei_exec_ctx_get_output(&ctx);
		if (!(val & EINJ_OP_BUSY))
			break;
		if (einj_timedout(&timeout))
			return -EIO;

For example, the OSPM will may warn:

    Firmware does not respond in time.

And a message is printed on the console:
    echo: write error: Input/output error

Will -EBUSY or -ETIME for timeout be better?

> 
> -Tony

Thank you for comments.

Best Regards.
Shuai
  

>> I don't see how reporting -EBUSY for the "Unknown Failure" case is
>> actually better.
>
> Tony, did you misunderstand this patch?
>
> The original code report -EBUSY for both "Unknown Failure" and
> "Invalid Access" cases.

I mixed up what was already in the kernel with what the patch was changing.

> This patch intends to report -EINVAL for "Invalid Access" case
> and keeps reporting -EBUSY for "Unknown Failure" case unchanged.
> Although -EBUSY for "Unknown Failure" case is not a good choice.
> Will -EIO for "Unknown failure" case be better?

Is this for some real use case?

Do you have a BIOS EINJ implementation that is returning these different codes?

What will the user do differently if they see these different error strings?

  # echo 1 > error_inject
  ... different error messages here ...

-Tony
  

On 2023/3/21 AM12:32, Luck, Tony wrote:
>>> I don't see how reporting -EBUSY for the "Unknown Failure" case is
>>> actually better.
>>
>> Tony, did you misunderstand this patch?
>>
>> The original code report -EBUSY for both "Unknown Failure" and
>> "Invalid Access" cases.
> 
> I mixed up what was already in the kernel with what the patch was changing.
> 
>> This patch intends to report -EINVAL for "Invalid Access" case
>> and keeps reporting -EBUSY for "Unknown Failure" case unchanged.
>> Although -EBUSY for "Unknown Failure" case is not a good choice.
>> Will -EIO for "Unknown failure" case be better?
> 
> Is this for some real use case?
> 
> Do you have a BIOS EINJ implementation that is returning these different codes?

Yes, our BIOS tester complains that EINJ always reports EBUSY, and he has no idea
about it. It can not help him determine whether it is a BIOS bug or an injection
operation error.

> What will the user do differently if they see these different error strings?
> 
>   # echo 1 > error_inject
>   ... different error messages here ...

For example, with original code:

	# select a invalid core or device to inject
	# echo 1 > error_inject
	echo: write error: Device or resource busy

When tester sees that, he will submit a bug to BIOS developer.

Actually, firmware will do some platform dependent sanity checks and returns
different error codes. In this case, user injects to a invalid device, platform
returns "Invalid Access". And user is expected to see:

	# select a invalid core or device to inject
	# echo 1 > error_inject
	echo: write error: Invalid argument

Then user is expected to check his injection argument first.


> 
> -Tony
> 

Best Regards,
Shuai
  

> Actually, firmware will do some platform dependent sanity checks and returns
> different error codes. In this case, user injects to a invalid device, platform
> returns "Invalid Access". And user is expected to see:
>
>	# select a invalid core or device to inject
>	# echo 1 > error_inject
>	echo: write error: Invalid argument
>
> Then user is expected to check his injection argument first.

Thanks. This makes sense. You want EINVAL when the user chose
bad arguments, and some other code for problem in BIOS.

If the BIOS has an issue, is it possible, or likely, that it is a temporary
problem? If so, EBUSY may be OK. The message " Device or resource busy"
might encourage the user to wait and try again.

If it is not going to get better by itself, then one of:

#define EIO              5      /* I/O error */
#define ENXIO            6      /* No such device or address */

might be a better choice.

-Tony
  

On 2023/3/22 AM12:09, Luck, Tony wrote:
>> Actually, firmware will do some platform dependent sanity checks and returns
>> different error codes. In this case, user injects to a invalid device, platform
>> returns "Invalid Access". And user is expected to see:
>>
>> 	# select a invalid core or device to inject
>> 	# echo 1 > error_inject
>> 	echo: write error: Invalid argument
>>
>> Then user is expected to check his injection argument first.
> 
> Thanks. This makes sense. You want EINVAL when the user chose
> bad arguments, and some other code for problem in BIOS.

Yes, exactly.

> 
> If the BIOS has an issue, is it possible, or likely, that it is a temporary
> problem? If so, EBUSY may be OK. The message " Device or resource busy"
> might encourage the user to wait and try again.
> 
> If it is not going to get better by itself, then one of:
> 
> #define EIO              5      /* I/O error */
> #define ENXIO            6      /* No such device or address */
> 
> might be a better choice.

Yes, BIOS may temporarily not complete error injection (ACPI_EINJ_EXECUTE_OPERATION)
on time, in which case, kernel return EIO.

	for (;;) {
		rc = apei_exec_run(&ctx, ACPI_EINJ_CHECK_BUSY_STATUS);
		if (rc)
			return rc;
		val = apei_exec_ctx_get_output(&ctx);
		if (!(val & EINJ_OP_BUSY))
			break;
		if (einj_timedout(&timeout))
			return -EIO;

In summary, you are asking that:

- report -EINVAL for "Invalid Access" case
- keeps reporting -EBUSY for "Unknown Failure" case unchanged.
- and keeps EIO for temporarily time out unchanged.

right? (This patch is doing so)

> 
> -Tony

Thanks.

Best Regards,
Shuai
  

> Fix to return -EINVAL in the __einj_error_inject() error handling case
> instead of -EBUSY, when explicitly indicated by the platform in the status
> of the completed operation.

Needs a bit longer description on the use case based on follow up discussion.
Key information is the EINVAL is an indicator to the user that the parameters they
supplied cannot be used for injection.

But for the code:

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony
  

On Wed, Mar 22, 2023 at 5:13 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> > Fix to return -EINVAL in the __einj_error_inject() error handling case
> > instead of -EBUSY, when explicitly indicated by the platform in the status
> > of the completed operation.
>
> Needs a bit longer description on the use case based on follow up discussion.
> Key information is the EINVAL is an indicator to the user that the parameters they
> supplied cannot be used for injection.

Right.

So Shuai, please resend the patch with a more elaborate changelog.

> But for the code:
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>

And add the above tag to it when resending.
  

On 2023/3/23 AM12:07, Luck, Tony wrote:
>> Fix to return -EINVAL in the __einj_error_inject() error handling case
>> instead of -EBUSY, when explicitly indicated by the platform in the status
>> of the completed operation.
> 
> Needs a bit longer description on the use case based on follow up discussion.
> Key information is the EINVAL is an indicator to the user that the parameters they
> supplied cannot be used for injection.
> 
> But for the code:
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> 
> -Tony

Got, I will rephrase the commit log based on follow up discussion.

Thank you for reviews.

Cheers,
Shuai
  

On 2023/3/23 AM12:52, Rafael J. Wysocki wrote:
> On Wed, Mar 22, 2023 at 5:13 PM Luck, Tony <tony.luck@intel.com> wrote:
>>
>>> Fix to return -EINVAL in the __einj_error_inject() error handling case
>>> instead of -EBUSY, when explicitly indicated by the platform in the status
>>> of the completed operation.
>>
>> Needs a bit longer description on the use case based on follow up discussion.
>> Key information is the EINVAL is an indicator to the user that the parameters they
>> supplied cannot be used for injection.
> 
> Right.
> 
> So Shuai, please resend the patch with a more elaborate changelog.

Ok, I will do it later.

> 
>> But for the code:
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
> 
> And add the above tag to it when resending.

Got it.

Thank you.

Best Regards.
Shuai