Ayn Odin2 kernel 7.0 support#9706
Conversation
📝 WalkthroughWalkthroughBumps SM8550 kernel baseline from 6.18 to 7.0 for the edge channel; adds ~40 kernel patches that introduce many new drivers (panels, backlight, LED, PWM, regulator, joystick), DT board includes and boards for multiple AYN devices, NoC/QoS/OPP DTS updates, driver fixes, soc-serial-derived MAC/BD_ADDR, and a compat-input prctl flag. ChangesKernel Version & Defconfig
Drivers, Panels, Backlights, PWM, LEDs, Regulators, Input
Interconnect / QoS / NoC / OPP / Caches / EAS (DTS changes)
Device-tree: AYN common include and board DTs
Soc-serial derived MAC / BD_ADDR
Compat input syscalls via prctl
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
05
|
Audio is not working yet. |
kasimling
force-pushed
the
odin2_kernel_7.0
branch
from
07a0378 to
6e53af8
Compare
|
Now audio is working |
kasimling
force-pushed
the
odin2_kernel_7.0
branch
from
6e53af8 to
be4183d
Compare
kasimling
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (14)
patch/kernel/archive/sm8550-7.0/0022-Input-hynitron_cstxxx-specific-patch-for-Odin2-Mini.patch-44-58 (1)
44-58:⚠️ Potential issue | 🟠 MajorRegulator left enabled on driver unbind / late probe failure.
devm_regulator_get()only releases the reference on detach; it does not callregulator_disable(). Thevddregulator is enabled with no matching disable in any error path (e.g., failures frominit_input,request_threaded_irq, etc.) nor on.remove(). Each unbind/rebind cycle will leave the regulator with an extra enable count, eventually preventing it from being disabled by other consumers.Prefer
devm_regulator_get_enable()(available since 5.18) so disable is tied to device lifetime, or register a devm action.🛡️ Suggested fix using devm_regulator_get_enable
- ts_data->vdd = devm_regulator_get(&client->dev, "vdd"); - if (IS_ERR(ts_data->vdd)) { - err = PTR_ERR(ts_data->vdd); - if (err != -EPROBE_DEFER) - dev_err(&client->dev, "Failed to request vdd regulator: %d\n", err); - return err; - } - - err = regulator_enable(ts_data->vdd); - if (err) { - dev_err(&client->dev, "enable regulator failed: %d\n", err); - return err; - } + err = devm_regulator_get_enable(&client->dev, "vdd"); + if (err) + return dev_err_probe(&client->dev, err, + "Failed to enable vdd regulator\n");If you go this route, the
struct regulator *vddfield added tohynitron_ts_data(line 27) can also be dropped, since it is otherwise unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0022-Input-hynitron_cstxxx-specific-patch-for-Odin2-Mini.patch` around lines 44 - 58, The patch enables the vdd regulator with devm_regulator_get() + regulator_enable() but never disables it on error or device remove, leaking enable counts; replace the get+enable sequence by using devm_regulator_get_enable(&client->dev, "vdd") (available since 5.18) so the regulator is automatically disabled on device detach, or alternatively register a devm_add_action to call regulator_disable on cleanup; update references to ts_data->vdd and, if you choose devm_regulator_get_enable and the vdd field is otherwise unused, remove the struct regulator *vdd from hynitron_ts_data to avoid holding an unused pointer.patch/kernel/archive/sm8550-7.0/0027-HACK-fix-usb-boot-hang.patch-15-30 (1)
15-30:⚠️ Potential issue | 🟠 MajorHACK acknowledged — please scope and document the workaround.
A few concerns with removing the
XHCI_STS_CNR(Controller-Not-Ready) handshake wholesale:
- Spec compliance. The xHCI spec requires software to wait for
CNR=0before writing operational registers. Theop_reg_basehalt/XHCI_IRQSwrites immediately following the deletion may now race with controller-ready state on slow-to-init xHCIs and produce undefined behavior — though on the affected Odin2 hardware the original 5s wait was apparently never clearing, hence the hang.- Blast radius.
quirk_usb_handoff_xhci()runs for every xHCI device on the system. Removing the wait globally in the SM8550 kernel affects all current/future xHCI controllers, not just the SoC-internal one that triggered the hang. Consider bounding the workaround with apci_match_id()check for the specific vendor/device that hangs, or shortening the timeout (e.g., 100 ms) instead of dropping it entirely.- Silent regression risk. The accompanying
dev_warn("xHCI HW not ready after 5 sec...")is also removed, so if a future regression re-introduces a non-ready controller, there will be no log signal.- Commit metadata. Please expand the commit message to describe the root cause (which xHCI / firmware revision hangs in CNR), so the next person rebasing this onto kernel 7.1+ knows whether the HACK is still needed.
Acknowledging the
HACK:prefix indicates this is intentional for now; treating as a major (not blocker) flag for a more targeted follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0027-HACK-fix-usb-boot-hang.patch` around lines 15 - 30, The removed XHCI CNR handshake in quirk_usb_handoff_xhci (removal of the XHCI_STS_CNR wait and dev_warn) is too broad; reinstate a bounded workaround: detect the specific problematic controller (use pci_match_id with the vendor/device ID for the Odin2/SoC-internal xHCI) and only skip or shorten the CNR handshake for that ID, otherwise perform a short timeout handshake (e.g., handshake(op_reg_base + XHCI_STS_OFFSET, XHCI_STS_CNR, 0, 100000, 10)) and keep the dev_warn logging on timeout (including the readl status). Also update the commit message to document the affected xHCI device/firmware and rationale for the scoped HACK so future rebases can assess necessity.patch/kernel/archive/sm8550-7.0/0025-input-RSInput-Add-regulator.patch-67-76 (1)
67-76:⚠️ Potential issue | 🟠 MajorRegulator enable leaks on probe failure.
regulator_enable(drv->vdd)is called early in probe (line 73) but subsequent error paths—GPIO failures (lines 80–82),serdev_device_openfailure (line 102), andrsinput_init_commandsfailure—allreturn errorwithout a matchingregulator_disable. The disable only runs inrsinput_remove(), which is not called when probe fails. This leaves the regulator enabled with an unbalanced refcount, preventing re-probe/rebind and potentially keeping power on a non-functional device.Prefer the device-managed variant so cleanup is automatic:
🔧 Suggested fix using devm_regulator_get_enable
- drv->vdd = devm_regulator_get(&serdev->dev, "vdd"); - if (IS_ERR(drv->vdd)) { - error = PTR_ERR(drv->vdd); - return error; - } - - error = regulator_enable(drv->vdd); - if (error < 0) { - return error; - } + error = devm_regulator_get_enable(&serdev->dev, "vdd"); + if (error) + return dev_err_probe(&serdev->dev, error, + "Failed to enable vdd regulator\n");Then drop the
struct regulator *vdd;field fromstruct rsinput_driverand remove theregulator_disable(drv->vdd);call inrsinput_remove()(line 109).The function is available in Linux 6.2+ and your target kernel 7.0, and since the driver needs only simple enable-on-probe/disable-on-remove lifecycle with no runtime control,
devm_regulator_get_enable()is the correct and idiomatic choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0025-input-RSInput-Add-regulator.patch` around lines 67 - 76, The probe enables the regulator via regulator_enable(drv->vdd) after getting it with devm_regulator_get, but on later probe failures the code returns without disabling the regulator, leaking a refcount; replace the get+enable pair with the device-managed devm_regulator_get_enable(&serdev->dev, "vdd") and remove manual regulator_disable(drv->vdd) and the vdd field from struct rsinput_driver so the regulator is automatically disabled on probe failure and removal; update references to drv->vdd (e.g., in probe and rsinput_remove) accordingly and drop regulator_disable(drv->vdd) from rsinput_remove().patch/kernel/archive/sm8550-7.0/0006-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch-36-57 (1)
36-57:⚠️ Potential issue | 🟠 MajorClock refcount leak:
clk_prepare_enableis unbalanced.
sc8280xp_snd_startup()callsclk_prepare_enable(pdata->i2s_clk)on every PCM open forPRIMARY/SECONDARY_MI2S_RX, butsc8280xp_be_ops.shutdownis still wired toqcom_snd_sdw_shutdown, which won't disablei2s_clk. Each open increments the prepare/enable refcount permanently — across stream churn this leaks the clock and keeps the I2S block powered.Also,
clk_prepare_enable()and the twosnd_soc_dai_set_fmt()return values are silently ignored.🛠️ Suggested fix
Add a corresponding shutdown that releases the clock for the same DAI ids, and propagate errors from startup:
+static void sc8280xp_snd_shutdown(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); + + switch (cpu_dai->id) { + case PRIMARY_MI2S_RX: + case SECONDARY_MI2S_RX: + clk_disable_unprepare(pdata->i2s_clk); + break; + } + + qcom_snd_sdw_shutdown(substream); +} @@ static const struct snd_soc_ops sc8280xp_be_ops = { .startup = sc8280xp_snd_startup, - .shutdown = qcom_snd_sdw_shutdown, + .shutdown = sc8280xp_snd_shutdown,Also applies to: 62-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0006-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch` around lines 36 - 57, The startup currently calls clk_prepare_enable(pdata->i2s_clk) and ignores return values from clk_prepare_enable and snd_soc_dai_set_fmt, leaking the clock; add a matching shutdown handler (e.g., implement sc8280xp_snd_shutdown and wire it into sc8280xp_be_ops.shutdown or call clk_disable_unprepare in the existing shutdown path) that checks cpu_dai->id for PRIMARY_MI2S_RX and SECONDARY_MI2S_RX and calls clk_disable_unprepare(pdata->i2s_clk); in sc8280xp_snd_startup check and propagate errors from clk_prepare_enable and from the two snd_soc_dai_set_fmt calls, and on any error undo prior work (disable/unprepare the clock if it was enabled) before returning the error code so refcounts stay balanced.patch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patch-69-93 (1)
69-93:⚠️ Potential issue | 🟠 MajorBroken error handling and partial-enable state in
sgm3804_enable.Two issues here:
ret |= regmap_write(...)ORs three potentially-negative errno values together. The result is not a meaningful errno (e.g.,(-EIO) | (-ENODEV)≈-1) and silently masks which write failed. Errors must be checked sequentially and returned immediately.- On a write failure mid-sequence, the chip is left half-programmed, the reset GPIOs remain asserted high, and
data->enabledstaysfalse, so the regulator core thinks the rail is off while the silicon is partially enabled. A subsequentenable()retry then re-asserts everything from a non-clean state.🛠️ Suggested fix
- ret |= regmap_write(regmap, 0x00, 0x0c); - ret |= regmap_write(regmap, 0x01, 0x0c); - ret |= regmap_write(regmap, 0x03, 0x03); - if (ret) { - dev_err(rdev->dev.parent, "Failed to enable SGM3804 regulator\n"); - return ret; - } + ret = regmap_write(regmap, 0x00, 0x0c); + if (ret) + goto err; + ret = regmap_write(regmap, 0x01, 0x0c); + if (ret) + goto err; + ret = regmap_write(regmap, 0x03, 0x03); + if (ret) + goto err; data->enabled = true; return 0; + +err: + dev_err(rdev->dev.parent, "Failed to enable SGM3804 regulator: %d\n", ret); + if (data->reset_gpio[1]) + gpiod_set_value_cansleep(data->reset_gpio[1], 0); + if (data->reset_gpio[0]) + gpiod_set_value_cansleep(data->reset_gpio[0], 0); + return ret;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patch` around lines 69 - 93, The sgm3804_enable function must check each regmap_write sequentially and return the first negative errno instead of OR-ing results; perform regmap_write to 0x00, check ret and return on error, then 0x01, check/return, then 0x03, check/return. On any failure undo any previous successful state changes: drive any reset_gpio[] back to their previous (low) state and ensure data->enabled remains false before returning the error; only set data->enabled = true after all writes succeed. Also include the actual errno in the dev_err message using the failed return value to aid debugging.patch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patch-133-187 (1)
133-187:⚠️ Potential issue | 🟠 MajorLED node-name suffixes are inverted vs. declared colors and grouping.
Across both
htr3212landhtr3212rcontrollers the per-channel node names use_b/_g/_rsuffixes that don't match either thelabelstring or thecolorvalue. For example:
ledl_b1(suffix "b") →label = "l:r1",color = LED_COLOR_ID_REDledl_r1(suffix "r") →label = "l:b1",color = LED_COLOR_ID_BLUEledl_g1(suffix "g") →label = "l:g1",color = LED_COLOR_ID_GREENThe
multi-led*parents then list channels in declaration order:multi-ledr1 { leds = <&ledr_r4>, <&ledr_g4>, <&ledr_b4>; // RED, GREEN, BLUE order? };But by the actual
colorvalues,ledr_r4 = BLUEandledr_b4 = RED, so this group is effectively wired BGR rather than RGB. Someleds-group-multicolorconsumers do honor list order for color reporting/debug, and userspace-visible labels (l:r1vs nodeledl_b1) will not match what callers expect when looking at sysfs paths.Please reconcile naming so node-name suffix,
label, andcoloragree, and update themulti-led*leds = <…>lists accordingly. If the current wiring is intentional (e.g., to expose a particular hardware-channel-to-color mapping), document it in a comment so it isn't read as a copy/paste mistake.Minor: line 343 has a space-before-tab on
color = <LED_COLOR_ID_BLUE>;insideledr_r2.Also applies to: 217-277, 315-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patch` around lines 133 - 187, Per the review, per-channel node names and their color metadata are inconsistent: nodes like ledl_b1/ledl_r1/ledl_g1 have suffixes that don't match their label or color properties (e.g., ledl_b1 labeled "l:r1" but color LED_COLOR_ID_RED), which in turn makes multi-led groupings (multi-ledr1/multi-ledl1/... using leds = <&ledr_r4>, <&ledr_g4>, <&ledr_b4>; etc.) ordered wrongly (BGR vs RGB). Fix by renaming per-channel nodes or correcting their label and color properties so the node-name suffix (_r/_g/_b), the label string (e.g., "l:r1"/"l:g1"/"l:b1"), and the color property (LED_COLOR_ID_RED/GREEN/BLUE) all agree for each htr3212l/htr3212r channel (ledr_*/ledl_*), and then reorder the leds = <...> lists in multi-ledr1, multi-ledl1, multi-ledr2, multi-ledl2, multi-ledr3, multi-ledl3, multi-ledr4, multi-ledl4 to match RGB order accordingly; if wiring intentionally maps differently, add a comment above the affected multi-led* node explaining the hardware mapping. Also fix the whitespace issue at the noted color = <LED_COLOR_ID_BLUE>; to remove the space-before-tab.patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch-116-135 (1)
116-135:⚠️ Potential issue | 🟠 MajorSerial-derived BD_ADDR unconditionally overrides the device's NVM/factory address.
generate_bdaddr_from_serial()is now called before any check of the existing/factory BD_ADDR, and on success the driver writes that synthetic address intohdev->public_addrandhdev->bdaddr. Devices that ship with a valid, vendor-programmed BD_ADDR (from QCA NVM) lose it after this patch — including across unrelated boards that also usebtqca.c.Since this driver is shared with all QCA Bluetooth UART devices, this change has broad impact. Please gate the new behavior behind a board/DT property (or a Kconfig option) so non-AYN boards keep the upstream NVM-based logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 116 - 135, The patch currently unconditionally generates and writes a serial-derived BD_ADDR (via generate_bdaddr_from_serial, qca_set_bdaddr and bacpy to hdev->public_addr/hdev->bdaddr), which overwrites a valid NVM/factory address; modify the logic to first check for an existing valid BD_ADDR using qca_check_bdaddr (or the existing check in this file) and only proceed to call generate_bdaddr_from_serial/qca_set_bdaddr and update hdev addresses if no valid NVM BD_ADDR exists AND a board-enabling flag is set; add a DT property (or Kconfig flag) check (e.g., ofdev/bdaddr-from-serial or a new bool like allow_serial_bdaddr) before generating/setting the synthetic address so non-AYN boards retain upstream NVM behavior.patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch-279-303 (1)
279-303:⚠️ Potential issue | 🟠 MajorRace condition:
qcom_serial_numberupdated without synchronization.
qcom_socinfo_remove()clearsqcom_serial_number = NULLwithout any synchronization, whilebtqcaandath12kdereference the same pointer from their own probe/setup paths. Ifsocinfois unloaded (or its probe is repeated for any reason) while a consumer is in the middle ofgenerate_bdaddr_from_serial()/generate_macaddr_from_serial(), you get a TOCTOU NULL deref between theif (!serial)check and the subsequentstrlen(serial)/strncpy(...).Also note
qs->attr.serial_numberis allocated withdevm_kasprintf(), so whenqcom_socinfois removed the memory backing the global pointer is freed by devres before any consumer is told to stop using it — i.e. a use-after-free is possible even in the absence of a NULL race.Practical mitigations:
- Pass the serial through a stable interface (e.g.,
qcom_smem_get(...)directly, or a getter that returns a copy under a lock/RCU).- At minimum, make
qcom_serial_numberpoint to a string with module/static lifetime (notdevm_*) so it remains valid for the lifetime of the kernel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 279 - 303, The global qcom_serial_number is assigned in qcom_socinfo_probe and cleared in qcom_socinfo_remove without synchronization, allowing TOCTOU NULL-deref or use-after-free in consumers like generate_bdaddr_from_serial/generate_macaddr_from_serial (used by btqca/ath12k). Fix by giving consumers a stable string lifetime or a safe getter: either allocate a module/static copy (e.g., kstrdup at probe and kfree at remove) and publish it via rcu_assign_pointer/synchronize_rcu, or implement a getter function that returns a freshly allocated copy under a lock/RCU; ensure qcom_socinfo_remove uses synchronize_rcu (or the lock) before freeing/clearing the pointer so consumers cannot see freed memory.patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch-249-268 (1)
249-268:⚠️ Potential issue | 🟠 MajorSame concern in ath12k: forcing serial-derived MAC affects all ath12k devices.
ath12k_mac_hw_register()now prefersgenerate_macaddr_from_serial()over the existingpdevs_macaddr_valid/ar->ab->mac_addrpaths. Like the btqca change, this affects every ath12k-based device, not just AYN Odin 2 — boards that have valid per-pdev factory MAC addresses will now have them silently replaced by an OUI-00:03:7Fsynthetic address derived from the SoC serial.Please make this opt-in (DT property, module parameter, or Kconfig) so other ath12k users retain upstream behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 249 - 268, The patch forces a serial-derived MAC for all ath12k devices by unconditionally calling generate_macaddr_from_serial() in ath12k_mac_hw_register(), replacing existing pdevs_macaddr_valid / ar->ab->mac_addr behavior; make this opt-in by adding a runtime flag (prefer a device-tree boolean like "qcom,use-serial-mac" on the ath12k DT node or a module parameter) and only call generate_macaddr_from_serial() when that flag is set, otherwise keep the original logic that checks ar->ab->pdevs_macaddr_valid and falls back to ar->ab->mac_addr with ar->pdev_idx adjustment; update the conditional around generate_macaddr_from_serial(), and ensure log messages (ath12k_info) clearly state when the serial-derived MAC path was chosen (include the DT/module flag name and the generated MAC), leaving all other code paths and variables (ar->mac_addr, ath12k_reverse_mac, pdevs_macaddr_valid) unchanged.patch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patch-151-194 (1)
151-194:⚠️ Potential issue | 🟠 MajorRegulator enable refcount leak on probe error and remove paths.
The
regulator_bulk_enable()call at line 99 is not matched by aregulator_bulk_disable()call:
- On probe error after this point (e.g., regmap init failure at line 108, backlight register failure at line 116), the regulators stay enabled.
odin2miniled_remove()(lines 128-133) only updates backlight state; it does not disable the supplies or de-assert the enable GPIO, so the regulator enable refcount grows by one with each removal.Use
devm_regulator_bulk_get_enable()(available in kernel 6.0+) instead ofdevm_regulator_bulk_get_const()followed by manualregulator_bulk_enable(). This automatically balances enable/disable on devm release and simplifies error handling. Alternatively, add explicitregulator_bulk_disable()calls in both the error path andodin2miniled_remove().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patch` around lines 151 - 194, The probe enables regulators with regulator_bulk_enable(ARRAY_SIZE(miniled_supplies), miniled->supplies) but never disables them on errors or in odin2miniled_remove(), leaking regulator refcounts; replace the manual get+enable pattern with devm_regulator_bulk_get_enable(&client->dev, ARRAY_SIZE(miniled_supplies), miniled->supplies) (or, if not available, call regulator_bulk_disable(...) on all error exits after enable and in odin2miniled_remove()) so that the supplies are properly balanced; update the probe error paths (after regmap init and backlight registration failures) and odin2miniled_remove() to either rely on devm_regulator_bulk_get_enable or explicitly call regulator_bulk_disable for miniled->supplies to release the regulators and deassert the enable GPIO.patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch-327-361 (1)
327-361:⚠️ Potential issue | 🟠 MajorDon't call
rsinput_init_commands()(which sleeps) from the serdev RX callback.
rsinput_rx()runs on the serdev RX path; on a bad checksum it invokesrsinput_init_commands(drv)which callsmsleep(100)twice andserdev_device_write_buf(). Sleeping/blocking from the receive callback can deadlock against the controller's own write/RX completion or simply stall the UART. Either schedule the re-init from a workqueue, or just discard the bad frame and let the next valid one resume processing.Also note: validating one large checksum across
counthere is redundant with the per-frame validation already done inrsinput_process_data()(line 298-304). The MCU may legitimately deliver multiple packets per RX chunk; treating a multi-frame buffer as a single CRC region rejects valid data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch` around lines 327 - 361, The serdev RX callback rsinput_rx must not call rsinput_init_commands (which sleeps and writes) or perform a single checksum across the entire received chunk; instead drop that global checksum logic and forward the raw buffer to rsinput_process_data so its per-frame validation can handle multiple packets per RX, and if you still need to trigger a re-init on persistent errors, queue a work item (work_struct/delayed_work) from rsinput_rx that calls rsinput_init_commands context-safely; specifically remove the compute_checksum vs. received_checksum branch in rsinput_rx, stop invoking rsinput_init_commands there, copy into drv->rx_buf and call rsinput_process_data(drv, drv->rx_buf, count), and implement a workqueue handler that calls rsinput_init_commands when scheduled.patch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patch-184-207 (1)
184-207:⚠️ Potential issue | 🟠 Major
always-onis read from the parent device, not the LED child node.
htr3212_parse_child_dt()receiveschild(the per-LED OF node) but Line 203 passesdev(the parent I²C device) todevice_property_read_bool("always-on"). The result is the same for every LED registered, so the per-LEDalways-onsemantics implied by the helper name don't actually work — and a parent-levelalways-on(rare on multi-LED controllers) will silently disable suspend/resume for all children.🔧 Suggested fix
- if (!device_property_read_bool(dev, "always-on")) + if (!of_property_read_bool(child, "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patch` around lines 184 - 207, htr3212_parse_child_dt currently calls device_property_read_bool(dev, "always-on") which reads the parent device instead of the per-LED OF child node, so every LED gets the same always-on behavior; change that call to of_property_read_bool(child, "always-on") (i.e., use the child device_node and of_property_read_bool) to honor per-LED "always-on" properties and keep the existing logic that sets cdev->flags |= LED_CORE_SUSPENDRESUME when the per-LED "always-on" is absent.patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch-248-287 (1)
248-287:⚠️ Potential issue | 🟠 Major
prev_statesshould be per-device, not a function-static.
static unsigned long prev_states;(Line 250) is shared across every probedrsinputinstance and persists across module load/unload boundaries via BSS. With more than one gamepad attached the second device sees the first device's previous state and emits spurious key changes. Move it intostruct rsinput_driver.🔧 Suggested fix
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_key_states; }; … - static unsigned long prev_states; unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); - unsigned long current_states = keys, changes; + unsigned long current_states = keys, changes; int i; - bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); + bitmap_xor(&changes, ¤t_states, &drv->prev_key_states, ARRAY_SIZE(keymap)); … - prev_states = keys; + drv->prev_key_states = keys;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch` around lines 248 - 287, The bug is that prev_states is a function-static shared across devices; move it into struct rsinput_driver and update handle_cmd_status to use a per-device bitmap: add a field like unsigned long prev_states[BITS_TO_LONGS(ARRAY_SIZE(keymap))] to struct rsinput_driver, in handle_cmd_status allocate local unsigned long arrays for current_states and changes sized with BITS_TO_LONGS(ARRAY_SIZE(keymap)) (initialize current_states from the 16/32-bit keys value), call bitmap_xor(changes, current_states, drv->prev_states, ARRAY_SIZE(keymap)), iterate for_each_set_bit over changes to report keys, then copy current_states back into drv->prev_states using bitmap_copy(drv->prev_states, current_states, ARRAY_SIZE(keymap)) instead of the existing prev_states = keys assignment; keep all other reporting logic unchanged.patch/kernel/archive/sm8550-7.0/0019-drm-panel-Add-panel-driver-for-Chipone-ICNA35XX-base.patch-120-140 (1)
120-140:⚠️ Potential issue | 🟠 Major
prepare()accesses mode from stale atomic state.
icna35xx_get_current_mode()reads fromconnector->state->crtc->stateduringprepare(), which runs before the new atomic state is committed. In the DRM atomic modeset path, the new mode is in the new atomic state and not yet swapped intocrtc->state. Since bothicna3512_init_sequence()andicna3520_init_sequence()depend on this lookup to select vastly different register sequences for 60 Hz vs 120 Hz operation, a stale mode lookup can silently program incorrect panel timings. The fallbackreturn 0selects the 120 Hz descriptor, which may not match the requested mode.The proper approach is to use
drm_atomic_get_new_crtc_state()to access the committed mode during atomic operations, or migrate to adrm_bridgewith atomic callbacks that receive the atomic state. Panelprepare()is not the right place for mode-dependent initialization; mode-aware setup belongs inenable()or bridge atomic hooks, called after the atomic state is finalized.Applies to both
icna3512_init_sequence()andicna3520_init_sequence().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0019-drm-panel-Add-panel-driver-for-Chipone-ICNA35XX-base.patch` around lines 120 - 140, icna35xx_get_current_mode currently reads connector->state->crtc->state during prepare(), which can return stale atomic state; change the lookup to use drm_atomic_get_new_crtc_state(...) against the incoming atomic state (or otherwise obtain the new crtc_state passed into atomic callbacks) so the chosen index reflects the pending mode, and move mode-dependent register selection out of prepare() into enable() or bridge atomic hooks where the new atomic state is finalized; update callers icna3512_init_sequence() and icna3520_init_sequence() to call icna35xx_get_current_mode from those later, atomic-aware contexts rather than from prepare().
🟡 Minor comments (12)
patch/kernel/archive/sm8550-7.0/0023-Add-Silergy-SY7758-backlight-driver.patch-181-186 (1)
181-186:⚠️ Potential issue | 🟡 MinorMinor:
remove()does not power down the controller.
sy7758_remove()only forces brightness to 0 via the backlight core; the IC remains enabled (register0x01 = 0x85from init). Devm cleanup will release the regmap/backlight, but on devices where the regulator/pins stay live, the controller may sit in its "on" state with brightness clamped to 0. If that is intentional (e.g. handled by DT regulators or pinctrlsleepstate from the AYN DTS), feel free to ignore; otherwise consider an explicit shutdown register write here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0023-Add-Silergy-SY7758-backlight-driver.patch` around lines 181 - 186, sy7758_remove currently only zeros the backlight via backlight_update_status but doesn't power down the IC; update sy7758_remove to fetch the driver state (e.g. struct sy7758 *sy = i2c_get_clientdata(client)), perform a guarded regmap write to the power/control register used in init (write the shutdown value to register 0x01, e.g. regmap_write(sy->regmap, 0x01, 0x00)), handle NULL checks for sy and sy->regmap, then set backlight_dev->props.brightness = 0 and call backlight_update_status(backlight_dev); ensure the order powers down the chip before releasing/devm cleanup so the controller isn’t left enabled.config/sources/families/sm8550.conf-23-27 (1)
23-27:⚠️ Potential issue | 🟡 MinorRegenerate or update edge kernel config header to reflect 7.0.
The
config/kernel/linux-sm8550-edge.configheader still reads# Armbian defconfig generated with 6.18. WithKERNEL_MAJOR_MINORnow"7.0"andKERNELBRANCH='branch:linux-7.0.y', regenerate the edge config against 7.0 or refresh the header comment so maintainers can identify which kernel version produced the defconfig. Regeneration will also surface any CONFIG symbols that were dropped or renamed between 6.18 and 7.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/sm8550.conf` around lines 23 - 27, The edge kernel defconfig header is stale relative to KERNEL_MAJOR_MINOR="7.0" and KERNELBRANCH='branch:linux-7.0.y'; regenerate the edge defconfig against kernel 7.0 (or at minimum update the header comment) so the top-of-file comment and CONFIG symbols match the 7.0 build, and ensure any dropped/renamed CONFIG_* entries are resolved; look for the edge defconfig referenced by the sm8550 edge profile and update the header and contents to reflect kernel 7.0 accordingly.patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch-46-57 (1)
46-57:⚠️ Potential issue | 🟡 Minor
hdmi-pwr-supplyis not part of thehdmi-connectorbinding.The
hdmi-connectorbinding only specifieslabel,type,hpd-gpios,ddc-i2c-bus,cec-*and the standardportgraph.hdmi-pwr-supplywill be silently ignored by the connector driver (and trigger adt_binding_checkwarning). Sincevdd_hdmi_1v8is already pulled in via the LT8912 bridge'svdd-supplyreference, this property is redundant.hdmi-out { compatible = "hdmi-connector"; type = "d"; hpd-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>; - hdmi-pwr-supply = <&vdd_hdmi_1v8>; port {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch` around lines 46 - 57, The hdmi-out node uses the "hdmi-connector" binding which doesn't include hdmi-pwr-supply, so remove the hdmi-pwr-supply = <&vdd_hdmi_1v8>; line from the hdmi-out node (the hdmi-out node and its port/hdmi_con: endpoint remain unchanged); rely on the LT8912 bridge's existing vdd-supply reference (lt8912_out / LT8912 bridge) which already pulls in vdd_hdmi_1v8 to avoid the dt_binding_check warning and redundant property.patch/kernel/archive/sm8550-7.0/0014-Input-edt-ft5x06-add-no_regmap_bulk_read-option.patch-39-48 (1)
39-48:⚠️ Potential issue | 🟡 MinorMinor: avoid sleeping on the final retry; consider backoff for unrecognized errors.
Two small inefficiencies in the retry loop:
- On the 3rd failing attempt with
-ETIMEDOUT/-EAGAIN, the function still callsusleep_range(2000, 4000)and then immediately exits via thetries == 3check. The sleep adds latency to the error return path with no benefit.- For any other error code, the loop spins three times without delay, which can hammer the bus on persistent faults.
♻️ Suggested tighter retry logic
for (tries = 0; tries < 3; tries++) { ret = regmap_read(map, start + off, &v); if (!ret) { dst[off] = (u8)v; break; } - if (ret == -ETIMEDOUT || ret == -EAGAIN) - usleep_range(2000, 4000); + if (tries < 2 && + (ret == -ETIMEDOUT || ret == -EAGAIN)) + usleep_range(2000, 4000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0014-Input-edt-ft5x06-add-no_regmap_bulk_read-option.patch` around lines 39 - 48, The retry loop calling regmap_read should avoid sleeping after the final attempt and should back off on non-retry-specific errors instead of busy-spinning; modify the block around regmap_read so that usleep_range(2000, 4000) is only invoked when tries < 2 (i.e. not after the last retry) and add a small sleep/backoff (e.g. usleep_range with a smaller delay or exponential backoff based on tries) for unexpected ret values before retrying; adjust the logic that currently checks ret == -ETIMEDOUT || ret == -EAGAIN to only decide sleep timing, and ensure the loop still fills dst[off] and breaks on success as it does now (refer to regmap_read, usleep_range, tries, ret, start, off, dst).patch/kernel/archive/sm8550-7.0/0037-arm64-dts-qcom-Add-AYN-Odin2.patch-234-267 (1)
234-267:⚠️ Potential issue | 🟡 MinorRMI4 touchscreen node is missing
vdd-supply.The RMI4 I2C transport driver requests two regulators via
devm_regulator_bulk_get(): "vdd" and "vio". The Odin2 node only declaresvio-supply = <&vreg_l12b_1p8>. Withoutvdd-supply, the driver cannot manage the VDD rail and the touchscreen may fail during suspend/resume or cold-boot probing if the rail is not latched on from the bootloader. Wire the actual VDD rail (e.g., anothervreg_*regulator that feeds the controller) so the kernel can manage it explicitly.Also on this hunk:
- Line 247 has trailing whitespace.
- Line 285 has
reg = <0x48> ;(stray space before;).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0037-arm64-dts-qcom-Add-AYN-Odin2.patch` around lines 234 - 267, The touchscreen@20 RMI4 node only declares vio-supply but the rmi4 driver also needs vdd-supply; add a vdd-supply property to touchscreen@20 (pointing to the board's actual VDD regulator phandle, e.g. replace &vreg_l12b_1p8 or add &vreg_<vdd_regulator_name> as appropriate) so the driver can manage VDD, and keep the existing vio-supply; also remove the trailing whitespace on the pinctrl line (after vio-supply) and fix the stray space before the semicolon in the reg assignment (the "reg = <0x48> ;" occurrence) so formatting is clean.patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch-22-25 (1)
22-25:⚠️ Potential issue | 🟡 MinorDead code:
static_bdaddr/static_macaddrare declared but never referenced.
static const bdaddr_t static_bdaddr(btqca lines 22-25) andstatic const struct macaddr_t static_macaddr(ath12k lines 161-163) are defined but never used anywhere in the patch. Either wire them up as a fallback when the serial-based generation fails, or drop them.Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 22 - 25, The patch introduces unused constants static_bdaddr and static_macaddr; either remove these dead declarations or wire them as fallbacks: update the serial-based address generation code (e.g., the function(s) that call into the serial/EFUSE path for Bluetooth and Wi-Fi address generation) to detect failure and return the static_bdaddr or static_macaddr values as the fallback BD_ADDR/MAC, or simply delete the static const bdaddr_t static_bdaddr and static const struct macaddr_t static_macaddr definitions to eliminate dead code; reference the exact symbols static_bdaddr and static_macaddr when making the change.patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch-99-100 (1)
99-100:⚠️ Potential issue | 🟡 MinorCommented-out
qca_check_bdaddr()and__maybe_unusedmark left dead code in tree.Lines 138-141 contain the original
qca_check_bdaddr()flow as//-commented code, and the function is now annotated__maybe_unused(line 100) because it has no remaining callers. This leaves a permanently dead function plus dead commented logic inbtqca.c.If the new serial-derived BD_ADDR path is the intended permanent behavior, please remove
qca_check_bdaddr()and the commented call entirely. If it's meant to be a fallback (e.g., when the serial number is unavailable or invalid), wire it up explicitly so both paths remain exercised and reviewable.Also applies to: 138-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 99 - 100, The qca_check_bdaddr() function is now unused (marked __maybe_unused) and the original call-flow is left commented out in btqca.c (around the commented lines previously 138-142), leaving dead code; either remove the qca_check_bdaddr() definition and the commented call-site entirely if the serial-derived BD_ADDR path is intended as the permanent behavior, or restore and wire the original check as an explicit fallback by removing the __maybe_unused annotation and reintroducing a clear branch that calls qca_check_bdaddr(hdev, config) when the serial-derived BD_ADDR is missing/invalid so both paths are compiled and exercised (update any callers in btqca.c accordingly and delete stale comment blocks).patch/kernel/archive/sm8550-7.0/0010-gpu-panel-add-Pocket-ACE-panel-driver.patch-51-54 (1)
51-54:⚠️ Potential issue | 🟡 MinorAddress FIXME placeholders before merge.
- Line 52:
// Copyright (c) 2025 FIXME- Line 54:
// Copyright (c) 2013, The Linux Foundation. All rights reserved. (FIXME)- Line 283:
MODULE_AUTHOR("linux-mdss-dsi-panel-driver-generator <fix@me>"); // FIXMEThese leftover generator placeholders should be replaced with real copyright/author attribution.
Also applies to: 283-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0010-gpu-panel-add-Pocket-ACE-panel-driver.patch` around lines 51 - 54, Replace the generator placeholders left in header comments and the MODULE_AUTHOR string: update the comment lines containing "Copyright (c) 2025 FIXME" and the vendor-tree copyright comment with the correct copyright holder(s) and year(s), and change MODULE_AUTHOR("linux-mdss-dsi-panel-driver-generator <fix@me>"); to a real author name and contact (or the vendor/maintainer) so no FIXME/fix@me placeholders remain; locate the occurrences by searching for the top-of-patch header comments and the MODULE_AUTHOR(...) symbol and substitute the real attribution text.patch/kernel/archive/sm8550-7.0/0039-arm64-dts-qcom-Add-AYN-Odin2-Portal.patch-143-213 (1)
143-213:⚠️ Potential issue | 🟡 MinorLED node names contradict their
colorattributes, causing phandle references to point to unexpected colors.The pattern is systematic across all 12 LEDs on each controller:
ledl_b1hascolor = LED_COLOR_ID_RED(node name says blue, but actually red)ledl_g1hascolor = LED_COLOR_ID_GREEN(matches)ledl_r1hascolor = LED_COLOR_ID_BLUE(node name says red, but actually blue)The same pattern repeats for
ledl_b2-b4,ledl_g2-g4,ledl_r2-r4and all right-side LEDs (ledr_*).This causes
multi-ledXnodes (lines 68-122) to wire up LEDs in BGR order instead of RGB when they reference phandles like&ledr_r4(which actually points to a BLUE LED due to the name/color mismatch).Confirm this reflects intentional hardware-specific channel mapping, or rename the LED nodes to match their actual colors to avoid future confusion.
Minor: Line 276 has a stray space before the tab indentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0039-arm64-dts-qcom-Add-AYN-Odin2-Portal.patch` around lines 143 - 213, The LED node names (ledl_b1..ledl_r4 and ledr_*) do not match their color properties (e.g., ledl_b1 has color = LED_COLOR_ID_RED, ledl_r1 has LED_COLOR_ID_BLUE) causing phandles used by the multi-led nodes (multi-ledX) to map BGR instead of RGB; fix by either renaming the LED nodes to reflect their actual color (e.g., rename ledl_b1 -> ledl_r1) or by swapping the color properties to match the node names for each node in the &i2c0 htr3212l block (nodes ledl_b1..ledl_r4 and corresponding ledr_* if present), and also remove the stray space before the tab indentation near the multi-led definitions to normalize formatting.patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch-54-54 (1)
54-54:⚠️ Potential issue | 🟡 MinorDrop the global
#define DEBUG.Forcing
DEBUGfrom a driver source is a debug artifact — it floods the log withdev_dbgfrom this and any header that conditionally enables debug. The follow-up patch0025-input-RSInput-Add-regulator.patchalready removes it, so consider folding the removal into this patch to avoid landing it in a release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch` at line 54, Remove the global "#define DEBUG" from this patch (it forces dev_dbg logging across the tree); instead leave DEBUG undefined here and fold its removal into this patch so the follow-up "0025-input-RSInput-Add-regulator.patch" change is not required; locate the `#define DEBUG` line in the patch/diff and delete it so driver headers and other sources aren’t forced into debug logging.patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch-1397-1410 (1)
1397-1410:⚠️ Potential issue | 🟡 MinorInconsistent label vs. node name for the primary DSI TE suspend state.
The label is
dsi_p_te_suspend(primary) but the node name isdsi-s-te-suspend-state(secondary). The other three TE/PRESENT states are consistent (dsi_p_te_active: dsi-p-te-active-state,dsi_s_te_active: dsi-s-te-active-state,dsi_s_te_suspend: dsi-s-te-suspend-state), so this looks like a copy-paste typo. Two pinctrl entries currently share the namedsi-s-te-suspend-state, which is also a DTC duplicate-name concern.🔧 Suggested fix
- dsi_p_te_suspend: dsi-s-te-suspend-state { + dsi_p_te_suspend: dsi-p-te-suspend-state { pins = "gpio86";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch` around lines 1397 - 1410, Label/node mismatch: the primary DSI TE suspend pinctrl uses label dsi_p_te_suspend but the node is incorrectly named dsi-s-te-suspend-state; rename the node to dsi-p-te-suspend-state to match the label and the naming pattern used by dsi_p_te_active, and ensure no other pinctrl node reuses the same node name to avoid DTC duplicate-name conflicts (verify the nodes dsi_p_te_active, dsi_s_te_active, dsi_s_te_suspend for consistency).patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch-613-614 (1)
613-614:⚠️ Potential issue | 🟡 MinorIndentation: mixed spaces inside
wcn7850-pmu.
bt-enable-gpios = ...(line 614) is indented with spaces while the rest of this node uses tabs. Just a cosmetic nit but inconsistent with the surrounding file style.🔧 Suggested fix
wlan-enable-gpios = <&tlmm 80 GPIO_ACTIVE_HIGH>; - bt-enable-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>; + bt-enable-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch` around lines 613 - 614, In the wcn7850-pmu device tree node the property lines use mixed indentation: change the leading spaces on the bt-enable-gpios line so it uses a tab like the surrounding lines (make the indentation consistent with wlan-enable-gpios and other entries in the wcn7850-pmu node); locate the lines declaring wlan-enable-gpios and bt-enable-gpios and replace the space indentation of bt-enable-gpios with a tab to match file style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a71b2b0-8978-41cb-b933-2879b4cbee53
📒 Files selected for processing (45)
config/sources/families/sm8550.confpatch/kernel/archive/sm8550-7.0/0001-msm-adreno-enable-A32.patchpatch/kernel/archive/sm8550-7.0/0002-qcom-dispcc-sm8550-Fix-disp_cc_mdss_mdp_clk_src.patchpatch/kernel/archive/sm8550-7.0/0003-input-rmi4-add-reset-gpio.patchpatch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patchpatch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patchpatch/kernel/archive/sm8550-7.0/0006-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patchpatch/kernel/archive/sm8550-7.0/0007-mmc-sdhci-msm-Toggle-the-FIFO-write-clock-after-unga.patchpatch/kernel/archive/sm8550-7.0/0008-ASoC-codecs-aw88166-AYN-SM8550-Specific-modification.patchpatch/kernel/archive/sm8550-7.0/0009-arm64-dts-qcom-Added-pmk8550_pwm.patchpatch/kernel/archive/sm8550-7.0/0010-gpu-panel-add-Pocket-ACE-panel-driver.patchpatch/kernel/archive/sm8550-7.0/0012-input-touchscreen-porting-downstream-hynitron-driver.patchpatch/kernel/archive/sm8550-7.0/0013-qcom_battmgr-Set-battery-name.patchpatch/kernel/archive/sm8550-7.0/0014-Input-edt-ft5x06-add-no_regmap_bulk_read-option.patchpatch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patchpatch/kernel/archive/sm8550-7.0/0017-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patchpatch/kernel/archive/sm8550-7.0/0018-drm-panel-Add-panel-driver-for-Xm-Plus-XM91080G-base.patchpatch/kernel/archive/sm8550-7.0/0019-drm-panel-Add-panel-driver-for-Chipone-ICNA35XX-base.patchpatch/kernel/archive/sm8550-7.0/0020-drm-panel-Add-panel-driver-for-DDIC-CH13726A-based-p.patchpatch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patchpatch/kernel/archive/sm8550-7.0/0022-Input-hynitron_cstxxx-specific-patch-for-Odin2-Mini.patchpatch/kernel/archive/sm8550-7.0/0023-Add-Silergy-SY7758-backlight-driver.patchpatch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patchpatch/kernel/archive/sm8550-7.0/0025-input-RSInput-Add-regulator.patchpatch/kernel/archive/sm8550-7.0/0026-drm-msm-remove-DRIVER_SYNCOBJ_TIMELINE.patchpatch/kernel/archive/sm8550-7.0/0027-HACK-fix-usb-boot-hang.patchpatch/kernel/archive/sm8550-7.0/0028-SM8550-Fix-L2-cache-for-CPU2-and-add-cache-sizes.patchpatch/kernel/archive/sm8550-7.0/0029-SM8550-Add-DDR-LLCC-L3-CPU-bandwidth-scaling.patchpatch/kernel/archive/sm8550-7.0/0030-arm64-dts-qcom-sm8550-Update-EAS-properties.patchpatch/kernel/archive/sm8550-7.0/0031-arm64-dts-qcom-sm8550-add-UART15.patchpatch/kernel/archive/sm8550-7.0/0032-drm-panel-Add-Retroid-Pocket-6-panel.patchpatch/kernel/archive/sm8550-7.0/0033-arm64-dts-qcom-sm8550-Describe-GPU_CC-power-plumbing.patchpatch/kernel/archive/sm8550-7.0/0034-interconnect-qcom-sm8550-Enable-QoS-configuration.patchpatch/kernel/archive/sm8550-7.0/0035-dts-qcom-sm8550-add-opp-acd-level.patchpatch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patchpatch/kernel/archive/sm8550-7.0/0037-arm64-dts-qcom-Add-AYN-Odin2.patchpatch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patchpatch/kernel/archive/sm8550-7.0/0039-arm64-dts-qcom-Add-AYN-Odin2-Portal.patchpatch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patchpatch/kernel/archive/sm8550-7.0/0041-ROCKNIX-set-boot-fanspeed.patchpatch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patchpatch/kernel/archive/sm8550-7.0/0043-power-supply-rename-qcom-battmgr-sysfs.patchpatch/kernel/archive/sm8550-7.0/0044-Enable-64-bit-processes-to-use-compat-input-syscalls.patchpatch/kernel/archive/sm8550-7.0/v2_20260314_vladimir_zapolskiy_arm64_dts_qcom_sm8x50_enable_uhs_i_sdr50_and_sdr104_sd_card_modes.patchpatch/kernel/archive/sm8550-7.0/v5_20251120_quic_utiwari_crypto_qce_add_runtime_pm_and_interconnect_bandwidth_scaling_support.patch
| + error = serdev_device_open(serdev); | ||
| + if (error) | ||
| + return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n"); | ||
| + | ||
| + drv->serdev = serdev; | ||
| + drv->sequence_number = 0; | ||
| + | ||
| + serdev_device_set_drvdata(serdev, drv); | ||
| + | ||
| + error = serdev_device_set_baudrate(serdev, 115200); | ||
| + if (error < 0) | ||
| + return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); | ||
| + | ||
| + serdev_device_set_flow_control(serdev, false); | ||
| + | ||
| + drv->input = devm_input_allocate_device(&serdev->dev); | ||
| + if (!drv->input) | ||
| + return -ENOMEM; | ||
| + | ||
| + drv->input->phys = "rsinput-gamepad/input0"; | ||
| + | ||
| + error = device_property_read_string(&serdev->dev, "gamepad-name", &drv->input->name); | ||
| + if (error) { | ||
| + drv->input->name = "RSInput Gamepad"; | ||
| + } | ||
| + | ||
| + device_property_read_u32(&serdev->dev, "gamepad-bus", &gamepad_bus); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-vid", &gamepad_vid); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-pid", &gamepad_pid); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-rev", &gamepad_rev); | ||
| + | ||
| + drv->input->id.bustype = (u16)gamepad_bus; | ||
| + drv->input->id.vendor = (u16)gamepad_vid; | ||
| + drv->input->id.product = (u16)gamepad_pid; | ||
| + drv->input->id.version = (u16)gamepad_rev; | ||
| + | ||
| + __set_bit(EV_KEY, drv->input->evbit); | ||
| + for (int i = 0; i < ARRAY_SIZE(keymap); i++) | ||
| + input_set_capability(drv->input, EV_KEY, keymap[i]); | ||
| + | ||
| + __set_bit(EV_ABS, drv->input->evbit); | ||
| + for (int i = ABS_X; i <= ABS_RZ; i++) | ||
| + input_set_abs_params(drv->input, i, -0x580, 0x580, | ||
| + 0, 0); | ||
| + | ||
| + input_set_abs_params(drv->input, ABS_Z, 0, 1830, 0, 30); | ||
| + input_set_abs_params(drv->input, ABS_RZ, 0, 1830, 0, 30); | ||
| + | ||
| + error = input_register_device(drv->input); | ||
| + if (error) | ||
| + return error; | ||
| + | ||
| + serdev_device_set_client_ops(serdev, &rsinput_rx_ops); | ||
| + | ||
| + error = rsinput_init_commands(drv); | ||
| + if (error < 0) { | ||
| + serdev_device_close(serdev); | ||
| + return error; | ||
| + } |
There was a problem hiding this comment.
Probe ordering and error-path leaks around serdev_device_open().
Two related defects:
serdev_device_open()is called beforeserdev_device_set_client_ops()(line 468), so any bytes the MCU pushes during this window have noreceive_bufregistered yet (anddrvis set withserdev_device_set_drvdataonly afterwards on line 423). RX is silently dropped, and theserdev_device_set_baudrate(115200)on line 425 also runs before client ops are set.- Once
serdev_device_open()succeeds (line 416), every subsequent error return must close the device. As written,serdev_device_set_baudratefailure (line 427),devm_input_allocate_devicereturning NULL (line 433), andinput_register_devicefailing (line 466) all return withoutserdev_device_close(), leaking the open serdev.
🔧 Recommended ordering / cleanup
+ drv->serdev = serdev;
+ drv->sequence_number = 0;
+ serdev_device_set_drvdata(serdev, drv);
+ serdev_device_set_client_ops(serdev, &rsinput_rx_ops);
+
error = serdev_device_open(serdev);
if (error)
return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n");
- drv->serdev = serdev;
- drv->sequence_number = 0;
-
- serdev_device_set_drvdata(serdev, drv);
-
error = serdev_device_set_baudrate(serdev, 115200);
- if (error < 0)
- return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n");
+ if (error < 0) {
+ serdev_device_close(serdev);
+ return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n");
+ }
serdev_device_set_flow_control(serdev, false);
drv->input = devm_input_allocate_device(&serdev->dev);
- if (!drv->input)
- return -ENOMEM;
+ if (!drv->input) {
+ serdev_device_close(serdev);
+ return -ENOMEM;
+ }
…
error = input_register_device(drv->input);
- if (error)
- return error;
-
- serdev_device_set_client_ops(serdev, &rsinput_rx_ops);
+ if (error) {
+ serdev_device_close(serdev);
+ return error;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 416 - 474, The serdev client ops and driver data are being set
after serdev_device_open(), which allows RX to be dropped, and multiple error
returns after a successful serdev_device_open() leak the open device; move
serdev_device_set_client_ops(serdev, &rsinput_rx_ops) and
serdev_device_set_drvdata(serdev, drv) so they are set before calling
serdev_device_open(), or alternatively call serdev_device_open() only after
those are set, and ensure every path that returns after serdev_device_open()
succeeds (e.g., failures in serdev_device_set_baudrate,
devm_input_allocate_device, input_register_device, rsinput_init_commands) calls
serdev_device_close(serdev) to clean up the opened device.
| + priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | ||
| + if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) | ||
| + return -EPROBE_DEFER; | ||
| + | ||
| + priv->vdd = devm_regulator_get(dev, "vdd"); | ||
| + if (IS_ERR(priv->vdd)) { | ||
| + ret = PTR_ERR(priv->vdd); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + ret = regulator_enable(priv->vdd); | ||
| + if (ret < 0) { | ||
| + return ret; | ||
| + } | ||
| + | ||
| + gpiod_set_value_cansleep(priv->sdb, 1); |
There was a problem hiding this comment.
SDB GPIO acquisition leaks ERR_PTR for non--EPROBE_DEFER failures, then dereferences it.
devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) returns an ERR_PTR on any failure; the code only handles -EPROBE_DEFER. For any other error (e.g. -ENOENT if not described in DT, -EBUSY, etc.) priv->sdb stays as the error pointer, the function continues into regulator_enable() and then on Line 303 calls gpiod_set_value_cansleep(priv->sdb, 1), which dereferences a non-NULL invalid pointer → kernel oops at probe time. The same pointer is later used in htr3212_remove() (Line 338).
Either treat sdb as truly required (handle all errors) or as optional (devm_gpiod_get_optional).
🔧 Suggested fix (treat sdb as optional, like the SN3112 driver does)
- priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH);
- if (PTR_ERR(priv->sdb) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
+ priv->sdb = devm_gpiod_get_optional(dev, "sdb", GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->sdb))
+ return dev_err_probe(dev, PTR_ERR(priv->sdb),
+ "failed to get sdb GPIO\n");…and guard the gpiod_set_value_cansleep() calls with if (priv->sdb) in both probe and remove.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patch`
around lines 288 - 303, priv->sdb is left as an ERR_PTR when devm_gpiod_get(dev,
"sdb", GPIOD_OUT_HIGH) fails with errors other than -EPROBE_DEFER, then later
dereferenced by gpiod_set_value_cansleep and in htr3212_remove, causing an oops;
fix by acquiring the GPIO optionally (use devm_gpiod_get_optional or check
IS_ERR and return non-zero immediately) and then guard all uses of priv->sdb
(the gpiod_set_value_cansleep() call in probe and any accesses in
htr3212_remove()) with if (priv->sdb) so you never dereference an ERR_PTR.
| +struct sn3112 { | ||
| + struct device *pdev; | ||
| + struct regmap *regmap; | ||
| + struct mutex lock; | ||
| + struct regulator *vdd; | ||
| + uint8_t pwm_val[SN3112_CHANNELS]; | ||
| + uint8_t pwm_en_reg[3]; | ||
| + bool pwm_en[SN3112_CHANNELS]; | ||
| +#if IS_ENABLED(CONFIG_GPIOLIB) | ||
| + struct gpio_desc *sdb; | ||
| +#endif | ||
| +}; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Linux kernel set_bit clear_bit alignment requirements unsigned long
💡 Result:
Top web search findings:
- include/linux/bitops.h at master · torvalds/linux: /* * Many architecture-specific non-atomic bitops contain inline asm code and due * to that the compiler can't optimize them to compile-time expressions or * constants. In contrary, generic_() helpers are defined in pure C and * compilers optimize them just well. * Therefore, to make
unsigned long foo = 0; __set_bit(BAR, &foo)effectively * equal tounsigned long foo = BIT(BAR), pick the generic C alternative when * the arguments can be resolved at compile time. That expression itself is a * constant and doesn't bring any functional changes to the rest of cases. * The casts touintptr_tare needed to mitigate-Waddresswarnings when * passing a bitmap from .bss or .data (->!!addris always true). /#definebitop(op, nr, addr) \ ((__builtin_constant_p(nr) && \ __builtin_constant_p((uintptr_t)(addr) != (uintptr_t)NULL) && \ (uintptr_t)(addr) != (uintptr_t)NULL && \ __builtin_constant_p((const unsigned long )(addr))) ? \ const##op(nr, addr) : op(nr, addr)) [...] / * The following macros are non-atomic versions of their non-underscored * counterparts. /#define__set_bit(nr, addr) bitop(___set_bit, nr, addr)#define__clear_bit(nr, addr) bitop(___clear_bit, nr, addr)#define__change_bit(nr, addr) bitop(___change_bit, nr, addr)#define__test_and_set_bit(nr, addr) bitop(___test_and_set_bit, nr, addr)#define__test_and_clear_bit(nr, addr) bitop(___test_and_clear_bit, nr, addr)#define__test_and_change_bit(nr, addr) bitop(___test_and_change_bit, nr, addr) [...] __check_bitop_pr(__set_bit); __check_bitop_pr(__clear_bit); __check_bitop_pr(__change_bit); __check_bitop_pr(__test_and_set_bit); __check_bitop_pr(__test_and_clear_bit); __check_bitop_pr(__test_and_change_bit); __check_bitop_pr(test_bit); __ [...] bitop_pr(test_bit_acquire); [...] /* * assign_bit - Assign value to a bit in memory *@nr: the bit to set *@addr: the address to start counting from *@value: the value to assign /#defineassign_bit(nr, addr, value) \ ((value) ? set_bit((nr), (addr)) : clear_bit((nr), (addr)))#define__assign_bit(nr, addr, value) \ ((value) ? __set_bit((nr), (addr)) : __clear_bit((nr), (addr))) [...] /* * ptr_set_bit - Set bit in a pointer's value *@nr: the bit to set *@addr: the address of the pointer variable * * Example: * void p = foo(); * ptr_set_bit(bit, &p); /#define__ptr_set_bit(nr, addr) \ ({ \ typecheck_pointer((addr)); \ set_bit(nr, (unsigned long )(addr)); \ }) [...] ifndef set_mask_bits [...]#defineset_mask_bits(ptr, mask, bits) \ ({ \ const typeof((ptr)) mask = (mask), bits = (bits); \ typeof((ptr)) old, new__; \ \ old__ = READ_ONCE((ptr)); \ do { \ new__ = (old__ & ~mask__) | bits__; \ } while (!try_cmpxchg(ptr, &old__, new__)); \ \ old__; \ })#endif - arch/x86/include/asm/bitops.h at 16f73eb02d7e1765ccab3d2018e0bd98eb93d973 · torvalds/linux: /** * set_bit - Atomically set a bit in memory *
@nr: the bit to set *@addr: the address to start counting from * * This function is atomic and may not be reordered. See __set_bit() * if you do not require the atomic guarantees. * * Note: there are no guarantees that this function will not be reordered * on non x86 architectures, so if you are writing portable code, * make sure not to rely on its reordering guarantees. * * Note that@nrmay be almost arbitrarily large; this function is not * restricted to acting on a single-word quantity. / static __always_inline void set_bit(long nr, volatile unsigned long addr) { if (IS_IMMEDIATE(nr)) { asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR(nr, addr) : "iq" ((u8)CONST_MASK(nr)) : "memory"); } else { asm volatile(LOCK_PREFIX "bts %1,%0" : BITOP_ADDR(addr) : "Ir" (nr) : "memory"); } } [...] / * __set_bit - Set a bit in memory *@nr: the bit to set *@addr: the address to start counting from * * Unlike set_bit(), this function is non-atomic and may be reordered. * If it's called on the same region of memory simultaneously, the effect * may be that only one operation succeeds. / static __always_inline void __set_bit(long nr, volatile unsigned long addr) { asm volatile("bts %1,%0" : ADDR : "Ir" (nr) : "memory"); } [...] / * test_and_set_bit - Set a bit and return its old value *@nr: Bit to set *@addr: Address to count from * * This operation is atomic and cannot be reordered. * It also implies a memory barrier. / static __always_inline bool test_and_set_bit(long nr, volatile unsigned long addr) { GEN_BINARY_RMWcc(LOCK_PREFIX "bts", addr, "Ir", nr, "%0", c [...] / * [...] test_and_set_bit - Set a bit and return its old value *@nr: Bit to set *@addr: Address to count from * * This operation is non-atomic and can be reordered. * If two examples of this operation race, one can appear to succeed * but [...] fail. You must protect multiple accesses with a lock [...] / static [...] bit(long nr, volatile unsigned [...] t" [...] ) [...] ) (old [...] ), ADDR : "Ir" (nr [...] / * ffs - find first set bit in word *@x: the word to search * * This is defined the same way as the libc and compiler builtin ffs * routines, therefore differs in spirit from [...] other bitops. * * ffs(value) returns 0 if value is 0 or the position of the first * set bit if value is nonzero. The first (least significant) bit * is at position 1. / static __always_inline int ffs( [...] /* * fls - find [...] set bit in word *@x: the word to search * * This is defined in a similar way as the libc and compiler builtin * ffs, but returns the position of the most significant set bit. * * fls(value) [...] 0 if value is [...] 0 or the position of the last * set bit if value is nonzero. The [...] (most significant) bit is * at position 32 [...] / static _always [...] int fls( [...] /* * fls64 - find last set bit in a 64-bit word *@x: the word to search * * This is defined in a similar way as the libc and compiler builtin * ffsll, but returns the position of the most significant set bit. * * fls64(value) returns 0 if value is 0 or the position of the last * set bit if value is nonzero. The last (most significant) bit is * at position 64. /#ifdefCONFIG_X86_64 [...] int fls64(__u64 x) [...] int [...] pos = -1; / * AMD [...] 4 says BSRQ won [...] t clobber [...] dest reg if x==0; Intel [...] 4 says the * dest reg is undefined if x==0, but their CPU architect says its * [...] is written to [...] it to the same as before [...] */ asm("bsrq %1,%q0" [...] ) - include/linux/bitops.h - kernel/common - Git at Google: | /* SPDX-License-Identifier: GPL-2.0 / | | --- | |
#ifndef_LINUX_BITOPS_H | |#define_LINUX_BITOPS_H | |#include| |#include| | / Set bits in the first 'n' bytes when loaded from memory / | |#ifdef__LITTLE_ENDIAN | | # define aligned_byte_mask(n) ((1UL << 8(n))-1) | |#else| | # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) | |#endif| |#defineBITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) | |#defineBITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) | |#defineBITS_TO_BYTES(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(char)) | [...] | extern unsigned int sw_hweight8(unsigned int w); | [...] | extern unsigned int sw_hweight16(unsigned int w); | [...] | extern unsigned int sw_hweight32(unsigned int w); | | extern unsigned long sw_hweight64(u64 w); | [...] /* | [...] here because some architectures need generic_ffs/fls in | | * scope | | / [...] |#definefor_each_set_bit(bit, addr, size) \ | | for ((bit) = find_first_bit((addr), (size)); \ | | (bit) < (size); \ | | (bit) = find_next_bit((addr), (size), (bit) + 1)) | | / same as for_each_set_bit() but use bit as value to start with / | |#definefor_each_set_bit_from(bit, addr, size) \ | | for ((bit) = find_next_bit((addr), (size), (bit)); \ | | (bit) < (size); \ | | (bit) = find_next_bit((addr), (size), (bit) + 1)) | [...] fls( [...] / We could be slightly more clever with -1 here [...] | } [...] | /** | | * assign_bit - Assign value to a bit in memory | | *@nr: the bit to set | | *@addr: the address to start counting from | | *@value: the value to assign | | */ | | static always_inline void assign_bit(long nr, volatile unsigned long addr, | | bool value) | | { | | if (value) | | set_bit(nr, addr); | | else | | clear_bit(nr, addr); | | } | [...] | static always_inline void assign_bit(long nr, volatile unsigned long addr, | | bool value) | | { | | if (value) | | __set_bit(nr, addr); | | else | | __clear_bit(nr, addr); | | } | [...]#ifdefKERNEL | [...] |#ifndefset_mask_bits | |#defineset_mask_bits(ptr, mask, bits) \ | | ({ \ | | const typeof((ptr)) mask = (mask), bits = (bits); \ | | typeof((ptr)) old, new; \ | | \ | | do { \ | | old = READ_ONCE(*(ptr)); \ | | new = (old & ~mask) | bits__; \ | | } while (cmpxchg(ptr, old__, new__) != old__); \ | | \ | | old__; \ | | }) | [...]#endif| - tools/include/linux/bitops.h at master · torvalds/linux: # File: torvalds/linux/tools/include/linux/bitops.h - Repository: torvalds/linux | Linux kernel source tree | 227K stars | C - Branch: master
h /* SPDX-License-Identifier: GPL-2.0 */ `#ifndef` _TOOLS_LINUX_BITOPS_H_ `#define` _TOOLS_LINUX_BITOPS_H_ `#include` <asm/types.h> `#include` <limits.h> `#ifndef` __WORDSIZE `#define` __WORDSIZE (__SIZEOF_LONG__ * 8) `#endif` `#ifndef` BITS_PER_LONG # define BITS_PER_LONG __WORDSIZE `#endif` `#include` <linux/bits.h> `#include` <linux/compiler.h> `#define` BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) `#define` BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) `#define` BITS_TO_U64(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) `#define` BITS_TO_U32(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) `#define` BITS_TO_BYTES(nr) DIV_ROUND_UP(nr, BITS_PER_TYPE(char)) `#define` BYTES_TO_BITS(nb) ((nb) * BITS_PER_BYTE) extern unsigned int __sw_hweight8(unsigned int w); extern unsigned int __sw_hweight16(unsigned int w); extern unsigned int __sw_hweight32(unsigned int w); extern unsigned long __sw_hweight64(__u64 w); /* * Defined here because those may be needed by architecture-specific static * inlines. */ `#define` bitop(op, nr, addr) \ op(nr, addr) `#define` __set_bit(nr, addr) bitop(___set_bit, nr, addr) `#define` __clear_bit(nr, addr) bitop(___clear_bit, nr, addr) `#define` __change_bit(nr, addr) bitop(___change_bit, nr, addr) `#define` __test_and_set_bit(nr, addr) bitop(___test_and_set_bit, nr, addr) `#define` __test_and_clear_bit(nr, addr) bitop(___test_and_clear_bit, nr, addr) `#define` __test_and_change_bit(nr, addr) bitop(___test_and_change_bit, nr, addr) `#define` test_bit(nr, addr) bitop(_test_bit, nr, addr) /* * Include this here because some architectures need generic_ffs/fls in * scope * * XXX: this needs to be asm/bitops.h, when we get to per arch optimizations */ `#include` <asm-generic/bitops.h> `#define` for_each_set_bit(bit, addr, size) \ for ((bit) = find_first_bit((addr), (size)); \ (bit) < (size); \ (bit) = find_next_bit((addr), (size), (bit) + 1)) `#define` for_each_clear_bit(bit, addr, size) \ for ((bit) = find_first_zero_bit((addr), (size)); \ (bit) < (size); \ (bit) = find_next_zero_bit((addr), (size), (bit) + 1)) /* same as for_each_set_bit() but use bit as value to start with */ `#define` for_each_set_bit_from(bit, addr, size) \ for ((bit) = find_next_bit((addr), (size), (bit)); \ (bit) < (size); \ (bit) = find_next_bit((addr), (size), (bit) + 1)) static inline unsigned long hweight_long(unsigned long w) { return sizeof(w) == 4 ? hweight32(w) : hweight64(w); } static inline unsigned int fls_long(unsigned long l) { if (sizeof(l) == 4) return fls(l); return fls64(l); } /** * rol32 - rotate a 32-bit value left * `@word`: value to rotate * `@shift`: bits to roll */ static inline __u32 rol32(__u32 word, unsigned int shift) { return (word << shift) | (word >> ((-shift) & 31)); } /** * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit * `@value`: value to sign extend * `@index`: 0 based bit index (0<=index<64) to sign bit */ static __always_inline __s64 sign_extend64(__u64 value, int index) { __u8 shift = 63 - index; return (__s64)(value << shift) >> shift; } `#endif`
Citations:
- 1: https://github.com/torvalds/linux/blob/master/include/linux/bitops.h
- 2: https://github.com/torvalds/linux/blob/16f73eb02d7e1765ccab3d2018e0bd98eb93d973/arch/x86/include/asm/bitops.h
- 3: https://android.googlesource.com/kernel/common/+/c6d60ce1de85be87257578570cde30a81de264ae/include/linux/bitops.h
- 4: https://github.com/torvalds/linux/blob/master/tools/include/linux/bitops.h
🏁 Script executed:
find . -name "*0016*pwm*SN3112*" -o -name "0016*" | grep -i pwm | head -20Repository: armbian/build
Length of output: 135
🏁 Script executed:
fd "0016.*pwm" --type fRepository: armbian/build
Length of output: 133
🏁 Script executed:
find . -path "*sm8550-7.0*" -name "*0016*" -type fRepository: armbian/build
Length of output: 135
🏁 Script executed:
cat -n patch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patch | head -150Repository: armbian/build
Length of output: 5576
🏁 Script executed:
# Verify struct field adjacency and potential clobbering
cat -n patch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patch | sed -n '80,95p'Repository: armbian/build
Length of output: 541
set_bit/clear_bit on a uint8_t[3] cast to unsigned long * causes buffer overflow and data corruption.
pwm_en_reg is declared as uint8_t pwm_en_reg[3] (3 bytes). Lines 132/134 pass (ulong *)&priv->pwm_en_reg[reg] to the atomic set_bit/clear_bit helpers, which operate on unsigned long-sized words. On arm64, these helpers execute 32-bit or 64-bit load-exclusive/store-exclusive sequences that read/write 4–8 bytes starting from the address—overflowing into the adjacent pwm_en[] field and corrupting data. Alignment is also incorrect; uint8_t elements have no guarantee of unsigned long alignment.
The data is mutex-protected and not visible to other CPUs, so atomic operations are not needed. Replace with plain bitwise operations on the byte:
🔧 Suggested fix
if (enabled)
- set_bit(bit, (ulong *)&priv->pwm_en_reg[reg]);
+ priv->pwm_en_reg[reg] |= BIT(bit);
else
- clear_bit(bit, (ulong *)&priv->pwm_en_reg[reg]);
+ priv->pwm_en_reg[reg] &= ~BIT(bit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patch`
around lines 80 - 91, The code uses set_bit/clear_bit on elements of
priv->pwm_en_reg (declared uint8_t pwm_en_reg[3]) by casting to unsigned long*,
causing buffer overflow and alignment issues; replace those atomic helper calls
with plain byte-wise bit operations on priv->pwm_en_reg[reg] (e.g.,
priv->pwm_en_reg[reg] |= BIT(bit); and priv->pwm_en_reg[reg] &= ~BIT(bit);) and
remove the (ulong *) casts, ensuring modifications remain protected by the
existing struct sn3112.lock mutex (use mutex_lock/mutex_unlock around the byte
writes if not already done) and leave pwm_en[] updates untouched.
| +static void ch13726a_reset(struct ch13726a_panel *ctx) | ||
| +{ | ||
| + gpiod_set_value_cansleep(ctx->reset_gpio, 1); | ||
| + usleep_range(10000, 11000); | ||
| + gpiod_set_value_cansleep(ctx->reset_gpio, 0); | ||
| + usleep_range(10000, 11000); | ||
| + gpiod_set_value_cansleep(ctx->reset_gpio, 1); | ||
| + usleep_range(10000, 11000); | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look up reset-gpios polarity for ch13726a panel in the new sm8550-7.0 DTS files
rg -nP --type=dts -C2 'compatible\s*=\s*"ch13726a'
rg -nP -C5 'compatible\s*=\s*"ch13726a'Repository: armbian/build
Length of output: 17056
🏁 Script executed:
# Search for reset-gpios property in the Thor DTS patch
rg -n "reset-gpio|reset_gpio" patch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patch -C 3
# Also look at the full panel node in the Thor patch
rg -n -A 20 'panel@0 {' patch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patch | head -60Repository: armbian/build
Length of output: 940
Reset sequence ends with reset GPIO asserted; inconsistent with DSI command transmission.
The ch13726a_reset() function sets the reset GPIO 1 → 0 → 1 and returns with the line at logical 1. With GPIO_ACTIVE_HIGH (as specified in the AYN Thor DTS at reset-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>), a value of 1 means the GPIO is in its active (asserted) state, leaving the panel held in reset. Immediately after this function returns, ch13726a_on() is called to send MIPI DSI commands, which cannot work while the panel is held in reset.
Other panel drivers in the same patch (td4328_reset, xm91080g_reset, ar06_4inch_reset) all end their reset sequences with the GPIO deasserted (0), and the error path in ch13726a_prepare() (line 149) also sets the GPIO to 0 for release.
The reset sequence should end with gpiod_set_value_cansleep(ctx->reset_gpio, 0) to release the panel from reset before DSI commands are transmitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0020-drm-panel-Add-panel-driver-for-DDIC-CH13726A-based-p.patch`
around lines 89 - 97, The ch13726a_reset() routine currently toggles reset GPIO
1→0→1 and leaves it asserted (logical 1), preventing DSI commands in
ch13726a_on(); change the final gpio state to deassert the line by ensuring the
function ends with gpiod_set_value_cansleep(ctx->reset_gpio, 0) so the panel is
released from reset before command transmission; mirror the behavior used in
td4328_reset/xm91080g_reset/ar06_4inch_reset and the error-path in
ch13726a_prepare() to keep reset semantics consistent.
| + miniled->enable = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH); | ||
| + if (PTR_ERR(miniled->enable) == -EPROBE_DEFER){ | ||
| + return -EPROBE_DEFER; | ||
| + } | ||
| + | ||
| + gpiod_set_value_cansleep(miniled->enable, 1); | ||
| + usleep_range(10000, 11000); |
There was a problem hiding this comment.
Missing IS_ERR check on devm_gpiod_get() return value.
devm_gpiod_get() returns an ERR_PTR on failure. The code only checks for -EPROBE_DEFER; for any other error (e.g., -ENOENT, -EINVAL), miniled->enable will hold an error pointer that is then dereferenced by gpiod_set_value_cansleep() on line 162, causing a kernel oops.
🐛 Proposed fix
- miniled->enable = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
- if (PTR_ERR(miniled->enable) == -EPROBE_DEFER){
- return -EPROBE_DEFER;
- }
+ miniled->enable = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
+ if (IS_ERR(miniled->enable))
+ return dev_err_probe(&client->dev, PTR_ERR(miniled->enable),
+ "Failed to get enable GPIO\n");📝 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.
| + miniled->enable = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH); | |
| + if (PTR_ERR(miniled->enable) == -EPROBE_DEFER){ | |
| + return -EPROBE_DEFER; | |
| + } | |
| + | |
| + gpiod_set_value_cansleep(miniled->enable, 1); | |
| + usleep_range(10000, 11000); | |
| miniled->enable = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH); | |
| if (IS_ERR(miniled->enable)) | |
| return dev_err_probe(&client->dev, PTR_ERR(miniled->enable), | |
| "Failed to get enable GPIO\n"); | |
| gpiod_set_value_cansleep(miniled->enable, 1); | |
| usleep_range(10000, 11000); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patch`
around lines 157 - 163, devm_gpiod_get() can return ERR_PTRs and the code
currently only handles -EPROBE_DEFER; change the logic around miniled->enable to
use IS_ERR(miniled->enable) and handle the error: if PTR_ERR(miniled->enable) ==
-EPROBE_DEFER return -EPROBE_DEFER, otherwise return PTR_ERR(miniled->enable)
(or appropriate errno) before calling gpiod_set_value_cansleep; ensure the
IS_ERR check precedes any use of miniled->enable to avoid dereferencing an error
pointer.
| + data->reset_gpio[1] = devm_gpiod_get_index(dev, "reset", 1, GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(data->reset_gpio[1])) | ||
| + dev_warn(dev, "failed to get second reset GPIO\n"); |
There was a problem hiding this comment.
Critical: secondary reset GPIO is left as ERR_PTR and later dereferenced.
The second reset GPIO is fetched with the non-optional devm_gpiod_get_index. On failure (i.e., DT only describes one reset GPIO, the common case), the code only logs a warning but does not clear data->reset_gpio[1]:
data->reset_gpio[1] = devm_gpiod_get_index(dev, "reset", 1, GPIOD_OUT_HIGH);
if (IS_ERR(data->reset_gpio[1]))
dev_warn(dev, "failed to get second reset GPIO\n");Then sgm3804_enable/sgm3804_disable do:
if (data->reset_gpio[1])
gpiod_set_value_cansleep(data->reset_gpio[1], 1);Since IS_ERR(p) is non-NULL, the guard is always true and gpiod_set_value_cansleep() is called on an ERR_PTR, which will dereference a kernel poison pointer (e.g., (void *)-ENOENT) and oops the first time the regulator is enabled on any board with only one reset GPIO.
🛠️ Suggested fix — use the optional variant
- data->reset_gpio[1] = devm_gpiod_get_index(dev, "reset", 1, GPIOD_OUT_HIGH);
- if (IS_ERR(data->reset_gpio[1]))
- dev_warn(dev, "failed to get second reset GPIO\n");
+ data->reset_gpio[1] = devm_gpiod_get_index_optional(dev, "reset", 1,
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(data->reset_gpio[1]))
+ return dev_err_probe(dev, PTR_ERR(data->reset_gpio[1]),
+ "failed to get second reset GPIO\n");Also applies to: 76-81, 99-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patch`
around lines 159 - 161, The secondary reset GPIO is obtained with
devm_gpiod_get_index and left as an ERR_PTR, which later causes
gpiod_set_value_cansleep to dereference it in sgm3804_enable/sgm3804_disable;
change the acquisition to the optional variant (devm_gpiod_get_index_optional)
or explicitly set data->reset_gpio[1] = NULL on IS_ERR, and update all similar
occurrences (the other reset GPIO fetches in the same patch) so that later
checks use a true NULL test before calling gpiod_set_value_cansleep; ensure you
modify the calls where data->reset_gpio[1] is assigned and the corresponding
guards in sgm3804_enable/sdm3804_disable to avoid using ERR_PTR.
| + vdd_bl_5v0: vdd-bl-5v0-regulator { | ||
| + compatible = "regulator-fixed"; | ||
| + regulator-name = "vdd_bl_5v0"; | ||
| + | ||
| + regulator-min-microvolt = <5000000>; | ||
| + regulator-max-microvolt = <5000000>; | ||
| + | ||
| + gpio = <&tlmm 55 GPIO_ACTIVE_HIGH>; | ||
| + enable-active-high; | ||
| + }; |
There was a problem hiding this comment.
GPIO 55 is claimed by two consumers — backlight will fail to probe.
Both vdd_bl_5v0 (fixed regulator at lines 70‑79) and the backlight@46 node (enable-gpios at line 126) request <&tlmm 55 GPIO_ACTIVE_HIGH> exclusively. Since &display_panel consumes vdd_bl_5v0 via blvdd-supply, both drivers will try to gpiod_get() GPIO 55, and whichever probes second will get -EBUSY. In practice this will leave either the panel power chain or the backlight controller un-initialized.
Pick one owner of the GPIO:
- If the backlight chip itself drives the rail through its own enable pin, drop the
vdd_bl_5v0fixed regulator and removeblvdd-supplyfrom&display_panel(or wire it through the backlight node instead). - If the rail must be a separate regulator, remove
enable-gpiosfrombacklight@46and have the panel/backlight referencevdd_bl_5v0as a supply.
Also applies to: 122-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch`
around lines 70 - 79, GPIO 55 is being requested by both the fixed regulator
node vdd_bl_5v0 and the backlight node backlight@46 via enable-gpios, causing
probe conflicts; decide one owner and modify the DTS accordingly: either remove
the vdd_bl_5v0 node and delete blvdd-supply from &display_panel (or route the
supply via the backlight node) if the backlight chip should drive the rail, or
keep vdd_bl_5v0 as the regulator and remove enable-gpios from backlight@46 and
make the panel/backlight reference vdd_bl_5v0 via blvdd-supply so only the
regulator controls GPIO 55; update both the vdd_bl_5v0 node (lines 70-79) and
backlight@46 (lines ~122-130) to reflect the chosen single owner.
| { | ||
| - if (in_compat_syscall()) { | ||
| + if (current->compat_input || (in_compat_syscall() && !COMPAT_USE_64BIT_TIME)) { | ||
| struct ff_effect_compat *compat_effect; | ||
|
|
||
| if (size != sizeof(struct ff_effect_compat)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Linux kernel struct ff_effect_compat COMPAT_USE_64BIT_TIME input_ff_effect_from_user
💡 Result:
The Linux kernel provides 32-bit compatibility support for the input subsystem in drivers/input/input-compat.[ch], including handling of force feedback effects via struct ff_effect_compat. struct ff_effect_compat is defined in drivers/input/input-compat.h as a 32-bit compatible version of struct ff_effect (from include/uapi/linux/input.h). Key differences include using compat_ulong_t and compat_uptr_t for pointers and sizes in ff_periodic_effect_compat (e.g., custom_len and custom_data), while other fields match ff_effect. The function input_ff_effect_from_user(const char user *buffer, size_t size, struct ff_effect *effect) copies a user-space ff_effect into kernel space, handling compatibility: if (current->compat_input || (in_compat_syscall && !COMPAT_USE_64BIT_TIME)) { struct ff_effect_compat *compat_effect; if (size != sizeof(struct ff_effect_compat)) return -EINVAL; compat_effect = (struct ff_effect_compat )effect; / copy_from_user(compat_effect, buffer, sizeof(*compat_effect)); / / convert compat_effect -> effect / } else { / direct copy_from_user(effect, buffer, size); */ } COMPAT_USE_64BIT_TIME is an architecture-specific macro (defined in asm/compat.h, e.g., for x32 ABI on x86_64) indicating if 32-bit processes use 64-bit time_t. When true, compat handling for time fields (like input_event timestamps) is skipped, but for ff_effect, compat conversion occurs unconditionally during in_compat_syscall unless overridden by current->compat_input (a prctl opt-in for 64-bit processes). This ensures 32-bit userspace can upload ff_effects via ioctl(EVIOCSFF) on 64-bit kernels, with the kernel converting between struct ff_effect_compat and struct ff_effect for drivers. Sources confirm this behavior across kernel versions up to recent (e.g., v6.19-rc8). The patch in result 3 (not mainline as of 2026-04-25) adds prctl for 64-bit opt-in but uses !COMPAT_USE_64BIT_TIME consistently.
Citations:
- 1: slp/linux@897560f
- 2: https://codebrowser.dev/linux/linux/drivers/input/input-compat.h.html
- 3: https://lkml.iu.edu/1202.2/01387.html
🏁 Script executed:
find . -name "*0044-Enable-64-bit-processes-to-use-compat-input-syscalls.patch" -type fRepository: armbian/build
Length of output: 155
🏁 Script executed:
cat ./patch/kernel/archive/sm8550-7.0/0044-Enable-64-bit-processes-to-use-compat-input-syscalls.patchRepository: armbian/build
Length of output: 4479
🏁 Script executed:
# Search for COMPAT_USE_64BIT_TIME definitions to understand which architectures use it
rg "COMPAT_USE_64BIT_TIME\s*=" --type c --max-count=20Repository: armbian/build
Length of output: 39
🌐 Web query:
ff_effect_compat structure time fields COMPAT_USE_64BIT_TIME architecture x86 x32
💡 Result:
The ff_effect_compat structure is the 32-bit compatibility version of struct ff_effect used in the Linux input subsystem for force feedback effects. It is defined in drivers/input/input-compat.h for CONFIG_COMPAT builds. Key differences relevant to time fields and COMPAT_USE_64BIT_TIME on x86 x32 architecture: - ff_effect.replay uses struct ff_replay { u16 length; u16 delay; }; - ff_trigger uses struct ff_trigger { u16 button; u16 interval; }; No direct time_t fields (like struct timeval or timespec) in ff_effect or ff_replay/ff_trigger, so no direct impact from COMPAT_USE_64BIT_TIME on these structures themselves. However, COMPAT_USE_64BIT_TIME affects the input event compatibility layer: - In input-compat.h and input-compat.c, input_event_size and input_event_from_user/to_user use: if (in_compat_syscall && !COMPAT_USE_64BIT_TIME) then use struct input_event_compat { compat_ulong_t sec; compat_ulong_t usec; ... } (32-bit time), else native 64-bit kernel_timespec or similar. - For x86 x32 (ILP32 on x86_64), COMPAT_USE_64BIT_TIME=1 by default, so it uses 64-bit time_t even in compat mode, bypassing the 32-bit compat_event conversion for input events. For ff_effect specifically: - input_ff_effect_from_user(const char user *buffer, size_t size, struct ff_effect *effect) in input-compat.c converts struct ff_effect_compat to ff_effect if in_compat_syscall. - struct ff_effect_compat has: - struct ff_periodic_effect_compat periodic with compat_uptr_t custom_data (pointer, 32-bit in compat). - No time_t fields, so unaffected by COMPAT_USE_64BIT_TIME directly. - x32 uses native ff_effect (64-bit pointers in u union via padding/array), but compat ioctls may trigger conversion if !COMPAT_USE_64BIT_TIME (though typically enabled). On x86 x32, since COMPAT_USE_64BIT_TIME=y, ff_effect_compat handling is used only for true 32-bit compat tasks wanting 32-bit time, but x32 apps see native 64-bit time layout. The "time fields" likely refer to replay.length/delay (in ms, u16) or envelope attack_length/fade_length (in ms, u16), which are identical in compat/native and not time_t. Sources confirm ff_effect_compat exists for compat ioctl EVIOCSFF/EVIOCGFF on 32-bit userspace, with x32 preferring native due to COMPAT_USE_64BIT_TIME.
Citations:
- 1: https://codebrowser.dev/linux/linux/drivers/input/input-compat.h.html
- 2: https://lkml.iu.edu/1202.2/01322.html
- 3: https://lkml.org/lkml/2012/3/19/524
- 4: https://github.com/torvalds/linux/blob/master/drivers/input/ff-core.c
- 5: https://docs.huihoo.com/doxygen/linux/kernel/3.7/structff__effect.html
Behavioral regression in input_ff_effect_from_user on COMPAT_USE_64BIT_TIME architectures.
The original guard for force-feedback was just:
if (in_compat_syscall())The patch adds !COMPAT_USE_64BIT_TIME:
if (current->compat_input || (in_compat_syscall() && !COMPAT_USE_64BIT_TIME))COMPAT_USE_64BIT_TIME controls whether struct input_event timestamps use 32-bit or 64-bit representation. struct ff_effect_compat has no time fields — it differs from native ff_effect only in pointer size (compat_uptr_t vs pointer). The compat decision for FF must depend on in_compat_syscall() alone.
On architectures where COMPAT_USE_64BIT_TIME=1 (x86 x32, arm64 ILP32), a 32-bit task calling EVIOCSFF will now skip the compat path entirely:
- Condition evaluates to:
(FALSE) || (TRUE && FALSE)= FALSE - Userspace sends
struct ff_effect_compat(32-bit pointers) - Kernel interprets it as native
struct ff_effect(64-bit pointers) - Result: data corruption or EINVAL from size mismatch
🛠️ Fix — remove `!COMPAT_USE_64BIT_TIME` from ff path
- if (current->compat_input || (in_compat_syscall() && !COMPAT_USE_64BIT_TIME)) {
+ if (current->compat_input || in_compat_syscall()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0044-Enable-64-bit-processes-to-use-compat-input-syscalls.patch`
around lines 47 - 52, The compat guard in input_ff_effect_from_user incorrectly
mixes COMPAT_USE_64BIT_TIME into the decision, causing 32-bit tasks on
COMPAT_USE_64BIT_TIME architectures to be treated as native and corrupt
ff_effect handling; change the conditional back to depend only on
current->compat_input or in_compat_syscall() so that struct ff_effect_compat
(which has no time fields) is used whenever the syscall is a compat syscall:
remove the "&& !COMPAT_USE_64BIT_TIME" clause from the if that checks
current->compat_input || (in_compat_syscall() && !COMPAT_USE_64BIT_TIME) so it
becomes current->compat_input || in_compat_syscall().
be4183d to
7f95f87
Compare
|
Odin2 portal seems failing to boot by merging this pull quest. edge-7.0.1 |
@pdapandapda, thanks for testing. What's the failure? Any error message? |
When logging into the system during init boot，although the usbgadget is recognized by PC， |
|
[ 153.577404] [drm:dpu_kms_wait_for_commit_done:525] [dpu error]wait for commit done returned -110 |
|
@pdapandapda Thanks. Will take a look at it. |
kasimling
force-pushed
the
odin2_kernel_7.0
branch
from
7f95f87 to
f7ff2d3
Compare
|
@pdapandapda , it seems I missed one display related patch. Added it now. Please help re-test and let me know if still any problems. Thanks! |
|
@pdapandapda, did you build the whole image or only kernel deb files? |
The full image |
OK. Let me try to get an odin2portal device to check it |
@pdapandapda , could you please try setting "DisableDisplayHW" to "false" in LinuxLoader.cfg? Thanks. |
|
@kasimling Just a random off-topic question: Does Armbian boot on the Odin 2 from the SD card, or do you have to flash the entire internal memory? I'm asking because they recently released the kernel for the Odin 3, and I'd like to know if we can add support for it—and avoid flashing the internal memory, since, as far as I can tell, there isn't even a guide for reflashing Android if you mess something up. |
It boots from SD card. Regarding to Odin 3 support, my understanding is if we can get a working ABL which can boot Linux, we can try building Armbian for it. |
This is a good news! thank you for your reply, i have one odin 3 and i want try use linux but on micro sd for testing |
|
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Kept odin2portal to 6.18 since there's boot issue reported Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
kasimling
force-pushed
the
odin2_kernel_7.0
branch
from
f7ff2d3 to
7e851a4
Compare
|
Keep odin2portal to kernel 6.18. @pdapandapda , please let me know if still any problems. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch (1)
1084-1102: Add explicitfirmware-nameproperty to both AW88166 speaker amp nodes.The AW88166 driver now supports reading a
firmware-nameproperty from device tree (with fallback toAW88166_ACF_FILEif unset). The pairedspk_amp_landspk_amp_rnodes currently omit this property, leaving them reliant on the driver default. Since other devices in this same DTS already use explicitfirmware-nameentries (e.g.,zap-shader,remoteproc_cdsp), adding it here would enable per-channel and board-variant firmware tuning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch` around lines 1084 - 1102, Add an explicit firmware-name property to both AW88166 speaker amp nodes so the driver reads board/channel-specific firmware instead of falling back to its default: update spk_amp_l (spk_amp_l@34) to include firmware-name = "aw88166_spk_l.acf" and update spk_amp_r (spk_amp_r@35) to include firmware-name = "aw88166_spk_r.acf" (or other board-specific filenames matching your existing naming convention).patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (2)
99-101: ⚡ Quick winRemove the commented-out
qca_check_bdaddr()call and its__maybe_unusedworkaroundDisabling the old call by comment rather than deletion leaves dead code and forces the function to be annotated
__maybe_unusedto suppress warnings. Either keep the function and call it (as a fallback — see the major issue above), or delete both the call site and the function entirely.♻️ If the function is truly no longer needed, remove it cleanly
-static int __maybe_unused qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *config) +static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *config) { ... }And at the call site:
- /* Disable reading BD_ADDR from NVM */ - //err = qca_check_bdaddr(hdev, &config); - //if (err) - // return err; + /* (call removed — address is set from serial number above) */Or, preferably, restore the call as a fallback (see the major-issue comment).
Also applies to: 139-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 99 - 101, The qca_check_bdaddr function was left annotated __maybe_unused because its call site was commented out; either remove the dead function and its commented-out invocation together, or re-enable the call and use qca_check_bdaddr as a proper fallback. Locate the function qca_check_bdaddr and the commented-out invocation in the Wi‑Fi/BT init path, then either delete both the function and the commented call, or restore the call so the function is used (remove __maybe_unused if restored) and ensure error handling follows the existing fallback logic.
20-20: ⚡ Quick winShare
extern const char *qcom_serial_numbervia a header instead of duplicating itBoth
btqca.candmac.ccarry an identical independentexterndeclaration. If the definition insocinfo.cever changes type, both consumers will silently diverge. The declaration belongs in a shared header (e.g.,include/linux/soc/qcom/socinfo.h), which both files can include.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` at line 20, Move the shared declaration for qcom_serial_number into a single shared header (e.g., socinfo.h): add one declaration for the symbol qcom_serial_number to that header, include that header from btqca.c and mac.c, and remove the duplicate extern declarations in those consumers; ensure the definition in socinfo.c matches the declaration's type and update the include directives in btqca.c and mac.c so both consume the single header-only declaration before rebuilding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@patch/kernel/archive/sm8550-7.0/0018-drm-panel-Add-panel-driver-for-Xm-Plus-XM91080G-base.patch`:
- Around line 23-31: Remove the spurious dependency on BACKLIGHT_CLASS_DEVICE
from the Kconfig for DRM_PANEL_BOE_XM91080G: edit the config block for
DRM_PANEL_BOE_XM91080G and delete the line "depends on BACKLIGHT_CLASS_DEVICE"
(the panel driver panel-boe-xm91080g.c does not include or use the backlight
API), leaving the other dependencies (OF, DRM_MIPI_DSI) and the select
DRM_KMS_HELPER intact so the driver can be built when BACKLIGHT_CLASS_DEVICE=n.
In
`@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch`:
- Around line 1404-1409: Rename the duplicated node label and node name so the
primary TE suspend state uses the unique name matching its phandle label: change
the node labeled dsi_p_te_suspend currently defined as dsi-s-te-suspend-state to
dsi-p-te-suspend-state; ensure the label remains dsi_p_te_suspend and other
properties (pins, function, drive-strength, bias-pull-down) stay the same so it
no longer conflicts with the existing dsi_s_te_suspend: dsi-s-te-suspend-state
node.
In
`@patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch`:
- Around line 174-181: The hdmi_out_in endpoint's data-lanes value mismatches
the mdss_dsi0_out endpoint causing driver/DSI lane confusion; update the
hdmi_out_in endpoint (symbol: hdmi_out_in) so its data-lanes use the same
numbering as mdss_dsi0_out (symbol: mdss_dsi0_out) — i.e., change data-lanes
from <1 2 3 4> to <0 1 2 3> to follow the standard DSI convention where lane 0
is the clock lane and ensure both endpoints match.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 279-302: The global qcom_serial_number currently causes a
use-after-free because it points at devm-managed qs->attr.serial_number set in
qcom_socinfo_probe and cleared in qcom_socinfo_remove; fix by making the pointer
RCU-safe: replace direct assignments with rcu_assign_pointer(qcom_serial_number,
qs->attr.serial_number) in qcom_socinfo_probe and
rcu_assign_pointer(qcom_serial_number, NULL) in qcom_socinfo_remove, and update
all consumers (e.g., code in btqca.c and mac.c) to acquire an RCU read-side lock
(rcu_read_lock()), load the pointer via rcu_dereference(qcom_serial_number), use
strlen/strncpy while holding the lock, then rcu_read_unlock(); alternatively, if
you prefer non-RCU semantics, protect qcom_serial_number with a mutex and have
consumers kstrdup the string under that lock before operating on it so they
never touch freed memory.
- Around line 266-267: The log is printing the wrong buffer and using the wrong
format: replace the call that currently logs generated_macaddr.b with one that
logs the installed address ar->mac_addr and use the %pM specifier; i.e., after
ath12k_reverse_mac(...) call, change the ath12k_info invocation to reference
ar->mac_addr and %pM so the log reflects the actual installed MAC (symbols to
edit: ath12k_reverse_mac, ath12k_info, ar->mac_addr, generated_macaddr.b).
- Around line 161-163: Remove the unused static variable static_macaddr (and any
identical dead code pattern like static_bdaddr in btqca.c) to eliminate the
-Wunused-variable warning: locate the declaration of static_macaddr and delete
it (or, if it was intended to be used, wire it into the appropriate
initialization function instead) so no unused static macaddr symbol remains.
- Around line 23-25: The variable static_bdaddr (bdaddr_t static_bdaddr) is
declared but never used, causing -Wunused-variable; remove the entire
static_bdaddr declaration (the block initializing .b = { 0x00, 0x03, 0x7F, 0x33,
0x22, 0x11 }) from the patch so the unused symbol is eliminated and no warning
is generated.
- Around line 119-123: generate_bdaddr_from_serial() can return -EINVAL when
qcom_serial_number is NULL, causing qca_uart_setup() to abort and prevent BT
from coming up; restore the NVM fallback by calling qca_check_bdaddr() when
generate_bdaddr_from_serial() fails with -EINVAL (or when qcom_serial_number is
NULL) instead of returning the error. Update qca_uart_setup() to: call
generate_bdaddr_from_serial(hdev, &generated_bdaddr), if it succeeds use that
address, but if it returns -EINVAL or indicates missing serial, log a warning
and call qca_check_bdaddr(hdev) to attempt NVM-based BD_ADDR; only return a
fatal error if both methods fail. Ensure logs clearly indicate which method
produced the BD_ADDR (serial vs NVM).
---
Nitpick comments:
In
`@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch`:
- Around line 1084-1102: Add an explicit firmware-name property to both AW88166
speaker amp nodes so the driver reads board/channel-specific firmware instead of
falling back to its default: update spk_amp_l (spk_amp_l@34) to include
firmware-name = "aw88166_spk_l.acf" and update spk_amp_r (spk_amp_r@35) to
include firmware-name = "aw88166_spk_r.acf" (or other board-specific filenames
matching your existing naming convention).
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 99-101: The qca_check_bdaddr function was left annotated
__maybe_unused because its call site was commented out; either remove the dead
function and its commented-out invocation together, or re-enable the call and
use qca_check_bdaddr as a proper fallback. Locate the function qca_check_bdaddr
and the commented-out invocation in the Wi‑Fi/BT init path, then either delete
both the function and the commented call, or restore the call so the function is
used (remove __maybe_unused if restored) and ensure error handling follows the
existing fallback logic.
- Line 20: Move the shared declaration for qcom_serial_number into a single
shared header (e.g., socinfo.h): add one declaration for the symbol
qcom_serial_number to that header, include that header from btqca.c and mac.c,
and remove the duplicate extern declarations in those consumers; ensure the
definition in socinfo.c matches the declaration's type and update the include
directives in btqca.c and mac.c so both consume the single header-only
declaration before rebuilding.
🪄 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: 828cc796-b6bd-4828-9d8e-fa156f50a311
📒 Files selected for processing (47)
config/kernel/linux-sm8550-edge.configconfig/sources/families/sm8550.confpatch/kernel/archive/sm8550-7.0/0001-msm-adreno-enable-A32.patchpatch/kernel/archive/sm8550-7.0/0002-qcom-dispcc-sm8550-Fix-disp_cc_mdss_mdp_clk_src.patchpatch/kernel/archive/sm8550-7.0/0003-input-rmi4-add-reset-gpio.patchpatch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patchpatch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patchpatch/kernel/archive/sm8550-7.0/0006-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patchpatch/kernel/archive/sm8550-7.0/0007-mmc-sdhci-msm-Toggle-the-FIFO-write-clock-after-unga.patchpatch/kernel/archive/sm8550-7.0/0008-ASoC-codecs-aw88166-AYN-SM8550-Specific-modification.patchpatch/kernel/archive/sm8550-7.0/0009-arm64-dts-qcom-Added-pmk8550_pwm.patchpatch/kernel/archive/sm8550-7.0/0010-gpu-panel-add-Pocket-ACE-panel-driver.patchpatch/kernel/archive/sm8550-7.0/0012-input-touchscreen-porting-downstream-hynitron-driver.patchpatch/kernel/archive/sm8550-7.0/0013-qcom_battmgr-Set-battery-name.patchpatch/kernel/archive/sm8550-7.0/0014-Input-edt-ft5x06-add-no_regmap_bulk_read-option.patchpatch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patchpatch/kernel/archive/sm8550-7.0/0017-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patchpatch/kernel/archive/sm8550-7.0/0018-drm-panel-Add-panel-driver-for-Xm-Plus-XM91080G-base.patchpatch/kernel/archive/sm8550-7.0/0019-drm-panel-Add-panel-driver-for-Chipone-ICNA35XX-base.patchpatch/kernel/archive/sm8550-7.0/0020-drm-panel-Add-panel-driver-for-DDIC-CH13726A-based-p.patchpatch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patchpatch/kernel/archive/sm8550-7.0/0022-Input-hynitron_cstxxx-specific-patch-for-Odin2-Mini.patchpatch/kernel/archive/sm8550-7.0/0023-Add-Silergy-SY7758-backlight-driver.patchpatch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patchpatch/kernel/archive/sm8550-7.0/0025-input-RSInput-Add-regulator.patchpatch/kernel/archive/sm8550-7.0/0026-drm-msm-remove-DRIVER_SYNCOBJ_TIMELINE.patchpatch/kernel/archive/sm8550-7.0/0027-HACK-fix-usb-boot-hang.patchpatch/kernel/archive/sm8550-7.0/0028-SM8550-Fix-L2-cache-for-CPU2-and-add-cache-sizes.patchpatch/kernel/archive/sm8550-7.0/0029-SM8550-Add-DDR-LLCC-L3-CPU-bandwidth-scaling.patchpatch/kernel/archive/sm8550-7.0/0030-arm64-dts-qcom-sm8550-Update-EAS-properties.patchpatch/kernel/archive/sm8550-7.0/0031-arm64-dts-qcom-sm8550-add-UART15.patchpatch/kernel/archive/sm8550-7.0/0032-drm-panel-Add-Retroid-Pocket-6-panel.patchpatch/kernel/archive/sm8550-7.0/0033-arm64-dts-qcom-sm8550-Describe-GPU_CC-power-plumbing.patchpatch/kernel/archive/sm8550-7.0/0034-interconnect-qcom-sm8550-Enable-QoS-configuration.patchpatch/kernel/archive/sm8550-7.0/0035-dts-qcom-sm8550-add-opp-acd-level.patchpatch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patchpatch/kernel/archive/sm8550-7.0/0037-arm64-dts-qcom-Add-AYN-Odin2.patchpatch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patchpatch/kernel/archive/sm8550-7.0/0039-arm64-dts-qcom-Add-AYN-Odin2-Portal.patchpatch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patchpatch/kernel/archive/sm8550-7.0/0041-ROCKNIX-set-boot-fanspeed.patchpatch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patchpatch/kernel/archive/sm8550-7.0/0043-power-supply-rename-qcom-battmgr-sysfs.patchpatch/kernel/archive/sm8550-7.0/0044-Enable-64-bit-processes-to-use-compat-input-syscalls.patchpatch/kernel/archive/sm8550-7.0/0130-restore_widebus_calculation_for_CMDMode_panels.patchpatch/kernel/archive/sm8550-7.0/v2_20260314_vladimir_zapolskiy_arm64_dts_qcom_sm8x50_enable_uhs_i_sdr50_and_sdr104_sd_card_modes.patchpatch/kernel/archive/sm8550-7.0/v5_20251120_quic_utiwari_crypto_qce_add_runtime_pm_and_interconnect_bandwidth_scaling_support.patch
✅ Files skipped from review due to trivial changes (12)
- patch/kernel/archive/sm8550-7.0/0043-power-supply-rename-qcom-battmgr-sysfs.patch
- patch/kernel/archive/sm8550-7.0/0009-arm64-dts-qcom-Added-pmk8550_pwm.patch
- patch/kernel/archive/sm8550-7.0/0013-qcom_battmgr-Set-battery-name.patch
- patch/kernel/archive/sm8550-7.0/0001-msm-adreno-enable-A32.patch
- patch/kernel/archive/sm8550-7.0/0002-qcom-dispcc-sm8550-Fix-disp_cc_mdss_mdp_clk_src.patch
- patch/kernel/archive/sm8550-7.0/0031-arm64-dts-qcom-sm8550-add-UART15.patch
- patch/kernel/archive/sm8550-7.0/0021-backlight-odin2mini-backlight-Add-support-for-AYN-Od.patch
- patch/kernel/archive/sm8550-7.0/v2_20260314_vladimir_zapolskiy_arm64_dts_qcom_sm8x50_enable_uhs_i_sdr50_and_sdr104_sd_card_modes.patch
- patch/kernel/archive/sm8550-7.0/0041-ROCKNIX-set-boot-fanspeed.patch
- patch/kernel/archive/sm8550-7.0/0039-arm64-dts-qcom-Add-AYN-Odin2-Portal.patch
- patch/kernel/archive/sm8550-7.0/0024-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patch
- patch/kernel/archive/sm8550-7.0/0005-leds-Add-driver-for-HEROIC-HTR3212.patch
🚧 Files skipped from review as they are similar to previous changes (21)
- config/sources/families/sm8550.conf
- patch/kernel/archive/sm8550-7.0/0027-HACK-fix-usb-boot-hang.patch
- patch/kernel/archive/sm8550-7.0/0022-Input-hynitron_cstxxx-specific-patch-for-Odin2-Mini.patch
- patch/kernel/archive/sm8550-7.0/0025-input-RSInput-Add-regulator.patch
- patch/kernel/archive/sm8550-7.0/0030-arm64-dts-qcom-sm8550-Update-EAS-properties.patch
- patch/kernel/archive/sm8550-7.0/0035-dts-qcom-sm8550-add-opp-acd-level.patch
- patch/kernel/archive/sm8550-7.0/0040-arm64-dts-qcom-Add-AYN-Thor.patch
- patch/kernel/archive/sm8550-7.0/0020-drm-panel-Add-panel-driver-for-DDIC-CH13726A-based-p.patch
- patch/kernel/archive/sm8550-7.0/0006-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch
- patch/kernel/archive/sm8550-7.0/0014-Input-edt-ft5x06-add-no_regmap_bulk_read-option.patch
- patch/kernel/archive/sm8550-7.0/0010-gpu-panel-add-Pocket-ACE-panel-driver.patch
- config/kernel/linux-sm8550-edge.config
- patch/kernel/archive/sm8550-7.0/0016-pwm-Add-SI-EN-SN3112-PWM-support.patch
- patch/kernel/archive/sm8550-7.0/0008-ASoC-codecs-aw88166-AYN-SM8550-Specific-modification.patch
- patch/kernel/archive/sm8550-7.0/0017-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch
- patch/kernel/archive/sm8550-7.0/0029-SM8550-Add-DDR-LLCC-L3-CPU-bandwidth-scaling.patch
- patch/kernel/archive/sm8550-7.0/0032-drm-panel-Add-Retroid-Pocket-6-panel.patch
- patch/kernel/archive/sm8550-7.0/0023-Add-Silergy-SY7758-backlight-driver.patch
- patch/kernel/archive/sm8550-7.0/0028-SM8550-Fix-L2-cache-for-CPU2-and-add-cache-sizes.patch
- patch/kernel/archive/sm8550-7.0/0004-input-Add-driver-for-RSInput-Gamepad.patch
- patch/kernel/archive/sm8550-7.0/0037-arm64-dts-qcom-Add-AYN-Odin2.patch
| +config DRM_PANEL_BOE_XM91080G | ||
| + tristate "Xm-Plus XM91080G-based panels" | ||
| + depends on OF | ||
| + depends on DRM_MIPI_DSI | ||
| + depends on BACKLIGHT_CLASS_DEVICE | ||
| + select DRM_KMS_HELPER | ||
| + help | ||
| + Say Y if you want to enable support for panels based on the | ||
| + Xm-Plus XM91080G controller. |
There was a problem hiding this comment.
Remove spurious depends on BACKLIGHT_CLASS_DEVICE.
The driver (panel-boe-xm91080g.c) includes no backlight headers and makes zero calls into the backlight class device API. The depends on BACKLIGHT_CLASS_DEVICE constraint is not only unused — it unnecessarily prevents the driver from being enabled in configurations where BACKLIGHT_CLASS_DEVICE=n, which is a valid build scenario for minimal embedded targets.
🔧 Proposed fix
config DRM_PANEL_BOE_XM91080G
tristate "Xm-Plus XM91080G-based panels"
depends on OF
depends on DRM_MIPI_DSI
- depends on BACKLIGHT_CLASS_DEVICE
select DRM_KMS_HELPER
help
Say Y if you want to enable support for panels based on the
Xm-Plus XM91080G controller.📝 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.
| +config DRM_PANEL_BOE_XM91080G | |
| + tristate "Xm-Plus XM91080G-based panels" | |
| + depends on OF | |
| + depends on DRM_MIPI_DSI | |
| + depends on BACKLIGHT_CLASS_DEVICE | |
| + select DRM_KMS_HELPER | |
| + help | |
| + Say Y if you want to enable support for panels based on the | |
| + Xm-Plus XM91080G controller. | |
| config DRM_PANEL_BOE_XM91080G | |
| tristate "Xm-Plus XM91080G-based panels" | |
| depends on OF | |
| depends on DRM_MIPI_DSI | |
| select DRM_KMS_HELPER | |
| help | |
| Say Y if you want to enable support for panels based on the | |
| Xm-Plus XM91080G controller. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0018-drm-panel-Add-panel-driver-for-Xm-Plus-XM91080G-base.patch`
around lines 23 - 31, Remove the spurious dependency on BACKLIGHT_CLASS_DEVICE
from the Kconfig for DRM_PANEL_BOE_XM91080G: edit the config block for
DRM_PANEL_BOE_XM91080G and delete the line "depends on BACKLIGHT_CLASS_DEVICE"
(the panel driver panel-boe-xm91080g.c does not include or use the backlight
API), leaving the other dependencies (OF, DRM_MIPI_DSI) and the select
DRM_KMS_HELPER intact so the driver can be built when BACKLIGHT_CLASS_DEVICE=n.
| + dsi_p_te_suspend: dsi-s-te-suspend-state { | ||
| + pins = "gpio86"; | ||
| + function = "mdp_vsync"; | ||
| + drive-strength = <2>; | ||
| + bias-pull-down; | ||
| + }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib, re, collections
path = pathlib.Path("patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch")
added = []
for line in path.read_text().splitlines():
if line.startswith("+++"):
continue
if line.startswith("+"):
added.append(line[1:])
text = "\n".join(added)
names = re.findall(r'^\s*(?:[A-Za-z0-9_]+:\s+)?([A-Za-z0-9,_-]+)\s*\{', text, re.M)
for name, count in collections.Counter(names).items():
if name == "dsi-s-te-suspend-state":
print(f"{name}: {count}")
PYRepository: armbian/build
Length of output: 83
🏁 Script executed:
sed -n '1395,1415p' patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patchRepository: armbian/build
Length of output: 478
🏁 Script executed:
sed -n '1420,1445p' patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patchRepository: armbian/build
Length of output: 558
Rename the primary TE suspend state node.
Line 1404 defines dsi_p_te_suspend: dsi-s-te-suspend-state, which has the same node name as dsi_s_te_suspend: dsi-s-te-suspend-state at line 1432. In DTS, sibling nodes with identical names merge or conflict, causing compiler errors or unpredictable behavior. The first node must be renamed to dsi-p-te-suspend-state to match its phandle label convention.
Suggested fix
- dsi_p_te_suspend: dsi-s-te-suspend-state {
+ dsi_p_te_suspend: dsi-p-te-suspend-state {
pins = "gpio86";
function = "mdp_vsync";
drive-strength = <2>;
bias-pull-down;
};📝 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.
| + dsi_p_te_suspend: dsi-s-te-suspend-state { | |
| + pins = "gpio86"; | |
| + function = "mdp_vsync"; | |
| + drive-strength = <2>; | |
| + bias-pull-down; | |
| + }; | |
| dsi_p_te_suspend: dsi-p-te-suspend-state { | |
| pins = "gpio86"; | |
| function = "mdp_vsync"; | |
| drive-strength = <2>; | |
| bias-pull-down; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0036-arm64-dts-qcom-Add-AYN-QCS8550-Common.patch`
around lines 1404 - 1409, Rename the duplicated node label and node name so the
primary TE suspend state uses the unique name matching its phandle label: change
the node labeled dsi_p_te_suspend currently defined as dsi-s-te-suspend-state to
dsi-p-te-suspend-state; ensure the label remains dsi_p_te_suspend and other
properties (pins, function, drive-strength, bias-pull-down) stay the same so it
no longer conflicts with the existing dsi_s_te_suspend: dsi-s-te-suspend-state
node.
| + port@0 { | ||
| + reg = <0>; | ||
| + | ||
| + hdmi_out_in: endpoint { | ||
| + data-lanes = <1 2 3 4>; | ||
| + remote-endpoint = <&mdss_dsi0_out>; | ||
| + }; | ||
| + }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check data-lanes usage patterns in other SM8550 DTS files
echo "=== DSI data-lanes patterns in sm8550 DTS files ==="
rg -n "data-lanes" --glob "*sm8550*.dts*" --glob "*qcs8550*.dts*"
echo ""
echo "=== mdss_dsi0_out configurations in qcom DTS ==="
ast-grep --pattern 'mdss_dsi0_out {
$$$
}'Repository: armbian/build
Length of output: 326
🏁 Script executed:
# Read the patch file to see the full context
cat -n patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch | sed -n '170,210p'Repository: armbian/build
Length of output: 994
🏁 Script executed:
# Search for data-lanes in all patch files and other configs
echo "=== data-lanes in patch files ==="
rg -n "data-lanes" patch/kernel/archive/ -t patch
echo ""
echo "=== Search for mdss_dsi in patch files ==="
rg -n "mdss_dsi" patch/kernel/archive/sm8550-7.0/ -t patch | head -50Repository: armbian/build
Length of output: 213
🏁 Script executed:
# Look for other HDMI bridge configurations in the patch directory
echo "=== HDMI/DSI bridge configs ==="
rg -n "lt8912|hdmi" patch/kernel/archive/sm8550-7.0/ -t patch -i | head -30Repository: armbian/build
Length of output: 137
🏁 Script executed:
# Search for mdss_dsi0_out in source code and documentation
echo "=== mdss_dsi0_out in source directories ==="
rg -n "mdss_dsi0_out" --max-count=20
echo ""
echo "=== DSI data-lanes patterns in source ==="
rg "data-lanes.*=" -B2 -A2 --max-count=20 | head -100Repository: armbian/build
Length of output: 12909
🏁 Script executed:
# Look for kernel documentation on DSI lanes
echo "=== Search for DSI lane documentation ==="
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | xargs grep -l "data-lanes\|DSI.*lane" 2>/dev/null | head -10Repository: armbian/build
Length of output: 99
🏁 Script executed:
# Search for other sm8550 related DTS or device tree files in the repo
echo "=== SM8550 DTS related files ==="
fd -t f "sm8550" | head -20Repository: armbian/build
Length of output: 1533
🏁 Script executed:
# Search for LT8912 bridge driver and configuration
echo "=== LT8912 references in repo ==="
rg -i "lt8912" -A5 -B5 | head -100Repository: armbian/build
Length of output: 8617
🏁 Script executed:
# Check kernel/linux directory for any bridge documentation or driver stubs
echo "=== Searching for bridge-related DT bindings documentation ==="
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | xargs grep -l "lt8912\|hdmi.*bridge" 2>/dev/nullRepository: armbian/build
Length of output: 122
🏁 Script executed:
# Search patch comments or commit messages for LT8912 lane explanations
echo "=== Checking patch files for LT8912 context ==="
rg -n "lt8912|hdmi.*bridge" patch/kernel/archive/sm8550-7.0/0038 -B10 -A10 | head -80Repository: armbian/build
Length of output: 200
🏁 Script executed:
# Search for any upstream kernel references or comments about LT8912B
echo "=== Checking for kernel driver stubs or references ==="
find . -type f -name "*.c" -o -name "*.h" | xargs grep -l "lt8912\|lontium" 2>/dev/null | head -20Repository: armbian/build
Length of output: 112
🌐 Web query:
LT8912B kernel driver data-lanes DSI HDMI bridge configuration lane mapping
💡 Result:
The LT8912B MIPI DSI to HDMI bridge kernel driver (drivers/gpu/drm/bridge/lontium-lt8912b.c) configures data lanes through standard Linux DRM device tree bindings. Configuration: - Device tree port@0 endpoint requires 'data-lanes' property: array of lane indices (0-based) mapping DSI controller lanes to LT8912B physical pins, e.g., data-lanes = <1 2 3 4>; (ConnectCore 93) or <0 1 2 3>; (binding example). Supports 1-4 lanes. - Driver parses with drm_of_get_data_lanes_count_ep(dev->of_node, 0, -1, 1, 4), storing count in lt->data_lanes. - Sets mipi_dsi_device->lanes = lt->data_lanes. - Programs LT8912B register 0x13 (I2C_CEC_DSI bus) = lt->data_lanes & 3 (2-bit count value, e.g., 1=0x1, 2=0x2, 3=0x3, 4=0x0?). No explicit lane remapping/swizzling in driver; follows standard data-lanes convention where array order defines sequential mapping (index 0=first data lane, etc.). Hardware pinout per LT8912B datasheet (1 clock + 1-4 data lanes). Binding mandates data-lanes on DSI input endpoint.
Citations:
- 1: https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/lontium-lt8912b.c
- 2: https://lkml.iu.edu/2102.3/03456.html
- 3: https://www.digi.com/resources/documentation/digidocs/embedded/dey/5.0/cc93/bsp-video-hdmi_r_93.html
- 4: https://github.com/digi-embedded/linux/blob/v6.6/nxp/dey-5.0/maint/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
DSI data-lanes mismatch between bridge input and DSI output endpoints requires correction.
The hdmi_out_in endpoint specifies data-lanes = <1 2 3 4> while mdss_dsi0_out specifies data-lanes = <0 1 2 3>. The LT8912B kernel driver parses these values directly without any lane remapping and passes them to the DSI device. Standard DSI convention uses lane 0 for clock with data lanes numbered 1–4, making these specifications incompatible. Both endpoints must use the same lane numbering for correct operation.
This mismatch likely contributes to the DRM/display failures reported in the PR comments ("enc33 frame done timeout"). Align the data-lanes specifications on both endpoints—either both <0 1 2 3> or both <1 2 3 4>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch/kernel/archive/sm8550-7.0/0038-arm64-dts-qcom-Add-AYN-Odin2-Mini.patch`
around lines 174 - 181, The hdmi_out_in endpoint's data-lanes value mismatches
the mdss_dsi0_out endpoint causing driver/DSI lane confusion; update the
hdmi_out_in endpoint (symbol: hdmi_out_in) so its data-lanes use the same
numbering as mdss_dsi0_out (symbol: mdss_dsi0_out) — i.e., change data-lanes
from <1 2 3 4> to <0 1 2 3> to follow the standard DSI convention where lane 0
is the clock lane and ensure both endpoints match.
| +static const bdaddr_t static_bdaddr = { | ||
| + .b = { 0x00, 0x03, 0x7F, 0x33, 0x22, 0x11 } | ||
| +}; |
There was a problem hiding this comment.
static_bdaddr is declared but never used — remove it
This variable is a leftover from development and will produce a -Wunused-variable compiler warning. Many kernel build configurations treat warnings as errors.
🗑️ Proposed fix
-/* Define a static, predefined BD_ADDR structure */
-static const bdaddr_t static_bdaddr = {
- .b = { 0x00, 0x03, 0x7F, 0x33, 0x22, 0x11 }
-};
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 23 - 25, The variable static_bdaddr (bdaddr_t static_bdaddr) is
declared but never used, causing -Wunused-variable; remove the entire
static_bdaddr declaration (the block initializing .b = { 0x00, 0x03, 0x7F, 0x33,
0x22, 0x11 }) from the patch so the unused symbol is eliminated and no warning
is generated.
| + err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); | ||
| + if (err) { | ||
| + bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); | ||
| + return err; | ||
| + } |
There was a problem hiding this comment.
Hard fatal failure when serial number is unavailable — no fallback
If qcom_serial_number is NULL (e.g., SMEM serial number absent, or socinfo not probed yet due to deferred init ordering), generate_bdaddr_from_serial() returns -EINVAL and qca_uart_setup() propagates it as a fatal error — BT will not come up at all. The previous qca_check_bdaddr() fallback (NVM-based address) is entirely removed.
For robustness, especially for any future SM8550 board that doesn't populate the SMEM serial field, the NVM path should be the fallback rather than a hard failure:
💡 Suggested approach
- /* Generate BD_ADDR from qcom_serial_number */
- err = generate_bdaddr_from_serial(hdev, &generated_bdaddr);
- if (err) {
- bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number");
- return err;
- }
-
- /* Set the generated BD_ADDR */
- err = qca_set_bdaddr(hdev, &generated_bdaddr);
- if (err) {
- bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number");
+ err = generate_bdaddr_from_serial(hdev, &generated_bdaddr);
+ if (!err) {
+ err = qca_set_bdaddr(hdev, &generated_bdaddr);
+ if (err) {
+ bt_dev_err(hdev, "Failed to set BD_ADDR from serial number");
+ return err;
+ }
+ bacpy(&hdev->public_addr, &generated_bdaddr);
+ bacpy(&hdev->bdaddr, &generated_bdaddr);
+ bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr);
+ } else {
+ bt_dev_warn(hdev, "Serial-based BD_ADDR unavailable, falling back to NVM");
+ err = qca_check_bdaddr(hdev, &config);
+ if (err)
return err;
}
-
- /* Update hdev->public_addr and hdev->bdaddr */
- bacpy(&hdev->public_addr, &generated_bdaddr);
- bacpy(&hdev->bdaddr, &generated_bdaddr);
- bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 119 - 123, generate_bdaddr_from_serial() can return -EINVAL when
qcom_serial_number is NULL, causing qca_uart_setup() to abort and prevent BT
from coming up; restore the NVM fallback by calling qca_check_bdaddr() when
generate_bdaddr_from_serial() fails with -EINVAL (or when qcom_serial_number is
NULL) instead of returning the error. Update qca_uart_setup() to: call
generate_bdaddr_from_serial(hdev, &generated_bdaddr), if it succeeds use that
address, but if it returns -EINVAL or indicates missing serial, log a warning
and call qca_check_bdaddr(hdev) to attempt NVM-based BD_ADDR; only return a
fatal error if both methods fail. Ensure logs clearly indicate which method
produced the BD_ADDR (serial vs NVM).
| +static const struct macaddr_t static_macaddr = { | ||
| + .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } | ||
| +}; |
There was a problem hiding this comment.
static_macaddr is declared but never used — remove it
Identical issue to static_bdaddr in btqca.c; will generate a -Wunused-variable warning.
🗑️ Proposed fix
-/* Define a static, predefined MAC_ADDR structure */
-static const struct macaddr_t static_macaddr = {
- .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 }
-};
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 161 - 163, Remove the unused static variable static_macaddr (and
any identical dead code pattern like static_bdaddr in btqca.c) to eliminate the
-Wunused-variable warning: locate the declaration of static_macaddr and delete
it (or, if it was intended to be used, wire it into the appropriate
initialization function instead) so no unused static macaddr symbol remains.
| + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | ||
| + ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); |
There was a problem hiding this comment.
Log reports the wrong variable with the wrong format specifier
Line 267 logs generated_macaddr.b (the pre-reversal intermediate buffer) using %pMR, rather than ar->mac_addr (the value actually installed) using %pM. The displayed address happens to be numerically correct by a double-reversal coincidence, but the intent is opaque and fragile — any future change to the reversal logic will silently produce an incorrect log.
🐛 Proposed fix
- ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b);
- ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b);
+ ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b);
+ ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr);📝 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.
| + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | |
| + ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); | |
| ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | |
| ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 266 - 267, The log is printing the wrong buffer and using the wrong
format: replace the call that currently logs generated_macaddr.b with one that
logs the installed address ar->mac_addr and use the %pM specifier; i.e., after
ath12k_reverse_mac(...) call, change the ath12k_info invocation to reference
ar->mac_addr and %pM so the log reflects the actual installed MAC (symbols to
edit: ath12k_reverse_mac, ath12k_info, ar->mac_addr, generated_macaddr.b).
| +/* Global variable to hold the serial number */ | ||
| +const char *qcom_serial_number; | ||
| +EXPORT_SYMBOL(qcom_serial_number); | ||
| + | ||
| struct qcom_socinfo { | ||
| struct soc_device *soc_dev; | ||
| struct soc_device_attribute attr; | ||
| @@ -891,6 +895,9 @@ static int qcom_socinfo_probe(struct platform_device *pdev) | ||
| le32_to_cpu(info->serial_num)); | ||
| if (!qs->attr.serial_number) | ||
| return -ENOMEM; | ||
| + | ||
| + /* Assign the serial number to the global variable */ | ||
| + qcom_serial_number = qs->attr.serial_number; | ||
| } | ||
|
|
||
| qs->soc_dev = soc_device_register(&qs->attr); | ||
| @@ -914,6 +921,9 @@ static void qcom_socinfo_remove(struct platform_device *pdev) | ||
| soc_device_unregister(qs->soc_dev); | ||
|
|
||
| socinfo_debugfs_exit(qs); | ||
| + | ||
| + /* Clear the global serial number */ | ||
| + qcom_serial_number = NULL; |
There was a problem hiding this comment.
Use-after-free race on qcom_serial_number
The global pointer is assigned during probe and cleared in remove(), but the memory it references (qs->attr.serial_number, a kasprintf-allocated string managed by devm) is freed only after remove() returns. A concurrent consumer in btqca.c or mac.c can:
- Read the non-NULL pointer into a local
serialvariable. qcom_socinfo_remove()runs, setsqcom_serial_number = NULL, returns.- Platform core frees the devm memory (
qs->attr.serial_number). - Consumer calls
strlen(serial)→ use-after-free.
The if (!serial) NULL-check in each consumer guards against a NULL pointer but not against freed memory, because the local serial captures the pointer value before the NULL-set.
The fix requires either:
- RCU: export with
rcu_assign_pointer/ clear withrcu_assign_pointer(qcom_serial_number, NULL), and consumers usercu_read_lock()+rcu_dereference()+ hold the read-lock for the duration ofstrlen/strncpy. - A mutex or
kstrdup-based copy: make consumers take a copy under a lock and operate on the copy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-7.0/0042-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 279 - 302, The global qcom_serial_number currently causes a
use-after-free because it points at devm-managed qs->attr.serial_number set in
qcom_socinfo_probe and cleared in qcom_socinfo_remove; fix by making the pointer
RCU-safe: replace direct assignments with rcu_assign_pointer(qcom_serial_number,
qs->attr.serial_number) in qcom_socinfo_probe and
rcu_assign_pointer(qcom_serial_number, NULL) in qcom_socinfo_remove, and update
all consumers (e.g., code in btqca.c and mac.c) to acquire an RCU read-side lock
(rcu_read_lock()), load the pointer via rcu_dereference(qcom_serial_number), use
strlen/strncpy while holding the lock, then rcu_read_unlock(); alternatively, if
you prefer non-RCU semantics, protect qcom_serial_number with a mutex and have
consumers kstrdup the string under that lock before operating on it so they
never touch freed memory.
3 participants
Description
This PR adds kernel 7.0 support Ayn Odin2 device
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Configuration & Updates