GHES: Add a notify chain for process memory section.#1804
GHES: Add a notify chain for process memory section.#1804wangchenlu2236 wants to merge 1 commit into
Conversation
Add a notify chain for process memory section, with which other modules might do error recovery. Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com> Tested-by: Wang Xiongfeng <wangxiongfeng2@huawei.com> Tested-by: Tyler Baicar <tbaicar@codeaurora.org> Reviewed-by: Hanjun Guo <guohanjun@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> Tested-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Wang Chenlu <wangchenlu2236@phytium.com.cn>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces a new atomic notifier chain for GHES-processed platform memory errors and propagates rich context about each memory error (notification type, severity, and CPER payload) to external modules for error recovery. Sequence diagram for GHES memory error notify chain usagesequenceDiagram
participant ExternalModule
participant ghes_mem_err_chain
participant GHES
ExternalModule->>ghes_mem_err_chain: atomic_notifier_chain_register(ghes_mem_err_chain, notifier_block)
GHES->>GHES: ghes_do_proc(ghes)
GHES->>ghes_mem_err_chain: atomic_notifier_call_chain(ghes_mem_err_chain, severity, ghes_mem_err)
ghes_mem_err_chain->>ExternalModule: notifier_block.callback(ghes_mem_err)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @wangchenlu2236. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
I think that there are them from xiexiuqi@openeuler kernel: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
ghes_mem_err_chainnotifier andstruct ghes_mem_errare defined but the code still callsatomic_notifier_call_chain(&ghes_report_chain, sev, mem_err), so consider switching this call to useghes_mem_err_chainand pass aghes_mem_errinstance to actually exercise the new interface. - If notifier callbacks are expected to access the
ghes_mem_errbeyond the duration ofghes_do_proc, you may want to clarify/adjust the lifetime of thestruct ghes_mem_err mem(currently stack-allocated) to avoid use-after-return issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ghes_mem_err_chain` notifier and `struct ghes_mem_err` are defined but the code still calls `atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err)`, so consider switching this call to use `ghes_mem_err_chain` and pass a `ghes_mem_err` instance to actually exercise the new interface.
- If notifier callbacks are expected to access the `ghes_mem_err` beyond the duration of `ghes_do_proc`, you may want to clarify/adjust the lifetime of the `struct ghes_mem_err mem` (currently stack-allocated) to avoid use-after-return issues.
## Individual Comments
### Comment 1
<location path="drivers/acpi/apei/ghes.c" line_range="706-714" />
<code_context>
if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
+ struct ghes_mem_err mem;
+
+ mem.notify_type = ghes->generic->notify.type;
+ mem.severity = gdata->error_severity;
+ mem.mem_err = mem_err;
atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
</code_context>
<issue_to_address>
**issue (bug_risk):** New `ghes_mem_err` wrapper is never used and `ghes_mem_err_chain` is not called here.
You construct and populate `struct ghes_mem_err mem` but still call `atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);` with the old chain and the original `mem_err` pointer. As a result, the new wrapper and `ghes_mem_err_chain` are unused at this site and no behavior changes.
If the goal is to pass the richer context, this should likely call:
```c
atomic_notifier_call_chain(&ghes_mem_err_chain, sev, &mem);
```
Otherwise, consider removing `struct ghes_mem_err` here to avoid dead code and API confusion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) { | ||
| struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); | ||
| struct ghes_mem_err mem; | ||
|
|
||
| mem.notify_type = ghes->generic->notify.type; | ||
| mem.severity = gdata->error_severity; | ||
| mem.mem_err = mem_err; | ||
|
|
||
| atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err); |
There was a problem hiding this comment.
issue (bug_risk): New ghes_mem_err wrapper is never used and ghes_mem_err_chain is not called here.
You construct and populate struct ghes_mem_err mem but still call atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err); with the old chain and the original mem_err pointer. As a result, the new wrapper and ghes_mem_err_chain are unused at this site and no behavior changes.
If the goal is to pass the richer context, this should likely call:
atomic_notifier_call_chain(&ghes_mem_err_chain, sev, &mem);Otherwise, consider removing struct ghes_mem_err here to avoid dead code and API confusion.
There was a problem hiding this comment.
Pull request overview
This PR aims to add a new notifier mechanism for GHES processing of platform memory error sections, so other kernel modules can observe (and potentially react to) these events with additional context beyond the existing ghes_report_chain callback payload.
Changes:
- Introduces a new
struct ghes_mem_errcontext struct ininclude/acpi/ghes.h. - Adds and exports a new
ghes_mem_err_chainatomic notifier head indrivers/acpi/apei/ghes.c. - Extends GHES memory-section processing to populate the new context struct (but currently does not notify the new chain).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/acpi/ghes.h | Adds a new GHES memory-error context struct and declares an exported atomic notifier chain. |
| drivers/acpi/apei/ghes.c | Defines/exports the notifier chain and prepares a structured memory-error context during section processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata); | ||
| struct ghes_mem_err mem; | ||
|
|
||
| mem.notify_type = ghes->generic->notify.type; | ||
| mem.severity = gdata->error_severity; | ||
| mem.mem_err = mem_err; | ||
|
|
||
| atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err); |
Add a notify chain for process memory section, with which other modules might do error recovery.
Summary by Sourcery
Introduce a notifier chain for reporting GHES platform memory errors so other modules can react to process memory error events.
New Features:
Enhancements: