uefi: x86: current (6.12) and edge (6.18): add Apple T2 patches #8879
Conversation
WalkthroughAdds extensive Apple T2 support across the x86 UEFI family: new drivers (BCE, Touch Bar DRM, HID backlight/keyboard, magicmouse T2 support), large applesmc refactors (per-device state, ACPI/MMIO/port paths, fan/BCLM), i915/gmux quirks, APFS build integration, and config auto-fetch for T2 patches. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as uefi-x86.conf
participant Fetch as PatchFetcher
participant FS as Filesystem
Config->>Fetch: check applet2 branch & patches
alt patches missing
Fetch->>FS: mkdir & download tarball
Fetch->>FS: extract & prune non-patch files
Fetch->>Config: trigger rewrite & exit
else patches present
Fetch-->>Config: informational skip
end
sequenceDiagram
participant ACPI as ACPI bus
participant Probe as applesmc_add
participant Resources as Resource Walk
participant MMIO as MMIO init
participant Port as Port I/O init
participant SMC as applesmc_device
Probe->>ACPI: add device
ACPI->>Resources: walk resources (_CRS)
Resources->>MMIO: detect ACPI_FIXED_MEMORY32?
alt MMIO available & valid
MMIO->>MMIO: map iomem, validate LDKN
MMIO->>SMC: enable iomem path
else
Resources->>Port: find I/O port region
Port->>SMC: set port_base, request region
end
SMC-->>Probe: probe complete (per-device context ready)
sequenceDiagram
participant USB as USB HID device
participant Probe as magicmouse_probe
participant Config as T2 config table
participant Input as input_setup
USB->>Probe: attach device
Probe->>Probe: match product ID
alt WellSPRING T2
Probe->>Config: load model geometry/res
Config->>Input: configure MT axes & ranges
Probe->>Probe: register T2 raw handler
else legacy
Probe->>Input: standard setup
end
Probe-->>USB: device ready for events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas needing extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Nitpick comments (6)
patch/kernel/archive/uefi-x86-6.12/3009-applesmc-battery-charge-limiter.patch (1)
24-28: Redundant negative value check for unsigned type.The condition
val < 0on line 27 is always false becausevalis declared asu8(unsigned). Thekstrtou8function already ensures non-negative values, so this check is redundant.Apply this diff to simplify:
if (kstrtou8(sysfsbuf, 10, &val) < 0) return -EINVAL; - if (val < 0 || val > 100) + if (val > 100) return -EINVAL;config/sources/families/uefi-x86.conf (1)
55-55: Make tree command conditional.The
treecommand may not be available in all build environments, causing "command not found" errors in the build log.Apply this diff to make it conditional:
- tree "${SRC}/patch/kernel/${KERNELPATCHDIR}" + command -v tree &>/dev/null && tree "${SRC}/patch/kernel/${KERNELPATCHDIR}" || ls -la "${SRC}/patch/kernel/${KERNELPATCHDIR}"Alternatively, remove it entirely since the alert message on line 60 already communicates the directory location.
patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patch (1)
50-53: Consider reviewing Kconfig dependency strategy.The use of
selectfor SOUND, SND, SND_PCM, and SND_JACK forces these subsystems to be enabled. While this ensures audio support is available for the T2 chip, usingselectcan be problematic if these subsystems have unmet dependencies.Consider whether
depends onwould be more appropriate, or verify that the aggressiveselectapproach is intentional for the Apple T2 use case.patch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch (1)
29-29: Switch to kernel-style#endifcomment.Line 29 uses
#endif //APPLE_BCE_H. The kernel style guide expects a block comment here. Please change it to#endif /* APPLE_BCE_H */to stay compliant. As per coding guidelines.-#endif //APPLE_BCE_H +#endif /* APPLE_BCE_H */patch/kernel/archive/uefi-x86-6.12/1002-Put-apple-bce-in-drivers-staging.patch (1)
63-63: Remove the stray apostrophe.Line 63 ends the help text with an extra
', which will show up inmenuconfig. Dropping it keeps the prompt clean.- If "M" is selected, the module will be called apple-bce.' + If "M" is selected, the module will be called apple-bce.patch/kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch (1)
258-284: Check return values from backlight operations.The calls to
backlight_device_set_brightness()at lines 264, 268, and 278 don't check return values. While backlight operations are not critical to core functionality, errors should be logged for diagnostics.Consider checking and logging errors:
if (!kbd->has_dimmed) { - backlight_device_set_brightness(kbd->backlight_dev, 1); + ret = backlight_device_set_brightness(kbd->backlight_dev, 1); + if (ret) + pr_debug("Failed to dim backlight: %d\n", ret); kbd->has_dimmed = true;Apply similar checks at lines 268 and 278.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (48)
config/sources/families/uefi-x86.conf(1 hunks)patch/kernel/archive/uefi-x86-6.12/1002-Put-apple-bce-in-drivers-staging.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1004-Fix-freezing-on-turning-off-camera.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1005-HID-hid-appletb-bl-add-driver-for-the-backlight-of-A.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1007-HID-multitouch-Get-the-contact-ID-from-HID_DG_TRANSD.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1008-HID-multitouch-support-getting-the-tip-state-from-HI.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1009-HID-multitouch-take-cls-maxcontacts-into-account-for.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1010-HID-multitouch-specify-that-Apple-Touch-Bar-is-direc.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1011-HID-multitouch-add-device-ID-for-Apple-Touch-Bar.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1013-lib-vsprintf-Add-support-for-generic-FourCCs-by-exte.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1015-drm-tiny-add-driver-for-Apple-Touch-Bars-in-x86-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/2008-i915-4-lane-quirk-for-mbp15-1.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3002-applesmc-make-io-port-base-addr-dynamic.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3003-applesmc-switch-to-acpi_device-from-platform.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3004-applesmc-key-interface-wrappers.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3005-applesmc-basic-mmio-interface-implementation.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3006-applesmc-fan-support-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3008-applesmc-make-applesmc_remove-void.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3009-applesmc-battery-charge-limiter.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4002-HID-quirks-remove-T2-devices-from-hid_mouse_ignore_l.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4003-HID-apple-ignore-the-trackpad-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4005-HID-apple-Add-necessary-IDs-and-support-for-replacem.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4006-HID-magicmouse-Add-MacBookPro15-1-replacement-trackp.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/8002-Necessary-modifications-to-build-APFS-with-the-kerne.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/1003-Fix-freezing-on-turning-off-camera.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/2008-i915-4-lane-quirk-for-mbp15-1.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3002-applesmc-make-io-port-base-addr-dynamic.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3003-applesmc-switch-to-acpi_device-from-platform.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3004-applesmc-key-interface-wrappers.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3005-applesmc-basic-mmio-interface-implementation.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3006-applesmc-fan-support-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3008-applesmc-make-applesmc_remove-void.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4003-HID-apple-ignore-the-trackpad-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4005-HID-magicmouse-fix-regression-breaking-support-for-M.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: rpardini
Repo: armbian/build PR: 8780
File: config/sources/families/include/uefi_common.inc:44-44
Timestamp: 2025-10-18T11:52:36.964Z
Learning: The uefi-riscv64 and uefi-x86 UEFI families in the Armbian build system do not have kernel patches and never have had patches. Missing patch directories like archive/uefi-riscv64-6.18 and archive/uefi-x86-6.18 are expected and normal for these families.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Learnt from: amazingfate
Repo: armbian/build PR: 8453
File: lib/functions/compilation/patch/drivers_network.sh:213-216
Timestamp: 2025-08-02T14:59:28.145Z
Learning: The wireless driver patches wireless-rtl8812au-Fix-6.16.patch, wireless-rtl8811cu-Fix-6.16.patch, and wireless-rtl88x2bu-Fix-6.16.patch in the Armbian build system are backward compatible and can be applied unconditionally without version checks, even though they're named for 6.16+ fixes.
📚 Learning: 2025-10-18T11:52:36.964Z
Learnt from: rpardini
Repo: armbian/build PR: 8780
File: config/sources/families/include/uefi_common.inc:44-44
Timestamp: 2025-10-18T11:52:36.964Z
Learning: The uefi-riscv64 and uefi-x86 UEFI families in the Armbian build system do not have kernel patches and never have had patches. Missing patch directories like archive/uefi-riscv64-6.18 and archive/uefi-x86-6.18 are expected and normal for these families.
Applied to files:
config/sources/families/uefi-x86.confpatch/kernel/archive/uefi-x86-6.18/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patchpatch/kernel/archive/uefi-x86-6.18/3008-applesmc-make-applesmc_remove-void.patchpatch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch
📚 Learning: 2025-10-14T05:08:11.785Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-08-02T05:46:10.664Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-02T05:46:10.664Z
Learning: In the Armbian build system, the modern recommended approach for kernel configuration is to use the kernel-config command via "./compile.sh BOARD=boardname BRANCH=branchname kernel-config" instead of the deprecated KERNEL_CONFIGURE=yes flag. This provides a two-step workflow: configure using menuconfig, then build, with better transparency and control over configuration changes.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-12T19:28:38.491Z
Learnt from: Grippy98
Repo: armbian/build PR: 8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.491Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-06-25T03:42:09.086Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-06-25T03:40:52.109Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-22T21:59:36.334Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:200-200
Timestamp: 2025-09-22T21:59:36.334Z
Learning: Functions named with the pattern `armbian_kernel_config__*` in lib/functions/compilation/armbian-kernel.sh are automatically discovered and invoked by Armbian's extension system via metaprogramming using `compgen -A function`, without requiring explicit registration or calls.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-08-30T06:48:09.091Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T06:48:09.091Z
Learning: In lib/functions/compilation/armbian-kernel.sh, the user prefers flexible grep patterns over anchored ones for BTRFS configuration checks, but agrees to use quiet grep (-q) to avoid polluting build logs.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-07-27T15:54:35.119Z
Learnt from: pyavitz
Repo: armbian/build PR: 8421
File: config/kernel/linux-sunxi64-edge.config:2168-2173
Timestamp: 2025-07-27T15:54:35.119Z
Learning: In the Armbian build system, staging 802.11 drivers like CONFIG_RTLLIB, CONFIG_RTL8192E, CONFIG_R8712U are kept enabled alongside upstream rtw88 drivers because rtw88 is still under development and suffers from reliability issues. The staging drivers serve as necessary fallbacks when upstream drivers are unstable, prioritizing working hardware over avoiding technical conflicts.
Applied to files:
patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patchpatch/kernel/archive/uefi-x86-6.12/1002-Put-apple-bce-in-drivers-staging.patch
🔇 Additional comments (52)
patch/kernel/archive/uefi-x86-6.18/2008-i915-4-lane-quirk-for-mbp15-1.patch (5)
1-14: LGTM: Proper patch header and attribution.The patch header follows standard git format with clear attribution to both the submitter (Orlando Chamberlain) and original author (Kerem Karabay). The subject and description adequately explain the purpose.
19-28: LGTM: Quirk check implementation is correct.The quirk check is properly placed before platform-specific logic, ensuring the quirk takes precedence. The implementation follows the existing pattern using
intel_has_quirk()and early return.
37-59: LGTM: Quirk handler and registration follow established patterns.The handler function correctly uses
intel_set_quirk()and includes helpful logging. The comprehensive comment explains the dual-GPU boot scenario clearly. The quirk table entry is appropriately placed at the end with device IDs matching MacBookPro15,1 hardware (Coffee Lake GT2 iGPU, Apple vendor ID).
66-71: LGTM: Enum addition follows best practices.The new
QUIRK_DDI_A_FORCE_4_LANESenum value is correctly appended to the end of theintel_quirk_idenum, preserving existing ordinal values and following the established naming convention.
1-76: Contextual note: This marks the first kernel patches for uefi-x86.Based on retrieved learnings, the uefi-x86 family previously had no kernel patches. This PR intentionally adds T2-specific patches from the linux-t2 project to enable Apple T2 Mac support. The patch structure and content are solid.
Based on learnings.
patch/kernel/archive/uefi-x86-6.12/4002-HID-quirks-remove-T2-devices-from-hid_mouse_ignore_l.patch (4)
1-13: LGTM! Well-formed patch header.The patch header contains all required metadata with proper attribution and clear documentation of the change's purpose.
14-32: LGTM! Clean and targeted device removal.The patch correctly removes only the Apple WellSpring T2 device entries from the ignore list while preserving other WellSpring variants and devices. This selective approach enables HID driver binding specifically for T2 trackpads without affecting other devices.
33-35: LGTM! Standard patch footer.
1-35: Note: New direction for uefi-x86 family.This patch represents a significant change for the uefi-x86 family, which historically has not included kernel patches. The addition of Apple T2 support patches is well-documented in the PR objectives and aligns with enabling T2 Mac hardware support.
Based on learnings.
patch/kernel/archive/uefi-x86-6.12/4006-HID-magicmouse-Add-MacBookPro15-1-replacement-trackp.patch (3)
1-13: LGTM! Patch metadata is well-formed.The patch header follows standard kernel patch conventions with proper attribution, date, subject, and sign-off.
22-32: LGTM! Constants follow established patterns.The J680_ALT trackpad geometry constants are consistently defined following the same structure as the existing J680 and J213 variants.
40-81: ****The constant
USB_DEVICE_ID_APPLE_WELLSPRINGT2_J680_ALTis properly defined in the preceding patch4005-HID-apple-Add-necessary-IDs-and-support-for-replacem.patch(line 59:#define USB_DEVICE_ID_APPLE_WELLSPRINGT2_J680_ALT 0x0278). Since patches in a series are applied sequentially, this definition is available when patch 4006 is applied. No compilation issue exists.Likely an incorrect or invalid review comment.
patch/kernel/archive/uefi-x86-6.18/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch (3)
14-31: Code logic is sound; enforces exact dimension matching.The change from
>to!=comparisons and the corresponding log message update from "too small" to "not valid" correctly implements stricter BIOS framebuffer validation. This ensures the driver only reuses BIOS framebuffers that exactly match the required dimensions, which can prevent display corruption on Apple T2 systems.
1-34: Note: uefi-x86 family now has kernel patches.Based on learnings from previous reviews, the uefi-x86 family historically had no kernel patches. This PR introduces patches for Apple T2 support, which represents a significant change in approach for this family. The patch directory
archive/uefi-x86-6.18being populated is expected and intentional for this PR.Based on learnings
1-12: Subject and commit message don't accurately describe the patch logic.The subject line states "Discard large BIOS framebuffers" and the commit message says "the BIOS framebuffer is larger than what panel requires," but the actual code change has different semantics:
- Original logic (lines 22-23): Checked if
sizes->fb_width > fb->base.width, which triggers when the BIOS framebuffer is TOO SMALL (required size exceeds provided size).- New logic (lines 24-25): Checks if
sizes->fb_width != fb->base.width, which triggers on ANY mismatch (larger OR smaller).The documentation should state that the patch discards BIOS framebuffers that don't exactly match the required dimensions, not just "large" ones.
Note: Since these patches originate from the linux-t2 project, you may want to report this documentation inconsistency upstream to t2linux/linux-t2-patches if not already addressed.
⛔ Skipped due to learnings
Learnt from: rpardini Repo: armbian/build PR: 8780 File: config/sources/families/include/uefi_common.inc:44-44 Timestamp: 2025-10-18T11:52:36.964Z Learning: The uefi-riscv64 and uefi-x86 UEFI families in the Armbian build system do not have kernel patches and never have had patches. Missing patch directories like archive/uefi-riscv64-6.18 and archive/uefi-x86-6.18 are expected and normal for these families.Learnt from: tabrisnet Repo: armbian/build PR: 8661 File: lib/functions/compilation/armbian-kernel.sh:194-199 Timestamp: 2025-09-25T18:37:00.330Z Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.Learnt from: tabrisnet Repo: armbian/build PR: 8661 File: lib/functions/compilation/armbian-kernel.sh:194-199 Timestamp: 2025-09-25T18:37:00.330Z Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.patch/kernel/archive/uefi-x86-6.18/1003-Fix-freezing-on-turning-off-camera.patch (3)
1-12: LGTM: Proper patch format and good traceability.The patch header follows standard git patch conventions and includes a link to the detailed issue discussion, which provides excellent context for the fix.
17-54: LGTM: Solid fix for double-cancellation race condition.The cancellation guard logic is well-implemented:
- Saves the old state before modification to enable correct decision-making
- Prevents re-entry by checking for already-cancelled URBs and returning early
- Uses
old_statefor subsequent conditionals sincevurb->stateis now CANCELLED- Maintains proper locking throughout the critical section
- The early return correctly skips giveback/cleanup since those were already performed
The inline comment and debug message enhance maintainability.
55-67: LGTM: Clean enum extension.The new
BCE_VHCI_URB_CANCELLEDstate is appropriately placed at the end of the enum with proper formatting. The blank line provides logical separation, and the trailing comma follows modern C conventions.patch/kernel/archive/uefi-x86-6.12/3009-applesmc-battery-charge-limiter.patch (1)
62-92: LGTM! Well-integrated error handling.The BCLM group is properly integrated into the module initialization and cleanup sequences with correct error unwinding paths. The goto label chain (out_bclm → out_info → out) ensures proper cleanup order in case of failures.
patch/kernel/archive/uefi-x86-6.12/2008-i915-4-lane-quirk-for-mbp15-1.patch (4)
1-14: LGTM: Patch header is well-formed.The patch header follows standard git format with clear attribution. The commit message concisely explains the purpose, and the attribution to the original author (Kerem Karabay) is properly documented.
15-36: LGTM: Quirk check correctly implemented.The quirk check is properly placed before the platform-wide Broxton/Geminilake logic, allowing device-specific overrides. The new
displayvariable is correctly derived and used for the quirk check.
37-69: LGTM: Quirk handler and registration follow established patterns.The implementation correctly follows the existing quirk pattern:
- Handler function properly sets the quirk bit and logs for debugging
- Helpful comment explains the firmware issue and impact
- Registry entry uses appropriate PCI IDs for the target hardware (Apple MacBookPro15,1)
70-79: LGTM: Enum value properly added.The new quirk enum value is correctly appended to the end of the
intel_quirk_idenum, which is good practice for maintaining ABI stability. The naming follows the established convention.patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patch (4)
1-16: Patch header and attribution are correct.The patch properly attributes the original author and includes rpardini's explanation of the strategic placement to avoid conflicts with Armbian's EXTRAWIFI patching. This aligns with the PR description.
17-27: LGTM: Kconfig sourcing follows conventions.The addition of the apple-bce Kconfig source follows the established pattern in the staging Kconfig.
28-39: LGTM: Strategic Makefile placement documented.The apple-bce entry is correctly placed early in the Makefile (after media, before other drivers) to avoid conflicts with Armbian's EXTRAWIFI sed-based patching, as documented in the patch description.
64-74: LGTM: Makefile properly uses conditional compilation.The change from
obj-mtoobj-$(CONFIG_APPLE_BCE)correctly makes the driver build conditional on the Kconfig option, following kernel build system conventions.patch/kernel/archive/uefi-x86-6.12/1015-drm-tiny-add-driver-for-Apple-Touch-Bars-in-x86-Macs.patch (5)
1-53: LGTM: Patch metadata is well-formed.The patch header follows kernel conventions with proper attribution, co-developer credits, and upstream review acknowledgment. The commit message provides good context about the driver's purpose, testing status, and design rationale for packed structs.
54-103: LGTM: Build system integration is correct.The MAINTAINERS entry, Kconfig, and Makefile changes follow kernel conventions with appropriate dependencies.
119-260: LGTM: Driver structures and constants are well-defined.The protocol structures and device state definitions are clear. The use of
__packedattributes is explained in the commit message as a protocol requirement.
750-852: LGTM: Mode configuration setup is well-structured.The mode config setup correctly handles the coordinate system transformation between the device and framebuffer, which is well-documented in the comments at Lines 800-806.
914-947: LGTM: Driver lifecycle management is appropriate.The disconnect and shutdown handlers properly clean up DRM resources. The shutdown handler correctly clears the framebuffer since content persists across boots.
patch/kernel/archive/uefi-x86-6.12/1004-Fix-freezing-on-turning-off-camera.patch (1)
30-52: Cancellation path update looks solid.Saving the prior state and guarding against double cancellation gives us deterministic resume logic and avoids redundant work. Nice improvement.
patch/kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch (10)
56-70: LGTM!The Kconfig entry correctly declares dependencies and selects the backlight driver, ensuring all required components are available.
74-83: LGTM!Standard kernel Makefile entry with appropriate placement.
150-178: LGTM!Data structure is well-organized with appropriate fields for HID, input handling, backlight, and power management state. The keymap correctly translates function keys to their media/brightness equivalents.
180-241: LGTM!Mode setting implementation correctly handles power management, HID field updates, and error cases. The sysfs interface properly validates mode values against the documented range (0-3).
323-395: LGTM!Input event handling and device connection logic is well-implemented with proper error handling, reference counting, and state management for Fn key toggling.
397-418: LGTM!Input device configuration correctly resets the capabilities blindly set by usbhid and properly initializes the Touch Bar keymap.
420-461: LGTM!Device matching logic correctly identifies Apple internal keyboards and trackpads using vendor ID, keybit flags, and product string matching. The approach of using the product string is well-justified in the comments.
548-567: LGTM!Power management implementation correctly saves state on suspend, turns off the Touch Bar, and restores the previous mode on resume. Using
reset_resumeis appropriate for USB HID devices.
569-599: LGTM!Module registration, device table, and metadata are properly configured. The soft dependency ensures the backlight driver loads first, and all authors are appropriately credited.
599-621: Verify intended behavior when CONFIG_HID_APPLETB_KBD is disabled.The Touch Bar display device is moved from the
CONFIG_HID_APPLEsection to the newCONFIG_HID_APPLETB_KBDsection. This means:
- When
CONFIG_HID_APPLETB_KBD=y/m: Device uses the new specialized driver ✓- When
CONFIG_HID_APPLETB_KBD=n: Device has no driver (removed from generic HID_APPLE)This is likely intentional since the Touch Bar requires specialized handling that the generic Apple HID driver cannot provide. However, confirm this behavior aligns with the project's expectations for systems that don't enable the T2-specific config option.
patch/kernel/archive/uefi-x86-6.12/1011-HID-multitouch-add-device-ID-for-Apple-Touch-Bar.patch (5)
1-19: LGTM! Patch header is well-formed.The patch header follows git format-patch conventions correctly, includes proper attribution with Co-developed-by tags, and provides clear context about the change.
20-31: LGTM! Documentation update is appropriate.The Kconfig help text addition correctly documents Apple Touch Bar support and maintains alphabetical ordering.
36-40: LGTM! Constant definition follows conventions.The MT_CLS_APPLE_TOUCHBAR constant is properly defined with sequential value 0x0114, following the existing pattern.
57-65: LGTM! Probe guard provides appropriate validation.The runtime check ensures the driver only binds to Apple Touch Bar devices with the expected HID report structure (HID_DG_TOUCHPAD with HID_DG_TRANSDUCER_INDEX). Returning -ENODEV allows other drivers to attempt binding if validation fails.
This restrictive approach is appropriate for T2 Mac-specific hardware support.
44-53: Based on the verification:MT_QUIRK_APPLE_TOUCHBAR is properly defined and patch ordering is correct.
The MT_QUIRK_APPLE_TOUCHBAR constant is defined in patch 1007, and since patch 1007 is applied before patch 1011 numerically, there are no dependency ordering issues.
Regarding maxcontacts = 11: The Touch Bar supports up to 10 simultaneous inputs, but the kernel code validates this value. When MT_QUIRK_APPLE_TOUCHBAR is set with a non-zero maxcontacts value, the kernel driver uses the class-defined maxcontacts. The value of 11 appears intentional and within acceptable bounds—the kernel will handle it appropriately during device initialization.
patch/kernel/archive/uefi-x86-6.12/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch (1)
22-29: Equality check avoids mismatched BIOS FB reuse.Line 24 now bails out whenever the BIOS-provided framebuffer dimensions diverge from what fb_helper requested, which prevents reusing the oversized 4K buffers that were corrupting output. The updated log string also reflects the broader mismatch case.
patch/kernel/archive/uefi-x86-6.18/4003-HID-apple-ignore-the-trackpad-on-T2-Macs.patch (1)
32-68: Nice quirk hook to hand off the trackpadSkipping the USB mouse interface when APPLE_IGNORE_MOUSE is set keeps hid-apple on the keyboard path while leaving the trackpad free for hid-magicmouse—exactly what we need on T2 hardware.
patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch (1)
1058-1072: Restore correct smc lookup in brightness_set.Line 1062 now does
dev_get_drvdata(led_cdev->dev), but the LED class storesled_cdevitself as that device's drvdata. We end up treating astruct led_classdev *asstruct applesmc_device *, so the first brightness change will scribble through random memory. Please fetchsmcfrom the parent (dev_get_drvdata(led_cdev->dev->parent)) or explicitly set the LED device's drvdata tosmcright after registration. Without that fix the driver oopses on the first LED update. citeturn0comment0Apply this diff:
- struct applesmc_device *smc = dev_get_drvdata(led_cdev->dev); + struct applesmc_device *smc = dev_get_drvdata(led_cdev->dev->parent);Likely an incorrect or invalid review comment.
patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch (1)
1-1218: Overall patch structure and approach is sound.This patch successfully refactors the Apple SMC driver from static/global state to per-device driver data, which is the correct pattern for kernel drivers. The lifecycle management in probe/remove is proper, cleanup ordering is correct, and the resource management follows best practices.
However, two critical NULL pointer dereference bugs were identified in callback functions that would cause crashes when the hardware is accessed. These need to be fixed before the patch can be safely applied.
...kernel/archive/uefi-x86-6.12/1005-HID-hid-appletb-bl-add-driver-for-the-backlight-of-A.patch
Show resolved
Hide resolved
...kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch
Show resolved
Hide resolved
patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch
Show resolved
Hide resolved
patch/kernel/archive/uefi-x86-6.18/3002-applesmc-make-io-port-base-addr-dynamic.patch
Show resolved
Hide resolved
patch/kernel/archive/uefi-x86-6.18/3006-applesmc-fan-support-on-T2-Macs.patch
Show resolved
Hide resolved
patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch
Show resolved
Hide resolved
...kernel/archive/uefi-x86-6.18/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch
Show resolved
Hide resolved
- from linux-t2 project: https://github.com/t2linux/linux-t2-patches - https://t2linux.org/ - x86: add .config hook `custom_kernel_config__applet2()` - original patches from t2linux for 6.18 and 6.12 - rewrite patches against v6.18-rc4 and v6.12.57 - `7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch` needed special attention for correct patch attribution (missing From) - `1002-Put-apple-bce-in-drivers-staging` needs reordering to the top to avoid conflicts with EXTRAWIFI sed-based "patching"
rpardini
force-pushed
the
pr/uefi-x86-current-612-and-edge-618-add-Apple-T2-patches
branch
from
3182054 to
faf0b83
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (23)
patch/kernel/archive/uefi-x86-6.12/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch (2)
80-83: Fix the module parameter name typo in MODULE_PARM_DESC.The
MODULE_PARM_DESCusesforce_idginstead of the correct parameter nameforce_igd. This causesmodinfoto display the wrong parameter name, and users attempting to passforce_idgwill find the option is not recognized.Apply this diff:
static bool force_igd; module_param(force_igd, bool, 0); -MODULE_PARM_DESC(force_idg, "Switch gpu to igd on module load. Make sure that you have apple-set-os set up and the iGPU is in `lspci -s 00:02.0`. (default: false) (bool)"); +MODULE_PARM_DESC(force_igd, "Switch gpu to igd on module load. Make sure that you have apple-set-os set up and the iGPU is in `lspci -s 00:02.0`. (default: false) (bool)");
91-102: Fix reference leak and error message typo.Two issues in this block:
Reference leak:
pci_get_domain_bus_and_slot()increments the device refcount, but the success branch never releases it. Aftervga_set_default_device(pdev)(which takes its own reference), you must callpci_dev_put(pdev)to drop the temporary reference.Typo in error message: The error log uses
force_idginstead of the correct parameter nameforce_igd.Apply this diff:
if (force_igd) { struct pci_dev *pdev; pdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(2, 0)); if (pdev) { pr_info("Switching to IGD"); gmux_switchto(VGA_SWITCHEROO_IGD); vga_set_default_device(pdev); + pci_dev_put(pdev); } else { - pr_err("force_idg is true, but couldn't find iGPU at 00:02.0! Is apple-set-os working?"); + pr_err("force_igd is true, but couldn't find iGPU at 00:02.0! Is apple-set-os working?"); } }patch/kernel/archive/uefi-x86-6.12/1005-HID-hid-appletb-bl-add-driver-for-the-backlight-of-A.patch (2)
204-205: Critical: Array index bounds checking issue already flaggedThis code segment has a critical out-of-bounds vulnerability that was already identified in a previous review. The module parameter
appletb_bl_def_brightnesscan be negative (user-supplied via module load), but only the upper bound is checked. A negative index will cause an out-of-bounds read ofappletb_bl_brightness_mapin kernel space.
213-217: Backlight state initialization issue already flaggedThe backlight properties structure doesn't set
props.brightness, which defaults to 0. This creates a mismatch between the hardware state (initialized at line 204) and the backlight device state, potentially causing the Touch Bar to blank unexpectedly on first update.patch/kernel/archive/uefi-x86-6.12/1013-lib-vsprintf-Add-support-for-generic-FourCCs-by-exte.patch (1)
99-116: Missing case 'n' for network byte orderAs noted in the previous review, the switch statement lacks a
case 'n':branch to handle network-order FourCCs. This causes%p4cnto fall into the default path and print(%p4?)at runtime.patch/kernel/archive/uefi-x86-6.18/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch (2)
108-112: Fix force_igd naming in metadata and error message.
MODULE_PARM_DESCstill registers help text underforce_idg, and the error log mirrors that typo, somodinfo apple-gmuxhides the description and the log misleads anyone debugging. Please rename both strings toforce_igdand keep the wording consistent.
Apply this diff:-static bool force_igd; -module_param(force_igd, bool, 0); -MODULE_PARM_DESC(force_idg, "Switch gpu to igd on module load. Make sure that you have apple-set-os set up and the iGPU is in `lspci -s 00:02.0`. (default: false) (bool)"); +static bool force_igd; +module_param(force_igd, bool, 0); +MODULE_PARM_DESC(force_igd, "Switch GPU to IGD on module load. Make sure that you have apple-set-os set up and the iGPU is in `lspci -s 00:02.0`. (default: false) (bool)"); @@ - } else { - pr_err("force_idg is true, but couldn't find iGPU at 00:02.0! Is apple-set-os working?"); + } else { + pr_err("force_igd is true, but couldn't find iGPU at 00:02.0! Is apple-set-os working?");Also applies to: 100-101
95-101: Drop the pci_dev reference after switching.
pci_get_domain_bus_and_slot()bumps the refcount; withoutpci_dev_put(pdev);we leak that reference for the module’s lifetime. Add the put right aftervga_set_default_device(pdev);.
Apply this diff:if (pdev) { pr_info("Switching to IGD"); gmux_switchto(VGA_SWITCHEROO_IGD); vga_set_default_device(pdev); + pci_dev_put(pdev); } else {patch/kernel/archive/uefi-x86-6.12/1015-drm-tiny-add-driver-for-Apple-Touch-Bars-in-x86-Macs.patch (5)
469-478: Fix memory leak when response allocation fails.This issue was previously identified and remains unresolved. If the
requestallocation at Line 469 succeeds but theresponseallocation at Line 474 fails, the function returns-ENOMEMwithout freeingrequest, causing a memory leak.Apply this diff to fix the leak:
appletbdrm_state->request = kzalloc(request_size, GFP_KERNEL); - if (!appletbdrm_state->request) + if (!appletbdrm_state->request) { return -ENOMEM; + } appletbdrm_state->response = kzalloc(sizeof(*appletbdrm_state->response), GFP_KERNEL); - if (!appletbdrm_state->response) + if (!appletbdrm_state->response) { + kfree(appletbdrm_state->request); + appletbdrm_state->request = NULL; return -ENOMEM; + }
507-511: Avoid calling end_cpu_access when begin_cpu_access failed.This issue was previously identified and remains unresolved. If
drm_gem_fb_begin_cpu_accessfails at Line 507, jumping toend_fb_cpu_accesswill calldrm_gem_fb_end_cpu_accessat Line 578. Calling the end function without a successful begin may lead to undefined behavior or imbalanced reference counting.Consider returning immediately on failure instead:
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret) { drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret); - goto end_fb_cpu_access; + return ret; }
513-519: Fix timestamp truncation when assigning to msg_id.This issue was previously identified and remains unresolved. At Line 518, a 64-bit
timestampvalue is assigned tomsg_id, which is au8field (see Line 219). This causes truncation and only the least significant byte is used, potentially causing message ID collisions.Verify the protocol specification:
- If
msg_idshould indeed be a single byte derived from timestamp, use an explicit cast and document the intention.- If
msg_idneeds the full timestamp value, change its type instruct appletbdrm_fb_requestto__le64or adjust the protocol handling.request->header.size = cpu_to_le32(request_size - sizeof(request->header)); request->unk_10 = cpu_to_le16(1); - request->msg_id = timestamp; + request->msg_id = (u8)(timestamp & 0xFF); /* Explicitly truncate for protocol */Or if the field size is wrong:
struct appletbdrm_fb_request { struct appletbdrm_msg_request_header header; __le16 unk_10; - u8 msg_id; - u8 unk_13[29]; + __le64 msg_id; + u8 unk_13[21];
863-867: Fix NULL pointer dereference in error path.This issue was previously identified and remains unresolved. At Line 865,
drm_err(drm, ...)is called butdrmis NULL (initialized at Line 860). This will cause issues when trying to log the error message.Use
dev_err(dev, ...)instead since the device pointer is available:ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL); if (ret) { - drm_err(drm, "appletbdrm: Failed to find bulk endpoints\n"); + dev_err(dev, "appletbdrm: Failed to find bulk endpoints\n"); return ret; }
899-909: Add proper cleanup after drm_dev_register succeeds.This issue was previously identified and remains unresolved. If
drm_dev_registersucceeds at Line 899 butappletbdrm_clear_displayfails at Line 905, the function returns without callingdrm_dev_unregister, leaving the DRM device registered and potentially causing resource leaks or inconsistent state.Apply this diff to add proper cleanup:
ret = drm_dev_register(drm, 0); if (ret) { drm_err(drm, "Failed to register DRM device\n"); return ret; } ret = appletbdrm_clear_display(adev); if (ret) { drm_err(drm, "Failed to clear display\n"); + drm_dev_unregister(drm); return ret; }patch/kernel/archive/uefi-x86-6.12/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch (1)
315-333: Initialize the T2 geometry and guard unknown PIDs.
min_x,max_x, etc. stay uninitialized whenmagicmouse_t2_configs[]has no match; we then pass garbage intomagicmouse_setup_input_int_tpd, leading to undefined behavior once a new PID appears. Please initialize the fields, break once a match is found, and bail out cleanly when the PID is unknown.Apply this diff:
- int min_x, min_y, max_x, max_y, res_x, res_y; - - for (size_t i = 0; i < ARRAY_SIZE(magicmouse_t2_configs); i++) { + int min_x = 0, min_y = 0, max_x = 0, max_y = 0, res_x = 0, res_y = 0; + bool found = false; + size_t i; + + for (i = 0; i < ARRAY_SIZE(magicmouse_t2_configs); i++) { if (magicmouse_t2_configs[i].id == hdev->product) { min_x = magicmouse_t2_configs[i].min_x; min_y = magicmouse_t2_configs[i].min_y; max_x = magicmouse_t2_configs[i].max_x; max_y = magicmouse_t2_configs[i].max_y; res_x = magicmouse_t2_configs[i].res_x; res_y = magicmouse_t2_configs[i].res_y; + found = true; + break; } } + if (!found) { + hid_err(hdev, "no T2 geometry for product 0x%04x\n", hdev->product); + return -ENODEV; + } + int ret = magicmouse_setup_input_int_tpd(input, hdev, min_x, min_y, max_x, max_y, res_x, res_y, false);patch/kernel/archive/uefi-x86-6.18/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch (1)
269-284: Guard against unmatched T2 product IDsWhen a new or mismatched product ID slips through this loop,
min_x/max_x/res_*remain uninitialized and we pass stack garbage intomagicmouse_setup_input_int_tpd(). Bail out when no entry matches (and stop iterating once we do find one) to keep the axis setup deterministic.-static int magicmouse_setup_input_t2(struct input_dev *input, - struct hid_device *hdev) -{ - int min_x, min_y, max_x, max_y, res_x, res_y; - - for (size_t i = 0; i < ARRAY_SIZE(magicmouse_t2_configs); i++) { - if (magicmouse_t2_configs[i].id == hdev->product) { - min_x = magicmouse_t2_configs[i].min_x; - min_y = magicmouse_t2_configs[i].min_y; - max_x = magicmouse_t2_configs[i].max_x; - max_y = magicmouse_t2_configs[i].max_y; - res_x = magicmouse_t2_configs[i].res_x; - res_y = magicmouse_t2_configs[i].res_y; - } - } - - int ret = magicmouse_setup_input_int_tpd(input, hdev, min_x, min_y, - max_x, max_y, res_x, res_y, false); +static int magicmouse_setup_input_t2(struct input_dev *input, + struct hid_device *hdev) +{ + int min_x = 0, min_y = 0, max_x = 0, max_y = 0, res_x = 0, res_y = 0; + bool found = false; + size_t i; + int ret; + + for (i = 0; i < ARRAY_SIZE(magicmouse_t2_configs); i++) { + if (magicmouse_t2_configs[i].id == hdev->product) { + min_x = magicmouse_t2_configs[i].min_x; + min_y = magicmouse_t2_configs[i].min_y; + max_x = magicmouse_t2_configs[i].max_x; + max_y = magicmouse_t2_configs[i].max_y; + res_x = magicmouse_t2_configs[i].res_x; + res_y = magicmouse_t2_configs[i].res_y; + found = true; + break; + } + } + + if (!found) + return -ENODEV; + + ret = magicmouse_setup_input_int_tpd(input, hdev, min_x, min_y, + max_x, max_y, res_x, res_y, false);patch/kernel/archive/uefi-x86-6.12/3009-applesmc-battery-charge-limiter.patch (1)
18-33: Fix sysfs store prototype mismatch
DEVICE_ATTRexpectsssize_t foo(struct device *, struct device_attribute *, const char *, size_t). Dropping theconsthere makes the function pointer assignment incompatible, which will fail the build.-static ssize_t applesmc_BCLM_store(struct device *dev, - struct device_attribute *attr, char *sysfsbuf, size_t count) +static ssize_t applesmc_BCLM_store(struct device *dev, + struct device_attribute *attr, const char *sysfsbuf, size_t count)patch/kernel/archive/uefi-x86-6.18/3006-applesmc-fan-support-on-T2-Macs.patch (1)
68-80: Correct applesmc_u32_to_float normalisationThis helper leaves the implicit leading 1 inside the mantissa and shifts the whole integer, so every RPM written through the float path comes out doubled (e.g. 3000 → 6000). Normalize the mantissa before combining it with the biased exponent.
-static inline u32 applesmc_u32_to_float(u32 d) -{ - u32 dc = d, bc = 0, exp; - - if (!d) - return 0; - - while (dc >>= 1) - ++bc; - exp = 0x7f + bc; - - return (u32) ((exp << 23) | - ((d << (23 - (exp - 0x7f))) & ((1u << 23) - 1))); -} +static inline u32 applesmc_u32_to_float(u32 d) +{ + u32 tmp = d; + u32 msb = 0; + u32 mant; + + if (!d) + return 0; + + while (tmp >>= 1) + msb++; + + mant = d - (1u << msb); + if (msb >= 23) + mant >>= msb - 23; + else + mant <<= 23 - msb; + + return ((msb + 0x7f) << 23) | mant; +}patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch (1)
18-19: Signature mismatch (duplicate from past review)The store function signature uses non-const
char *sysfsbufbut sysfs expectsconst char *. This issue was previously flagged.patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch (1)
519-537: NULL dereference in polling callback (duplicate from past review)Line 521 retrieves
smcviadev_get_drvdata(&idev->dev)butinput_set_drvdata()is never called during input device initialization (lines 903-969), so this will dereference NULL. This issue was previously flagged.patch/kernel/archive/uefi-x86-6.12/3006-applesmc-fan-support-on-T2-Macs.patch (2)
45-60: Undefined behavior from negative shifts (duplicate from past review)When
exp > 23, the expression(23 - exp)becomes negative, causing undefined behavior in the right shift. Whenexp > 31,(1u << exp)is also undefined. This issue was previously flagged.
67-79: Undefined behavior from negative shifts (duplicate from past review)When
bc > 23, the shift amount(23 - (exp - 0x7f))becomes negative, causing undefined behavior. This is part of the previously flagged float conversion issue.patch/kernel/archive/uefi-x86-6.18/3002-applesmc-make-io-port-base-addr-dynamic.patch (1)
36-228: Critical: port_base uninitialized (duplicate from past review)The
port_basefield is added at line 40 but never initialized. Sincesmcis kzalloc'd,port_basestarts at 0, causing the first I/O operation (line 59:inb(smc->port_base + APPLESMC_CMD_PORT)) to access port 4 instead of 0x304. This will break hardware access on existing systems. This critical issue was previously flagged.patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch (2)
519-537: Critical: NULL pointer dereference in input device poll callback.This issue was already identified in a previous review: the
applesmc_idev_pollcallback at line 521 attempts to retrieve the device context usingdev_get_drvdata(&idev->dev), butinput_set_drvdata()is never called to store thesmcpointer. This will result in a NULL pointer dereference when accessingsmc->rest_xandsmc->rest_yat lines 534-535.The fix requires:
- Adding
input_set_drvdata(smc->idev, smc)after line 948 inapplesmc_create_accelerometer- Changing line 521 to use
input_get_drvdata(idev)instead
741-754: Critical: Incorrect device context retrieval in LED brightness callback.This issue was already identified in a previous review: at line 744, the code uses
dev_get_drvdata(led_cdev->dev)to obtain theapplesmc_devicepointer, but this is incorrect because the LED subsystem creates its own device whenled_classdev_register()is called. Theled_cdev->devpoints to the LED-specific device, not the parent platform device.The correct approach is to use
container_of()to retrieve the containingapplesmc_devicestructure, similar to howapplesmc_backlight_setdoes it correctly at line 735:struct applesmc_device *smc = container_of(led_cdev, struct applesmc_device, backlight_dev);
🧹 Nitpick comments (1)
patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch (1)
421-457: LGTM: Probe implementation properly allocates and initializes per-device context.The
applesmc_probefunction correctly allocates the per-device structure, initializes resources, and handles error paths with proper unwinding.Line 451's
platform_set_drvdata(dev, NULL)is unnecessary since the memory is freed immediately after, but it's harmless and may serve as defensive programming.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (48)
config/sources/families/uefi-x86.conf(1 hunks)patch/kernel/archive/uefi-x86-6.12/1002-Put-apple-bce-in-drivers-staging.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1004-Fix-freezing-on-turning-off-camera.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1005-HID-hid-appletb-bl-add-driver-for-the-backlight-of-A.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1007-HID-multitouch-Get-the-contact-ID-from-HID_DG_TRANSD.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1008-HID-multitouch-support-getting-the-tip-state-from-HI.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1009-HID-multitouch-take-cls-maxcontacts-into-account-for.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1010-HID-multitouch-specify-that-Apple-Touch-Bar-is-direc.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1011-HID-multitouch-add-device-ID-for-Apple-Touch-Bar.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1013-lib-vsprintf-Add-support-for-generic-FourCCs-by-exte.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/1015-drm-tiny-add-driver-for-Apple-Touch-Bars-in-x86-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/2008-i915-4-lane-quirk-for-mbp15-1.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3002-applesmc-make-io-port-base-addr-dynamic.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3003-applesmc-switch-to-acpi_device-from-platform.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3004-applesmc-key-interface-wrappers.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3005-applesmc-basic-mmio-interface-implementation.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3006-applesmc-fan-support-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3008-applesmc-make-applesmc_remove-void.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/3009-applesmc-battery-charge-limiter.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4002-HID-quirks-remove-T2-devices-from-hid_mouse_ignore_l.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4003-HID-apple-ignore-the-trackpad-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4005-HID-apple-Add-necessary-IDs-and-support-for-replacem.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/4006-HID-magicmouse-Add-MacBookPro15-1-replacement-trackp.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch(1 hunks)patch/kernel/archive/uefi-x86-6.12/8002-Necessary-modifications-to-build-APFS-with-the-kerne.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/1003-Fix-freezing-on-turning-off-camera.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/2008-i915-4-lane-quirk-for-mbp15-1.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/2009-apple-gmux-allow-switching-to-igpu-at-probe.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3002-applesmc-make-io-port-base-addr-dynamic.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3003-applesmc-switch-to-acpi_device-from-platform.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3004-applesmc-key-interface-wrappers.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3005-applesmc-basic-mmio-interface-implementation.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3006-applesmc-fan-support-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3008-applesmc-make-applesmc_remove-void.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4003-HID-apple-ignore-the-trackpad-on-T2-Macs.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4004-HID-magicmouse-Add-support-for-trackpads-found-on-T2.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/4005-HID-magicmouse-fix-regression-breaking-support-for-M.patch(1 hunks)patch/kernel/archive/uefi-x86-6.18/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- patch/kernel/archive/uefi-x86-6.18/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch
- patch/kernel/archive/uefi-x86-6.12/1010-HID-multitouch-specify-that-Apple-Touch-Bar-is-direc.patch
- patch/kernel/archive/uefi-x86-6.12/3008-applesmc-make-applesmc_remove-void.patch
- patch/kernel/archive/uefi-x86-6.12/1008-HID-multitouch-support-getting-the-tip-state-from-HI.patch
- patch/kernel/archive/uefi-x86-6.18/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch
- patch/kernel/archive/uefi-x86-6.18/3008-applesmc-make-applesmc_remove-void.patch
- patch/kernel/archive/uefi-x86-6.12/1002-Put-apple-bce-in-drivers-staging.patch
- patch/kernel/archive/uefi-x86-6.18/2008-i915-4-lane-quirk-for-mbp15-1.patch
- patch/kernel/archive/uefi-x86-6.18/1003-Fix-freezing-on-turning-off-camera.patch
- patch/kernel/archive/uefi-x86-6.12/4002-HID-quirks-remove-T2-devices-from-hid_mouse_ignore_l.patch
- patch/kernel/archive/uefi-x86-6.12/3007-applesmc-Add-iMacPro-to-applesmc_whitelist.patch
- patch/kernel/archive/uefi-x86-6.12/1006-HID-hid-appletb-kbd-add-driver-for-the-keyboard-mode.patch
- patch/kernel/archive/uefi-x86-6.18/1002-Put-apple-bce-in-drivers-staging.patch
- patch/kernel/archive/uefi-x86-6.12/1004-Fix-freezing-on-turning-off-camera.patch
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: rpardini
Repo: armbian/build PR: 8780
File: config/sources/families/include/uefi_common.inc:44-44
Timestamp: 2025-10-18T11:52:36.964Z
Learning: The uefi-riscv64 and uefi-x86 UEFI families in the Armbian build system do not have kernel patches and never have had patches. Missing patch directories like archive/uefi-riscv64-6.18 and archive/uefi-x86-6.18 are expected and normal for these families.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
📚 Learning: 2025-10-18T11:52:36.964Z
Learnt from: rpardini
Repo: armbian/build PR: 8780
File: config/sources/families/include/uefi_common.inc:44-44
Timestamp: 2025-10-18T11:52:36.964Z
Learning: The uefi-riscv64 and uefi-x86 UEFI families in the Armbian build system do not have kernel patches and never have had patches. Missing patch directories like archive/uefi-riscv64-6.18 and archive/uefi-x86-6.18 are expected and normal for these families.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-03-31T12:57:13.880Z
Learnt from: libiunc
Repo: armbian/build PR: 8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-25T18:37:00.330Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-22T22:08:54.273Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-09-22T22:08:54.273Z
Learning: When extracting comments from Linux Kconfig files, def_bool and def_tristate entries use their parameter as a dependency/default condition, not as a feature description. Comments should be generated differently for these entry types to avoid misleading inline documentation.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-10-26T12:56:29.185Z
Learnt from: rpardini
Repo: armbian/build PR: 8820
File: config/sources/families/include/meson64_common.inc:51-53
Timestamp: 2025-10-26T12:56:29.185Z
Learning: In the Armbian build framework, scripts/config correctly handles kernel configuration option names both with and without the CONFIG_ prefix, so opts_m+=("CONFIG_RTL8822CS") and opts_m+=("RTL8822CS") are both valid and will work correctly.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-22T21:52:01.225Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-201
Timestamp: 2025-09-22T21:52:01.225Z
Learning: In lib/functions/compilation/armbian-kernel.sh, the kernel_config_modifying_hashes array is not universally required for all kernel configuration functions - some functions like armbian_kernel_config__netkit() operate without it, and adding entries with '=m' would be incorrect when the actual result might be '=y' for options already built-in.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-08-30T06:48:09.091Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T06:48:09.091Z
Learning: In lib/functions/compilation/armbian-kernel.sh, the user prefers flexible grep patterns over anchored ones for BTRFS configuration checks, but agrees to use quiet grep (-q) to avoid polluting build logs.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-14T11:37:35.089Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:65-72
Timestamp: 2025-09-14T11:37:35.089Z
Learning: In the Armbian build system, patch directories referenced in BOOTPATCHDIR and KERNELPATCHDIR configurations can be non-existent without causing build failures. Empty patch directories are also ignored by git, so missing patch directories should not be flagged as errors during code review.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-10-14T05:08:11.785Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-08-02T05:46:10.664Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-02T05:46:10.664Z
Learning: In the Armbian build system, the modern recommended approach for kernel configuration is to use the kernel-config command via "./compile.sh BOARD=boardname BRANCH=branchname kernel-config" instead of the deprecated KERNEL_CONFIGURE=yes flag. This provides a two-step workflow: configure using menuconfig, then build, with better transparency and control over configuration changes.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-06-25T03:42:09.086Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-06-25T03:40:52.109Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-12T19:28:38.491Z
Learnt from: Grippy98
Repo: armbian/build PR: 8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.491Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-14T06:29:18.958Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:64-70
Timestamp: 2025-09-14T06:29:18.958Z
Learning: In the Armbian build system, vendor branch configurations in family files are designed to be shared across multiple SoCs within the same family that use the same vendor kernel tree. For example, rk35xx and rockchip-rk3588 families both use identical vendor branch settings (same KERNELSOURCE, KERNELBRANCH, and KERNELPATCHDIR), demonstrating that vendor branches are intentionally generic rather than SoC-specific.
Applied to files:
config/sources/families/uefi-x86.conf
📚 Learning: 2025-09-22T21:59:36.334Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:200-200
Timestamp: 2025-09-22T21:59:36.334Z
Learning: Functions named with the pattern `armbian_kernel_config__*` in lib/functions/compilation/armbian-kernel.sh are automatically discovered and invoked by Armbian's extension system via metaprogramming using `compgen -A function`, without requiring explicit registration or calls.
Applied to files:
config/sources/families/uefi-x86.conf
🔇 Additional comments (31)
patch/kernel/archive/uefi-x86-6.12/7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patch (3)
1-10: LGTM: Patch attribution and metadata are correct.The patch header is properly formatted with correct attribution to the original author from the t2linux project. The commit message clearly describes the issue (oversized BIOS framebuffers on 4K panels) and the fix.
18-31: LGTM: Logic change correctly addresses oversized BIOS framebuffers.The change from
>to!=expands the validation to catch both undersized AND oversized BIOS framebuffers. The original code only detected framebuffers that were too small (whensizes->fb_width > fb->base.width), but the commit message indicates the actual problem on 4K Apple T2 panels is BIOS framebuffers that are too LARGE. This fix correctly handles both cases by checking for any size mismatch.The updated log message "not valid" is more accurate than "too small" given the expanded validation logic.
14-17: LGTM: Patch format is correct.The placeholder git object IDs (111111111111..222222222222) are standard for out-of-tree patches that will be applied to different kernel versions. The patch format is correct and compatible with the Armbian build system's kernel patching process.
Also applies to: 32-34
patch/kernel/archive/uefi-x86-6.12/2008-i915-4-lane-quirk-for-mbp15-1.patch (5)
1-14: LGTM! Proper patch format and attribution.The patch header follows standard git format-patch conventions with clear attribution to both the committer and original author. The subject and description appropriately explain the quirk's purpose for MacBookPro15,1 when the dGPU is the boot GPU.
15-36: LGTM! Quirk integration follows established patterns.The quirk check is correctly placed after the saved port bits check and before platform-specific logic. The addition of the
displayvariable (line 23) is necessary for theintel_has_quirk()call and doesn't interfere with the existingdev_privusage.
45-55: LGTM! Well-documented quirk handler.The quirk handler follows the established pattern in the codebase, with a clear multi-line comment explaining the firmware limitation on dual GPU Macs. The implementation correctly uses
intel_set_quirk()and provides appropriate logging.
65-66: LGTM! Correct quirk registration.The quirk registration for Apple MacBookPro15,1 follows the exact format of other entries in the
intel_quirks[]array, with appropriate device IDs (0x3e9b, 0x106b, 0x0176).
78-78: LGTM! Proper enum addition.The new
QUIRK_DDI_A_FORCE_4_LANESenum value is correctly added at the end of theintel_quirk_idenumeration, following naming conventions and maintaining compatibility.patch/kernel/archive/uefi-x86-6.12/1005-HID-hid-appletb-bl-add-driver-for-the-backlight-of-A.patch (2)
163-234: Well-structured probe with proper error handlingThe probe function correctly validates HID fields, uses
dev_err_probefor consistent error reporting, and implements proper cleanup paths that unwind resources in the correct order (close before stop). The mix of devm and manual resource management is appropriate here.
265-287: Correct device ownership migrationThe quirks table correctly transfers
USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHTfrom the genericCONFIG_HID_APPLEsection to the new specializedCONFIG_HID_APPLETB_BLdriver, ensuring the device binds to the appropriate driver.patch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch (1)
1-10: LGTM: Patch header and approach are correct.The patch header is properly formatted, and replacing
#pragma oncewith traditional include guards is the correct approach for Linux kernel code to ensure compatibility with sparse and kernel coding standards.patch/kernel/archive/uefi-x86-6.12/1009-HID-multitouch-take-cls-maxcontacts-into-account-for.patch (1)
26-31: Max contacts override covers the Apple quirk cleanly.Ensuring
td->maxcontactspicks up the class-specific value before the default prevents the Touch Bar from being capped at 10 contacts. Nice integration with the new quirk.patch/kernel/archive/uefi-x86-6.12/1007-HID-multitouch-Get-the-contact-ID-from-HID_DG_TRANSD.patch (1)
44-64: Transducer-index contact IDs handled well.Gating the TRANSDUCER_INDEX path behind
MT_QUIRK_APPLE_TOUCHBARkeeps legacy devices untouched while letting the Touch Bar expose distinct contact IDs. Good defensive fall-through usage.patch/kernel/archive/uefi-x86-6.12/1011-HID-multitouch-add-device-ID-for-Apple-Touch-Bar.patch (1)
61-76: Probe guard plus class wiring looks solid.The new class quirks align with the preceding patches, and the probe check for a TRANSDUCER_INDEX field should keep false positives out of the driver bind list. Device table addition is scoped correctly to the Apple vendor ID.
patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch (2)
35-45: LGTM!The show function correctly reads the BCLM value and formats it for sysfs output.
54-90: LGTM!The BCLM node group is properly integrated into the module lifecycle with correct error handling and cleanup paths.
patch/kernel/archive/uefi-x86-6.12/3004-applesmc-key-interface-wrappers.patch (3)
30-187: LGTM!The systematic rename to
port_*prefixes is consistent and all internal call sites have been updated correctly.
190-231: LGTM!The new wrapper functions correctly encapsulate port-level operations. The big-endian encoding of the index in
get_smc_key_by_index(line 222-224) is intentional for the SMC protocol.
263-298: LGTM!The refactored
applesmc_get_entry_by_indexnow uses cleaner higher-level wrappers and the logic flow is correct.patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch (1)
420-469: LGTM!The per-device context allocation, initialization, and cleanup are properly structured with correct error handling paths.
patch/kernel/archive/uefi-x86-6.12/3006-applesmc-fan-support-on-T2-Macs.patch (1)
84-224: LGTM!The fan speed and manual control handlers properly detect and handle both FLOAT_TYPE (T2 Macs) and legacy formats, with appropriate fallbacks and error handling.
patch/kernel/archive/uefi-x86-6.18/3004-applesmc-key-interface-wrappers.patch (1)
30-298: LGTM!The refactoring to port-based wrappers is consistent and correct. All internal call sites have been properly updated, and the new helper functions correctly encapsulate SMC protocol operations.
patch/kernel/archive/uefi-x86-6.12/3003-applesmc-switch-to-acpi_device-from-platform.patch (3)
121-145: LGTM!The ACPI resource walking correctly identifies IO port regions and validates the address length before accepting the resource.
67-119: LGTM!The ACPI device lifecycle (add/remove) is correctly implemented with proper resource initialization order and complete error handling.
174-259: LGTM!The ACPI driver registration is correctly structured, and the module init/exit paths are appropriately simplified since ACPI handles device matching and resource management.
patch/kernel/archive/uefi-x86-6.18/3003-applesmc-switch-to-acpi_device-from-platform.patch (1)
121-259: LGTM!The ACPI migration is correctly implemented with proper resource walking, lifecycle management, and driver registration.
patch/kernel/archive/uefi-x86-6.18/3002-applesmc-make-io-port-base-addr-dynamic.patch (1)
287-306: LGTM with caveatThe region requests still use the fixed
APPLESMC_PORT_BASE, which is consistent with this transitional patch. The subsequent ACPI patch (3003) will update these to use the dynamicsmc->port_baseafter ACPI resource discovery.patch/kernel/archive/uefi-x86-6.12/3001-applesmc-convert-static-structures-to-drvdata.patch (4)
16-79: LGTM: Per-device context structure is well-designed.The conversion from static/global variables to a per-device
applesmc_devicestructure is sound and properly encapsulates all device-specific state including registers, sensors, backlight, and sysfs state.
472-494: LGTM: PM callbacks correctly retrieve per-device context.The PM resume and restore callbacks properly use
dev_get_drvdata(dev)to retrieve theapplesmc_devicestructure. This is correct for platform device PM callbacks, wheredevis the platform device.
1005-1040: LGTM: Backlight module creation properly initializes work and LED device.The
applesmc_create_key_backlightfunction correctly:
- Creates a dedicated workqueue
- Initializes the work structure with the callback
- Sets up the embedded LED class device fields
- Handles registration failure by destroying the workqueue
The error handling at lines 1024-1025 properly cleans up the workqueue if LED registration fails.
1048-1157: LGTM: Module lifecycle management is properly structured.The
applesmc_create_modulesandapplesmc_destroy_modulesfunctions properly manage the creation and destruction of all driver modules with correct error unwinding in reverse order.
| # Auto-grab patches from t2linux github if not already present | ||
| if [[ ! -d "${SRC}/patch/kernel/${KERNELPATCHDIR}" ]]; then | ||
| # Grab from github .tar.gz and extract there; patches are in the root of the archive | ||
| GH_URL="https://github.com/t2linux/linux-t2-patches/archive/refs/heads/${KERNEL_MAJOR_MINOR}.tar.gz" | ||
| display_alert "Fetching Apple T2 patches from" "GH_URL=${GH_URL}" "info" | ||
| mkdir -pv "${SRC}/patch/kernel/${KERNELPATCHDIR}" | ||
| curl -L "${GH_URL}" | tar -xz --strip-components=1 -C "${SRC}/patch/kernel/${KERNELPATCHDIR}" | ||
| tree "${SRC}/patch/kernel/${KERNELPATCHDIR}" | ||
| # remove any non-patch files and directories | ||
| # remove .github and .gitignore | ||
| rm -rf "${SRC}/patch/kernel/${KERNELPATCHDIR}/.github" "${SRC}/patch/kernel/${KERNELPATCHDIR}/.gitignore" | ||
| find "${SRC}/patch/kernel/${KERNELPATCHDIR}" -type f ! -name "*.patch" -delete | ||
| display_alert "Apple T2 patches downloaded to" "DIR=${SRC}/patch/kernel/${KERNELPATCHDIR}; go run rewrite-kernel-patches" "info" | ||
| exit 0 | ||
| else |
There was a problem hiding this comment.
Remove the unconditional exit 0.
This file is sourced during the build; hitting exit 0 aborts the whole build process the first time the patch directory is missing, so no images ever get produced. Please drop the exit (or turn it into logic that returns to the caller without killing the build) so the workflow can continue after fetching the patches.
Apply this diff:
- display_alert "Apple T2 patches downloaded to" "DIR=${SRC}/patch/kernel/${KERNELPATCHDIR}; go run rewrite-kernel-patches" "info"
- exit 0
+ display_alert "Apple T2 patches downloaded to" "DIR=${SRC}/patch/kernel/${KERNELPATCHDIR}; go run rewrite-kernel-patches" "info"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Auto-grab patches from t2linux github if not already present | |
| if [[ ! -d "${SRC}/patch/kernel/${KERNELPATCHDIR}" ]]; then | |
| # Grab from github .tar.gz and extract there; patches are in the root of the archive | |
| GH_URL="https://github.com/t2linux/linux-t2-patches/archive/refs/heads/${KERNEL_MAJOR_MINOR}.tar.gz" | |
| display_alert "Fetching Apple T2 patches from" "GH_URL=${GH_URL}" "info" | |
| mkdir -pv "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| curl -L "${GH_URL}" | tar -xz --strip-components=1 -C "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| tree "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| # remove any non-patch files and directories | |
| # remove .github and .gitignore | |
| rm -rf "${SRC}/patch/kernel/${KERNELPATCHDIR}/.github" "${SRC}/patch/kernel/${KERNELPATCHDIR}/.gitignore" | |
| find "${SRC}/patch/kernel/${KERNELPATCHDIR}" -type f ! -name "*.patch" -delete | |
| display_alert "Apple T2 patches downloaded to" "DIR=${SRC}/patch/kernel/${KERNELPATCHDIR}; go run rewrite-kernel-patches" "info" | |
| exit 0 | |
| else | |
| # Auto-grab patches from t2linux github if not already present | |
| if [[ ! -d "${SRC}/patch/kernel/${KERNELPATCHDIR}" ]]; then | |
| # Grab from github .tar.gz and extract there; patches are in the root of the archive | |
| GH_URL="https://github.com/t2linux/linux-t2-patches/archive/refs/heads/${KERNEL_MAJOR_MINOR}.tar.gz" | |
| display_alert "Fetching Apple T2 patches from" "GH_URL=${GH_URL}" "info" | |
| mkdir -pv "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| curl -L "${GH_URL}" | tar -xz --strip-components=1 -C "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| tree "${SRC}/patch/kernel/${KERNELPATCHDIR}" | |
| # remove any non-patch files and directories | |
| # remove .github and .gitignore | |
| rm -rf "${SRC}/patch/kernel/${KERNELPATCHDIR}/.github" "${SRC}/patch/kernel/${KERNELPATCHDIR}/.gitignore" | |
| find "${SRC}/patch/kernel/${KERNELPATCHDIR}" -type f ! -name "*.patch" -delete | |
| display_alert "Apple T2 patches downloaded to" "DIR=${SRC}/patch/kernel/${KERNELPATCHDIR}; go run rewrite-kernel-patches" "info" | |
| else |
🤖 Prompt for AI Agents
In config/sources/families/uefi-x86.conf around lines 47 to 61, the
unconditional "exit 0" after downloading patches aborts the entire build when
this file is sourced; remove that unconditional exit and instead return to the
caller so the build can continue. Replace "exit 0" with "return 0" (or remove it
entirely) and if this file may be executed directly add a safe guard like "if [[
"${BASH_SOURCE[0]}" != "${0}" ]]; then return 0; else exit 0; fi" so sourcing
returns without killing the process while direct execution still exits.
| -extern struct apple_bce_device *global_bce; | ||
| \ No newline at end of file | ||
| +extern struct apple_bce_device *global_bce; | ||
| + |
There was a problem hiding this comment.
Use traditional C comment style instead of C99-style comment.
The Linux kernel coding style discourages C99-style comments (//) in favor of traditional C comments (/* */).
Apply this diff to fix the comment style:
-#endif //APPLE_BCE_H
+#endif /* APPLE_BCE_H */Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In patch/kernel/archive/uefi-x86-6.12/1003-Fix-sparse-errors.patch around line
29, there is a C99-style single-line comment (//) which violates the kernel
coding style; replace the // comment with an equivalent traditional C comment
using /* ... */ at the same location, preserving the exact comment text and
punctuation, and ensure any trailing whitespace or formatting is adjusted to
match surrounding lines.
| %p4ch Host byte order | ||
| %p4cn Network byte order | ||
| %p4cl Little-endian | ||
| %p4cb Big-endian |
There was a problem hiding this comment.
Commit message promises %p4cn but implementation is missing
The commit message advertises support for %p4cn (network byte order), but this format specifier is never implemented in the code. The switch statement at lines 99-116 has no case 'n': branch, causing %p4cn to fall through to the default error path and print (%p4?) at runtime.
🤖 Prompt for AI Agents
In
patch/kernel/archive/uefi-x86-6.12/1013-lib-vsprintf-Add-support-for-generic-FourCCs-by-exte.patch
around lines 11-14 and the switch at ~99-116, the commit promised a %p4cn
(network byte order) specifier but the switch lacks a case 'n', so %p4cn falls
to the default error path; add a case 'n' to the switch and implement it to set
the byte-order handling to network byte order (treat as big-endian, same
behavior as the existing 'b' branch), ensuring it returns/continues like the
other valid branches so %p4cn prints correctly instead of producing the (%p4?)
error; run unit tests for format specifiers after the change.
| +:: | ||
| + %p4c[h[R]lb] gP00 (0x67503030) |
There was a problem hiding this comment.
Documentation omits promised %p4cn format
The format syntax shown here (%p4c[h[R]lb]) excludes the n specifier that the commit message promises on line 12. Similarly, the description on lines 56-58 lists only h, hR, l, and b, with no mention of network byte order.
| defined $qualifier && $qualifier !~ /^w/) || | ||
| ($extension eq "4" && | ||
| - defined $qualifier && $qualifier !~ /^cc/)) { | ||
| + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { |
There was a problem hiding this comment.
Checkpatch accepts %p4cn but code rejects it
The regex pattern /^c[hnlbc]/ includes n, allowing %p4cn to pass validation, but the actual code (lines 99-116) has no corresponding handler. This mismatch will let invalid format strings through checkpatch only to fail at runtime.
🤖 Prompt for AI Agents
In
patch/kernel/archive/uefi-x86-6.12/1013-lib-vsprintf-Add-support-for-generic-FourCCs-by-exte.patch
around line 161, the regex `/^c[hnlbc]/` wrongly allows the 'n' qualifier even
though there is no handler for `%p4cn` in the implementation (lines 99–116);
update the regex to remove 'n' (e.g. `/^c[hlbc]/`) so checkpatch rejects
unsupported `%p4cn`, or alternatively add the corresponding handler code for the
'n' qualifier — choose one: either delete 'n' from the character class to match
existing handlers or implement the missing handler and leave the regex as-is.
| + ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION); | ||
| + if (ret) | ||
| + return ret; | ||
| + |
There was a problem hiding this comment.
Fix memory leak when send message fails.
If appletbdrm_send_msg fails at Line 380, the function returns immediately at Line 382 without freeing the info struct allocated at Line 376, causing a memory leak.
Apply this diff to fix the leak:
ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
if (ret)
- return ret;
+ goto free_info;| + tristate "Apple file system support" | ||
| + select LIBCRC32C | ||
| + select ZLIB_INFLATE | ||
| + select NLS | ||
| + select BUFFER_HEAD | ||
| + select FS_IOMAP | ||
| + select LEGACY_DIRECT_IO | ||
| + help | ||
| + If you say Y here, you will be able to mount APFS partitions | ||
| + with read-only access. Write access is experimental and will | ||
| + corrupt your container. |
There was a problem hiding this comment.
Explicitly depend on BLOCK
Line 46 introduces config APFS_FS, but it doesn’t declare depends on BLOCK. Every block-based filesystem in this section (e.g., HFSPLUS_FS, XFS_FS) carries that dependency so Kconfig can refuse the option when the block layer is disabled. Without it, selecting APFS in a CONFIG_BLOCK=n configuration triggers unresolved select BUFFER_HEAD warnings and will produce build failures if someone forces the symbol on. Please add the missing dependency.
config APFS_FS
tristate "Apple file system support"
+ depends on BLOCK
select LIBCRC32C📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + tristate "Apple file system support" | |
| + select LIBCRC32C | |
| + select ZLIB_INFLATE | |
| + select NLS | |
| + select BUFFER_HEAD | |
| + select FS_IOMAP | |
| + select LEGACY_DIRECT_IO | |
| + help | |
| + If you say Y here, you will be able to mount APFS partitions | |
| + with read-only access. Write access is experimental and will | |
| + corrupt your container. | |
| tristate "Apple file system support" | |
| depends on BLOCK | |
| select LIBCRC32C | |
| select ZLIB_INFLATE | |
| select NLS | |
| select BUFFER_HEAD | |
| select FS_IOMAP | |
| select LEGACY_DIRECT_IO | |
| help | |
| If you say Y here, you will be able to mount APFS partitions | |
| with read-only access. Write access is experimental and will | |
| corrupt your container. |
🤖 Prompt for AI Agents
In
patch/kernel/archive/uefi-x86-6.12/8002-Necessary-modifications-to-build-APFS-with-the-kerne.patch
around lines 45 to 55, the new APFS_FS tristate lacks a "depends on BLOCK" line;
add a "depends on BLOCK" immediately after the tristate declaration and before
the select lines so the Kconfig will disable APFS when the block layer is off
and avoid unresolved select BUFFER_HEAD warnings.
| static void applesmc_backlight_set(struct work_struct *work) | ||
| { | ||
| - applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2); | ||
| + struct applesmc_device *smc = container_of(work, struct applesmc_device, backlight_work); | ||
| + | ||
| + applesmc_write_key(smc, BACKLIGHT_KEY, smc->backlight_state, 2); | ||
| } | ||
| -static DECLARE_WORK(backlight_work, &applesmc_backlight_set); | ||
|
|
||
| static void applesmc_brightness_set(struct led_classdev *led_cdev, | ||
| enum led_brightness value) | ||
| { | ||
| + struct applesmc_device *smc = dev_get_drvdata(led_cdev->dev); | ||
| int ret; | ||
|
|
||
| - backlight_state[0] = value; | ||
| - ret = queue_work(applesmc_led_wq, &backlight_work); | ||
| + smc->backlight_state[0] = value; | ||
| + ret = queue_work(smc->backlight_wq, &smc->backlight_work); | ||
|
|
||
| if (debug && (!ret)) | ||
| dev_dbg(led_cdev->dev, "work was already on the queue.\n"); | ||
| @@ -982,11 +1058,12 @@ static void applesmc_brightness_set(struct led_classdev *led_cdev, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify LED device parent relationship
Line 744 uses dev_get_drvdata(led_cdev->dev) to retrieve smc, but this assumes the LED device's parent has the drvdata set. The LED is registered with &smc->dev->dev as parent (line 1023), and drvdata should be available through that relationship. Please verify this is correct.
🏁 Script executed:
#!/bin/bash
# Search for led_classdev_register usage patterns in the kernel to verify drvdata access through parent device
rg -n -C3 'led_classdev_register.*dev.*dev' --type=cLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find and examine the patch file to verify LED registration and drvdata setup
fd -type f -name "3001-applesmc-convert-static-structures-to-drvdata.patch" | head -1 | xargs cat | grep -n -A10 -B5 "led_classdev_register"Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for dev_set_drvdata calls in the patch to verify when drvdata is set
fd -type f -name "3001-applesmc-convert-static-structures-to-drvdata.patch" | head -1 | xargs cat | grep -n "dev_set_drvdata\|dev_get_drvdata"Length of output: 170
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the patch file contents for LED registration
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -A10 -B5 "led_classdev_register"
Length of output: 905
🏁 Script executed:
#!/bin/bash
# Search for dev_set_drvdata calls in the patch
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n "dev_set_drvdata\|dev_get_drvdata"Length of output: 1292
🏁 Script executed:
#!/bin/bash
# Get a broader view of the LED initialization section from the patch
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -A20 "led_classdev_unregister\|led_classdev_register" | head -60Length of output: 1520
🏁 Script executed:
#!/bin/bash
# Look for platform_device_register or probe function where drvdata is set
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n "dev_set_drvdata\|platform_set_drvdata" Length of output: 76
🏁 Script executed:
#!/bin/bash
# Find the applesmc_probe function and surrounding area where registration happens
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -A5 -B5 "platform_driver_register\|applesmc_probe"Length of output: 1699
---
🏁 Script executed:
```shell
#!/bin/bash
# Find applesmc_probe in detail to see device creation and drvdata setup
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -A30 "^@@.*applesmc_probe" | head -80
Length of output: 1033
🏁 Script executed:
#!/bin/bash
# Check if led_classdev_register is followed by any drvdata operations
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -A15 "led_classdev_register.*smc->backlight_dev"Length of output: 559
🏁 Script executed:
#!/bin/bash
# Search for where smc->dev is initialized to understand the device hierarchy
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n "smc->dev.*=" | head -20Length of output: 21
🏁 Script executed:
#!/bin/bash
# Search for complete LED initialization including backlight_dev setup
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | grep -n -B10 -A10 "backlight_dev.name"Length of output: 839
🏁 Script executed:
#!/bin/bash
# Check the complete applesmc_create_key_backlight function to see all LED setup
fd --type file "3001-applesmc-convert-static-structures-to-drvdata.patch" 2>/dev/null | head -1 | xargs cat | sed -n '1000,1050p'Length of output: 1641
---
🌐 Web query:
```web
Linux kernel LED subsystem led_classdev_register drvdata parent device inheritance
💡 Result:
Short answer
- led_classdev_register creates a struct device for the LED by calling device_create/device_create_with_groups(parent, ..., drvdata = led_cdev). That makes the new /sys/class/leds/ device have the passed parent device and stores led_cdev in the device's drvdata. [1][2][3]
What that means in practice
- The LED class device's ->parent is the device you pass to led_classdev_register, so sysfs hierarchy and parent/child relationships inherit from that parent. [2]
- The device's drvdata pointer is set to the led_classdev you passed (so dev_get_drvdata(dev) returns the led_classdev). The LED core uses that to map sysfs/device callbacks back to the led_classdev instance. [2][3]
- Use devm_led_classdev_register/_ext for resource-managed registration; the kernel also has newer led_classdev_register_ext that allows passing led_init_data (fwnode/device-name composition) when you need OF/ACPI/fwnode info. [1][4]
References
- Linux LED class docs (registration API). [1]
- led-class.c (led_classdev_register uses device_create_with_groups with drvdata and parent). [2]
- device_create() kernel API (parent and drvdata parameters). [3]
- recent kernel changes: led_classdev_register_ext / devm_led_classdev_register_ext (init_data/fwnode support). [4]
Fix LED device drvdata access at line 744
The code incorrectly assumes dev_get_drvdata(led_cdev->dev) returns smc. However, the LED class device's drvdata is set to the led_classdev passed to led_classdev_register, not to arbitrary driver data. At line 744, this call retrieves the led_classdev structure (&smc->backlight_dev), not the smc pointer, causing a type mismatch.
Use container_of(&led_cdev, struct applesmc_device, backlight_dev) instead:
struct applesmc_device *smc = container_of(led_cdev, struct applesmc_device, backlight_dev);Alternatively, explicitly set the LED device drvdata after registration via dev_set_drvdata(&smc->backlight_dev.dev, smc) at line 1023.
🤖 Prompt for AI Agents
In
patch/kernel/archive/uefi-x86-6.18/3001-applesmc-convert-static-structures-to-drvdata.patch
around lines 732–754 (specifically line ~744) the code uses
dev_get_drvdata(led_cdev->dev) expecting an applesmc_device but the LED drvdata
actually points to the led_classdev, causing a type mismatch; fix by obtaining
the applesmc_device via container_of from the led_classdev (i.e. derive smc from
the led_cdev pointer using container_of with the backlight_dev member) when
entering applesmc_brightness_set, or alternatively set the LED device drvdata to
smc right after registering the led_classdev so dev_get_drvdata returns smc;
update the code to use one of these two approaches consistently and remove the
incorrect dev_get_drvdata usage.
| + if (val < 0 || val > 100) | ||
| + return -EINVAL; |
There was a problem hiding this comment.
Remove dead code: u8 is always >= 0
val is declared as u8 (line 22), so the condition val < 0 is always false and can be removed.
Apply this diff:
- if (val < 0 || val > 100)
+ if (val > 100)
return -EINVAL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + if (val < 0 || val > 100) | |
| + return -EINVAL; | |
| if (val > 100) | |
| return -EINVAL; |
🤖 Prompt for AI Agents
In patch/kernel/archive/uefi-x86-6.18/3009-applesmc-battery-charge-limiter.patch
around lines 27 to 28, the condition checks "val < 0 || val > 100" but `val` is
declared as `u8` on line 22 so `val < 0` is dead code; remove the `val < 0 ||`
portion and keep only the `val > 100` check, returning -EINVAL when val exceeds
100.
1 participant
custom_kernel_config__applet2()7001-drm-i915-fbdev-Discard-BIOS-framebuffers-exceeding-h.patchneeded special attention for correct patch attribution (missing From)
1002-Put-apple-bce-in-drivers-stagingneeds reordering to thetop to avoid conflicts with EXTRAWIFI sed-based "patching"
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Hardware Support