Add rtl8xxxu-openipc package and RTL8188FU firmware - #2259
Closed
shipa-2 wants to merge 1 commit into
Closed
Conversation
mac80211 rtl8xxxu backport for RTL8188FTV (0bda:f179) on old kernels, plus rtl8188fufw.bin install option in linux-firmware-openipc. Co-authored-by: Cursor <cursoragent@cursor.com>
3 tasks
PR Summary by QodoAdd rtl8xxxu-openipc backport package and RTL8188FU firmware option
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. C2H skb leak
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+5588
to
+5591
| } | ||
|
|
||
| out: | ||
| dev_kfree_skb(skb); |
There was a problem hiding this comment.
1. C2h skb leak 🐞 Bug ☼ Reliability
rtl8xxxu_c2hcmd_callback() drains priv->c2hcmd_queue but only frees a single skb at the end, leaking every dequeued skb except the last. This will steadily increase kernel memory usage when C2H events are frequent.
Agent Prompt
### Issue description
`rtl8xxxu_c2hcmd_callback()` dequeues potentially multiple SKBs from `priv->c2hcmd_queue`, but only calls `dev_kfree_skb(skb)` once after the loop. This leaks SKBs.
### Issue Context
This is a kernel driver; leaking SKBs in an event path can create sustained memory growth.
### Fix Focus Areas
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_core.c[5538-5592]
### What to change
- Change the loop to dequeue-and-process in a way that frees **each** dequeued skb exactly once, e.g.:
- `while ((skb = skb_dequeue(&priv->c2hcmd_queue)) != NULL) { ...; dev_kfree_skb(skb); }`
- Ensure `skb` is not double-freed on exit paths (set to `NULL` after freeing if keeping an `out:` label).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+6634
to
+6637
| cancel_delayed_work_sync(&priv->ra_watchdog); | ||
|
|
||
| rtl8xxxu_free_rx_resources(priv); | ||
| rtl8xxxu_free_tx_resources(priv); |
There was a problem hiding this comment.
2. Stop workqueue race 🐞 Bug ☼ Reliability
rtl8xxxu_stop() frees RX/TX resources after setting shutdown, but it does not cancel/flush rx_urb_wq (and c2hcmd_work), so already-queued work can still run and submit URBs or touch driver state after teardown. This can cause use-after-free, re-arming URBs after stop, or crashes on disconnect.
Agent Prompt
### Issue description
`rtl8xxxu_stop()` sets `priv->shutdown = true`, kills anchored URBs, and then frees RX/TX resources, but it does not synchronously cancel/flush work items that can still run (`rx_urb_wq`, `c2hcmd_work`). `rtl8xxxu_rx_urb_work()` also does not check `priv->shutdown` before resubmitting URBs.
### Issue Context
- `rtl8xxxu_queue_rx_urb()` schedules `rx_urb_wq`.
- `rtl8xxxu_rx_urb_work()` resubmits URBs.
- `rtl8723bu_handle_c2h()` schedules `c2hcmd_work`.
- `rtl8xxxu_stop()` frees resources without canceling those works.
### Fix Focus Areas
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_core.c[5261-5328]
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_core.c[5538-5641]
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_core.c[6605-6638]
### What to change
- In `rtl8xxxu_stop()` (before freeing RX/TX resources):
- `cancel_work_sync(&priv->rx_urb_wq);`
- `cancel_work_sync(&priv->c2hcmd_work);`
- purge `priv->c2hcmd_queue` (dequeue + `dev_kfree_skb()` until empty).
- Add a shutdown guard in `rtl8xxxu_rx_urb_work()` to avoid resubmitting URBs if `priv->shutdown` became true (free the URB/SKB instead).
- Ensure ordering: prevent new scheduling first (set shutdown), then cancel work, then kill URBs, then free resources.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+41
| int rtl8xxxu_debug = RTL8XXXU_DEBUG_EFUSE; | ||
| static bool rtl8xxxu_ht40_2g; | ||
| static bool rtl8xxxu_dma_aggregation; | ||
| static int rtl8xxxu_dma_agg_timeout = -1; |
There was a problem hiding this comment.
3. Efuse debug default enabled 🐞 Bug ◔ Observability
rtl8xxxu_debug defaults to RTL8XXXU_DEBUG_EFUSE, and rtl8188fu_parse_efuse() dumps raw efuse content via dev_info when that flag is set. This will spam kernel logs on every probe by default and can expose device-specific values (e.g., calibration data/MAC-related fields) unnecessarily.
Agent Prompt
### Issue description
The module debug mask is initialized to `RTL8XXXU_DEBUG_EFUSE`, which enables verbose EFUSE dumping at `dev_info()` level during normal operation.
### Issue Context
`rtl8188fu_parse_efuse()` prints the EFUSE dump when the EFUSE debug bit is enabled.
### Fix Focus Areas
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_core.c[36-42]
- general/package/rtl8xxxu-openipc/src/rtl8xxxu_8188f.c[716-749]
### What to change
- Change the default to `int rtl8xxxu_debug = 0;` (or another non-verbose default).
- Keep EFUSE dumping behind the module param so it is only enabled intentionally.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7
to
+10
| Mainline rtl8xxxu mac80211 driver (RTL8188F backport to | ||
| kernel 3.4.43). Replaces the vendor rtl8188fu driver and | ||
| fixes the WPA2 4-way handshake failure seen with the | ||
| vendor driver on the GK7102. |
There was a problem hiding this comment.
4. No vendor-driver exclusion 🐞 Bug ⚙ Maintainability
The new rtl8xxxu-openipc package help text says it replaces the vendor rtl8188fu driver, but Buildroot allows enabling both rtl8188fu-openipc and rtl8xxxu-openipc simultaneously. This creates an inconsistent configuration where two competing solutions for the same chipset can be built into the image.
Agent Prompt
### Issue description
`rtl8xxxu-openipc` is described as replacing the vendor `rtl8188fu` driver, but nothing in Kconfig prevents selecting both packages.
### Issue Context
Both packages are present in `general/package/Config.in`, so they are independently selectable.
### Fix Focus Areas
- general/package/rtl8xxxu-openipc/Config.in[1-10]
- general/package/rtl8188fu-openipc/Config.in[1-4]
- general/package/Config.in[91-95]
### What to change
- Add a dependency to prevent simultaneous selection, e.g. in `rtl8xxxu-openipc/Config.in`:
- `depends on !BR2_PACKAGE_RTL8188FU_OPENIPC`
- and/or add a reciprocal comment in the vendor driver config.
- Alternatively, document and enforce which one should be used via a `choice` block if that matches project conventions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
This thread is closed as no response has been received |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rtl8xxxu-openipc(mac80211 backport for RTL8188FTV0bda:f179, includingRCR_APPEND_FCS).rtl8188fufw.binoption tolinux-firmware-openipc; package selects parent firmware + RTL8188FU blob.Test plan
BR2_PACKAGE_RTL8XXXU_OPENIPCon a GK710x board config and build/lib/firmware/rtlwifi/rtl8188fufw.bin8188fupathPart of splitting #2256. Board enablement / toolchain flag / overlay are separate.
Made with Cursor