Re: [PATCH v2] ACPI, APEI, EINJ: Relax platform response timeout to 1 second.

[Shuai Xue]

Hi, Tony,

>>> +/* Firmware should respond within 1 seconds */
>>> +#define FIRMWARE_TIMEOUT	(1 * MSEC_PER_SEC)
>>>  #define ACPI5_VENDOR_BIT	BIT(31)
>>>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>>>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>>> @@ -40,6 +40,8 @@
>>>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
>>>   */
>>>  static int acpi5;
>>> +static int timeout_default = FIRMWARE_TIMEOUT;
>>> +module_param(timeout_default, int, 0644);
>>
>> You've set the default to 1 second. Who would use this parameter?
>> Do you anticipate systems that take even longer to inject?
>> A user might set a shorter limit ... but I don't see why they
>> would want to.
> No, I don't. EINJ provides a hardware error injection mechanism to develop
> and debug firmware code and hardware RAS feature. While we test on Arm
> platform, it cannot meet the original timeout limit. Therefore, we send
> this patch to relax the upper bound of timeout. In order to facilitate
> other platforms to encounter the same problems, we expose timeout as a
> configurable parameter in user space.

What's your opinion about this interface?

Regards,

Shuai.


On 2021/10/24 PM5:10, Shuai Xue wrote:
> Hi, Tony,
> 
> Thank you for your comments.
> 
>> I know I pointed you to msleep() ... sorry, I was wrong. For a
>> 1 ms sleep the recommendation is to use usleep_range()
>>
>> See this write-up in Documentation/timers/timers-howto.rst:
>>
>>                - Why not msleep for (1ms - 20ms)?
>>                        Explained originally here:
>>                                https://lore.kernel.org/r/15327.1186166232@xxxxxxx
>>
>>                        msleep(1~20) may not do what the caller intends, and
>>                        will often sleep longer (~20 ms actual sleep for any
>>                        value given in the 1~20ms range). In many cases this
>>                        is not the desired behavior.
>>
>> To answer the question posed in that document on "What is a good range?"
>>
>> I don't think injection cares too much about precision here. Maybe go
>> with
>>
>> 	usleep_range(1000, 5000);
>> [with #defines for SLEEP_UNIT_MIN, SLEEP_UNIT_MAX instead of those
>> numbers]
> Got it. Thank you. I will change it latter.
> 
> 
>>> +/* Firmware should respond within 1 seconds */
>>> +#define FIRMWARE_TIMEOUT	(1 * MSEC_PER_SEC)
>>>  #define ACPI5_VENDOR_BIT	BIT(31)
>>>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>>>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>>> @@ -40,6 +40,8 @@
>>>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
>>>   */
>>>  static int acpi5;
>>> +static int timeout_default = FIRMWARE_TIMEOUT;
>>> +module_param(timeout_default, int, 0644);
>>
>> You've set the default to 1 second. Who would use this parameter?
>> Do you anticipate systems that take even longer to inject?
>> A user might set a shorter limit ... but I don't see why they
>> would want to.
> No, I don't. EINJ provides a hardware error injection mechanism to develop
> and debug firmware code and hardware RAS feature. While we test on Arm
> platform, it cannot meet the original timeout limit. Therefore, we send
> this patch to relax the upper bound of timeout. In order to facilitate
> other platforms to encounter the same problems, we expose timeout as a
> configurable parameter in user space.
> 
> 
>>>  struct set_error_type_with_address {
>>>  	u32	type;
>>> @@ -171,12 +173,12 @@ static int einj_get_available_error_type(u32 *type)
>>>
>>>  static int einj_timedout(u64 *t)
>>>  {
>>> -	if ((s64)*t < SPIN_UNIT) {
>>> +	if ((s64)*t < SLEEP_UNIT) {
>>>  		pr_warn(FW_WARN "Firmware does not respond in time\n");
>>>  		return 1;
>>>  	}
>>> -	*t -= SPIN_UNIT;
>>> -	ndelay(SPIN_UNIT);
>>> +	*t -= SLEEP_UNIT;
>>> +	msleep(SLEEP_UNIT);
>>>  	touch_nmi_watchdog();
>>
>> Since we are sleeping instead of spinning, maybe we don't need to
>> touch the nmi watchdog?
> Agree. I will delete it in next version.
> 
> Regards,
> Shuai
> 
> On 2021/10/23 AM7:54, Luck, Tony wrote:
>> On Fri, Oct 22, 2021 at 09:44:24PM +0800, Shuai Xue wrote:
>>> When injecting an error into the platform, the OSPM executes an
>>> EXECUTE_OPERATION action to instruct the platform to begin the injection
>>> operation. And then, the OSPM busy waits for a while by continually
>>> executing CHECK_BUSY_STATUS action until the platform indicates that the
>>> operation is complete. More specifically, the platform is limited to
>>> respond within 1 millisecond right now. This is too strict for some
>>> platforms.
>>>
>>> For example, in Arm platform, when injecting a Processor Correctable error,
>>> the OSPM will warn:
>>>     Firmware does not respond in time.
>>>
>>> And a message is printed on the console:
>>>     echo: write error: Input/output error
>>>
>>> We observe that the waiting time for DDR error injection is about 10 ms
>>> and that for PCIe error injection is about 500 ms in Arm platform.
>>>
>>> In this patch, we relax the response timeout to 1 second and allow user to
>>> pass the time out value as a argument.
>>>
>>> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
>>> ---
>>> Changelog v1 -> v2:
>>> - Implemented the timeout in msleep instead of udelay.
>>> - Link to the v1 patch: https://lkml.org/lkml/2021/10/14/1402
>>> ---
>>>  drivers/acpi/apei/einj.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>>> index 133156759551..e411eb30e0ee 100644
>>> --- a/drivers/acpi/apei/einj.c
>>> +++ b/drivers/acpi/apei/einj.c
>>> @@ -28,9 +28,9 @@
>>>  #undef pr_fmt
>>>  #define pr_fmt(fmt) "EINJ: " fmt
>>>  
>>> -#define SPIN_UNIT		100			/* 100ns */
>>> -/* Firmware should respond within 1 milliseconds */
>>> -#define FIRMWARE_TIMEOUT	(1 * NSEC_PER_MSEC)
>>> +#define SLEEP_UNIT		1			/* 1ms */
>>
>> I know I pointed you to msleep() ... sorry, I was wrong. For a
>> 1 ms sleep the recommendation is to use usleep_range()
>>
>> See this write-up in Documentation/timers/timers-howto.rst:
>>
>>                 - Why not msleep for (1ms - 20ms)?
>>                         Explained originally here:
>>                                 https://lore.kernel.org/r/15327.1186166232@xxxxxxx
>>
>>                         msleep(1~20) may not do what the caller intends, and
>>                         will often sleep longer (~20 ms actual sleep for any
>>                         value given in the 1~20ms range). In many cases this
>>                         is not the desired behavior.
>>
>> To answer the question posed in that document on "What is a good range?"
>>
>> I don't think injection cares too much about precision here. Maybe go
>> with
>>
>> 	usleep_range(1000, 5000);
>> [with #defines for SLEEP_UNIT_MIN, SLEEP_UNIT_MAX instead of those
>> numbers]
>>
>>> +/* Firmware should respond within 1 seconds */
>>> +#define FIRMWARE_TIMEOUT	(1 * MSEC_PER_SEC)
>>>  #define ACPI5_VENDOR_BIT	BIT(31)
>>>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>>>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>>> @@ -40,6 +40,8 @@
>>>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
>>>   */
>>>  static int acpi5;
>>> +static int timeout_default = FIRMWARE_TIMEOUT;
>>> +module_param(timeout_default, int, 0644);
>>
>> You've set the default to 1 second. Who would use this parameter?
>> Do you anticipate systems that take even longer to inject?
>> A user might set a shorter limit ... but I don't see why they
>> would want to.
>>
>>>  
>>>  struct set_error_type_with_address {
>>>  	u32	type;
>>> @@ -171,12 +173,12 @@ static int einj_get_available_error_type(u32 *type)
>>>  
>>>  static int einj_timedout(u64 *t)
>>>  {
>>> -	if ((s64)*t < SPIN_UNIT) {
>>> +	if ((s64)*t < SLEEP_UNIT) {
>>>  		pr_warn(FW_WARN "Firmware does not respond in time\n");
>>>  		return 1;
>>>  	}
>>> -	*t -= SPIN_UNIT;
>>> -	ndelay(SPIN_UNIT);
>>> +	*t -= SLEEP_UNIT;
>>> +	msleep(SLEEP_UNIT);
>>>  	touch_nmi_watchdog();
>>
>> Since we are sleeping instead of spinning, maybe we don't need to
>> touch the nmi watchdog?
>>
>>>  	return 0;
>>>  }
>>> @@ -403,7 +405,7 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>>  			       u64 param3, u64 param4)
>>>  {
>>>  	struct apei_exec_context ctx;
>>> -	u64 val, trigger_paddr, timeout = FIRMWARE_TIMEOUT;
>>> +	u64 val, trigger_paddr, timeout = timeout_default;
>>>  	int rc;
>>>  
>>>  	einj_exec_ctx_init(&ctx);
>>> -- 
>>> 2.20.1.12.g72788fdb
>>>
>>
>> -Tony
>>