diff mbox series

[v11,2/3] mm: memory-failure: move return value documentation to function declaration

Message ID 20240204080144.7977-3-xueshuai@linux.alibaba.com
State New
Headers
Series None |

Commit Message

Shuai Xue Feb. 4, 2024, 8:01 a.m. UTC 
  Part of return value comments for memory_failure() were originally
documented at the call site. Move those comments to the function
declaration to improve code readability and to provide developers with
immediate access to function usage and return information.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 arch/x86/kernel/cpu/mce/core.c | 9 +--------
 mm/memory-failure.c            | 9 ++++++---
 2 files changed, 7 insertions(+), 11 deletions(-)

Comments

Borislav Petkov Feb. 26, 2024, 10:46 a.m. UTC | #1 
On Sun, Feb 04, 2024 at 04:01:43PM +0800, Shuai Xue wrote:
> Part of return value comments for memory_failure() were originally
> documented at the call site. Move those comments to the function
> declaration to improve code readability and to provide developers with
> immediate access to function usage and return information.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 9 +--------
>  mm/memory-failure.c            | 9 ++++++---
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index bc39252bc54f..822b21eb48ad 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1365,17 +1365,10 @@ static void kill_me_maybe(struct callback_head *cb)
>  		return;
>  	}
>  
> -	/*
> -	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
> -	 * to the current process with the proper error info,
> -	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
> -	 *
> -	 * In both cases, no further processing is required.
> -	 */
>  	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>  		return;
>  
> -	pr_err("Memory error not recovered");
> +	pr_err("Sending SIGBUS to current task due to memory error not recovered");

Unrelated change.

>  	kill_me_now(cb);
>  }
>  
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 636280d04008..d33729c48eff 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2175,9 +2175,12 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>   * Must run in process context (e.g. a work queue) with interrupts
>   * enabled and no spinlocks held.
>   *
> - * Return: 0 for successfully handled the memory error,
> - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
> - *         < 0(except -EOPNOTSUPP) on failure.
> + * Return values:
> + *   0             - success
> + *   -EOPNOTSUPP   - hwpoison_filter() filtered the error event.
> + *   -EHWPOISON    - sent SIGBUS to the current process with the proper
> + *                   error info by kill_accessing_process().

kill_accessing_process() is not the only one returning -EHWPOISON.

And if you look at the code, it should be:

	-EHWPOISON	- the page was already poisoned, potentially
			kill process

or so.
Shuai Xue Feb. 27, 2024, 1:27 a.m. UTC | #2 
On 2024/2/26 18:46, Borislav Petkov wrote:
> On Sun, Feb 04, 2024 at 04:01:43PM +0800, Shuai Xue wrote:
>> Part of return value comments for memory_failure() were originally
>> documented at the call site. Move those comments to the function
>> declaration to improve code readability and to provide developers with
>> immediate access to function usage and return information.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>  arch/x86/kernel/cpu/mce/core.c | 9 +--------
>>  mm/memory-failure.c            | 9 ++++++---
>>  2 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index bc39252bc54f..822b21eb48ad 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -1365,17 +1365,10 @@ static void kill_me_maybe(struct callback_head *cb)
>>  		return;
>>  	}
>>  
>> -	/*
>> -	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
>> -	 * to the current process with the proper error info,
>> -	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
>> -	 *
>> -	 * In both cases, no further processing is required.
>> -	 */
>>  	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>>  		return;
>>  
>> -	pr_err("Memory error not recovered");
>> +	pr_err("Sending SIGBUS to current task due to memory error not recovered");
> 
> Unrelated change.

Yes, I will drop the error message change.

> 
>>  	kill_me_now(cb);
>>  }
>>  
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 636280d04008..d33729c48eff 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2175,9 +2175,12 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>   * Must run in process context (e.g. a work queue) with interrupts
>>   * enabled and no spinlocks held.
>>   *
>> - * Return: 0 for successfully handled the memory error,
>> - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
>> - *         < 0(except -EOPNOTSUPP) on failure.
>> + * Return values:
>> + *   0             - success
>> + *   -EOPNOTSUPP   - hwpoison_filter() filtered the error event.
>> + *   -EHWPOISON    - sent SIGBUS to the current process with the proper
>> + *                   error info by kill_accessing_process().
> 
> kill_accessing_process() is not the only one returning -EHWPOISON.
> 
> And if you look at the code, it should be:
> 
> 	-EHWPOISON	- the page was already poisoned, potentially
> 			kill process
> 
> or so.
> 

You are right, will fix it in next version.

Thank you.

Best Regards.
Shuai
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bc39252bc54f..822b21eb48ad 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1365,17 +1365,10 @@  static void kill_me_maybe(struct callback_head *cb)
 		return;
 	}
 
-	/*
-	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
-	 * to the current process with the proper error info,
-	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
-	 *
-	 * In both cases, no further processing is required.
-	 */
 	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
 		return;
 
-	pr_err("Memory error not recovered");
+	pr_err("Sending SIGBUS to current task due to memory error not recovered");
 	kill_me_now(cb);
 }
 
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 636280d04008..d33729c48eff 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2175,9 +2175,12 @@  static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
  * Must run in process context (e.g. a work queue) with interrupts
  * enabled and no spinlocks held.
  *
- * Return: 0 for successfully handled the memory error,
- *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
- *         < 0(except -EOPNOTSUPP) on failure.
+ * Return values:
+ *   0             - success
+ *   -EOPNOTSUPP   - hwpoison_filter() filtered the error event.
+ *   -EHWPOISON    - sent SIGBUS to the current process with the proper
+ *                   error info by kill_accessing_process().
+ *   other negative values - failure
  */
 int memory_failure(unsigned long pfn, int flags)
 {