[Deepin-Kernel-SIG] [linux 6.6.y] [deepin] LoongArch: legacyboot: BPI: workaround old firmware with ISA#1631
Conversation
Reviewer's GuideRestores legacy BPI ISA I/O handling on LoongArch by registering a synthetic logic_pio range for ISA space, using either a fixed physical base under ACPI or a parsed DT isa node, and mapping it into the PCI I/O window at init time. Sequence diagram for legacy ISA I/O registration during LoongArch bootsequenceDiagram
participant KernelInit
participant register_legacy_isa_io
participant ACPI
participant DeviceTree
participant parse_isa_base
participant add_legacy_isa_io
participant logic_pio
participant MM
KernelInit->>register_legacy_isa_io: arch_initcall(register_legacy_isa_io)
activate register_legacy_isa_io
alt ACPI_enabled
register_legacy_isa_io->>ACPI: check acpi_disabled
ACPI-->>register_legacy_isa_io: acpi_disabled == false
register_legacy_isa_io->>register_legacy_isa_io: cpu_addr = ISA_PHY_IOBASE
register_legacy_isa_io->>register_legacy_isa_io: fwnode = kzalloc(fwnode)
else DeviceTree_only
register_legacy_isa_io->>parse_isa_base: parse_isa_base(&cpu_addr)
activate parse_isa_base
parse_isa_base->>DeviceTree: for_each_node_by_name(isa)
DeviceTree-->>parse_isa_base: np, ranges property
alt ranges_found
parse_isa_base->>DeviceTree: of_translate_address(np, ranges + 2)
DeviceTree-->>parse_isa_base: cpu_addr
parse_isa_base-->>register_legacy_isa_io: &np->fwnode
else ranges_not_found
parse_isa_base-->>register_legacy_isa_io: NULL
end
deactivate parse_isa_base
end
alt fwnode_non_null
register_legacy_isa_io->>add_legacy_isa_io: add_legacy_isa_io(fwnode, cpu_addr)
activate add_legacy_isa_io
add_legacy_isa_io->>add_legacy_isa_io: range = kzalloc(struct logic_pio_hwaddr)
add_legacy_isa_io->>add_legacy_isa_io: init range fields
add_legacy_isa_io->>logic_pio: logic_pio_register_range(range)
alt register_ok
logic_pio-->>add_legacy_isa_io: success
add_legacy_isa_io->>add_legacy_isa_io: check range->io_start == 0
alt io_start_zero
add_legacy_isa_io->>MM: ioremap_page_range(PCI_IOBASE + range->io_start,
MM-->>add_legacy_isa_io: mapping_result
else io_start_nonzero
add_legacy_isa_io->>logic_pio: logic_pio_unregister_range(range)
add_legacy_isa_io->>add_legacy_isa_io: kfree(range)
end
else register_failed
logic_pio-->>add_legacy_isa_io: error
add_legacy_isa_io->>add_legacy_isa_io: kfree(range)
end
add_legacy_isa_io-->>register_legacy_isa_io: ret
deactivate add_legacy_isa_io
else fwnode_null
register_legacy_isa_io->>register_legacy_isa_io: no ISA IO registration
end
register_legacy_isa_io-->>KernelInit: return 0
deactivate register_legacy_isa_io
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 found 1 issue, and left some high level feedback:
- The ACPI path allocates a
struct fwnode_handlewithkzallocbut never initializes or frees it, andfwnode_handleis typically managed by the firmware/OF/ACPI subsystems rather than heap-allocated directly; consider using an existing fwnode or a proper dummy fwnode helper, and ensuring any temporary allocations are freed. - In
parse_isa_base, thefor_each_node_by_nameloop logic looks off: the conditionif (!ranges || (ranges && len > 0)) break;will break as soon aslen > 0, and you also leak the reference acquired byof_node_get(np)and later usenpafter the loop, which may not correspond to the node that actually providedranges; consider restructuring the loop to only break once a valid ranges property is found, callingof_node_putappropriately, and using the node you translated from. - For the init-time allocations in
add_legacy_isa_ioandregister_legacy_isa_io,GFP_ATOMICis unnecessarily restrictive; usingGFP_KERNELwould be more appropriate in this non-atomic context and avoids failing allocations under normal memory pressure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ACPI path allocates a `struct fwnode_handle` with `kzalloc` but never initializes or frees it, and `fwnode_handle` is typically managed by the firmware/OF/ACPI subsystems rather than heap-allocated directly; consider using an existing fwnode or a proper dummy fwnode helper, and ensuring any temporary allocations are freed.
- In `parse_isa_base`, the `for_each_node_by_name` loop logic looks off: the condition `if (!ranges || (ranges && len > 0)) break;` will break as soon as `len > 0`, and you also leak the reference acquired by `of_node_get(np)` and later use `np` after the loop, which may not correspond to the node that actually provided `ranges`; consider restructuring the loop to only break once a valid ranges property is found, calling `of_node_put` appropriately, and using the node you translated from.
- For the init-time allocations in `add_legacy_isa_io` and `register_legacy_isa_io`, `GFP_ATOMIC` is unnecessarily restrictive; using `GFP_KERNEL` would be more appropriate in this non-atomic context and avoids failing allocations under normal memory pressure.
## Individual Comments
### Comment 1
<location path="arch/loongarch/kernel/legacy_boot.c" line_range="728" />
<code_context>
+{
+ int ret = 0;
+ unsigned long vaddr;
+ struct logic_pio_hwaddr *range;
+ range = kzalloc(sizeof(*range), GFP_ATOMIC);
+ if (!range)
+ return -ENOMEM;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use GFP_KERNEL instead of GFP_ATOMIC in init context
This runs in init context, not atomic/IRQ, so GFP_ATOMIC is unnecessarily restrictive and increases the chance of allocation failure under memory pressure. Please use GFP_KERNEL here for a more appropriate and consistent init-time allocation.
```suggestion
range = kzalloc(sizeof(*range), GFP_KERNEL);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| int ret = 0; | ||
| unsigned long vaddr; | ||
| struct logic_pio_hwaddr *range; | ||
| range = kzalloc(sizeof(*range), GFP_ATOMIC); |
There was a problem hiding this comment.
suggestion (bug_risk): Use GFP_KERNEL instead of GFP_ATOMIC in init context
This runs in init context, not atomic/IRQ, so GFP_ATOMIC is unnecessarily restrictive and increases the chance of allocation failure under memory pressure. Please use GFP_KERNEL here for a more appropriate and consistent init-time allocation.
| range = kzalloc(sizeof(*range), GFP_ATOMIC); | |
| range = kzalloc(sizeof(*range), GFP_KERNEL); |
|
Could you briefly describe the specific symptoms of the issue in the commit message? |
There was a problem hiding this comment.
Pull request overview
Restores legacy ISA I/O (Logic PIO) range registration on LoongArch to work around buggy/old firmware that does not correctly describe ISA I/O in ACPI/DSDT, enabling a usable PIO mapping during boot.
Changes:
- Add an
arch_initcall()hook to register a legacy ISA I/O Logic PIO range. - Add DT parsing helper to derive an ISA base when ACPI is disabled.
- Map the ISA MMIO-backed PIO window at
PCI_IOBASEviaioremap_page_range().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const __be32 *ranges = NULL; | ||
| int len; | ||
| struct device_node *node; | ||
| for_each_node_by_name(np, "isa") { | ||
| node = of_node_get(np); | ||
| if (!node) | ||
| break; | ||
| ranges = of_get_property(node, "ranges", &len); | ||
| if (!ranges || (ranges && len > 0)) | ||
| break; | ||
| } | ||
| if (ranges) { | ||
| ranges += 2; | ||
| *cpu_addr = of_translate_address(np, ranges); | ||
| return &np->fwnode; | ||
| } |
There was a problem hiding this comment.
The ranges property parsing in parse_isa_base() looks incorrect: ranges += 2 assumes a fixed cell layout, but ranges encoding depends on #address-cells/#size-cells and should be parsed with of_range_parser_init() / for_each_of_range() (as done in arch/loongarch/kernel/setup.c:544-558). As written, of_translate_address(np, ranges) can translate the wrong CPU address on DTs with different cell sizes.
| const __be32 *ranges = NULL; | |
| int len; | |
| struct device_node *node; | |
| for_each_node_by_name(np, "isa") { | |
| node = of_node_get(np); | |
| if (!node) | |
| break; | |
| ranges = of_get_property(node, "ranges", &len); | |
| if (!ranges || (ranges && len > 0)) | |
| break; | |
| } | |
| if (ranges) { | |
| ranges += 2; | |
| *cpu_addr = of_translate_address(np, ranges); | |
| return &np->fwnode; | |
| } | |
| struct of_range_parser parser; | |
| struct of_range range; | |
| for_each_node_by_name(np, "isa") { | |
| if (of_range_parser_init(&parser, np)) | |
| continue; | |
| for_each_of_range(&parser, &range) { | |
| *cpu_addr = range.cpu_addr; | |
| return &np->fwnode; | |
| } | |
| } |
| u64 cpu_addr; | ||
| if (!acpi_disabled) { | ||
| cpu_addr = ISA_PHY_IOBASE; | ||
| fwnode = kzalloc(sizeof(*fwnode), GFP_ATOMIC); |
There was a problem hiding this comment.
In the ACPI-enabled path, allocating a raw struct fwnode_handle with kzalloc() leaves it uninitialized (no fwnode_ops), which can break helpers like is_acpi_device_node() if anything later inspects this handle. If you need a synthetic fwnode for a static mapping, use acpi_alloc_fwnode_static() (which calls fwnode_init() with acpi_static_fwnode_ops) rather than open-coding kzalloc().
| fwnode = kzalloc(sizeof(*fwnode), GFP_ATOMIC); | |
| fwnode = acpi_alloc_fwnode_static(); |
| if (!acpi_disabled) { | ||
| cpu_addr = ISA_PHY_IOBASE; | ||
| fwnode = kzalloc(sizeof(*fwnode), GFP_ATOMIC); | ||
| } else { | ||
| fwnode = parse_isa_base(&cpu_addr); | ||
| } |
There was a problem hiding this comment.
register_legacy_isa_io() runs unconditionally as an arch_initcall() and will also attempt DT-based registration when acpi_disabled is true. But LoongArch already registers ISA PIO ranges from DT in arch_reserve_pio_range() (arch/loongarch/kernel/setup.c:534-572). Depending on initcall ordering, this can lead to duplicate registrations (-EEXIST) and spurious warnings from the other path. Consider gating this initcall to the legacy-BPI/ACPI workaround case only (e.g., have_bpi && bpi_version == BPI_VERSION_V1 && !acpi_disabled), and rely on the existing DT path otherwise.
| if (!acpi_disabled) { | |
| cpu_addr = ISA_PHY_IOBASE; | |
| fwnode = kzalloc(sizeof(*fwnode), GFP_ATOMIC); | |
| } else { | |
| fwnode = parse_isa_base(&cpu_addr); | |
| } | |
| if (!have_bpi || bpi_version != BPI_VERSION_V1 || acpi_disabled) | |
| return 0; | |
| cpu_addr = ISA_PHY_IOBASE; | |
| fwnode = kzalloc(sizeof(*fwnode), GFP_ATOMIC); |
| } | ||
| vaddr = (unsigned long)(PCI_IOBASE + range->io_start); | ||
| ret = ioremap_page_range(vaddr, vaddr + range->size, range->hw_start, | ||
| pgprot_device(PAGE_KERNEL)); |
There was a problem hiding this comment.
add_legacy_isa_io() returns the result of ioremap_page_range(), but on failure the newly-registered Logic PIO range is left registered (and the allocated range is leaked). Consider unregistering the range (and freeing the allocation) if ioremap_page_range() fails, to avoid a partially-initialized mapping remaining in the global io_range_list.
| pgprot_device(PAGE_KERNEL)); | |
| pgprot_device(PAGE_KERNEL)); | |
| if (ret) { | |
| logic_pio_unregister_range(range); | |
| kfree(range); | |
| } |
| for_each_node_by_name(np, "isa") { | ||
| node = of_node_get(np); | ||
| if (!node) | ||
| break; | ||
| ranges = of_get_property(node, "ranges", &len); | ||
| if (!ranges || (ranges && len > 0)) | ||
| break; | ||
| } |
There was a problem hiding this comment.
parse_isa_base() takes an extra reference with of_node_get(np) but never drops it. Also, for_each_node_by_name() already manages node refcounting via of_find_node_by_name(), so this extra of_node_get() will leak a reference (especially when the loop breaks). Please remove the redundant of_node_get() and ensure any early break path drops the current np reference if needed.
|
Please put it in the commit message. Since the BPI series has always been out‑of‑tree patches, the record in the PR will get lost as patches are ported across branches and repositories—but the commit message itself won't. |
deepin inclusion category: other Restore BPI logic to workaround with some buggy DSDT table with ISA IO, delete of logic for used in arch_reserve_pio_range. Log: [22:22:59:896] [ 0.632863] CPU 6 Unable to handle kernel paging request at virtual address ffff80000000802e, era == 900000000410645c, ra == 90000000041310b8␍␊ [22:22:59:905] [ 0.645489] Oops[#1]:␍␊ [22:22:59:909] [ 0.647740] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 6.6.40-loong64-desktop-hwe deepin-community#23.01.00.35␍␊ [22:22:59:917] [ 0.656213] Hardware name: KaiTian 90Y4A08AKX/KL3A60007A2000DTMB1, BIOS W0MKT0DA 06/25/2024␍␊ [22:22:59:924] [ 0.664513] pc 900000000410645c ra 90000000041310b8 tp 9000000100608000 sp 900000010060b4f0␍␊ [22:22:59:933] [ 0.672812] a0 000000000000002e a1 0000000000000087 a2 0000000000000008 a3 0000000000000000␍␊ [22:22:59:941] [ 0.681111] a4 0000000000000000 a5 0000000000000000 a6 0000000000000000 a7 0000000000000000␍␊ [22:22:59:950] [ 0.689410] t0 ffff800000008000 t1 0000000000008000 t2 0000000000000000 t3 0000000000000000␍␊ [22:22:59:958] [ 0.697709] t4 0000000000000000 t5 0000000000000000 t6 0000000000000000 t7 0000000000000000␍␊ [22:22:59:967] [ 0.706008] t8 0000000000000000 u0 00000000000000b4 s9 0000000000000000 s0 900000010105a2e0␍␊ [22:22:59:974] [ 0.714307] s1 000000000000002e s2 0000000000000000 s3 9000000100fcb840 s4 0000000000000008␍␊ [22:22:59:984] [ 0.722606] s5 900000010105a5f8 s6 900000010060b628 s7 9000000101058b40 s8 9000000005bcd000␍␊ [22:22:59:994] [ 0.730905] ra: 90000000041310b8 acpi_ev_address_space_dispatch+0x31c/0x384␍␊ [22:23:00:003] [ 0.738086] ERA: 900000000410645c acpi_os_write_port+0x28/0x120␍␊ [22:23:00:008] [ 0.744142] CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)␍␊ [22:23:00:013] [ 0.750287] PRMD: 00000004 (PPLV0 +PIE -PWE)␍␊ [22:23:00:018] [ 0.754614] EUEN: 00000000 (-FPE -SXE -ASXE -BTE)␍␊ [22:23:00:023] [ 0.759373] ECFG: 00071c1d (LIE=0,2-4,10-12 VS=7)␍␊ [22:23:00:028] [ 0.764131] ESTAT: 00020000 [PIS] (IS= ECode=2 EsubCode=0)␍␊ [22:23:00:033] [ 0.769580] BADV: ffff80000000802e␍␊ [22:23:00:033] [ 0.773040] PRID: 0014d000 (Loongson-64bit, Loongson-3A6000-HV)␍␊ [22:23:00:038] [ 0.779006] Modules linked in:␍␊ [22:23:00:043] [ 0.782033] Process swapper/0 (pid: 1, threadinfo=(ptrval), task=(ptrval))␍␊ [22:23:00:050] [ 0.790247] Stack : 9000000005bcd000 0000000000000001 9000000100eec040 90000000049a9a5c␍␊ [22:23:00:057] [ 0.798205] 0000000000000000 8f041dc7a5a6d059 900000010060b558 900000010003d600␍␊ [22:23:00:066] [ 0.806162] <0xa0> 0000000000000001 00000000000000ff 900000010105a2e0 0000000000000000␍␊ [22:23:00:074] [ 0.814119] 900000010060b628 0000000000000001 9000000101058b40 0000000000000000␍␊ [22:23:00:084] [ 0.822076] 0000000000000087 9000000004136db8 9000000100d91d20 0000000000000008␍␊ [22:23:00:092] [ 0.830033] 0000000000000008 9000000005bcd000 0000000000000001 900000010060b628␍␊ [22:23:00:102] [ 0.837990] 9000000101058b40 9000000004137278 0000000000000cc0 000000000000ab00␍␊ [22:23:00:107] [ 0.845946] 900000010060b828 8f041dc7a5a6d059 9000000100e4e000 0000000000000001␍␊ [22:23:00:114] [ 0.853903] 9000000005bcd000 9000000101058b40 ffffffffffffff00 9000000004137474␍␊ [22:23:00:121] [ 0.861860] 0000000000000cc0 900000010037df48 0000000000000dc0 0000000000000087␍␊ [22:23:00:129] [ 0.869817] ...␍␊ [22:23:00:133] [ 0.872241] Call Trace:␍␊ [22:23:00:138] [ 0.872243] [<900000000410645c>] acpi_os_write_port+0x28/0x120␍␊ [22:23:00:144] [ 0.880460] [<90000000041310b8>] acpi_ev_address_space_dispatch+0x31c/0x384␍␊ [22:23:00:148] [ 0.887377] [<9000000004136db8>] acpi_ex_access_region+0x6c/0x118␍␊ [22:23:00:155] [ 0.893432] [<9000000004137278>] acpi_ex_field_datum_io+0x134/0x23c␍␊ [22:23:00:161] [ 0.899658] [<9000000004137474>] acpi_ex_write_with_update_rule+0xf4/0x138␍␊ [22:23:00:166] [ 0.906489] [<90000000041370b8>] acpi_ex_insert_into_field+0x254/0x2e0␍␊ [22:23:00:173] [ 0.912974] [<9000000004136aa8>] acpi_ex_write_data_to_field+0x1a4/0x200␍␊ [22:23:00:180] [ 0.919633] [<900000000413c474>] acpi_ex_store_object_to_node+0x1c8/0x280␍␊ [22:23:00:187] [ 0.926379] [<9000000004138de4>] acpi_ex_opcode_1A_1T_1R+0x3f0/0x5c8␍␊ [22:23:00:195] [ 0.932692] [<900000000412c144>] acpi_ds_exec_end_op+0xe4/0x504␍␊ [22:23:00:203] [ 0.938575] [<90000000041497a8>] acpi_ps_parse_loop+0x60c/0x71c␍␊ [22:23:00:208] [ 0.944456] [<900000000414a7b8>] acpi_ps_parse_aml+0x118/0x400␍␊ [22:23:00:213] [ 0.950250] [<900000000414b528>] acpi_ps_execute_method+0x1e0/0x294␍␊ [22:23:00:220] [ 0.956476] [<9000000004142728>] acpi_ns_evaluate+0x1e8/0x308␍␊ [22:23:00:225] [ 0.962183] [<9000000004155618>] acpi_ut_evaluate_object+0xac/0x238␍␊ [22:23:00:229] [ 0.968410] [<900000000414e268>] acpi_rs_get_method_data+0x38/0x9c␍␊ [22:23:00:235] [ 0.974550] [<900000000414ed68>] acpi_walk_resources+0x8c/0x108␍␊ [22:23:00:240] [ 0.980430] [<9000000004116cd4>] acpi_dev_get_resources+0x98/0x10c␍␊ [22:23:00:248] [ 0.986572] [<9000000004114878>] acpi_init_device_object+0x308/0x48c␍␊ [22:23:00:254] [ 0.992885] [<9000000004114a94>] acpi_add_single_object+0x98/0x390␍␊ [22:23:00:259] [ 0.999025] [<9000000004114ee8>] acpi_bus_check_add+0x15c/0x26c␍␊ [22:23:00:265] [ 1.004906] [<9000000004146cf4>] acpi_ns_walk_namespace+0x108/0x284␍␊ [22:23:00:270] [ 1.011133] [<900000000414700c>] acpi_walk_namespace+0xb8/0x138␍␊ [22:23:00:276] [ 1.017013] [<90000000041150a8>] acpi_bus_scan+0x88/0x1f4␍␊ [22:23:00:282] [ 1.022375] [<90000000049f041c>] acpi_scan_init+0x19c/0x330␍␊ [22:23:00:287] [ 1.027913] [<90000000049f0018>] acpi_init+0xdc/0x164␍␊ [22:23:00:293] [ 1.032930] [<9000000003560c7c>] do_one_initcall+0xd0/0x278␍␊ [22:23:00:298] [ 1.038466] [<90000000049b1748>] do_initcalls+0x114/0x164␍␊ [22:23:00:304] [ 1.043831] [<90000000049b1980>] kernel_init_freeable+0x174/0x1b0␍␊ [22:23:00:309] [ 1.049885] [<900000000499b250>] kernel_init+0x28/0x138␍␊ [22:23:00:317] [ 1.055076] [<9000000003561ef0>] ret_from_kernel_thread+0xc/0x9c␍␊ [22:23:00:321] [ 1.061043] ␍␊ [22:23:00:326] [ 1.062514] Code: 28c3418c 1400010d 0010b58c <38101185> 3872000a 00150004 00150005 00150006 00150007 ␍␊ [22:23:00:334] [ 1.072205] ␍␊ [22:23:00:334] [ 1.073679] ---[ end trace 0000000000000000 ]---␍␊ [22:23:00:338] [ 1.078266] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b␍␊ [22:23:00:345] [ 1.085877] Kernel relocated by 0x3340000␍␊ [22:23:00:352] [ 1.089857] .text @ 0x9000000003540000␍␊ [22:23:00:356] [ 1.093663] .data @ 0x9000000004b10000␍␊ [22:23:00:360] [ 1.097468] .bss @ 0x900000000671ca00␍␊ Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
opsiff
force-pushed
the
linux-6.6.y-2026-04-15-loongarch
branch
from
aaa5e4f to
ac22499
Compare
done |
deepin inclusion
category: other
Restore BPI logic to workaround with some buggy DSDT table with ISA IO.
Summary by Sourcery
Restore legacy BPI handling for LoongArch systems to support ISA I/O on buggy firmware/DSDT configurations.
New Features:
Bug Fixes: