[Deepin-Kernel-SIG] [linux 6.6.y] [Phytium] some phytium driver fixes found by GitHub Copilot - #2078
Open
Avenger-285714 wants to merge 5 commits into
Open
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR hardens Phytium platform drivers by replacing a dynamically allocated reg-names array with a fixed stack array, adding missing resource-null checks and error paths, and initializing a resource pointer to avoid potential misuse in the SPI QSPI probe path. Sequence diagram for updated phytium_lbc_probe resource handlingsequenceDiagram
participant P as platform_device
participant D as device
participant L as phytium_lbc
participant R as resource
D->>L: phytium_lbc_probe(pdev)
alt dev.of_node
D->>P: platform_get_resource_byname(pdev, IORESOURCE_MEM, localbus)
P-->>R: resource(localbus)
D->>D: devm_ioremap_resource(dev, res) / lbc->io_base
alt IS_ERR(lbc->io_base)
D-->>D: return PTR_ERR(lbc->io_base)
end
D->>P: platform_get_resource_byname(pdev, IORESOURCE_MEM, lbc_mm)
P-->>R: resource(lbc_mm)
D->>D: devm_ioremap_resource(dev, res) / lbc->mm_base
alt IS_ERR(lbc->mm_base)
D-->>D: return PTR_ERR(lbc->mm_base)
end
else has_acpi_companion(dev)
D->>P: platform_get_resource(pdev, IORESOURCE_MEM, 0)
alt res == NULL
D-->>D: return -ENODEV
else res != NULL
D->>D: fwnode_property_read_string_array(dev->fwnode, reg-names, reg_names_array, 2)
D->>D: res->name = reg_names_array[0]
D->>D: devm_ioremap_resource(dev, res) / lbc->io_base
alt IS_ERR(lbc->io_base)
D-->>D: return PTR_ERR(lbc->io_base)
end
end
D->>P: platform_get_resource(pdev, IORESOURCE_MEM, 1)
alt res == NULL
D-->>D: return -ENODEV
else res != NULL
D->>D: res->name = reg_names_array[1]
D->>D: devm_ioremap_resource(dev, res) / lbc->mm_base
alt IS_ERR(lbc->mm_base)
D-->>D: return PTR_ERR(lbc->mm_base)
end
end
else no of_node and no ACPI companion
D->>D: dev_err(dev, no device tree node or ACPI companion found.)
D-->>D: return -ENODEV
end
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 |
![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 left some high level feedback:
- In the ACPI path of phytium_lbc_probe, the return value of fwnode_property_read_string_array() is ignored; consider checking it and failing probe if reg-names are missing so you don't assign NULL names to resources.
- phytium_lbc_probe still relies on 'res' when computing lbc->mm_size; ensure that in all early-return branches (including the new ENODEV paths) 'res' is either valid or not used afterward to avoid any accidental use of an uninitialized/incorrect resource.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the ACPI path of phytium_lbc_probe, the return value of fwnode_property_read_string_array() is ignored; consider checking it and failing probe if reg-names are missing so you don't assign NULL names to resources.
- phytium_lbc_probe still relies on 'res' when computing lbc->mm_size; ensure that in all early-return branches (including the new ENODEV paths) 'res' is either valid or not used afterward to avoid any accidental use of an uninitialized/incorrect resource.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
Fixes Phytium localbus and QSPI probe-time resource handling.
Changes:
- Removes invalid frees of managed localbus allocations.
- Validates localbus firmware and ACPI resources.
- Initializes the QSPI resource pointer.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
drivers/spi/spi-phytium-qspi.c |
Initializes the probe resource pointer. |
drivers/mtd/maps/phytium_lbc.c |
Improves allocation and resource error handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| struct device *dev = &pdev->dev; | ||
| struct spi_controller *ctrl; | ||
| struct resource *res; | ||
| struct resource *res = NULL; |
Member
Author
There was a problem hiding this comment.
除非固件有bug，不然没必要在这里做防御性修复
Clang warns that 'res' may be used uninitialized in
phytium_qspi_probe():
drivers/spi/spi-phytium-qspi.c:717:11: error: variable 'res' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
717 | else if (has_acpi_companion(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/spi/spi-phytium-qspi.c:723:45: note: uninitialized use occurs here
723 | qspi->io_base = devm_ioremap_resource(dev, res);
| ^~~
drivers/spi/spi-phytium-qspi.c:717:7: note: remove the 'if' if its condition is always true
717 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/spi/spi-phytium-qspi.c:688:22: note: initialize the variable 'res' to silence this warning
688 | struct resource *res;
| ^
| = NULL
1 error generated.
'res' is only assigned inside the 'if (dev->of_node)' /
'else if (has_acpi_companion(dev))' branches. When a device has
neither a DT node nor an ACPI companion, neither branch is taken and
'res' is passed uninitialized to devm_ioremap_resource(). GCC does
not report this, but with CONFIG_WERROR=y a clang build fails.
Initialize 'res' to NULL so that devm_ioremap_resource() handles the
missing-resource case gracefully (it returns -EINVAL for a NULL
resource) instead of consuming an uninitialized pointer.
Fixes: 8b02928 ("arm64: phytium: UEFI mode acpi table support for qspi/spi driver")
Assisted-by: GitHub Copilot:deepseek-v4-flash
Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
Fix a clang build error (fatal because CONFIG_WERROR=y) when compiling
for arm64:
drivers/mtd/maps/phytium_lbc.c:439:13: error: variable 'res' is used
uninitialized whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]
439 | } else if (has_acpi_companion(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/maps/phytium_lbc.c:460:31: note: uninitialized use occurs here
460 | lbc->mm_size = resource_size(res);
| ^~~
drivers/mtd/maps/phytium_lbc.c:439:9: note: remove the 'if' if its
condition is always true
439 | } else if (has_acpi_companion(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/maps/phytium_lbc.c:406:22: note: initialize the variable
'res' to silence this warning
406 | struct resource *res;
| ^
| = NULL
1 error generated.
In phytium_lbc_probe(), 'res' is only assigned inside the
"if (dev->of_node)" and "else if (has_acpi_companion(dev))" branches, but
it is dereferenced unconditionally afterwards by:
lbc->mm_size = resource_size(res);
When the probed platform device has neither a device tree node nor an
ACPI companion, 'res' is never assigned, so resource_size() dereferences
an uninitialized pointer. This is undefined behavior at runtime and is
also reported by clang's -Wsometimes-uninitialized, which is promoted to
a hard error by CONFIG_WERROR=y.
Reject devices with neither a device tree node nor an ACPI companion by
returning -ENODEV before 'res' is used. This guarantees 'res' is always
initialized when resource_size() is reached. No manual cleanup is needed
on this path because 'lbc' is allocated with devm_kzalloc() and is freed
automatically by devres when the probe fails.
Fixes: 64b35ad ("lbc: phytium: Add uefi support for localbus controller driver")
Assisted-by: GitHub Copilot:deepseek-v4-flash
Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
phytium_lbc_probe() allocates lbc with devm_kzalloc() but then calls kfree(lbc) on five error paths. devm_kzalloc() returns a pointer into the middle of the devres object (the devres header plus ARCH_DMA_MINALIGN padding sits in front of it), so kfree(lbc) is an interior-pointer free that corrupts the slab freelist; when the probe then fails, devres_release_all() frees the very same object again at its real base address - a double free. The production defconfig enables neither KASAN nor SLUB debugging, so on real firmware-error paths this is silent heap corruption. Simply drop the kfree() calls: devres releases the memory automatically when the probe fails. Spotted while addressing the reg_names_array leak flagged by code review in the same function. Fixes: 64b35ad ("lbc: phytium: Add uefi support for localbus controller driver") Reported-by: GitHub Copilot:Code Review model Assisted-by: Kimi Code:K3 Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
reg_names_array is kcalloc()'d unconditionally before the firmware-type branch, but it is only ever used in the ACPI branch and never freed anywhere: every successful probe leaks the 32-byte array (on the OF path it is not even used). The early return for devices with neither an OF node nor an ACPI companion, added later by commit b04ff35 ("mtd: maps: phytium_lbc: fix uninitialized use of 'res' in probe"), leaks it as well. The string pointers filled in by fwnode_property_read_string_array() are owned by the firmware side (ACPI documents that callers must not free them; OF points into the unflattened device tree), and devm_ioremap_resource() duplicates the resource name, so the array itself does not need to outlive the assignments. Replace the heap allocation with a stack array, which also removes the unchecked-NULL kcalloc failure mode. Fixes: 64b35ad ("lbc: phytium: Add uefi support for localbus controller driver") Reported-by: GitHub Copilot:Code Review model Assisted-by: Kimi Code:K3 Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
The ACPI branch dereferences the struct resource returned by
platform_get_resource() ("res->name = ...") without checking it,
oopsing on a NULL pointer when the platform device does not carry
the expected memory resources. The OF branch needs no such check
because devm_ioremap_resource() itself rejects a NULL resource.
Return -ENODEV when either resource is missing.
Fixes: 64b35ad ("lbc: phytium: Add uefi support for localbus controller driver")
Reported-by: GitHub Copilot:Code Review model
Assisted-by: Kimi Code:K3
Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
Avenger-285714
force-pushed
the
phytium-6.6
branch
from
August 14, 2026 07:03
ea25769 to
27e82a9
Compare
Member
Author
胡说八道 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Fix Phytium localbus and QSPI driver probing and resource handling.
Bug Fixes: