Draft
Conversation
github-actions
bot
added
05
Contributor
📝 WalkthroughWalkthroughBumps SM8550 kernel config to 6.19 and adds multiple new drivers, device-tree additions for Ayn Odin2, device drivers (input, panels, LEDs, PWM), audio and clock updates, serial-based MAC/BDADDR generation, DT binding docs, and various fixes across subsystems. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-348-348 (1)
348-348:⚠️ Potential issue | 🟡 MinorFix malformed author metadata string.
Line 348 is missing the closing
>in the author email.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch` at line 348, The MODULE_AUTHOR metadata line is malformed: fix the MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); entry by adding the missing closing '>' to the email so it becomes MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>"); ensuring the author string is properly terminated.patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-197-199 (1)
197-199:⚠️ Potential issue | 🟡 MinorRead
always-onfrom the LED child node, not the parent device.Line 197 reads
device_property_read_bool(dev, "always-on")from the controller device, but all per-LED configuration should come from the individual child node. Line 186 already demonstrates the correct pattern by reading theregproperty fromchild. This inconsistency prevents individual LEDs from having their ownalways-onsettings.Proposed fix
--- a/drivers/leds/leds-htr3212.c +++ b/drivers/leds/leds-htr3212.c @@ - 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-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch` around lines 197 - 199, The code currently calls device_property_read_bool(dev, "always-on") against the controller device; read the per-LED property from the child node instead. Replace the call to device_property_read_bool(dev, "always-on") with device_property_read_bool(child, "always-on") so each LED's cdev->flags |= LED_CORE_SUSPENDRESUME is set based on the child node's "always-on" property (same semantics as the existing reg read from child).patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch-107-119 (1)
107-119:⚠️ Potential issue | 🟡 MinorAvoid dereferencing
*dsp_dataon failed DSP reads.If
aw_dev_dsp_read_16bit()/aw_dev_dsp_read_32bit()fails,*dsp_datamay be uninitialized but is still logged. That can leak garbage values and make failures harder to debug.🔧 Suggested fix
static int __aw_dev_dsp_read(struct aw_device *aw_dev, unsigned short dsp_addr, unsigned int *dsp_data, unsigned char data_type) { u32 reg_value; int ret; mutex_lock(&aw_dev->dsp_lock); + *dsp_data = 0; switch (data_type) { case AW88166_DSP_16_DATA: ret = aw_dev_dsp_read_16bit(aw_dev, dsp_addr, dsp_data); if (ret) - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", - (u32)dsp_addr, *dsp_data); + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit failed, ret=%d", + (u32)dsp_addr, ret); break; case AW88166_DSP_32_DATA: ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); if (ret) - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", - (u32)dsp_addr, *dsp_data); + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit failed, ret=%d", + (u32)dsp_addr, ret); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch` around lines 107 - 119, The error log prints *dsp_data even when aw_dev_dsp_read_16bit()/aw_dev_dsp_read_32bit() fails and dsp_data may be uninitialized; update the error branches inside the switch (cases AW88166_DSP_16_DATA and AW88166_DSP_32_DATA) to avoid dereferencing *dsp_data on failure—use dev_err with only the dsp_addr (or a placeholder/hex 0) and the return code/ret to report the failure instead of printing *dsp_data, keeping the mutex_lock(&aw_dev->dsp_lock) and existing calls to aw_dev_dsp_read_16bit/aw_dev_dsp_read_32bit unchanged.patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch-118-119 (1)
118-119:⚠️ Potential issue | 🟡 MinorRemove the redundant
>= 0check for unsigned variable.At line 118, since
channelis declared asunsigned int(line 109), the conditionchannel >= 0is always true. Simplify to justchannel <= 2.Proposed fix
- if (channel >= 0 && channel <= 2) + if (channel <= 2) reg = 0, bit = channel + 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch` around lines 118 - 119, The conditional checking an unsigned variable `channel` includes a redundant `channel >= 0` test; update the conditional in the PWM handling where `reg` and `bit` are set (the block containing "if (channel >= 0 && channel <= 2) reg = 0, bit = channel + 3;") to remove the always-true `channel >= 0` and use only `channel <= 2`, leaving the assignment to `reg` and `bit` unchanged.
🤖 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-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 101-109: The code currently uses a function-static prev_states
array shared across devices, causing cross-device key-state bleed; add a
per-device key-state field to struct rsinput_driver (e.g., uint8_t
prev_states[...]) instead of static storage, remove the static prev_states,
initialize the new driver->prev_states in the probe/rsinput_driver setup, and
update all uses of prev_states (the code block around the receive/processing
functions at lines referenced as 209-237) to access the per-device field via the
rsinput_driver pointer (e.g., drv->prev_states) so each serdev device maintains
its own key history.
- Around line 157-181: rsinput_init_commands contains blocking msleep calls and
is invoked from the serial receive callback rsinput_rx on checksum failure; move
this synchronous re-init out of the RX path and run it in a deferred worker.
Create a worker function (e.g. rsinput_reinit_work_func) that calls the existing
rsinput_init_commands logic (remove msleep from the RX path), add a work_struct
(or delayed_work) to struct rsinput_driver and initialize it during driver
probe, and change rsinput_rx to queue the work (schedule_work/queue_work or
queue_delayed_work) instead of calling rsinput_init_commands directly; also add
a guard flag (atomic or bool + lock) in rsinput_driver to avoid queuing
duplicate re-init work.
- Around line 209-240: The CMD_STATUS handler reads up through FRAME_POS_DATA_14
but currently only checks payload_length >= 6; update the guard in
handle_cmd_status to require the full number of bytes used (e.g. payload_length
>= (FRAME_POS_DATA_14 + 1) or the explicit required byte count) before accessing
FRAME_POS_DATA_3..FRAME_POS_DATA_14, and keep the existing else branch that
warns on short frames; locate this in the handle_cmd_status function and change
only the payload_length check so all field accesses are bounds-checked.
- Around line 333-352: The GPIO acquisition code uses
devm_gpiod_get_optional(&serdev->dev, "boot"/"enable"/"reset", GPIOD_OUT_HIGH)
but only logs warnings on IS_ERR() and leaves error pointers in drv->boot_gpio /
drv->enable_gpio / drv->reset_gpio; change each IS_ERR branch to propagate the
error by setting error = PTR_ERR(...); dev_warn(...); return error (or goto
error_cleanup) so the probe fails instead of continuing with error-encoded
pointers that later get dereferenced (search for the probe function that
performs these devm_gpiod_get_optional calls and update those IS_ERR handlers to
return the PTR_ERR).
In
`@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 394-409: The backlight callback icna3512_bl_update_status clears
MIPI_DSI_MODE_LPM but returns early on error, leaving dsi->mode_flags mutated;
change it to save the original mode_flags before modifying, clear the LPM bit,
perform mipi_dsi_dcs_set_display_brightness_large, and always restore the
original mode_flags before every return (including error paths). Apply the same
pattern to the other backlight callback that manipulates dsi->mode_flags (the
one in the 411-426 range) so both backlight ops preserve and restore mode_flags
on success and failure.
- Around line 286-299: icna3512_prepare() currently returns early on failures
after calling drm_dsc_pps_payload_pack / mipi_dsi_picture_parameter_set /
mipi_dsi_compression_mode, leaving the panel partially powered; modify
icna3512_prepare to unwind the power/reset/regulator state before returning on
error: after any failed call to mipi_dsi_picture_parameter_set or
mipi_dsi_compression_mode, call the corresponding power-off and reset cleanup
routines used in icna3512_unprepare (disable regulators, deassert/enable reset
GPIOs, and revert any runtime PM state) so the panel is fully powered down on
error; use the same helper functions or sequence that icna3512_unprepare uses to
avoid duplication.
In
`@patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Around line 61-69: Add the missing regulator consumer header so calls to
devm_regulator_get, regulator_enable, and regulator_disable are properly
declared: include <linux/regulator/consumer.h> with the other headers at the top
of the patch (near the existing includes in the file), ensuring the regulator
consumer APIs are available to the compilation unit referencing
devm_regulator_get, regulator_enable, and regulator_disable.
- Around line 292-309: After enabling the regulator with
regulator_enable(priv->vdd) and asserting SDB via
gpiod_set_value_cansleep(priv->sdb, 1), failures from htr3212_init_regs(priv) or
htr3212_parse_dt(dev, priv) must unwind the power state; add an error/cleanup
label (e.g., err_disable_vdd) and on any non-zero ret from htr3212_init_regs or
htr3212_parse_dt jump to that label which will deassert SDB
(gpiod_set_value_cansleep(priv->sdb, 0)) and call regulator_disable(priv->vdd)
before returning the error, ensuring you still set
priv->client/i2c_set_clientdata(client, priv) only when probe will continue or
undo them in the cleanup path as needed.
In
`@patch/kernel/archive/sm8550-6.19/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 45-55: The startup path currently ignores return values and can
leave pdata->i2s_clk enabled on error; update the startup function so that each
call (clk_prepare_enable(pdata->i2s_clk), snd_soc_dai_set_fmt(cpu_dai,...),
snd_soc_dai_set_fmt(codec_dai,...), and qcom_snd_sdw_startup(substream)) checks
its return value, and on any failure calls clk_disable_unprepare(pdata->i2s_clk)
(if clk was successfully prepared) before returning the error code; ensure you
only call clk_disable_unprepare if clk_prepare_enable succeeded so the clock is
always unwound on startup failure.
In
`@patch/kernel/archive/sm8550-6.19/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch`:
- Around line 31-32: The qcom,use-level-shifter property currently only has a
description but lacks a type; update the device-tree binding entry for
qcom,use-level-shifter to include "type: boolean" alongside its description so
it conforms to the kernel dt-bindings meta-schema (i.e., add type: boolean to
the qcom,use-level-shifter property definition to satisfy dt-validate).
In
`@patch/kernel/archive/sm8550-6.19/0008-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch`:
- Around line 218-223: The panel's full power-down (td4328_unprepare) is
incorrectly bound to .disable; move the regulator/shutdown logic out of the
.disable callback and bind td4328_unprepare to the panel .unprepare hook
instead: update the td4328_panel_funcs struct to set .unprepare =
td4328_unprepare and ensure .disable only calls the lightweight
backlight/display-off routine (e.g., td4328_off or a dedicated backlight-off
helper) so regulators are only disabled during td4328_unprepare.
In `@patch/kernel/archive/sm8550-6.19/0011-HACK-fix-usb-boot-hang.patch`:
- Around line 19-30: The removal of the XHCI_STS_CNR handshake in
quirk_usb_handoff_xhci is applied globally and must be scoped or bounded:
restore the handshake call to wait for XHCI_STS_CNR on op_reg_base +
XHCI_STS_OFFSET but either (1) wrap the bypass only for the SM8550 device by
checking the PCI vendor/device ID on pdev (use the existing pdev pointer) and
skip the long wait only for that device, or (2) keep the handshake for all
devices but reduce the timeout from 5000000 to a short bounded value (e.g.
100000–200000 µs) so the code still observes XHCI_STS_CNR as required by the
spec while avoiding a long 5s stall; ensure the dev_warn path using val =
readl(op_reg_base + XHCI_STS_OFFSET) and pdev->dev remains for diagnostics when
timeout occurs.
In
`@patch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 248-261: The serial-derived branch (generate_macaddr_from_serial
-> generated_macaddr) currently writes the same MAC into ar->mac_addr for every
pdev, dropping the prior per-pdev uniqueness; after calling
ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b) and logging via
ath12k_info(ab,...), add the same per-pdev differentiation that the old path
used by incrementing the appropriate octet (e.g., ar->mac_addr[4] +=
ar->pdev_idx) so each pdev gets a unique MAC while preserving the
reversed/generated base address.
- Around line 272-275: Replace the exported raw global qcom_serial_number with a
guarded accessor API: remove EXPORT_SYMBOL(qcom_serial_number) and add a
function const char *qcom_get_serial_number(void) that returns the serial when
CONFIG_QCOM_SOCINFO is enabled and a NULL stub when disabled; update consumers
in BT_QCA and ATH12K to call qcom_get_serial_number() and null-check the result
before use, and ensure the SOCINFO driver that currently assigns/clears
qcom_serial_number continues to manage lifetime but no longer exposes the raw
pointer directly.
- Around line 124-127: The code currently returns the error from
qca_set_bdaddr(hdev, &generated_bdaddr) which aborts setup instead of falling
back to using the NVM BD_ADDR; change the handling in the qca_set_bdaddr block
so that on failure you log a warning (bt_dev_warn/bt_dev_err with the error
detail) but do not return—allow execution to continue so the NVM BD_ADDR path
can be used (i.e., remove the return err and clear/ignore err to proceed after
qca_set_bdaddr failure). Ensure the change references qca_set_bdaddr,
generated_bdaddr and hdev so readers can find the exact spot.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 171-181: The write loop assumes 16-bit words but doesn't validate
that len is even; add a check before locking/looping (referencing len and data)
to ensure len % 2 == 0 and return or handle an error if it is odd, so the
cpu_to_be16p cast and regmap_write calls (used with AW88166_DSPMADD_REG,
AW88166_DSPMDAT_REG, aw_dev->regmap and aw_dev->dsp_lock) never read past the
buffer; update the error path to cleanly unlock or abort before entering the for
(i = 0; i < len; i += 2) loop.
- Around line 209-237: The current SRAM check ignores return values from
aw_dev_dsp_read_16bit/aw_dev_dsp_write_16bit so transport I/O errors leave
reg_val stale and get masked as -EPERM; update the code around the
AW88166_DSP_ROM_CHECK_ADDR and AW88166_DSP_CFG_ADDR checks to capture returns
(e.g., int ret = aw_dev_dsp_read_16bit(...)/aw_dev_dsp_write_16bit(...)), check
if ret < 0 and on error unlock aw_dev->dsp_lock and return that ret (or
propagate it via a local ret variable to the common error path) instead of
jumping to error that always returns -EPERM, while keeping the existing dev_err
checks and mutex unlock semantics for the success and failure paths.
In
`@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch`:
- Around line 358-373: The teardown currently disables device power
(regulator_disable(priv->vdd)) while the PWM chip is still registered, leaving a
window where sn3112_pwm_apply() may run and call sn3112_write_reg() against a
powered-off device; to fix this, call pwmchip_remove(chip) before
regulator_disable(priv->vdd) (and before any further writes like
sn3112_write_reg to SN3112_REG_APPLY/ENABLE), keeping the GPIO shutdown
(gpiod_set_value(priv->sdb, 1)) sequence unchanged but ensuring the chip is
unregistered first to eliminate the race.
- Around line 85-86: Replace use of stdint's uint8_t with the kernel-native u8
type: change the declarations of pwm_val (SN3112_CHANNELS array) and
pwm_en_reg[3] (and the other occurrence at the location referenced around line
145) from uint8_t to u8 so the driver uses kernel types (look for the symbols
pwm_val and pwm_en_reg and the other uint8_t occurrence and update them to u8).
In
`@patch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 21-26: The DTB Makefile is missing the portal DTB entry: add the
qcs8550-ayn-odin2portal.dtb build target to the same dtb-$(CONFIG_ARCH_QCOM)
list so the newly added qcs8550-ayn-odin2portal.dts is compiled; specifically
update the QCOM DTB block to include "qcs8550-ayn-odin2portal.dtb" alongside the
existing qcs8550-ayn-odin2.dtb entry.
---
Minor comments:
In
`@patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Line 348: The MODULE_AUTHOR metadata line is malformed: fix the
MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); entry by adding the missing
closing '>' to the email so it becomes MODULE_AUTHOR("Teguh Sobirin
<teguh@sobir.in>"); ensuring the author string is properly terminated.
- Around line 197-199: The code currently calls device_property_read_bool(dev,
"always-on") against the controller device; read the per-LED property from the
child node instead. Replace the call to device_property_read_bool(dev,
"always-on") with device_property_read_bool(child, "always-on") so each LED's
cdev->flags |= LED_CORE_SUSPENDRESUME is set based on the child node's
"always-on" property (same semantics as the existing reg read from child).
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 107-119: The error log prints *dsp_data even when
aw_dev_dsp_read_16bit()/aw_dev_dsp_read_32bit() fails and dsp_data may be
uninitialized; update the error branches inside the switch (cases
AW88166_DSP_16_DATA and AW88166_DSP_32_DATA) to avoid dereferencing *dsp_data on
failure—use dev_err with only the dsp_addr (or a placeholder/hex 0) and the
return code/ret to report the failure instead of printing *dsp_data, keeping the
mutex_lock(&aw_dev->dsp_lock) and existing calls to
aw_dev_dsp_read_16bit/aw_dev_dsp_read_32bit unchanged.
In
`@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch`:
- Around line 118-119: The conditional checking an unsigned variable `channel`
includes a redundant `channel >= 0` test; update the conditional in the PWM
handling where `reg` and `bit` are set (the block containing "if (channel >= 0
&& channel <= 2) reg = 0, bit = channel + 3;") to remove the always-true
`channel >= 0` and use only `channel <= 2`, leaving the assignment to `reg` and
`bit` unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
config/sources/families/sm8550.confpatch/kernel/archive/sm8550-6.19/0001-arm64-dts-qcom-sm8550-add-UART15.patchpatch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patchpatch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patchpatch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patchpatch/kernel/archive/sm8550-6.19/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patchpatch/kernel/archive/sm8550-6.19/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patchpatch/kernel/archive/sm8550-6.19/0007-clk-qcom-gcc-sm8550-Keep-UFS-PHY-GDSCs-ALWAYS_ON.patchpatch/kernel/archive/sm8550-6.19/0008-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patchpatch/kernel/archive/sm8550-6.19/0009-arm64-dts-qcom-Added-pmk8550-pwm-support.patchpatch/kernel/archive/sm8550-6.19/0010-drivers-power-Fix-name-for-sc8280xp-battery.patchpatch/kernel/archive/sm8550-6.19/0011-HACK-fix-usb-boot-hang.patchpatch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patchpatch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patchpatch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patchpatch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
Comment on lines
+101
to
+109
| +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; | ||
| +}; |
Contributor
There was a problem hiding this comment.
Move key-state tracking from static storage into device state.
prev_states is currently function-static, so all devices share one key history. This can cause cross-device state bleed and wrong key transitions.
🔧 Proposed fix
struct rsinput_driver {
struct serdev_device *serdev;
struct input_dev *input;
@@
uint8_t rx_buf[256];
uint8_t sequence_number;
+ unsigned long prev_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;
@@
- bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap));
+ bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap));
@@
- prev_states = keys;
+ drv->prev_states = keys;Also applies to: 209-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 101 - 109, The code currently uses a function-static prev_states
array shared across devices, causing cross-device key-state bleed; add a
per-device key-state field to struct rsinput_driver (e.g., uint8_t
prev_states[...]) instead of static storage, remove the static prev_states,
initialize the new driver->prev_states in the probe/rsinput_driver setup, and
update all uses of prev_states (the code block around the receive/processing
functions at lines referenced as 209-237) to access the per-device field via the
rsinput_driver pointer (e.g., drv->prev_states) so each serdev device maintains
its own key history.
Comment on lines
+157
to
+181
| +static int rsinput_init_commands(struct rsinput_driver *drv) { | ||
| + int error; | ||
| + | ||
| + msleep(100); | ||
| + uint8_t version_request[] = {DATA_COMMOD_VERSION}; | ||
| + error = rsinput_send_command(drv, CMD_COMMOD, version_request, sizeof(version_request)); | ||
| + if (error < 0) { | ||
| + dev_err(&drv->serdev->dev, "Failed to request MCU version: %d\n", error); | ||
| + return error; | ||
| + } | ||
| + | ||
| + msleep(100); | ||
| + uint8_t mcu_params[] = { | ||
| + DATA_COMMOD_SET_PAR, | ||
| + 0x01, | ||
| + 0x00, 0x00, 0x00, 0x28, | ||
| + 0x00, 0x00, 0x00, 0x07 | ||
| + }; | ||
| + error = rsinput_send_command(drv, CMD_COMMOD, mcu_params, sizeof(mcu_params)); | ||
| + if (error < 0) { | ||
| + dev_err(&drv->serdev->dev, "Failed to set MCU parameters: %d\n", error); | ||
| + return error; | ||
| + } | ||
| + | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the patch file exists and explore its content
find . -name "0002-input-Add-driver-for-RSInput-Gamepad.patch" -type fRepository: armbian/build
Length of output: 222
🏁 Script executed:
# Find the rsinput_rx function and check for msleep calls
rg -n 'rsinput_rx|rsinput_init_commands|msleep' patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch | head -100Repository: armbian/build
Length of output: 552
🏁 Script executed:
# Extract context around line 307 where rsinput_init_commands is called from rsinput_rx
sed -n '300,315p' patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 383
🏁 Script executed:
# Look at the lines 281-309 referenced in the "Also applies to" part
sed -n '281,320p' patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 1155
Remove sleeping re-init from RX callback path.
rsinput_rx() is a serial device receive callback that invokes rsinput_init_commands() on checksum mismatch (line 307). That function contains blocking msleep(100) calls (lines 160, 168), which should never execute in a receive callback context. Recovery must be deferred to a worker, not executed synchronously in the RX path.
Minimal safe fix (remove blocking init from RX path)
- if (computed_checksum != received_checksum) {
- rsinput_init_commands(drv);
- goto error;
- }
+ if (computed_checksum != received_checksum)
+ goto error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 157 - 181, rsinput_init_commands contains blocking msleep calls and
is invoked from the serial receive callback rsinput_rx on checksum failure; move
this synchronous re-init out of the RX path and run it in a deferred worker.
Create a worker function (e.g. rsinput_reinit_work_func) that calls the existing
rsinput_init_commands logic (remove msleep from the RX path), add a work_struct
(or delayed_work) to struct rsinput_driver and initialize it during driver
probe, and change rsinput_rx to queue the work (schedule_work/queue_work or
queue_delayed_work) instead of calling rsinput_init_commands directly; also add
a guard flag (atomic or bool + lock) in rsinput_driver to avoid queuing
duplicate re-init work.
Comment on lines
+209
to
+240
| +static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { | ||
| + if (payload_length >= 6) { | ||
| + 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; | ||
| + int i; | ||
| + | ||
| + bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); | ||
| + | ||
| + for_each_set_bit(i, &changes, ARRAY_SIZE(keymap)) { | ||
| + input_report_key(drv->input, keymap[i], (current_states & BIT(i))); | ||
| + } | ||
| + | ||
| + input_report_abs(drv->input, ABS_HAT2X, | ||
| + 0x650 - (data[FRAME_POS_DATA_3] | (data[FRAME_POS_DATA_4] << 8))); | ||
| + input_report_abs(drv->input, ABS_HAT2Y, | ||
| + 0x650 - (data[FRAME_POS_DATA_5] | (data[FRAME_POS_DATA_6] << 8))); | ||
| + input_report_abs(drv->input, ABS_X, | ||
| + -(int16_t)(data[FRAME_POS_DATA_7] | (data[FRAME_POS_DATA_8] << 8))); | ||
| + input_report_abs(drv->input, ABS_Y, | ||
| + -(int16_t)(data[FRAME_POS_DATA_9] | (data[FRAME_POS_DATA_10] << 8))); | ||
| + input_report_abs(drv->input, ABS_RX, | ||
| + -(int16_t)(data[FRAME_POS_DATA_11] | (data[FRAME_POS_DATA_12] << 8))); | ||
| + input_report_abs(drv->input, ABS_RY, | ||
| + -(int16_t)(data[FRAME_POS_DATA_13] | (data[FRAME_POS_DATA_14] << 8))); | ||
| + | ||
| + input_sync(drv->input); | ||
| + prev_states = keys; | ||
| + | ||
| + } else { | ||
| + dev_warn(&drv->serdev->dev, "Invalid CMD_STATUS response length\n"); | ||
| + } |
Contributor
There was a problem hiding this comment.
Tighten CMD_STATUS payload bounds before field access.
The parser reads fields through FRAME_POS_DATA_14, but the guard only requires 6 bytes. This can read beyond the payload for short frames.
🔧 Proposed fix
-static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) {
- if (payload_length >= 6) {
+static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) {
+ if (payload_length >= 14) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 209 - 240, The CMD_STATUS handler reads up through
FRAME_POS_DATA_14 but currently only checks payload_length >= 6; update the
guard in handle_cmd_status to require the full number of bytes used (e.g.
payload_length >= (FRAME_POS_DATA_14 + 1) or the explicit required byte count)
before accessing FRAME_POS_DATA_3..FRAME_POS_DATA_14, and keep the existing else
branch that warns on short frames; locate this in the handle_cmd_status function
and change only the payload_length check so all field accesses are
bounds-checked.
Comment on lines
+333
to
+352
| + drv->boot_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->boot_gpio)) { | ||
| + error = PTR_ERR(drv->boot_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->enable_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->enable_gpio)) { | ||
| + error = PTR_ERR(drv->enable_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->reset_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->reset_gpio)) { | ||
| + error = PTR_ERR(drv->reset_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); | ||
| + } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the patch file exists and examine its structure
fd -n "0002-input-Add-driver-for-RSInput-Gamepad.patch" patch/kernel/archive/sm8550-6.19/Repository: armbian/build
Length of output: 287
🏁 Script executed:
# Read the relevant section of the patch to understand error handling
cat -n "patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch" | sed -n '320,360p'Repository: armbian/build
Length of output: 1617
🏁 Script executed:
# Search for gpiod_set_value_cansleep usage in the patch
rg -n "gpiod_set_value_cansleep" "patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch"Repository: armbian/build
Length of output: 411
🏁 Script executed:
# Look at the lines mentioned in "Also applies to" section
cat -n "patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch" | sed -n '350,370p'Repository: armbian/build
Length of output: 761
🏁 Script executed:
# Check if there's any control flow after the GPIO acquisition errors
# Get a broader view of the function
cat -n "patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch" | sed -n '300,450p'Repository: armbian/build
Length of output: 5516
Errors from optional GPIO acquisition must fail the probe, not just warn.
When devm_gpiod_get_optional() fails, it returns an error-encoded pointer (checked by IS_ERR()). The current code only logs a warning and continues, leaving the pointer in an error state. Later, the guards like if (drv->boot_gpio) do not protect against this—error pointers are non-NULL and will pass the check, causing gpiod_set_value_cansleep() to dereference an invalid pointer and crash.
🔧 Proposed fix
drv->boot_gpio =
devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH);
- if (IS_ERR(drv->boot_gpio)) {
- error = PTR_ERR(drv->boot_gpio);
- dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error);
- }
+ if (IS_ERR(drv->boot_gpio))
+ return dev_err_probe(&serdev->dev, PTR_ERR(drv->boot_gpio),
+ "Unable to get boot gpio\n");
@@
drv->enable_gpio =
devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH);
- if (IS_ERR(drv->enable_gpio)) {
- error = PTR_ERR(drv->enable_gpio);
- dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error);
- }
+ if (IS_ERR(drv->enable_gpio))
+ return dev_err_probe(&serdev->dev, PTR_ERR(drv->enable_gpio),
+ "Unable to get enable gpio\n");
@@
drv->reset_gpio =
devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(drv->reset_gpio)) {
- error = PTR_ERR(drv->reset_gpio);
- dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error);
- }
+ if (IS_ERR(drv->reset_gpio))
+ return dev_err_probe(&serdev->dev, PTR_ERR(drv->reset_gpio),
+ "Unable to get reset gpio\n");Also applies to lines 362–367 and 443–447 where the same GPIO pointers are dereferenced.
📝 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.
Suggested change
| + drv->boot_gpio = | |
| + devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); | |
| + if (IS_ERR(drv->boot_gpio)) { | |
| + error = PTR_ERR(drv->boot_gpio); | |
| + dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); | |
| + } | |
| + | |
| + drv->enable_gpio = | |
| + devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); | |
| + if (IS_ERR(drv->enable_gpio)) { | |
| + error = PTR_ERR(drv->enable_gpio); | |
| + dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); | |
| + } | |
| + | |
| + drv->reset_gpio = | |
| + devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); | |
| + if (IS_ERR(drv->reset_gpio)) { | |
| + error = PTR_ERR(drv->reset_gpio); | |
| + dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); | |
| + } | |
| drv->boot_gpio = | |
| devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); | |
| if (IS_ERR(drv->boot_gpio)) | |
| return dev_err_probe(&serdev->dev, PTR_ERR(drv->boot_gpio), | |
| "Unable to get boot gpio\n"); | |
| drv->enable_gpio = | |
| devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); | |
| if (IS_ERR(drv->enable_gpio)) | |
| return dev_err_probe(&serdev->dev, PTR_ERR(drv->enable_gpio), | |
| "Unable to get enable gpio\n"); | |
| drv->reset_gpio = | |
| devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); | |
| if (IS_ERR(drv->reset_gpio)) | |
| return dev_err_probe(&serdev->dev, PTR_ERR(drv->reset_gpio), | |
| "Unable to get reset gpio\n"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 333 - 352, The GPIO acquisition code uses
devm_gpiod_get_optional(&serdev->dev, "boot"/"enable"/"reset", GPIOD_OUT_HIGH)
but only logs warnings on IS_ERR() and leaves error pointers in drv->boot_gpio /
drv->enable_gpio / drv->reset_gpio; change each IS_ERR branch to propagate the
error by setting error = PTR_ERR(...); dev_warn(...); return error (or goto
error_cleanup) so the probe fails instead of continuing with error-encoded
pointers that later get dereferenced (search for the probe function that
performs these devm_gpiod_get_optional calls and update those IS_ERR handlers to
return the PTR_ERR).
Comment on lines
+286
to
+299
| + drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc); | ||
| + | ||
| + ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + /* Not required, ICNA3512 has DSC always enabled. */ | ||
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | ||
| + return ret; | ||
| + } |
Contributor
There was a problem hiding this comment.
Unwind regulator/reset state on late prepare failures.
If PPS or compression setup fails, icna3512_prepare() exits without powering down, leaving the panel in a partial-on state.
🔧 Proposed fix
ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps);
if (ret < 0) {
dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
- return ret;
+ goto err_poweroff;
}
@@
ret = mipi_dsi_compression_mode(pinfo->dsi, true);
if (ret < 0) {
dev_err(panel->dev, "failed to enable compression mode: %d\n", ret);
- return ret;
+ goto err_poweroff;
}
return 0;
+
+err_poweroff:
+ gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
+ regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies);
+ return ret;📝 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.
Suggested change
| + drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc); | |
| + | |
| + ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| + return ret; | |
| + } | |
| + | |
| + /* Not required, ICNA3512 has DSC always enabled. */ | |
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| + return ret; | |
| + } | |
| drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc); | |
| ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| goto err_poweroff; | |
| } | |
| /* Not required, ICNA3512 has DSC always enabled. */ | |
| ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| goto err_poweroff; | |
| } | |
| return 0; | |
| err_poweroff: | |
| gpiod_set_value_cansleep(pinfo->reset_gpio, 1); | |
| regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); | |
| return ret; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`
around lines 286 - 299, icna3512_prepare() currently returns early on failures
after calling drm_dsc_pps_payload_pack / mipi_dsi_picture_parameter_set /
mipi_dsi_compression_mode, leaving the panel partially powered; modify
icna3512_prepare to unwind the power/reset/regulator state before returning on
error: after any failed call to mipi_dsi_picture_parameter_set or
mipi_dsi_compression_mode, call the corresponding power-off and reset cleanup
routines used in icna3512_unprepare (disable regulators, deassert/enable reset
GPIOs, and revert any runtime PM state) so the panel is fully powered down on
error; use the same helper functions or sequence that icna3512_unprepare uses to
avoid duplication.
Comment on lines
+171
to
+181
| + mutex_lock(&aw_dev->dsp_lock); | ||
| + /* i2c write */ | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + for (i = 0; i < len; i += 2) { | ||
| + reg_val = cpu_to_be16p((u16 *)(data + i)); | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | ||
| + (u16)reg_val); | ||
| + if (ret) | ||
| + goto error_operation; |
Contributor
There was a problem hiding this comment.
Validate 16-bit payload alignment before the i += 2 write loop.
The loop writes 16-bit words, but len is not validated. If len is odd, the last iteration reads beyond the buffer.
🛡️ Suggested fix
mutex_lock(&aw_dev->dsp_lock);
/* i2c write */
ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base);
if (ret)
goto error_operation;
+ if (len & 0x1) {
+ dev_err(aw_dev->dev, "invalid dsp payload length: %u", len);
+ ret = -EINVAL;
+ goto error_operation;
+ }
for (i = 0; i < len; i += 2) {
reg_val = cpu_to_be16p((u16 *)(data + i));
ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG,
(u16)reg_val);📝 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.
Suggested change
| + mutex_lock(&aw_dev->dsp_lock); | |
| + /* i2c write */ | |
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | |
| + if (ret) | |
| + goto error_operation; | |
| + for (i = 0; i < len; i += 2) { | |
| + reg_val = cpu_to_be16p((u16 *)(data + i)); | |
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | |
| + (u16)reg_val); | |
| + if (ret) | |
| + goto error_operation; | |
| mutex_lock(&aw_dev->dsp_lock); | |
| /* i2c write */ | |
| ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | |
| if (ret) | |
| goto error_operation; | |
| if (len & 0x1) { | |
| dev_err(aw_dev->dev, "invalid dsp payload length: %u", len); | |
| ret = -EINVAL; | |
| goto error_operation; | |
| } | |
| for (i = 0; i < len; i += 2) { | |
| reg_val = cpu_to_be16p((u16 *)(data + i)); | |
| ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | |
| (u16)reg_val); | |
| if (ret) | |
| goto error_operation; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 171 - 181, The write loop assumes 16-bit words but doesn't validate
that len is even; add a check before locking/looping (referencing len and data)
to ensure len % 2 == 0 and return or handle an error if it is odd, so the
cpu_to_be16p cast and regmap_write calls (used with AW88166_DSPMADD_REG,
AW88166_DSPMDAT_REG, aw_dev->regmap and aw_dev->dsp_lock) never read past the
buffer; update the error path to cleanly unlock or abort before entering the for
(i = 0; i < len; i += 2) loop.
Comment on lines
+209
to
+237
| + mutex_lock(&aw_dev->dsp_lock); | ||
| /* read dsp_rom_check_reg */ | ||
| - aw_dev_dsp_read(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val, AW_DSP_16_DATA); | ||
| + aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val); | ||
| if (reg_val != AW88166_DSP_ROM_CHECK_DATA) { | ||
| dev_err(aw_dev->dev, "check dsp rom failed, read[0x%x] != check[0x%x]\n", | ||
| reg_val, AW88166_DSP_ROM_CHECK_DATA); | ||
| - return -EPERM; | ||
| + goto error; | ||
| } | ||
|
|
||
| /* check dsp_cfg_base_addr */ | ||
| - aw_dev_dsp_write(aw_dev, AW88166_DSP_CFG_ADDR, | ||
| - AW88166_DSP_ODD_NUM_BIT_TEST, AW_DSP_16_DATA); | ||
| - aw_dev_dsp_read(aw_dev, AW88166_DSP_CFG_ADDR, ®_val, AW_DSP_16_DATA); | ||
| + aw_dev_dsp_write_16bit(aw_dev, AW88166_DSP_CFG_ADDR, AW88166_DSP_ODD_NUM_BIT_TEST); | ||
| + aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_CFG_ADDR, ®_val); | ||
| if (reg_val != AW88166_DSP_ODD_NUM_BIT_TEST) { | ||
| dev_err(aw_dev->dev, "check dsp cfg failed, read[0x%x] != write[0x%x]\n", | ||
| reg_val, AW88166_DSP_ODD_NUM_BIT_TEST); | ||
| - return -EPERM; | ||
| + goto error; | ||
| } | ||
| + mutex_unlock(&aw_dev->dsp_lock); | ||
|
|
||
| return 0; | ||
| +error: | ||
| + mutex_unlock(&aw_dev->dsp_lock); | ||
| + return -EPERM; |
Contributor
There was a problem hiding this comment.
Propagate transport errors in SRAM check instead of masking them as -EPERM.
aw_dev_dsp_read_16bit()/aw_dev_dsp_write_16bit() return values are ignored. On I/O failure, reg_val can be stale and the function returns a misleading permission error.
🔧 Suggested fix
static int aw_dev_check_sram(struct aw_device *aw_dev)
{
unsigned int reg_val;
+ int ret;
mutex_lock(&aw_dev->dsp_lock);
/* read dsp_rom_check_reg */
- aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val);
+ ret = aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val);
+ if (ret)
+ goto io_error;
if (reg_val != AW88166_DSP_ROM_CHECK_DATA) {
dev_err(aw_dev->dev, "check dsp rom failed, read[0x%x] != check[0x%x]\n",
reg_val, AW88166_DSP_ROM_CHECK_DATA);
goto error;
}
/* check dsp_cfg_base_addr */
- aw_dev_dsp_write_16bit(aw_dev, AW88166_DSP_CFG_ADDR, AW88166_DSP_ODD_NUM_BIT_TEST);
- aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_CFG_ADDR, ®_val);
+ ret = aw_dev_dsp_write_16bit(aw_dev, AW88166_DSP_CFG_ADDR, AW88166_DSP_ODD_NUM_BIT_TEST);
+ if (ret)
+ goto io_error;
+ ret = aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_CFG_ADDR, ®_val);
+ if (ret)
+ goto io_error;
if (reg_val != AW88166_DSP_ODD_NUM_BIT_TEST) {
dev_err(aw_dev->dev, "check dsp cfg failed, read[0x%x] != write[0x%x]\n",
reg_val, AW88166_DSP_ODD_NUM_BIT_TEST);
goto error;
}
mutex_unlock(&aw_dev->dsp_lock);
return 0;
error:
mutex_unlock(&aw_dev->dsp_lock);
return -EPERM;
+io_error:
+ mutex_unlock(&aw_dev->dsp_lock);
+ return ret;
}📝 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.
Suggested change
| + mutex_lock(&aw_dev->dsp_lock); | |
| /* read dsp_rom_check_reg */ | |
| - aw_dev_dsp_read(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val, AW_DSP_16_DATA); | |
| + aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val); | |
| if (reg_val != AW88166_DSP_ROM_CHECK_DATA) { | |
| dev_err(aw_dev->dev, "check dsp rom failed, read[0x%x] != check[0x%x]\n", | |
| reg_val, AW88166_DSP_ROM_CHECK_DATA); | |
| - return -EPERM; | |
| + goto error; | |
| } | |
| /* check dsp_cfg_base_addr */ | |
| - aw_dev_dsp_write(aw_dev, AW88166_DSP_CFG_ADDR, | |
| - AW88166_DSP_ODD_NUM_BIT_TEST, AW_DSP_16_DATA); | |
| - aw_dev_dsp_read(aw_dev, AW88166_DSP_CFG_ADDR, ®_val, AW_DSP_16_DATA); | |
| + aw_dev_dsp_write_16bit(aw_dev, AW88166_DSP_CFG_ADDR, AW88166_DSP_ODD_NUM_BIT_TEST); | |
| + aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_CFG_ADDR, ®_val); | |
| if (reg_val != AW88166_DSP_ODD_NUM_BIT_TEST) { | |
| dev_err(aw_dev->dev, "check dsp cfg failed, read[0x%x] != write[0x%x]\n", | |
| reg_val, AW88166_DSP_ODD_NUM_BIT_TEST); | |
| - return -EPERM; | |
| + goto error; | |
| } | |
| + mutex_unlock(&aw_dev->dsp_lock); | |
| return 0; | |
| +error: | |
| + mutex_unlock(&aw_dev->dsp_lock); | |
| + return -EPERM; | |
| static int aw_dev_check_sram(struct aw_device *aw_dev) | |
| { | |
| unsigned int reg_val; | |
| int ret; | |
| mutex_lock(&aw_dev->dsp_lock); | |
| /* read dsp_rom_check_reg */ | |
| ret = aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_ROM_CHECK_ADDR, ®_val); | |
| if (ret) | |
| goto io_error; | |
| if (reg_val != AW88166_DSP_ROM_CHECK_DATA) { | |
| dev_err(aw_dev->dev, "check dsp rom failed, read[0x%x] != check[0x%x]\n", | |
| reg_val, AW88166_DSP_ROM_CHECK_DATA); | |
| goto error; | |
| } | |
| /* check dsp_cfg_base_addr */ | |
| ret = aw_dev_dsp_write_16bit(aw_dev, AW88166_DSP_CFG_ADDR, AW88166_DSP_ODD_NUM_BIT_TEST); | |
| if (ret) | |
| goto io_error; | |
| ret = aw_dev_dsp_read_16bit(aw_dev, AW88166_DSP_CFG_ADDR, ®_val); | |
| if (ret) | |
| goto io_error; | |
| if (reg_val != AW88166_DSP_ODD_NUM_BIT_TEST) { | |
| dev_err(aw_dev->dev, "check dsp cfg failed, read[0x%x] != write[0x%x]\n", | |
| reg_val, AW88166_DSP_ODD_NUM_BIT_TEST); | |
| goto error; | |
| } | |
| mutex_unlock(&aw_dev->dsp_lock); | |
| return 0; | |
| error: | |
| mutex_unlock(&aw_dev->dsp_lock); | |
| return -EPERM; | |
| io_error: | |
| mutex_unlock(&aw_dev->dsp_lock); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 209 - 237, The current SRAM check ignores return values from
aw_dev_dsp_read_16bit/aw_dev_dsp_write_16bit so transport I/O errors leave
reg_val stale and get masked as -EPERM; update the code around the
AW88166_DSP_ROM_CHECK_ADDR and AW88166_DSP_CFG_ADDR checks to capture returns
(e.g., int ret = aw_dev_dsp_read_16bit(...)/aw_dev_dsp_write_16bit(...)), check
if ret < 0 and on error unlock aw_dev->dsp_lock and return that ret (or
propagate it via a local ret variable to the common error path) instead of
jumping to error that always returns -EPERM, while keeping the existing dev_err
checks and mutex unlock semantics for the success and failure paths.
Comment on lines
+85
to
+86
| + uint8_t pwm_val[SN3112_CHANNELS]; | ||
| + uint8_t pwm_en_reg[3]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\buint8_t\b' patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patchRepository: armbian/build
Length of output: 162
🌐 Web query:
Linux kernel coding standards u8 vs uint8_t preferred type conventions
💡 Result:
- Inside the Linux kernel (core/drivers):
u8,u16,u32,u64(and signeds*) are explicitly permitted typedefs for “clear integer types”. The coding-style guidance says the Linux-specificu*types are allowed, but not mandatory for new code; when touching existing code, match what that code already uses. (docs.kernel.org) - For kernel↔userspace ABI (UAPI) structs/headers: don’t use
u32/uint32_t; use__u32,__u16, etc., because the kernel “cannot require C99 types” in userspace-visible interfaces. (docs.kernel.org) - About
uint8_t/uint32_t(<stdint.h>): the coding-style doc notes some object to C99uint*_t, and therefore permits the kernel’su*equivalents. (docs.kernel.org) - Practical convention: kernel code generally does not include
<stdint.h>; historically, maintainers have argued against using it in the kernel and favor__u*/u*types to avoid header/namespace issues. (yarchive.net)
Citations:
- 1: https://docs.kernel.org/6.10/process/coding-style.html
- 2: https://docs.kernel.org/6.10/process/coding-style.html
- 3: https://docs.kernel.org/6.10/process/coding-style.html
- 4: https://yarchive.net/comp/linux/kernel_headers.html?utm_source=openai
Use kernel-native u8 types instead of uint8_t.
Kernel driver code should use the kernel's native u8 type instead of uint8_t from <stdint.h>. This avoids type-definition conflicts and follows kernel coding conventions. Replace at lines 85, 86, and 145.
Proposed fix
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];
+ u8 pwm_val[SN3112_CHANNELS];
+ u8 pwm_en_reg[3];
bool pwm_en[SN3112_CHANNELS];
`#if` IS_ENABLED(CONFIG_GPIOLIB)
struct gpio_desc *sdb;
`#endif`
};
@@
static int sn3112_set_val_reg(struct sn3112 *priv, unsigned int channel,
- uint8_t val, bool write)
+ u8 val, bool write)
{Also applies to: 145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch`
around lines 85 - 86, Replace use of stdint's uint8_t with the kernel-native u8
type: change the declarations of pwm_val (SN3112_CHANNELS array) and
pwm_en_reg[3] (and the other occurrence at the location referenced around line
145) from uint8_t to u8 so the driver uses kernel types (look for the symbols
pwm_val and pwm_en_reg and the other uint8_t occurrence and update them to u8).
patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
Outdated
Show resolved
Hide resolved
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (8)
patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch (2)
394-426:⚠️ Potential issue | 🟠 MajorAlways restore
dsi->mode_flagsin both backlight callbacks (including errors).Both callbacks clear
MIPI_DSI_MODE_LPMand can return early on failure, leaking modified flags into later operations.🔧 Proposed fix
static int icna3512_bl_update_status(struct backlight_device *bl) { struct mipi_dsi_device *dsi = bl_get_data(bl); u16 brightness = backlight_get_brightness(bl); + unsigned long old_flags = dsi->mode_flags; int ret; dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness); - if (ret < 0) - return ret; - - dsi->mode_flags |= MIPI_DSI_MODE_LPM; - - return 0; + dsi->mode_flags = old_flags; + return ret < 0 ? ret : 0; } @@ static int icna3512_bl_get_brightness(struct backlight_device *bl) { struct mipi_dsi_device *dsi = bl_get_data(bl); u16 brightness; + unsigned long old_flags = dsi->mode_flags; int ret; dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness); + dsi->mode_flags = old_flags; if (ret < 0) return ret; - dsi->mode_flags |= MIPI_DSI_MODE_LPM; - return brightness; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch` around lines 394 - 426, Both backlight callbacks icna3512_bl_update_status and icna3512_bl_get_brightness clear MIPI_DSI_MODE_LPM and may return early on error, leaving dsi->mode_flags modified; fix by saving the original dsi->mode_flags into a local variable at the start of each function and always restoring it before any return (including error returns) — e.g., wrap the mipi_dsi_dcs_set_display_brightness_large and mipi_dsi_dcs_get_display_brightness_large calls so that on both success and failure the saved flags are written back to dsi->mode_flags (use a single exit path or goto/cleanup block to ensure restoration).
286-299:⚠️ Potential issue | 🟠 MajorUnwind panel power/reset state on PPS/compression failures.
After successful init, failures at Line 288 or Line 295 return directly, leaving regulators/reset in a prepared state.
🔧 Proposed fix
ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); if (ret < 0) { dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); - return ret; + goto err_poweroff; } @@ ret = mipi_dsi_compression_mode(pinfo->dsi, true); if (ret < 0) { dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); - return ret; + goto err_poweroff; } return 0; + +err_poweroff: + gpiod_set_value_cansleep(pinfo->reset_gpio, 1); + regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); + return ret; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch` around lines 286 - 299, The failure paths after drm_dsc_pps_payload_pack + mipi_dsi_picture_parameter_set and after mipi_dsi_compression_mode return early without undoing the panel power/reset/regulator prepare performed earlier; add cleanup calls to unwind the prepared power/reset state before returning (use the same helper(s) used elsewhere in this driver for that purpose—e.g., call the panel power-off/unprepare function used for earlier error paths such as chipone_icna3512_power_off or chipone_panel_unprepare, and deassert/reset any reset GPIOs and disable/unprepare regulators associated with pinfo); ensure these cleanup calls are invoked on both error branches (after failed mipi_dsi_picture_parameter_set and after failed mipi_dsi_compression_mode) before returning the error.patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch (1)
145-191:⚠️ Potential issue | 🟠 Major
AW88166_DSP_I2C_WRITESis never defined, making the#ifdefbranch dead code.The
#ifdef AW88166_DSP_I2C_WRITESblock (lines 146-167) will never compile since the macro isn't defined anywhere in this patch or the visible header. Either define the macro or remove the dead code path.Additionally, the active
#elsebranch (lines 168-184) writes 16-bit words but doesn't validate thatlenis even. Iflenis odd,cpu_to_be16p((u16 *)(data + i))reads beyond the buffer on the last iteration.🛡️ Suggested fix to add length validation
`#else` __be16 reg_val; mutex_lock(&aw_dev->dsp_lock); + if (len & 1) { + dev_err(aw_dev->dev, "invalid dsp payload length: %u", len); + ret = -EINVAL; + goto error_operation; + } /* i2c write */ ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch` around lines 145 - 191, The AW88166_DSP_I2C_WRITES branch is dead (AW88166_DSP_I2C_WRITES is never defined) and the active __else__ path that uses cpu_to_be16p((u16 *)(data + i)) can read past the buffer when len is odd; fix by removing the unused `#ifdef` branch (or define the macro if that path is required), and in the active branch where you loop with for (i = 0; i < len; i += 2) in the regmap_write loop validate/handle odd len before the loop (or at the end) — e.g., check if len % 2 != 0 and either return an error or pad the final byte safely so cpu_to_be16p cannot read past data; keep the mutex_lock/mutex_unlock/error_operation flow (AW88166_DSPMADD_REG, AW88166_DSPMDAT_REG, dsp_lock, error_operation, ret) intact when implementing the change.patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch (1)
85-86:⚠️ Potential issue | 🔴 CriticalUse kernel-native
u8instead ofuint8_tin this driver.Line 85, Line 86, and Line 145 use
uint8_t; kernel drivers should useu8for fixed-width 8-bit values.Proposed fix
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]; + u8 pwm_val[SN3112_CHANNELS]; + u8 pwm_en_reg[3]; bool pwm_en[SN3112_CHANNELS]; `#if` IS_ENABLED(CONFIG_GPIOLIB) struct gpio_desc *sdb; `#endif` }; @@ static int sn3112_set_val_reg(struct sn3112 *priv, unsigned int channel, - uint8_t val, bool write) + u8 val, bool write) {#!/bin/bash # Verification: confirm no stdint fixed-width integer types remain in this kernel patch. # Expected result after fix: no matches. rg -n '\buint(8|16|32|64)_t\b' patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patchAlso applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch` around lines 85 - 86, Replace usage of stdint fixed-width types with kernel-native types: change the declarations of pwm_val and pwm_en_reg (variables named "pwm_val" and "pwm_en_reg" in the SN3112 PWM driver) from uint8_t to u8, and scan the rest of the patch for any other uint8_t occurrences (e.g., the occurrence noted around the symbol references) and convert them to u8 so the driver uses kernel-native fixed-width 8-bit types.patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch (4)
209-240:⚠️ Potential issue | 🔴 CriticalTighten
CMD_STATUSpayload-length guard to full field usage.Line 210 validates only 6 bytes, but parsing reads through
FRAME_POS_DATA_14(Line 233). This can access beyond payload on short frames.🔧 Proposed fix
static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { - if (payload_length >= 6) { + if (payload_length >= (FRAME_POS_DATA_14 - FRAME_POS_DATA_1 + 1)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch` around lines 209 - 240, The CMD_STATUS handler (handle_cmd_status) currently checks payload_length >= 6 but then reads up to FRAME_POS_DATA_14, risking out-of-bounds access; update the guard to require the full number of bytes needed (at least FRAME_POS_DATA_14 + 1) before parsing, and return/dev_warn early if payload is too short; ensure you reference the same logic around keys/prev_states/keymap/bitmap_xor/for_each_set_bit and only call input_report_* and input_sync when the full frame is present.
281-309:⚠️ Potential issue | 🔴 CriticalDo not run blocking re-init from
receive_bufcallback path.On checksum mismatch, Line 307 calls
rsinput_init_commands(), which sleeps.receive_bufshould not run this synchronous recovery path.🔧 Minimal safe fix
- if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); - goto error; - } + if (computed_checksum != received_checksum) + goto error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch` around lines 281 - 309, rsinput_rx currently calls rsinput_init_commands() synchronously on checksum mismatch which can sleep; replace the direct call with an asynchronous schedule so the receive callback stays non-blocking. Modify rsinput_rx to not call rsinput_init_commands() directly on checksum failure but instead schedule a work item or delayed_work (e.g. rsinput->init_work) or queue_work on a driver workqueue and ensure rsinput_init_commands() runs in that work handler; add/initialize the work_struct (or workqueue) in the driver probe and use schedule_work/queue_work/queue_delayed_work in rsinput_rx to trigger the recovery asynchronously.
101-109:⚠️ Potential issue | 🟠 MajorMove key history from function-static storage into per-device state.
Line 211 uses
static unsigned long prev_states;, which is shared across all devices and can bleed state between controllers.🔧 Proposed fix
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; @@ uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_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; @@ - bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); + bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap)); @@ - prev_states = keys; + drv->prev_states = keys;Also applies to: 209-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch` around lines 101 - 109, The function uses a static variable prev_states (and similar function-static key-history variables between lines ~209-237) which creates shared state across devices; add a per-device field to struct rsinput_driver (e.g., uint32_t prev_states or appropriate type and any other history fields currently declared static), remove the function-static declarations, and replace all uses of those statics with the per-device fields (accessed via the rsinput_driver instance passed into the routines). Ensure the new fields are zero-initialized when the rsinput_driver is allocated in probe and that any helper functions take or derive the rsinput_driver pointer so they use driver->prev_states (and the other new fields) instead of the static names.
333-352:⚠️ Potential issue | 🔴 CriticalFail probe on optional GPIO acquisition errors; do not keep ERR_PTRs.
At Lines 335/342/349,
IS_ERR()only logs and continues. Later truthy checks (e.g., Line 354) can dereference error pointers viagpiod_set_value_cansleep().🔧 Proposed fix
drv->boot_gpio = devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); - if (IS_ERR(drv->boot_gpio)) { - error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); - } + if (IS_ERR(drv->boot_gpio)) + return dev_err_probe(&serdev->dev, PTR_ERR(drv->boot_gpio), + "Unable to get boot gpio\n"); @@ drv->enable_gpio = devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); - if (IS_ERR(drv->enable_gpio)) { - error = PTR_ERR(drv->enable_gpio); - dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); - } + if (IS_ERR(drv->enable_gpio)) + return dev_err_probe(&serdev->dev, PTR_ERR(drv->enable_gpio), + "Unable to get enable gpio\n"); @@ drv->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(drv->reset_gpio)) { - error = PTR_ERR(drv->reset_gpio); - dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); - } + if (IS_ERR(drv->reset_gpio)) + return dev_err_probe(&serdev->dev, PTR_ERR(drv->reset_gpio), + "Unable to get reset gpio\n");Also applies to: 354-367, 443-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch` around lines 333 - 352, The driver is storing ERR_PTRs from devm_gpiod_get_optional into drv->boot_gpio/enable_gpio/reset_gpio and only logging them, which risks later dereferencing (e.g., gpiod_set_value_cansleep); change the probe so that after each devm_gpiod_get_optional call you check IS_ERR() and propagate the error (return PTR_ERR(err)) instead of retaining the ERR_PTR, or set the drv->* pointer to NULL when the GPIO is truly optional; update the handling around devm_gpiod_get_optional calls for "boot", "enable", and "reset" (and similar blocks referenced) so no ERR_PTR is stored and the probe fails cleanly on acquisition errors.
🧹 Nitpick comments (3)
patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch (1)
666-672: Removal of ALSA controls reduces user-space configurability.This revert removes user-facing ALSA controls (volume, fade, profile, calibration). While this is acceptable for a compatibility HACK, consider documenting this limitation or tracking it for future restoration once upstream 6.19 driver issues are resolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch` around lines 666 - 672, The revert removed the ALSA user controls by dropping .controls and .num_controls from the snd_soc_component_driver instance soc_codec_dev_aw88166 which breaks user-space configurability; to fix, either restore the fields in soc_codec_dev_aw88166 by re-adding .controls = aw88166_controls and .num_controls = ARRAY_SIZE(aw88166_controls) so existing controls (aw88166_controls) are present, or if this revert must remain, add an explicit note in the patch header or a kernel documentation entry referencing this patch and the soc_codec_dev_aw88166 change stating that aw88166_controls are deliberately disabled and must be restored once upstream 6.19 issues are resolved.patch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
33-87: Minor style: Kernel prefers C-style comments.Lines 37, 52, 55, 60 use C++ style comments (
//). Kernel coding style typically uses/* */comments, though this is not strictly enforced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch` around lines 33 - 87, The function generate_bdaddr_from_serial uses C++-style '//' comments (e.g., around the last6 declaration, the Extract the last 6 characters line, Initialize the first 3 bytes line and the Convert... line); replace those with kernel-preferred C-style comments (/* ... */) in that function (symbols to locate: generate_bdaddr_from_serial, last6, byte_str, bdaddr->b) so the comments follow kernel coding style and keep the code logic unchanged.patch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1681-1709: De-duplicate identical variant blocks into the common DTSI.The
remoteproc_*,spk_amp_*, and&socdisable blocks are duplicated across both board DTS files. Moving these toqcs8550-ayn-common.dtsiwill reduce maintenance risk.Also applies to: 1941-1969
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch` around lines 1681 - 1709, Extract the identical node fragments (&remoteproc_adsp, &remoteproc_cdsp, &spk_amp_l, &spk_amp_r and the &soc inner nodes qcom_tzlog and arch_timer) from the two board DTS files and place them once into the shared common DTSI (common DTS include) so both boards include the single canonical definition; preserve the exact node names/labels and properties (firmware-name arrays, status = "okay"/"disabled"), keep the two-entry firmware-name lists for remoteproc_* and the aw883xx_acf.bin entries for spk_amp_{l,r}, and ensure inclusion order/overlays still let the board DTS override if needed.
🤖 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-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 281-315: The rsinput_rx function currently drops short fragments
instead of buffering them; change it to append incoming data into a persistent
receive buffer and only validate/process when a full frame is assembled: in
rsinput_rx (and using drv->rx_buf and a new/existing drv->rx_buf_pos field)
check count against buffer capacity, memcpy incoming data at rx_buf_pos,
increment rx_buf_pos, and only compute the checksum with compute_checksum and
call rsinput_process_data once rx_buf_pos indicates a full packet (use
MCU_PKT_SIZE_MIN or a length byte in the protocol to determine full-frame size);
on checksum mismatch call rsinput_init_commands and clear/reset rx_buf_pos, and
always guard against buffer overflow by rejecting/applying backpressure if
rx_buf_pos + count > sizeof(drv->rx_buf).
- Around line 421-435: The probe registers the input device via
input_register_device(drv->input) but on subsequent failure in
rsinput_init_commands(drv) the cleanup path (err_close) only calls
serdev_device_close(serdev) and returns, leaking the registered device; update
the err_close path so that if drv->input was successfully registered you call
input_unregister_device(drv->input) before serdev_device_close(serdev) (use the
same drv->input and serdev symbols and ensure error is returned after cleanup).
- Around line 184-205: The handler handle_cmd_commod reads the subcommand at
data[FRAME_POS_DATA_1] without checking payload_length; add a guard at the top
of handle_cmd_commod that verifies payload_length >= 1 (and returns or logs an
error via dev_err/dev_warn) before accessing data[FRAME_POS_DATA_1], and adjust
subsequent cases (e.g., DATA_COMMOD_VERSION, DATA_COMMOD_SET_PAR) to rely on
that check (keep the existing payload-length check for DATA_COMMOD_VERSION but
ensure the initial guard prevents out-of-bounds access).
- Around line 380-382: The code calls serdev_device_set_baudrate(serdev, 115200)
and assigns its unsigned return to error, then checks if (error < 0) which is
always false; change the check to detect unsupported/failed configuration by
testing the unsigned return properly—either use if (error == 0) to detect
unsupported controllers or compare error != 115200 to ensure the requested baud
was applied; update the branch that currently jumps to err_close (or adjust
error handling) when the baudrate is not supported or differs, referencing the
serdev_device_set_baudrate call, the serdev variable, and the err_close label.
In
`@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 113-121: The function icna3512_get_current_mode must null-check
pinfo->connector before dereferencing; update icna3512_get_current_mode to
return the default index (0) if pinfo is NULL or pinfo->connector is NULL (same
guard used for connector->state and connector->state->crtc), so that prepare()
running before get_modes() won't dereference a NULL pinfo->connector; locate the
function by name and add the connector null guard at the top of
icna3512_get_current_mode referencing struct panel_info and pinfo->connector.
In
`@patch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 258-263: The log prints generated_macaddr.b but the code actually
uses ar->mac_addr (modified by ath12k_reverse_mac and pdev_idx), so update the
logging call in the else branch to print the actual MAC buffer ar->mac_addr and
use the correct format specifier for a u8[] MAC (replace %pMR with %pM) — locate
the block with ath12k_reverse_mac(generated_macaddr.b), the subsequent
ar->mac_addr[4] += ar->pdev_idx, and the ath12k_info(...) call and change the
logged argument and format accordingly.
- Around line 155-158: The patch introduces an unused static variable
`static_macaddr`; remove the unused definition `static const struct macaddr_t
static_macaddr = { .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } };` from the diff
to avoid dead code and warnings—search for `static_macaddr` in the file to
confirm it's not referenced and delete the entire declaration block.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 118-119: Fix the typo in the dev_err message inside the error path
that logs DSP read failures: update the format string in the dev_err call (the
one using aw_dev->dev and variables dsp_addr and *dsp_data) to read "32-bit"
instead of "32r-bit" so the message correctly says "read dsp_addr[0x%x] 32-bit
dsp_data[0x%x] failed".
- Around line 529-533: The if statement in the playback/capture debug block uses
no space before the opening brace—change the conditional "if (substream->stream
== SNDRV_PCM_STREAM_PLAYBACK){" to include a space before the brace so it reads
"if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {", keeping the rest of the
block (dev_dbg(dai->dev, ...), else { ... }) unchanged; symbols to locate:
substream->stream, SNDRV_PCM_STREAM_PLAYBACK, dev_dbg, dai->dev.
- Around line 127-130: The error path for the regmap read is logging the wrong
variable (`ret` from a previous call) so capture and check the return value of
regmap_read() (e.g., int ret = regmap_read(aw_dev->regmap, AW88166_ID_REG,
®_value);) and then use that ret in the dev_err call (dev_err(aw_dev->dev,
"%s fail to clear chip state. ret=%d\n", __func__, ret);). Keep the existing
symbols (aw_dev, regmap_read, AW88166_ID_REG, reg_value, dev_err, __func__,
mutex_unlock(&aw_dev->dsp_lock)) and ensure the mutex_unlock remains after the
read and log.
---
Duplicate comments:
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 209-240: The CMD_STATUS handler (handle_cmd_status) currently
checks payload_length >= 6 but then reads up to FRAME_POS_DATA_14, risking
out-of-bounds access; update the guard to require the full number of bytes
needed (at least FRAME_POS_DATA_14 + 1) before parsing, and return/dev_warn
early if payload is too short; ensure you reference the same logic around
keys/prev_states/keymap/bitmap_xor/for_each_set_bit and only call input_report_*
and input_sync when the full frame is present.
- Around line 281-309: rsinput_rx currently calls rsinput_init_commands()
synchronously on checksum mismatch which can sleep; replace the direct call with
an asynchronous schedule so the receive callback stays non-blocking. Modify
rsinput_rx to not call rsinput_init_commands() directly on checksum failure but
instead schedule a work item or delayed_work (e.g. rsinput->init_work) or
queue_work on a driver workqueue and ensure rsinput_init_commands() runs in that
work handler; add/initialize the work_struct (or workqueue) in the driver probe
and use schedule_work/queue_work/queue_delayed_work in rsinput_rx to trigger the
recovery asynchronously.
- Around line 101-109: The function uses a static variable prev_states (and
similar function-static key-history variables between lines ~209-237) which
creates shared state across devices; add a per-device field to struct
rsinput_driver (e.g., uint32_t prev_states or appropriate type and any other
history fields currently declared static), remove the function-static
declarations, and replace all uses of those statics with the per-device fields
(accessed via the rsinput_driver instance passed into the routines). Ensure the
new fields are zero-initialized when the rsinput_driver is allocated in probe
and that any helper functions take or derive the rsinput_driver pointer so they
use driver->prev_states (and the other new fields) instead of the static names.
- Around line 333-352: The driver is storing ERR_PTRs from
devm_gpiod_get_optional into drv->boot_gpio/enable_gpio/reset_gpio and only
logging them, which risks later dereferencing (e.g., gpiod_set_value_cansleep);
change the probe so that after each devm_gpiod_get_optional call you check
IS_ERR() and propagate the error (return PTR_ERR(err)) instead of retaining the
ERR_PTR, or set the drv->* pointer to NULL when the GPIO is truly optional;
update the handling around devm_gpiod_get_optional calls for "boot", "enable",
and "reset" (and similar blocks referenced) so no ERR_PTR is stored and the
probe fails cleanly on acquisition errors.
In
`@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 394-426: Both backlight callbacks icna3512_bl_update_status and
icna3512_bl_get_brightness clear MIPI_DSI_MODE_LPM and may return early on
error, leaving dsi->mode_flags modified; fix by saving the original
dsi->mode_flags into a local variable at the start of each function and always
restoring it before any return (including error returns) — e.g., wrap the
mipi_dsi_dcs_set_display_brightness_large and
mipi_dsi_dcs_get_display_brightness_large calls so that on both success and
failure the saved flags are written back to dsi->mode_flags (use a single exit
path or goto/cleanup block to ensure restoration).
- Around line 286-299: The failure paths after drm_dsc_pps_payload_pack +
mipi_dsi_picture_parameter_set and after mipi_dsi_compression_mode return early
without undoing the panel power/reset/regulator prepare performed earlier; add
cleanup calls to unwind the prepared power/reset state before returning (use the
same helper(s) used elsewhere in this driver for that purpose—e.g., call the
panel power-off/unprepare function used for earlier error paths such as
chipone_icna3512_power_off or chipone_panel_unprepare, and deassert/reset any
reset GPIOs and disable/unprepare regulators associated with pinfo); ensure
these cleanup calls are invoked on both error branches (after failed
mipi_dsi_picture_parameter_set and after failed mipi_dsi_compression_mode)
before returning the error.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 145-191: The AW88166_DSP_I2C_WRITES branch is dead
(AW88166_DSP_I2C_WRITES is never defined) and the active __else__ path that uses
cpu_to_be16p((u16 *)(data + i)) can read past the buffer when len is odd; fix by
removing the unused `#ifdef` branch (or define the macro if that path is
required), and in the active branch where you loop with for (i = 0; i < len; i
+= 2) in the regmap_write loop validate/handle odd len before the loop (or at
the end) — e.g., check if len % 2 != 0 and either return an error or pad the
final byte safely so cpu_to_be16p cannot read past data; keep the
mutex_lock/mutex_unlock/error_operation flow (AW88166_DSPMADD_REG,
AW88166_DSPMDAT_REG, dsp_lock, error_operation, ret) intact when implementing
the change.
In
`@patch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patch`:
- Around line 85-86: Replace usage of stdint fixed-width types with
kernel-native types: change the declarations of pwm_val and pwm_en_reg
(variables named "pwm_val" and "pwm_en_reg" in the SN3112 PWM driver) from
uint8_t to u8, and scan the rest of the patch for any other uint8_t occurrences
(e.g., the occurrence noted around the symbol references) and convert them to u8
so the driver uses kernel-native fixed-width 8-bit types.
---
Nitpick comments:
In
`@patch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 33-87: The function generate_bdaddr_from_serial uses C++-style
'//' comments (e.g., around the last6 declaration, the Extract the last 6
characters line, Initialize the first 3 bytes line and the Convert... line);
replace those with kernel-preferred C-style comments (/* ... */) in that
function (symbols to locate: generate_bdaddr_from_serial, last6, byte_str,
bdaddr->b) so the comments follow kernel coding style and keep the code logic
unchanged.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 666-672: The revert removed the ALSA user controls by dropping
.controls and .num_controls from the snd_soc_component_driver instance
soc_codec_dev_aw88166 which breaks user-space configurability; to fix, either
restore the fields in soc_codec_dev_aw88166 by re-adding .controls =
aw88166_controls and .num_controls = ARRAY_SIZE(aw88166_controls) so existing
controls (aw88166_controls) are present, or if this revert must remain, add an
explicit note in the patch header or a kernel documentation entry referencing
this patch and the soc_codec_dev_aw88166 change stating that aw88166_controls
are deliberately disabled and must be restored once upstream 6.19 issues are
resolved.
In
`@patch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1681-1709: Extract the identical node fragments (&remoteproc_adsp,
&remoteproc_cdsp, &spk_amp_l, &spk_amp_r and the &soc inner nodes qcom_tzlog and
arch_timer) from the two board DTS files and place them once into the shared
common DTSI (common DTS include) so both boards include the single canonical
definition; preserve the exact node names/labels and properties (firmware-name
arrays, status = "okay"/"disabled"), keep the two-entry firmware-name lists for
remoteproc_* and the aw883xx_acf.bin entries for spk_amp_{l,r}, and ensure
inclusion order/overlays still let the board DTS override if needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
config/sources/families/sm8550.confpatch/kernel/archive/sm8550-6.19/0001-arm64-dts-qcom-sm8550-add-UART15.patchpatch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patchpatch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patchpatch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patchpatch/kernel/archive/sm8550-6.19/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patchpatch/kernel/archive/sm8550-6.19/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patchpatch/kernel/archive/sm8550-6.19/0007-clk-qcom-gcc-sm8550-Keep-UFS-PHY-GDSCs-ALWAYS_ON.patchpatch/kernel/archive/sm8550-6.19/0008-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patchpatch/kernel/archive/sm8550-6.19/0009-arm64-dts-qcom-Added-pmk8550-pwm-support.patchpatch/kernel/archive/sm8550-6.19/0010-drivers-power-Fix-name-for-sc8280xp-battery.patchpatch/kernel/archive/sm8550-6.19/0011-HACK-fix-usb-boot-hang.patchpatch/kernel/archive/sm8550-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patchpatch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patchpatch/kernel/archive/sm8550-6.19/0014-pwm-Add-SI-EN-SN3112-PWM-support.patchpatch/kernel/archive/sm8550-6.19/0015-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
🚧 Files skipped from review as they are similar to previous changes (6)
- patch/kernel/archive/sm8550-6.19/0010-drivers-power-Fix-name-for-sc8280xp-battery.patch
- config/sources/families/sm8550.conf
- patch/kernel/archive/sm8550-6.19/0009-arm64-dts-qcom-Added-pmk8550-pwm-support.patch
- patch/kernel/archive/sm8550-6.19/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch
- patch/kernel/archive/sm8550-6.19/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch
- patch/kernel/archive/sm8550-6.19/0004-leds-Add-driver-for-HEROIC-HTR3212.patch
Comment on lines
+184
to
+205
| +static void handle_cmd_commod(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { | ||
| + switch (data[FRAME_POS_DATA_1]) { | ||
| + case DATA_COMMOD_VERSION: | ||
| + if (payload_length >= 1) { | ||
| + char mcu_version[MCU_VERSION_MAX_LEN] = {0}; | ||
| + size_t version_length = payload_length; | ||
| + if (version_length > MCU_VERSION_MAX_LEN - 1) { | ||
| + version_length = MCU_VERSION_MAX_LEN - 1; | ||
| + } | ||
| + memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); | ||
| + mcu_version[version_length] = '\0'; | ||
| + dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version); | ||
| + } else { | ||
| + dev_err(&drv->serdev->dev, "Invalid MCU version response length\n"); | ||
| + } | ||
| + break; | ||
| + case DATA_COMMOD_SET_PAR: | ||
| + dev_info(&drv->serdev->dev, "MCU parameters set successfully\n"); | ||
| + break; | ||
| + default: | ||
| + dev_warn(&drv->serdev->dev, "Unhandled CMD_COMMOD sub-command: 0x%02x\n", data[FRAME_POS_DATA_1]); | ||
| + break; |
Contributor
There was a problem hiding this comment.
Guard CMD_COMMOD payload before reading subcommand byte.
At Line 185, data[FRAME_POS_DATA_1] is read unconditionally. For zero-length payloads this consumes a non-payload byte and misparses the frame.
🔧 Proposed fix
static void handle_cmd_commod(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) {
+ if (payload_length < 1) {
+ dev_err(&drv->serdev->dev, "Invalid CMD_COMMOD response length\n");
+ return;
+ }
+
switch (data[FRAME_POS_DATA_1]) {
case DATA_COMMOD_VERSION:
- if (payload_length >= 1) {
+ if (payload_length >= 1) {
char mcu_version[MCU_VERSION_MAX_LEN] = {0};
size_t version_length = payload_length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 184 - 205, The handler handle_cmd_commod reads the subcommand at
data[FRAME_POS_DATA_1] without checking payload_length; add a guard at the top
of handle_cmd_commod that verifies payload_length >= 1 (and returns or logs an
error via dev_err/dev_warn) before accessing data[FRAME_POS_DATA_1], and adjust
subsequent cases (e.g., DATA_COMMOD_VERSION, DATA_COMMOD_SET_PAR) to rely on
that check (keep the existing payload-length check for DATA_COMMOD_VERSION but
ensure the initial guard prevents out-of-bounds access).
Comment on lines
+281
to
+315
| +static size_t rsinput_rx(struct serdev_device *serdev, const u8 *data, size_t count) { | ||
| + struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); | ||
| + uint8_t received_checksum, computed_checksum; | ||
| + | ||
| + if (!drv || !data || count == 0) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Invalid RX data\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + if (count > sizeof(drv->rx_buf)) { | ||
| + dev_warn_ratelimited(&serdev->dev, "RX buffer overflow\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + memcpy(drv->rx_buf, data, count); | ||
| + | ||
| + if (count < MCU_PKT_SIZE_MIN) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Frame too short for checksum validation\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + received_checksum = drv->rx_buf[count - 1]; | ||
| + | ||
| + computed_checksum = compute_checksum(drv->rx_buf, count); | ||
| + | ||
| + if (computed_checksum != received_checksum) { | ||
| + rsinput_init_commands(drv); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + rsinput_process_data(drv, drv->rx_buf, count); | ||
| + | ||
| +error: | ||
| + return count; | ||
| +} |
Contributor
There was a problem hiding this comment.
RX path drops fragmented UART frames instead of reassembling stream data.
Line 297 rejects packets shorter than MCU_PKT_SIZE_MIN and returns. On UART, frames can be split across callbacks; this can cause systematic event loss/desync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 281 - 315, The rsinput_rx function currently drops short fragments
instead of buffering them; change it to append incoming data into a persistent
receive buffer and only validate/process when a full frame is assembled: in
rsinput_rx (and using drv->rx_buf and a new/existing drv->rx_buf_pos field)
check count against buffer capacity, memcpy incoming data at rx_buf_pos,
increment rx_buf_pos, and only compute the checksum with compute_checksum and
call rsinput_process_data once rx_buf_pos indicates a full packet (use
MCU_PKT_SIZE_MIN or a length byte in the protocol to determine full-frame size);
on checksum mismatch call rsinput_init_commands and clear/reset rx_buf_pos, and
always guard against buffer overflow by rejecting/applying backpressure if
rx_buf_pos + count > sizeof(drv->rx_buf).
Comment on lines
+380
to
+382
| + error = serdev_device_set_baudrate(serdev, 115200); | ||
| + if (error < 0) | ||
| + goto err_close; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Linux kernel serdev_device_set_baudrate return type semantics kernel 6.19
💡 Result:
In Linux v6.19 (and current -rc/mainline), serdev_device_set_baudrate(struct serdev_device *serdev, unsigned int speed) returns an unsigned int, and its semantics are:
- Return value is the actual baud rate selected by the controller, which may differ from the requested
speed(e.g., due to clock divisors / rounding). This is why the underlyingserdev_controller_ops::set_baudrate()was changed to return the actual speed (ktermios.c_ospeed) rather than the requested one. [3] - If there is no controller attached or the controller provides no
set_baudrateop, the helper returns0(i.e., “not supported / not set”). [1] - It is not an errno-returning API. Since the return type is unsigned, negative error codes are not meaningful; callers should treat
0as failure/unsupported and otherwise compare the returned speed to the requested speed if they require an exact rate. A typical pattern isactual = serdev_device_set_baudrate(...); if (actual != requested) .... [1][4]
So:
- success: returns
actual_baud(often equal tospeed) - failure/unsupported: returns
0 - do not check
ret < 0(that’s a bug/misuse, even if you might see it in some older driver code). [2]
Sources: function implementation in drivers/tty/serdev/core.c and signature/ops in include/linux/serdev.h [1][5], rationale for “return actual baudrate” change [3], example of “compare returned speed to requested” usage [4].
References
- Linux serdev core implementation (
serdev_device_set_baudrate()returns 0 if missing op, else returns controller’s value) — https://git.sceen.net/linux/linux-stable.git/tree/drivers/tty/serdev/core.c?id=7c314bdfb64e4bb8d2f829376ed56ce663483752 [1] - Example of incorrect
ret < 0check in a driver diff (illustrates common confusion) — https://lkml.org/lkml/2024/4/17/452 [2] - Patch discussion: return actual baudrate (
ktermios.c_ospeed) rather than requested — https://www.mail-archive.com/netdev%40vger.kernel.org/msg168933.html [3] - Example caller validating the returned speed matches the requested bitrate — https://www.spinics.net/lists/linux-input/msg91892.html [4]
- Header shows
unsigned intreturn type for both device helper and controller op — https://sources.debian.org/src/linux/6.5.10-1~bpo12%2B1/include/linux/serdev.h [5]
serdev_device_set_baudrate() returns unsigned int, making the if (error < 0) check always false.
The function returns the actual baud rate configured by the controller (or 0 if unsupported). The < 0 check cannot work with an unsigned return type. Replace with if (error == 0) to detect unsupported controllers, or compare the returned value against the requested speed to validate the configuration succeeded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 380 - 382, The code calls serdev_device_set_baudrate(serdev,
115200) and assigns its unsigned return to error, then checks if (error < 0)
which is always false; change the check to detect unsupported/failed
configuration by testing the unsigned return properly—either use if (error == 0)
to detect unsupported controllers or compare error != 115200 to ensure the
requested baud was applied; update the branch that currently jumps to err_close
(or adjust error handling) when the baudrate is not supported or differs,
referencing the serdev_device_set_baudrate call, the serdev variable, and the
err_close label.
Comment on lines
+421
to
+435
| + error = input_register_device(drv->input); | ||
| + if (error) { | ||
| + goto err_close; | ||
| + } | ||
| + | ||
| + error = rsinput_init_commands(drv); | ||
| + if (error < 0) { | ||
| + goto err_close; | ||
| + } | ||
| + | ||
| + return 0; | ||
| + | ||
| +err_close: | ||
| + serdev_device_close(serdev); | ||
| + return error; |
Contributor
There was a problem hiding this comment.
Unregister input device on post-registration probe failure.
If Line 421 succeeds and Line 426 fails, control jumps to err_close and returns without input_unregister_device(). This can leave a registered input device after failed probe.
🔧 Proposed fix
error = input_register_device(drv->input);
if (error) {
goto err_close;
}
error = rsinput_init_commands(drv);
if (error < 0) {
- goto err_close;
+ goto err_unregister_input;
}
return 0;
+err_unregister_input:
+ input_unregister_device(drv->input);
+ drv->input = NULL;
err_close:
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-6.19/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 421 - 435, The probe registers the input device via
input_register_device(drv->input) but on subsequent failure in
rsinput_init_commands(drv) the cleanup path (err_close) only calls
serdev_device_close(serdev) and returns, leaking the registered device; update
the err_close path so that if drv->input was successfully registered you call
input_unregister_device(drv->input) before serdev_device_close(serdev) (use the
same drv->input and serdev symbols and ensure error is returned after cleanup).
Comment on lines
+113
to
+121
| +static int icna3512_get_current_mode(struct panel_info *pinfo) | ||
| +{ | ||
| + struct drm_connector *connector = pinfo->connector; | ||
| + struct drm_crtc_state *crtc_state; | ||
| + int i; | ||
| + | ||
| + /* Return the default (first) mode if no info available yet */ | ||
| + if (!connector->state || !connector->state->crtc) | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
Add a null guard for pinfo->connector before dereference.
Line 120 dereferences connector unconditionally. If prepare() runs before get_modes() stores pinfo->connector, this can crash.
🔧 Proposed fix
static int icna3512_get_current_mode(struct panel_info *pinfo)
{
struct drm_connector *connector = pinfo->connector;
struct drm_crtc_state *crtc_state;
int i;
/* Return the default (first) mode if no info available yet */
- if (!connector->state || !connector->state->crtc)
+ if (!connector || !connector->state || !connector->state->crtc)
return 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`
around lines 113 - 121, The function icna3512_get_current_mode must null-check
pinfo->connector before dereferencing; update icna3512_get_current_mode to
return the default index (0) if pinfo is NULL or pinfo->connector is NULL (same
guard used for connector->state and connector->state->crtc), so that prepare()
running before get_modes() won't dereference a NULL pinfo->connector; locate the
function by name and add the connector null guard at the top of
icna3512_get_current_mode referencing struct panel_info and pinfo->connector.
Comment on lines
+155
to
+158
| +/* Define a static, predefined MAC_ADDR structure */ | ||
| +static const struct macaddr_t static_macaddr = { | ||
| + .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } | ||
| +}; |
Contributor
There was a problem hiding this comment.
Remove unused static_macaddr definition.
The static_macaddr variable is defined but never referenced anywhere in this patch. This appears to be leftover code that should be removed.
🧹 Proposed fix
-/* Define a static, predefined MAC_ADDR structure */
-static const struct macaddr_t static_macaddr = {
- .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 }
-};
-📝 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.
Suggested change
| +/* 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-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 155 - 158, The patch introduces an unused static variable
`static_macaddr`; remove the unused definition `static const struct macaddr_t
static_macaddr = { .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } };` from the diff
to avoid dead code and warnings—search for `static_macaddr` in the file to
confirm it's not referenced and delete the entire declaration block.
Comment on lines
+258
to
+263
| } else { | ||
| - ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); | ||
| + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | ||
| ar->mac_addr[4] += ar->pdev_idx; | ||
| + ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); | ||
| } |
Contributor
There was a problem hiding this comment.
Log message prints wrong address value.
Line 262 logs generated_macaddr.b but ar->mac_addr is what's actually being used (after byte reversal on line 260 and pdev_idx adjustment on line 261). The logged value won't match the actual address in use.
🐛 Proposed fix
} else {
ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b);
ar->mac_addr[4] += ar->pdev_idx;
- ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b);
+ ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr);
}Note: Changed format specifier from %pMR to %pM since ar->mac_addr is a u8[] array, not a bdaddr_t (which %pMR expects for reversed printing).
📝 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.
Suggested change
| } else { | |
| - ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); | |
| + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | |
| ar->mac_addr[4] += ar->pdev_idx; | |
| + ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); | |
| } | |
| } else { | |
| ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); | |
| ar->mac_addr[4] += ar->pdev_idx; | |
| 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-6.19/0012-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 258 - 263, The log prints generated_macaddr.b but the code actually
uses ar->mac_addr (modified by ath12k_reverse_mac and pdev_idx), so update the
logging call in the else branch to print the actual MAC buffer ar->mac_addr and
use the correct format specifier for a u8[] MAC (replace %pMR with %pM) — locate
the block with ath12k_reverse_mac(generated_macaddr.b), the subsequent
ar->mac_addr[4] += ar->pdev_idx, and the ath12k_info(...) call and change the
logged argument and format accordingly.
Comment on lines
+118
to
+119
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | ||
| + (u32)dsp_addr, *dsp_data); |
Contributor
There was a problem hiding this comment.
Typo in error message: "32r-bit" should be "32-bit".
Minor typo in the debug message.
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed",
+ dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed",📝 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.
Suggested change
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | |
| + (u32)dsp_addr, *dsp_data); | |
| dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", | |
| (u32)dsp_addr, *dsp_data); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 118 - 119, Fix the typo in the dev_err message inside the error
path that logs DSP read failures: update the format string in the dev_err call
(the one using aw_dev->dev and variables dsp_addr and *dsp_data) to read
"32-bit" instead of "32r-bit" so the message correctly says "read dsp_addr[0x%x]
32-bit dsp_data[0x%x] failed".
Comment on lines
+127
to
+130
| + /* clear dsp chip select state */ | ||
| + if (regmap_read(aw_dev->regmap, AW88166_ID_REG, ®_value)) | ||
| + dev_err(aw_dev->dev, "%s fail to clear chip state. ret=%d\n", __func__, ret); | ||
| + mutex_unlock(&aw_dev->dsp_lock); |
Contributor
There was a problem hiding this comment.
Misleading error message: ret doesn't reflect regmap_read failure.
The error message prints ret, but at this point ret holds the return value from aw_dev_dsp_read_*bit(), not from regmap_read(). The actual regmap error is discarded.
- if (regmap_read(aw_dev->regmap, AW88166_ID_REG, ®_value))
- dev_err(aw_dev->dev, "%s fail to clear chip state. ret=%d\n", __func__, ret);
+ if (regmap_read(aw_dev->regmap, AW88166_ID_REG, ®_value))
+ dev_err(aw_dev->dev, "%s fail to clear chip state\n", __func__);📝 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.
Suggested change
| + /* clear dsp chip select state */ | |
| + if (regmap_read(aw_dev->regmap, AW88166_ID_REG, ®_value)) | |
| + dev_err(aw_dev->dev, "%s fail to clear chip state. ret=%d\n", __func__, ret); | |
| + mutex_unlock(&aw_dev->dsp_lock); | |
| /* clear dsp chip select state */ | |
| if (regmap_read(aw_dev->regmap, AW88166_ID_REG, ®_value)) | |
| dev_err(aw_dev->dev, "%s fail to clear chip state\n", __func__); | |
| mutex_unlock(&aw_dev->dsp_lock); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 127 - 130, The error path for the regmap read is logging the wrong
variable (`ret` from a previous call) so capture and check the return value of
regmap_read() (e.g., int ret = regmap_read(aw_dev->regmap, AW88166_ID_REG,
®_value);) and then use that ret in the dev_err call (dev_err(aw_dev->dev,
"%s fail to clear chip state. ret=%d\n", __func__, ret);). Keep the existing
symbols (aw_dev, regmap_read, AW88166_ID_REG, reg_value, dev_err, __func__,
mutex_unlock(&aw_dev->dsp_lock)) and ensure the mutex_unlock remains after the
read and log.
Comment on lines
+529
to
+533
| + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){ | ||
| + dev_dbg(dai->dev, "STREAM_PLAYBACK stop"); | ||
| + } else { | ||
| + dev_dbg(dai->dev, "STREAM_CAPTURE stop"); | ||
| } |
Contributor
There was a problem hiding this comment.
Minor style: missing space before {.
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {📝 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.
Suggested change
| + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){ | |
| + dev_dbg(dai->dev, "STREAM_PLAYBACK stop"); | |
| + } else { | |
| + dev_dbg(dai->dev, "STREAM_CAPTURE stop"); | |
| } | |
| if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { | |
| dev_dbg(dai->dev, "STREAM_PLAYBACK stop"); | |
| } else { | |
| dev_dbg(dai->dev, "STREAM_CAPTURE stop"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8550-6.19/0013-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 529 - 533, The if statement in the playback/capture debug block
uses no space before the opening brace—change the conditional "if
(substream->stream == SNDRV_PCM_STREAM_PLAYBACK){" to include a space before the
brace so it reads "if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {",
keeping the rest of the block (dev_dbg(dai->dev, ...), else { ... }) unchanged;
symbols to locate: substream->stream, SNDRV_PCM_STREAM_PLAYBACK, dev_dbg,
dai->dev.
Author
|
Hold on this because I see a big SD card IO performance regression. It is caused by SDR104/SDR50 capability disabled in upstream dts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds kernel 6.19 support for Ayn Odin2 device. I dropped the downstream MMC host controller driver to switch to upstream one. The rest of patches are copied from 6.18.
How Has This Been Tested?
Verified following functions are working:
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
New Features
Bug Fixes