rtl8189es: fix compilation for 7.1 and re-enable#9810
Conversation
📝 WalkthroughWalkthroughEnables RTL8189ES for kernels >= 3.14, adds a conditional compatibility patch applied for Linux >= 7.1 that provides PPPoE struct fallbacks and updates cfg80211 handlers to use ChangesRTL8189ES Kernel 7.1 Compatibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/compilation/patch/drivers_network.sh (1)
51-95: Consider adding one runtime smoke-test note for the re-enabled path.Build coverage is good; since hardware validation wasn’t possible, it would help to track a follow-up Wi‑Fi association/basic traffic smoke test on one rtl8189es board for a 7.1+ kernel image.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/compilation/patch/drivers_network.sh` around lines 51 - 95, Add a runtime smoke-test note for the re-enabled rtl8189es path by inserting a short follow-up instruction or TODO near the re-enable block (where display_alert and the rtl8189esver variable are used in drivers_network.sh) so builders know to run a Wi‑Fi association/basic traffic test on a real rtl8189es board for kernel >= 7.1; also optionally emit a post-build reminder (using display_alert or a log message) after the block to run the hardware smoke test and record results in the test artifacts or issue tracker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/compilation/patch/drivers_network.sh`:
- Around line 51-95: Add a runtime smoke-test note for the re-enabled rtl8189es
path by inserting a short follow-up instruction or TODO near the re-enable block
(where display_alert and the rtl8189esver variable are used in
drivers_network.sh) so builders know to run a Wi‑Fi association/basic traffic
test on a real rtl8189es board for kernel >= 7.1; also optionally emit a
post-build reminder (using display_alert or a log message) after the block to
run the hardware smoke test and record results in the test artifacts or issue
tracker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0429727e-0073-44af-b4c2-1d618421776e
📒 Files selected for processing (2)
lib/functions/compilation/patch/drivers_network.shpatch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch
github-actions
Bot
added
05
EvilOlaf
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun and
amazingfate
as code owners
EvilOlaf
requested review from
NicoD-SBC,
SteeManMI,
adeepn,
brentr,
catalinii,
chainsx,
efectn,
iav,
joekhoobyar,
krachlatte,
lanefu,
paolosabatino,
prahal,
pyavitz,
rpardini,
schwar3kat and
teknoid
as code owners
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch (1)
229-252: 💤 Low valueConsider using named constants for IE type parameters in
rtw_cfg80211_set_mgnt_wpsp2pie()calls.The driver source defines
WIFI_PROBERSPandWIFI_ASSOCRSPconstants for these IE types instead of relying on inline magic numbers0x2and0x4. Adopting these symbolic names would make the code less brittle and more readable. That said, this patch mirrors the established pattern across multiple similar wireless driver patches in the repository, so alignment with that style is also reasonable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch` around lines 229 - 252, Replace the magic IE type literals 0x2 and 0x4 passed to rtw_cfg80211_set_mgnt_wpsp2pie with the driver-defined symbolic constants WIFI_PROBERSP and WIFI_ASSOCRSP respectively; update the two call sites (the block using settings->beacon.proberesp_ies/assocresp_ies and the block using info->proberesp_ies/assocresp_ies) so they pass WIFI_PROBERSP for probe response IEs and WIFI_ASSOCRSP for assoc response IEs to improve readability and maintain consistency with existing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/kernel/linux-meson64-bleedingedge.config`:
- Line 1109: The kernel config change accidentally removed unrelated options;
regenerate a minimal overlay so only CONFIG_RTL8189ES=m is added without
deleting others: run the prescribed command ./compile.sh rewrite-kernel-config
BOARD=meson64 BRANCH=bleedingedge to produce an updated config, verify the
regenerated file still contains CONFIG_POWER_RESET_MESON64=y and
CONFIG_RTL8189FS=m alongside the new CONFIG_RTL8189ES=m, and commit that
regenerated overlay instead of the hand-edited file.
In `@patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch`:
- Around line 67-100: The patch incorrectly changes cfg80211_rtw_add_key to
expect struct wireless_dev * under KERNEL_VERSION(7,1,0) but upstream still uses
struct net_device *; restore the original signature so cfg80211_rtw_add_key
takes struct net_device *ndev unconditionally, remove the added conditional
wdev->netdev assignment (the block that defines ndev from wdev) and any
references to a newly added wdev parameter, and apply the same rollback for the
other callbacks you touched (get_key, del_key, set_default_mgmt_key,
get_station, add_station, del_station, change_station, dump_station) so their
signatures match the kernel API (keep net_device * as the second param) and use
the passed ndev to derive padapter/rtw_wdev instead of introducing wireless_dev
parameters.
- Around line 225-253: This patch added unconditional WPS IE handling around
calls to rtw_cfg80211_set_mgnt_wpsp2pie after configuring beacons and after
rtw_add_beacon; remove those added blocks or guard them behind a kernel-version
check (e.g. `#if` LINUX_VERSION_CODE >= KERNEL_VERSION(7,1,0) ... `#endif`) so the
behavior only applies for 7.1+ as intended, and ensure the variable ret is
defined/initialized on all code paths before using if (ret == 0) (inspect the
code surrounding the first block that references ret and initialize or use the
correct status variable if necessary).
---
Nitpick comments:
In `@patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch`:
- Around line 229-252: Replace the magic IE type literals 0x2 and 0x4 passed to
rtw_cfg80211_set_mgnt_wpsp2pie with the driver-defined symbolic constants
WIFI_PROBERSP and WIFI_ASSOCRSP respectively; update the two call sites (the
block using settings->beacon.proberesp_ies/assocresp_ies and the block using
info->proberesp_ies/assocresp_ies) so they pass WIFI_PROBERSP for probe response
IEs and WIFI_ASSOCRSP for assoc response IEs to improve readability and maintain
consistency with existing code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60b1fe4d-33b5-49be-aa51-a12dd0d7f68e
📒 Files selected for processing (4)
config/kernel/linux-meson64-bleedingedge.configconfig/kernel/linux-rockchip64-bleedingedge.configlib/functions/compilation/patch/drivers_network.shpatch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch
✅ Files skipped from review due to trivial changes (1)
- config/kernel/linux-rockchip64-bleedingedge.config
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/compilation/patch/drivers_network.sh
| @@ -5137,6 +5183,15 @@ | ||
|
|
||
| adapter->mlmeextpriv.mlmext_info.hidden_ssid_mode = settings->hidden_ssid; | ||
|
|
||
| + // In cases like WPS, the proberesp and assocresp IEs vary from the beacon, and need to be explicitly set | ||
| + if (ret == 0) { | ||
| + if (settings->beacon.proberesp_ies && settings->beacon.proberesp_ies_len > 0) | ||
| + rtw_cfg80211_set_mgnt_wpsp2pie(ndev, (char *) settings->beacon.proberesp_ies, settings->beacon.proberesp_ies_len, 0x2 /*PROBE_RESP*/); | ||
| + | ||
| + if (settings->beacon.assocresp_ies && settings->beacon.assocresp_ies_len > 0) | ||
| + rtw_cfg80211_set_mgnt_wpsp2pie(ndev, (char *) settings->beacon.assocresp_ies, settings->beacon.assocresp_ies_len, 0x4 /*ASSOC_RESP*/); | ||
| + } | ||
| + | ||
| if (settings->ssid && settings->ssid_len) { | ||
| WLAN_BSSID_EX *pbss_network = &adapter->mlmepriv.cur_network.network; | ||
| WLAN_BSSID_EX *pbss_network_ext = &adapter->mlmeextpriv.mlmext_info.network; | ||
| @@ -5178,6 +5233,15 @@ | ||
|
|
||
| ret = rtw_add_beacon(adapter, info->head, info->head_len, info->tail, info->tail_len); | ||
|
|
||
| + // In cases like WPS, the proberesp and assocresp IEs vary from the beacon, and need to be explicitly set | ||
| + if (ret == 0) { | ||
| + if (info->proberesp_ies && info->proberesp_ies_len > 0) | ||
| + rtw_cfg80211_set_mgnt_wpsp2pie(ndev, (char *) info->proberesp_ies, info->proberesp_ies_len, 0x2 /*PROBE_RESP*/); | ||
| + | ||
| + if (info->assocresp_ies && info->assocresp_ies_len > 0) | ||
| + rtw_cfg80211_set_mgnt_wpsp2pie(ndev, (char *) info->assocresp_ies, info->assocresp_ies_len, 0x4 /*ASSOC_RESP*/); | ||
| + } | ||
| + |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm rtw_cfg80211_set_mgnt_wpsp2pie signature/usage and where `ret` is set before line ~5183 of ioctl_cfg80211.c
# (the patch references this driver path; we look for similar existing patches in the repo for context).
rg -nP --hidden -g 'patch/**' -C2 'rtw_cfg80211_set_mgnt_wpsp2pie'
echo "----"
rg -nP --hidden -g 'patch/**' -C2 'cfg80211_rtw_start_ap|adapter->mlmeextpriv\.mlmext_info\.hidden_ssid_mode'Repository: armbian/build
Length of output: 26850
🏁 Script executed:
cat patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch | head -300Repository: armbian/build
Length of output: 10185
🏁 Script executed:
grep -n "ret.*=" patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch | head -20Repository: armbian/build
Length of output: 269
🏁 Script executed:
# Get more context around the first function with the WPS IE code
sed -n '80,260p' patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patchRepository: armbian/build
Length of output: 6274
🏁 Script executed:
# Look for the cfg80211_rtw_start_ap function definition to see ret initialization
grep -B 30 "adapter->mlmeextpriv.mlmext_info.hidden_ssid_mode = settings->hidden_ssid;" patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch | head -50Repository: armbian/build
Length of output: 1274
🏁 Script executed:
# Search for where ret is assigned before line 5137 in the context (look for rtw_add_beacon or similar)
grep -n "rtw_add_beacon\|int ret\|ret =" patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch | grep -A2 -B2 "5137\|5178"Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Look at the full hunk context including function signature
sed -n '200,280p' patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patchRepository: armbian/build
Length of output: 3349
🏁 Script executed:
# Check if there are similar patches for other wireless drivers to see the pattern
rg -l "Fix-p2p-go-advertising" patch/misc/ | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 5 "if (ret == 0)" {} | head -20'Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Compare with the p2p-go-advertising patch to see if it has the same WPS code
cat patch/misc/wireless-rtl8189es-Fix-p2p-go-advertising.patch | head -100Repository: armbian/build
Length of output: 2525
🏁 Script executed:
# Check the dates - when was p2p-go-advertising patch merged vs this 7.1 patch?
# Look at git log or commit info if available
ls -la patch/misc/wireless-rtl8189es-* | grep -E "(Fix-p2p|Fix-building)"Repository: armbian/build
Length of output: 396
🏁 Script executed:
# Check if this WPS IE code appears in both patches and is identical
echo "=== From Fix-p2p-go-advertising.patch ===" && \
sed -n '29,45p' patch/misc/wireless-rtl8189es-Fix-p2p-go-advertising.patch && \
echo "" && \
echo "=== From Fix-building-on-7.1-kernel.patch ===" && \
sed -n '230,242p' patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patchRepository: armbian/build
Length of output: 1689
Remove the unconditional WPS IE handling code or gate it to 7.1+.
This patch mixes two distinct concerns: genuine 7.1+ build fixes (pppoe structures, cfg80211 API signature changes—all properly guarded with LINUX_VERSION_CODE checks) and unconditional WPS IE handling (lines 230–237 and 246–253) that has no kernel version guard. The WPS IE code runs on all kernels, silently altering runtime behavior on already-supported versions, which contradicts the patch's stated scope as a "7.1+ build fix."
The WPS IE code is identical to that in the separate wireless-rtl8189es-Fix-p2p-go-advertising.patch and belongs there, not here. Additionally, in the first WPS block (line 230), the prior assignment to ret is not visible in the patch context; confirm it is initialized on all code paths before the if (ret == 0) check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch` around lines
225 - 253, This patch added unconditional WPS IE handling around calls to
rtw_cfg80211_set_mgnt_wpsp2pie after configuring beacons and after
rtw_add_beacon; remove those added blocks or guard them behind a kernel-version
check (e.g. `#if` LINUX_VERSION_CODE >= KERNEL_VERSION(7,1,0) ... `#endif`) so the
behavior only applies for 7.1+ as intended, and ensure the variable ret is
defined/initialized on all code paths before using if (ret == 0) (inspect the
code surrounding the first block that references ret and initialize or use the
correct status variable if necessary).
There was a problem hiding this comment.
the patch won't be applied to any version prior to 7.1 anyways by conditional in drivers_network.sh
There was a problem hiding this comment.
@EvilOlaf, you're right — since drivers_network.sh gates the entire patch to kernels >= 7.1, the WPS IE handling blocks inside the patch will never be applied to pre-7.1 builds. My concern about them running unconditionally on older kernels was incorrect given that build-time context. Thanks for the clarification!
One minor follow-up worth considering: if wireless-rtl8189es-Fix-p2p-go-advertising.patch (which contains identical WPS IE code) is also applied on 7.1+ builds, the same WPS IE handling would be applied twice. You may want to verify whether both patches are applied in sequence on 7.1+ kernels and if so, whether that causes a double-application issue — but that's a separate and much lower priority concern.
✏️ Learnings added
Learnt from: EvilOlaf
Repo: armbian/build PR: 9810
File: patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch:225-253
Timestamp: 2026-05-11T15:36:23.559Z
Learning: In `armbian/build`, the patch `patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch` is only applied for Linux kernel >= 7.1 via an explicit version gate in `lib/functions/compilation/patch/drivers_network.sh`. Any code in this patch file without internal `#if LINUX_VERSION_CODE` guards is intentionally unconditional within the patch, because the patch itself is never applied to pre-7.1 builds.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kasimling
Repo: armbian/build PR: 9177
File: patch/kernel/archive/rockchip64-6.18/rk3588-1212-arm64-dts-Automatic-fan-speed-and-USB-3.0-Type-A-por.patch:37-41
Timestamp: 2026-01-10T04:28:58.758Z
Learning: When reviewing Linux kernel device-tree bindings involving thermal cooling devices, THERMAL_NO_LIMIT can be used for both min_state (lower) and max_state (upper). If used as lower/min_state, the framework converts it to 0; if used as upper/max_state, it converts to the cooling device's max_state. Verify patch entries like: cooling-device = <&fan THERMAL_NO_LIMIT 1> are valid and correctly reflect the intended bounds. Ensure consistent interpretation of NO_LIMIT in both min and max contexts and that the resulting values align with the device's actual limits.
Learnt from: kasimling
Repo: armbian/build PR: 9286
File: patch/kernel/archive/sm8550-6.18/0017-Disable-sdhci_pltfm_free-calling.patch:0-0
Timestamp: 2026-01-23T14:10:22.671Z
Learning: When reviewing patches porting drivers to kernel 6.18, remove calls to sdhci_pltfm_free() and sdhci_free_host() as these APIs were removed. Drivers should migrate to using devm_mmc_alloc_host() for device-managed allocation. In porting patches from older kernels (e.g., 6.12), verify that explicit free helpers are eliminated and that the patch uses the device-managed allocation pattern.
There was a problem hiding this comment.
I admit this is partially inconsistent because on one side since the patch itself is gated against the kernel version, in-patch guards are actually unnecessary. On the other side some in-patch gates are missing as you pointed out.
Well on the bottom line I don't care much. The initial code of the driver is like 10'ish years old and has been dragged over and over and over to the most recent kernel version. wifi on Linux is still a huge pain overall.
Anyway I may (or may not) check the double wps ie handling application.
|
What about the FS variant? |
WIP right now. issue is pretty much the same. Will do extra pr |
igorpecovnik
added
08
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/compilation/patch/drivers_network.sh (1)
51-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure rtl8189ES is intentionally enabled for the
bleedingedgekernel branch
- No
KERNEL_DRIVERS_SKIPentries referencingrtl8189eswere found in the repo config, so bleedingedge builds will includedriver_rtl8189ES.- The referenced
patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patchexists, so the 7.1 patch step won’t fail due to a missing file.- If bleedingedge should remain excluded for compatibility reasons, reintroduce the
BRANCH != bleedingedgeguard (or add an explicitKERNEL_DRIVERS_SKIPforrtl8189esin bleedingedge-targeted configs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/compilation/patch/drivers_network.sh` at line 51, The current conditional (the linux-version compare check in drivers_network.sh that gates inclusion of driver_rtl8189ES) allows the rtl8189ES driver to be built on the bleedingedge branch; either make this explicit by adding a branch guard or add a kernel-skip entry: update the logic around the linux-version compare (and associated inclusion of driver_rtl8189ES/patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch) to reintroduce a BRANCH != bleedingedge guard or add a KERNEL_DRIVERS_SKIP entry for rtl8189es in bleedingedge-targeted configs so bleedingedge builds exclude driver_rtl8189ES as intended.
♻️ Duplicate comments (1)
lib/functions/compilation/patch/drivers_network.sh (1)
102-102:⚠️ Potential issue | 🟡 MinorVerify that enabling rtl8189FS for bleedingedge branch is intentional.
Similar to rtl8189ES, the removal of the BRANCH condition now enables this driver for bleedingedge kernels >= 3.14. However, since the 7.1 compatibility patch is not yet available (see separate comment on lines 141-145), this change may be premature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/compilation/patch/drivers_network.sh` at line 102, The change removed the BRANCH guard and now enables the rtl8189FS driver for bleedingedge kernels >= 3.14; revert that by reintroducing a conditional around the linux-version compare so rtl8189FS is only enabled for supported branches (e.g., keep the original BRANCH check used for rtl8189ES) or explicitly exclude BRANCH=="bleedingedge" until the 7.1 compatibility patch exists; update the condition around the linux-version compare "${version}" ge 3.14 and reference the rtl8189FS enabling block to ensure bleedingedge kernels are not affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/functions/compilation/patch/drivers_network.sh`:
- Line 51: The current conditional (the linux-version compare check in
drivers_network.sh that gates inclusion of driver_rtl8189ES) allows the
rtl8189ES driver to be built on the bleedingedge branch; either make this
explicit by adding a branch guard or add a kernel-skip entry: update the logic
around the linux-version compare (and associated inclusion of
driver_rtl8189ES/patch/misc/wireless-rtl8189es-Fix-building-on-7.1-kernel.patch)
to reintroduce a BRANCH != bleedingedge guard or add a KERNEL_DRIVERS_SKIP entry
for rtl8189es in bleedingedge-targeted configs so bleedingedge builds exclude
driver_rtl8189ES as intended.
---
Duplicate comments:
In `@lib/functions/compilation/patch/drivers_network.sh`:
- Line 102: The change removed the BRANCH guard and now enables the rtl8189FS
driver for bleedingedge kernels >= 3.14; revert that by reintroducing a
conditional around the linux-version compare so rtl8189FS is only enabled for
supported branches (e.g., keep the original BRANCH check used for rtl8189ES) or
explicitly exclude BRANCH=="bleedingedge" until the 7.1 compatibility patch
exists; update the condition around the linux-version compare "${version}" ge
3.14 and reference the rtl8189FS enabling block to ensure bleedingedge kernels
are not affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72e036f1-f5e6-4e10-afcc-e6fedeb2a303
📒 Files selected for processing (3)
config/kernel/linux-meson64-bleedingedge.configconfig/kernel/linux-rockchip64-bleedingedge.configlib/functions/compilation/patch/drivers_network.sh
💤 Files with no reviewable changes (1)
- config/kernel/linux-meson64-bleedingedge.config
✅ Files skipped from review due to trivial changes (1)
- config/kernel/linux-rockchip64-bleedingedge.config
3 participants
Description
How Has This Been Tested?
Checklist:
Summary by CodeRabbit