CIX:Add OEM-specific ACPI OEM_ID check for default domain #1335
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds an ACPI IORT OEM_ID helper and uses it in the SMMU v3 default domain selection path to choose DMA as the default IOMMU domain on CIXTEK-based platforms while preserving existing HiSilicon behavior. Sequence diagram for default IOMMU domain selection with ACPI OEM_ID checksequenceDiagram
participant Device
participant SMMU as arm_smmu_def_domain_type
participant ACPI as acpi_check_oem_id
participant IORT as ACPI_IORT_table
Device->>SMMU: arm_smmu_def_domain_type(dev)
SMMU->>SMMU: Check if dev is PCI device
alt Device is PCI and IS_HISI_PTT_DEVICE
SMMU-->>Device: return IOMMU_DOMAIN_IDENTITY
else Device is PCI but not IS_HISI_PTT_DEVICE
SMMU->>ACPI: acpi_check_oem_id(dev, CIXTEK)
alt OEM_ID matches CIXTEK
ACPI->>IORT: acpi_get_table(IORT)
IORT-->>ACPI: iort_table with OEM_ID CIXTEK
ACPI-->>SMMU: true
SMMU-->>Device: return IOMMU_DOMAIN_DMA
else OEM_ID does not match or no table
ACPI-->>SMMU: false
SMMU-->>Device: return 0 (use default behavior)
end
else Device is not PCI
SMMU-->>Device: return 0 (use default behavior)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @Ghostor8. 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. Instructions 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. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
acpi_check_oem_id(), theacpi_get_table()result is never released; you should callacpi_put_table(iort_table)once you're done with it to avoid leaking the ACPI table mapping. - The
elsebranch added inarm_smmu_def_domain_type()ties the OEM_ID-based default domain selection to the case wherepdevis NULL; if the intention is to apply this policy independently of PCI enumeration, consider moving the ACPI OEM_ID check outside of theif (pdev)/elsestructure. - The
pr_warn("Failed to get table, %s\n", msg);log is quite generic; using a more specific message and/or adev_warn(dev, ...)to include the device context would make failures easier to trace on multi-device systems.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `acpi_check_oem_id()`, the `acpi_get_table()` result is never released; you should call `acpi_put_table(iort_table)` once you're done with it to avoid leaking the ACPI table mapping.
- The `else` branch added in `arm_smmu_def_domain_type()` ties the OEM_ID-based default domain selection to the case where `pdev` is NULL; if the intention is to apply this policy independently of PCI enumeration, consider moving the ACPI OEM_ID check outside of the `if (pdev)`/`else` structure.
- The `pr_warn("Failed to get table, %s\n", msg);` log is quite generic; using a more specific message and/or a `dev_warn(dev, ...)` to include the device context would make failures easier to trace on multi-device systems.
## Individual Comments
### Comment 1
<location> `drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2889-2891` </location>
<code_context>
+static bool acpi_check_oem_id(struct device *dev, const char *oem_id)
+{
+ if (oem_id && has_acpi_companion(dev)) {
+ struct acpi_table_header *iort_table;
+
+ acpi_status status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table);
+
+ if (ACPI_FAILURE(status)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The ACPI IORT table acquired with acpi_get_table() should be released with acpi_put_table() to avoid leaking references.
After checking `iort_table->oem_id`, call `acpi_put_table(iort_table);` in the success path (and any early returns after a successful `acpi_get_table`) before `acpi_check_oem_id` returns, so the table reference is properly released once it’s no longer needed.
</issue_to_address>
### Comment 2
<location> `drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2922-2923` </location>
<code_context>
if (IS_HISI_PTT_DEVICE(pdev))
return IOMMU_DOMAIN_IDENTITY;
- }
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** The new else block is attached to the dev_is_pci() if, meaning the ACPI OEM-ID check only runs for non-PCI devices; verify this control flow.
In this layout the `else` binds to `if (dev_is_pci(dev))`, not to `if (IS_HISI_PTT_DEVICE(pdev))`. That means the ACPI OEM-ID branch will only run when `!dev_is_pci(dev)`, which is likely not what you want if this is meant as a fallback for non-HISI PCI devices. Consider restructuring (e.g., a separate `if` after the PCI block or an `else` on the inner `if`), and fix the brace/indentation so the intended `if`/`else` pairing is clear.
</issue_to_address>
### Comment 3
<location> `drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:2886` </location>
<code_context>
+/*
+ * Check OEM_ID with a specific OEM provider.
+ */
+static bool acpi_check_oem_id(struct device *dev, const char *oem_id)
+{
+ if (oem_id && has_acpi_companion(dev)) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new OEM check helper and domain-type logic to use early returns and clearer boolean semantics to reduce nesting and clarify control flow.
You can simplify both the helper and the domain selection to reduce nesting and make the flow clearer, while keeping behavior identical (including only doing the OEM check for non‑PCI devices).
### 1. Flatten `acpi_check_oem_id()`
Use early returns and `bool` semantics instead of nested `if` and `0/1`:
```c
static bool acpi_check_oem_id(struct device *dev, const char *oem_id)
{
struct acpi_table_header *iort_table;
acpi_status status;
if (!oem_id || !has_acpi_companion(dev))
return false;
status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table);
if (ACPI_FAILURE(status)) {
if (status != AE_NOT_FOUND)
pr_warn("Failed to get table, %s\n",
acpi_format_exception(status));
return false;
}
return !strncmp(iort_table->oem_id, oem_id, ACPI_OEM_ID_SIZE);
}
```
This removes one nesting level and makes the success path stand out.
### 2. Reduce nesting in `arm_smmu_def_domain_type()`
Keep the OEM check restricted to non‑PCI devices as in your patch, but avoid the `else` and nested blocks by using early returns:
```c
static int arm_smmu_def_domain_type(struct device *dev)
{
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
if (IS_HISI_PTT_DEVICE(pdev))
return IOMMU_DOMAIN_IDENTITY;
/* Preserve current behavior: no OEM check for PCI devices */
return 0;
}
if (acpi_check_oem_id(dev, "CIXTEK"))
return IOMMU_DOMAIN_DMA;
return 0;
}
```
This keeps the behavior of your change (OEM path only for non‑PCI devices) but makes the control flow linear and easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR introduces OEM-specific ACPI identification support to improve system bring-up reliability and ensure correct IOMMU domain assignment on CIXTEK-based platforms. It adds a helper function to check ACPI OEM_ID from the IORT table and uses it to select a DMA default domain for matching systems.
- Adds
acpi_check_oem_id()helper function to query ACPI IORT table OEM_ID - Modifies
arm_smmu_def_domain_type()to returnIOMMU_DOMAIN_DMAfor CIXTEK OEM platforms - Provides platform-specific domain selection based on ACPI firmware identification
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2899
to
+2907
| return 0; | ||
| } | ||
|
|
||
| if (!strncmp(iort_table->oem_id, oem_id, 6)) | ||
| return 1; | ||
|
|
||
| } | ||
| return 0; |
There was a problem hiding this comment.
Inconsistent return value type. The function returns bool but uses integer literals 0 and 1. Use false and true for consistency with the boolean return type.
Suggested change
| return 0; | |
| } | |
| if (!strncmp(iort_table->oem_id, oem_id, 6)) | |
| return 1; | |
| } | |
| return 0; | |
| return false; | |
| } | |
| if (!strncmp(iort_table->oem_id, oem_id, 6)) | |
| return true; | |
| } | |
| return false; |
Member
There was a problem hiding this comment.
This suggestion should be applied
| return 0; | ||
| } | ||
|
|
||
| if (!strncmp(iort_table->oem_id, oem_id, 6)) |
There was a problem hiding this comment.
Magic number used without explanation. The value 6 represents the size of the ACPI OEM_ID field. Define a constant (e.g., ACPI_OEM_ID_SIZE) or use ACPI_OEM_ID_SIZE from ACPI headers if available, to make the code more maintainable and self-documenting.
Member
There was a problem hiding this comment.
Avoid using magic number, at lease a line of comment
| return IOMMU_DOMAIN_DMA; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line. Remove one of the two consecutive blank lines to maintain consistent code formatting.
Suggested change
Comment on lines
2924
to
2927
| } else { | ||
| if (acpi_check_oem_id(dev, "CIXTEK")) | ||
| return IOMMU_DOMAIN_DMA; | ||
| } |
There was a problem hiding this comment.
Incorrect indentation. The else block and its contents should not be indented as they are currently nested too deeply. The indentation should be reduced to align with the proper control flow once the brace issue is fixed.
Suggested change
| } else { | |
| if (acpi_check_oem_id(dev, "CIXTEK")) | |
| return IOMMU_DOMAIN_DMA; | |
| } | |
| } else { | |
| if (acpi_check_oem_id(dev, "CIXTEK")) | |
| return IOMMU_DOMAIN_DMA; | |
| } |
Comment on lines
+2891
to
+2904
| acpi_status status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table); | ||
|
|
||
| if (ACPI_FAILURE(status)) { | ||
| if (status != AE_NOT_FOUND) { | ||
| const char *msg = acpi_format_exception(status); | ||
|
|
||
| pr_warn("Failed to get table, %s\n", msg); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| if (!strncmp(iort_table->oem_id, oem_id, 6)) | ||
| return 1; |
There was a problem hiding this comment.
Missing acpi_put_table() call after acpi_get_table(). The ACPI table reference acquired on line 2891 must be released to avoid a memory leak. Add acpi_put_table(iort_table); before returning on line 2903 and ensure all return paths release the table.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cryolitia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MingcongBai
requested changes
Dec 1, 2025
Contributor
MingcongBai
left a comment
MingcongBai
left a comment
There was a problem hiding this comment.
补丁机理没毛病，但我个人认为这个补丁应该拆成两个，否则会存在显著的描述混杂。观察补丁内容，我们会发现补丁做了两件事情：
- 新增 ACPI OEM_ID 的检查功能（这是个特性）
- 基于 OEM_ID 检查，针对此芯平台设置 IOMMU_DOMAIN_DMA 属性（这是个平台规避）
将平台规避描述成新特性可能不是作者的意图，但最终得到的是一个记录不清的补丁。
而且，我建议仔细描述到底出了什么问题，解决了什么问题，否则后续出现回归或类似问题将难以厘清/归纳原因和现象。
opsiff
added a commit
to opsiff/UOS-kernel
that referenced
this pull request
Dec 1, 2025
…OUGH to default deepin inclusion category: other Removed CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y from defconfig to enforce strict DMA isolation by default. This change aligns ARM64 desktop kernel configuration with other arch. The config also affect cix in link [1]. Link: deepin-community#1335 Fixes: 7821b9fb89ca ("add deepin-community#880 config") Fixes: ce41a38 ("arm64: Add deepin_arm64_desktop_defconfig") Signed-off-by: Wentao Guan <[email protected]>
opsiff
added a commit
to opsiff/UOS-kernel
that referenced
this pull request
Dec 1, 2025
…OUGH to default deepin inclusion category: other Removed CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y from defconfig to enforce strict DMA isolation by default. This change aligns ARM64 desktop kernel configuration with other arch. The config also affect cix in link [1]. Note that may bring some affect in some phytium FT2000 or Kunpeng 920 device. Link: deepin-community#1335 Fixes: 7821b9fb89ca ("add deepin-community#880 config") Fixes: ce41a38 ("arm64: Add deepin_arm64_desktop_defconfig") Signed-off-by: Wentao Guan <[email protected]>
opsiff
added a commit
to opsiff/UOS-kernel
that referenced
this pull request
Dec 1, 2025
…OUGH to default deepin inclusion category: other Removed CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y from defconfig to enforce strict DMA isolation by default. This change aligns ARM64 desktop kernel configuration with other arch. The config also affect cix in link [1]. Note that may bring some affect in some phytium FT2000 or Kunpeng 920 device. Link: deepin-community#1335 Fixes: 7821b9fb89ca ("add deepin-community#880 config") Fixes: ce41a38 ("arm64: Add deepin_arm64_desktop_defconfig") Reported-by: Dylan.Wu" <[email protected]> Signed-off-by: Wentao Guan <[email protected]>
added 2 commits
December 4, 2025 16:32
adds a utility function acpi_check_oem_id() to detect whether the system firmware identifies a specific OEM_ID in the ACPI IORT table Signed-off-by: wenxue.ding <[email protected]> Signed-off-by: Dylan.Wu" <[email protected]>
extends the PCI device domain selection logic by introducing a vendor-specific path for CIXTEK platforms. Signed-off-by: wenxue.ding <[email protected]> Signed-off-by: Dylan.Wu" <[email protected]>
Ghostor8
force-pushed
the
linux-6.6.y
branch
3 times, most recently
from
02e483c to
0fa4d2c
Compare
updates align the USB controller settings with current system requirements and enhance compatibility and stability for USB gadget operations. Signed-off-by: Dylan.Wu" <[email protected]>
Ghostor8
force-pushed
the
linux-6.6.y
branch
2 times, most recently
from
8f06c87 to
d280979
Compare
aligns the driver with the existing HDA subsystem layout, improves code structure, and simplifies future maintenance Signed-off-by: Zichar.Zhang <[email protected]> Signed-off-by: Dylan.Wu" <[email protected]>
opsiff
reviewed
Dec 6, 2025
|
|
||
| static int rmem_dev_set_dma(struct device *dev, phys_addr_t base, size_t size) | ||
| { | ||
| dev_err(dev, "==== dev[%s], dev->dma[%s]", dev ? "1": "NULL", dev->dma_mem ? "1": "NULL"); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
improves system bring-up reliability and correct domain assignment on CIXTEK-based platforms
Summary by Sourcery
Adjust default IOMMU domain selection based on ACPI OEM_ID for specific platforms.
Bug Fixes:
Enhancements: