Added config improvements and fixes to ayn odin2 #9316
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two Ayn Odin2 GRUB board configs and first-run presets; BSP audio/loader configs; an SM8550 kernel defconfig and family mapping change; and a large sm8550-6.13 kernel patch series introducing the Qualcomm Iris video accelerator driver and associated DT, drivers, power, V4L2/VB2, firmware, and helper changes, plus miscellaneous drivers and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Userspace App
participant V4L2 as V4L2/VB2
participant Iris as Iris Driver (core)
participant HFI as HFI Queues / Host‑FW IPC
participant VPU as VPU Firmware
participant PM as PM/Resources
App->>V4L2: open + ioctls (REQBUF / QBUF / STREAMON)
V4L2->>Iris: forward vb2 ops / ioctls
Iris->>PM: pm_runtime_resume / prepare power, clocks, ICC
Iris->>HFI: enqueue session_start / config / buffer commands
HFI->>VPU: deliver commands via shared queues
VPU-->>HFI: response / interrupt
HFI->>Iris: ISR invokes response handlers
Iris->>V4L2: complete vb2 buffers / notify events
Iris->>PM: pm_runtime_put / adjust clocks/ICC (rgba(52,152,219,0.5))
V4L2->>App: buffer done / events
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
02
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
1 similar comment
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
I think we need to document where to get the ABL/grub for this
Squishy123
force-pushed
the
added-config-improvements-and-fixes-to-ayn-odin2
branch
from
a0d8ad2 to
0e57000
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
Squishy123
force-pushed
the
added-config-improvements-and-fixes-to-ayn-odin2
branch
from
0e57000 to
9996187
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
1 similar comment
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
Squishy123
force-pushed
the
added-config-improvements-and-fixes-to-ayn-odin2
branch
from
35f5bb3 to
6d8c548
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
|
Is there a good way to credit the ROCKNIX devs - since a lot of the stuff came from their repo? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
patch/kernel/archive/sm8550-6.13/0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch (1)
35-55:⚠️ Potential issue | 🟠 MajorMissing error handling for
clk_prepare_enableand potential resource leak.Two issues in this function:
clk_prepare_enable()at line 46 can fail but its return value is ignored.- If
qcom_snd_sdw_startup()fails, the clock remains enabled (resource leak).🔧 Proposed fix with error handling
static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) { unsigned int fmt = SND_SOC_DAIFMT_BP_FP; unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; struct snd_soc_pcm_runtime *rtd = substream->private_data; struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); + int ret; switch (cpu_dai->id) { case PRIMARY_MI2S_RX: - clk_prepare_enable(pdata->i2s_clk); + ret = clk_prepare_enable(pdata->i2s_clk); + if (ret) + return ret; snd_soc_dai_set_fmt(cpu_dai, fmt); snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); break; default: break; } - return qcom_snd_sdw_startup(substream); + ret = qcom_snd_sdw_startup(substream); + if (ret && cpu_dai->id == PRIMARY_MI2S_RX) + clk_disable_unprepare(pdata->i2s_clk); + + return ret; }patch/kernel/archive/sm8550-6.13/0033_leds--Add-driver-for-HEROIC-HTR3212.patch (3)
282-297:⚠️ Potential issue | 🔴 CriticalIncomplete GPIO error handling will crash on non-EPROBE_DEFER errors.
The code only handles
-EPROBE_DEFERfromdevm_gpiod_get(). If any other error occurs (e.g.,-ENOENT,-EIO),priv->sdbremains an error pointer, andgpiod_set_value_cansleep()on line 297 will dereference it, causing a kernel crash.Proposed fix
priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); - if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (IS_ERR(priv->sdb)) + return dev_err_probe(dev, PTR_ERR(priv->sdb), + "Failed to get sdb GPIO\n");
292-311:⚠️ Potential issue | 🟠 MajorRegulator leak on probe failure paths.
If
htr3212_init_regs()orhtr3212_parse_dt()fails afterregulator_enable()succeeds, the regulator is not disabled before returning. This leaves the regulator enabled, causing a resource leak.Proposed fix using goto-based cleanup
ret = regulator_enable(priv->vdd); - if (ret < 0) { + if (ret < 0) return ret; - } gpiod_set_value_cansleep(priv->sdb, 1); usleep_range(10000, 11000); priv->client = client; i2c_set_clientdata(client, priv); ret = htr3212_init_regs(priv); if (ret) - return ret; + goto err_disable_regulator; ret = htr3212_parse_dt(dev, priv); if (ret) - return ret; + goto err_disable_regulator; return 0; + +err_disable_regulator: + gpiod_set_value_cansleep(priv->sdb, 0); + regulator_disable(priv->vdd); + return ret; }
348-348:⚠️ Potential issue | 🟡 MinorMissing closing angle bracket in MODULE_AUTHOR.
The email address is missing the closing
>.Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");patch/kernel/archive/sm8550-6.13/0031_input--Add-driver-for-RSInput-Gamepad.patch (5)
209-241:⚠️ Potential issue | 🟠 MajorStatic variable
prev_statescauses state sharing between device instances.The
static unsigned long prev_statesat line 211 is shared across all driver instances. If multiple gamepads are connected, they will share button state tracking, leading to incorrect input reports.Move
prev_statesintostruct rsinput_driver.Suggested fix
Add to the struct:
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_states; };Then update handle_cmd_status:
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)); + bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap)); ... - prev_states = keys; + drv->prev_states = keys;
306-309:⚠️ Potential issue | 🟠 MajorRe-initializing MCU on every checksum failure is excessive and may block.
Calling
rsinput_init_commands()from the receive callback on checksum mismatch is problematic:
rsinput_init_commands()containsmsleep(100)calls, which may block inappropriately in the receive path- A single corrupted packet shouldn't trigger a full MCU re-initialization
- This could cause an initialization storm if line noise occurs
Consider simply discarding bad packets, or implementing a counter to reinit only after multiple consecutive failures.
Suggested fix
if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); + dev_dbg_ratelimited(&serdev->dev, "Checksum mismatch, discarding packet\n"); goto error; }
333-352:⚠️ Potential issue | 🔴 CriticalGPIO error handling leaves error pointers that will cause kernel oops.
When
devm_gpiod_get_optional()fails, the error pointer is stored but not cleared. Later, checks likeif (drv->boot_gpio)evaluate to true for error pointers, andgpiod_set_value_cansleep()on an error pointer will crash the kernel.Either set the pointer to NULL on error or return the error from probe.
Suggested 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); + dev_warn(&serdev->dev, "Unable to get boot gpio: %ld\n", PTR_ERR(drv->boot_gpio)); + drv->boot_gpio = NULL; } 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); + dev_warn(&serdev->dev, "Unable to get enable gpio: %ld\n", PTR_ERR(drv->enable_gpio)); + drv->enable_gpio = NULL; } 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); + dev_warn(&serdev->dev, "Unable to get reset gpio: %ld\n", PTR_ERR(drv->reset_gpio)); + drv->reset_gpio = NULL; }
379-420:⚠️ Potential issue | 🔴 CriticalResource leak:
serdev_device_close()missing on error paths after open.After
serdev_device_open()succeeds at line 370, several error paths return without closing the serdev:
- Line 381: baudrate setup failure
- Line 387: input allocation failure
- Line 420: input registration failure
The serdev must be closed before returning on these error paths.
Suggested fix using goto-based cleanup
error = serdev_device_set_baudrate(serdev, 115200); - if (error < 0) - return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); + if (error < 0) { + dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); + goto err_close; + } serdev_device_set_flow_control(serdev, false); drv->input = devm_input_allocate_device(&serdev->dev); - if (!drv->input) - return -ENOMEM; + if (!drv->input) { + error = -ENOMEM; + goto err_close; + } ... error = input_register_device(drv->input); - if (error) - return error; + if (error) + goto err_close; serdev_device_set_client_ops(serdev, &rsinput_rx_ops); error = rsinput_init_commands(drv); if (error < 0) { - serdev_device_close(serdev); - return error; + goto err_close; } return 0; + +err_close: + serdev_device_close(serdev); + return error; }
433-443:⚠️ Potential issue | 🟠 MajorRemove manual
input_unregister_device()for devm-managed input device.The input device is allocated with
devm_input_allocate_device(), which automatically handles cleanup including unregistration when the parent device is removed. Manually callinginput_unregister_device()on a devm-managed device can cause a double-free or use-after-free.Suggested fix
static void rsinput_remove(struct serdev_device *serdev) { struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); serdev_device_close(serdev); - input_unregister_device(drv->input); if (drv->enable_gpio) gpiod_set_value_cansleep(drv->enable_gpio, 0); if (drv->reset_gpio) gpiod_set_value_cansleep(drv->reset_gpio, 0); }patch/kernel/archive/sm8550-6.13/0032_drm-panel--Add-panel-driver-for-Chipone-ICNA3512-b.patch (1)
286-298:⚠️ Potential issue | 🟠 MajorResource leak on error paths after PPS or compression mode failure.
If
mipi_dsi_picture_parameter_setormipi_dsi_compression_modefails, the function returns without disabling the regulators that were enabled at line 269. This leaves regulators enabled and the GPIO in reset state without proper cleanup.🔧 Proposed fix to add cleanup on error paths
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_disable_regulators; } /* 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; + goto err_disable_regulators; } return 0; + +err_disable_regulators: + gpiod_set_value_cansleep(pinfo->reset_gpio, 1); + regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); + return ret; }config/boards/ayn-odin2.csc (1)
100-113:⚠️ Potential issue | 🟡 MinorGuard firmware loops against missing directories.
If
/lib/firmware/qcom/sm8550*doesn’t exist, the glob can expand to a literal pattern andadd_firmwarewill be called with a non-existent path.🔧 Proposed fix
@@ - for f in /lib/firmware/qcom/sm8550/* ; do + for f in /lib/firmware/qcom/sm8550/* ; do + [ -e "$f" ] || continue add_firmware "${f#/lib/firmware/}" done - for f in /lib/firmware/qcom/sm8550/ayn/* ; do + for f in /lib/firmware/qcom/sm8550/ayn/* ; do + [ -e "$f" ] || continue add_firmware "${f#/lib/firmware/}" done
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.13/0004_media--iris--introduce-iris-core-state-management-.patch`:
- Around line 252-260: The error cleanup after allocating core->sfr_vaddr
incorrectly calls dma_free_attrs(core->dev, sizeof(*q_tbl_hdr),
core->iface_q_table_vaddr, core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
replace the size argument with the original allocation size (queue_size) used
for core->iface_q_table_vaddr so the full DMA region is freed; ensure the
dma_free_attrs call still uses the matching DMA_ATTR_WRITE_COMBINE flag and the
same buffer pointers core->iface_q_table_vaddr/core->iface_q_table_daddr.
In
`@patch/kernel/archive/sm8550-6.13/0007_media--iris--introduce-host-firmware-interface-wit.patch`:
- Around line 1066-1281: The response-path can call
iris_hfi_gen2_handle_system_error(core, NULL) on validation failure but
iris_hfi_gen2_handle_system_error dereferences pkt->type; modify
iris_hfi_gen2_handle_system_error to first check if pkt is NULL and handle that
case (log a generic system error message, set core->state = IRIS_CORE_ERROR and
schedule_delayed_work(&core->sys_error_handler, ...)) instead of dereferencing
pkt, leaving existing behavior for non-NULL pkt; this avoids changing callers
like iris_hfi_gen2_handle_response while ensuring safe NULL handling.
In
`@patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch`:
- Around line 1383-1417: The kzallocs for inst->fmt_src and inst->fmt_dst in the
initialization block must be checked for NULL to avoid kernel panics; after each
kzalloc (allocating inst->fmt_src and inst->fmt_dst) verify the pointer and on
failure free any prior allocation, set a proper error return (e.g. -ENOMEM) and
abort initialization so the caller (iris_open) can handle it; update the
function that contains these symbols (references: inst->fmt_src, inst->fmt_dst,
iris_get_buffer_size, iris_vpu_buf_count) to perform the NULL checks, free the
other fmt pointer if needed, and return an error code instead of continuing to
dereference.
In
`@patch/kernel/archive/sm8550-6.13/0014_media--iris--implement-iris-v4l2_ctrl_ops.patch`:
- Around line 219-240: The function iris_session_init_caps currently
dereferences platform caps without checks; update it to first validate
core->iris_platform_data, core->iris_platform_data->inst_fw_caps and
core->iris_platform_data->inst_fw_caps_size (and optionally core->inst_fw_caps)
and return early if any are NULL or size is 0 to avoid probe-time NULL derefs.
Locate iris_session_init_caps and add these guards before using caps, then
proceed with the existing loop that uses iris_valid_cap_id and fills
core->inst_fw_caps only when inputs are present.
In
`@patch/kernel/archive/sm8550-6.13/0019_media--iris--allocate`,-initialize-and-queue-intern.patch:
- Around line 79-106: The function iris_create_internal_buffer adds the newly
allocated struct iris_buffer (variable buffer) into buffers->list before calling
dma_alloc_attrs, and if dma_alloc_attrs fails the code returns -ENOMEM leaving
buffer in the list and leaking memory; fix iris_create_internal_buffer by
undoing the prior list_add_tail on allocation failure: call
list_del(&buffer->list) and kfree(buffer) (no dma_free needed since kvaddr is
NULL) before returning the error from the dma_alloc_attrs call.
In
`@patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch`:
- Around line 392-396: The condition guarding STOP uses an equality test against
inst->sub_state (inst->sub_state != IRIS_INST_SUB_DRAIN) which is wrong for
bitflags; change the check to test the drain bit with a bitwise AND and negate
it (i.e. use !(inst->sub_state & IRIS_INST_SUB_DRAIN)) so STOP is blocked while
the IRIS_INST_SUB_DRAIN bit is set; update the branch that handles
V4L2_DEC_CMD_STOP and streaming (vb2_is_streaming(src_q)) to use this bitwise
test against inst->sub_state.
🟠 Major comments (15)
extensions/odin2-preset-firstrun.sh-41-48 (1)
41-48:⚠️ Potential issue | 🟠 MajorSecurity concern: Hardcoded weak default passwords.
Using
1234as default passwords for both root and user accounts is a security risk, especially if images are distributed publicly. Even if the first-run wizard prompts for password change, devices left unconfigured or images accessed before first boot are vulnerable.Consider using stronger randomly-generated defaults or requiring password setup during first-run without a preset.
extensions/odin2-preset-firstrun.sh-60-61 (1)
60-61:⚠️ Potential issue | 🟠 MajorDirectory will be owned by root, not the odin2 user.
The
pre_customize_image__hook runs during image build, but theodin2user is only preset (viaPRESET_USER_NAME) and won't be created until first boot. The cloned directory will be owned by root, causing permission issues when the user tries to access or modify their scripts.Fix by setting ownership after clone, or move this to a first-run hook that executes after user creation.
🐛 Proposed fix to set correct ownership
function pre_customize_image__add_odin2_scripts() { display_alert "Adding Odin2 Scripts" "${EXTENSION}" "info" chroot_sdcard mkdir -p /home/odin2/sys chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git /home/odin2/sys/odin2-scripts + # Set ownership to uid/gid 1000 (default first user) since odin2 user doesn't exist yet + chroot_sdcard chown -R 1000:1000 /home/odin2 }extensions/odin2-preset-firstrun.sh-61-61 (1)
61-61:⚠️ Potential issue | 🟠 MajorSupply chain risk: Git clone without version pinning.
Cloning from an external repository without pinning to a specific commit hash or tag means builds are not reproducible and vulnerable to upstream changes or compromise.
Pin to a specific commit or tag for reproducible and secure builds.
🔒 Proposed fix to pin to a specific commit
- chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git /home/odin2/sys/odin2-scripts + chroot_sdcard git clone --depth 1 --branch <TAG_OR_BRANCH> https://github.com/Squishy123/odin2-scripts.git /home/odin2/sys/odin2-scripts + # Or pin to specific commit: + # chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git /home/odin2/sys/odin2-scripts + # chroot_sdcard git -C /home/odin2/sys/odin2-scripts checkout <COMMIT_HASH>patch/kernel/archive/sm8550-6.13/0018_media--iris--subscribe-parameters-and-properties-t.patch-95-111 (1)
95-111:⚠️ Potential issue | 🟠 MajorPotential buffer overflow if
change_param_sizeexceeds 31.The
payloadarray is fixed at 32 elements, but the loop copieschange_param_sizeelements starting at index 1. Ifchange_param_size > 31, this will overflow.While platform data sizes are typically small and controlled, adding a bounds check would be defensive. Same issue exists in
iris_hfi_gen2_subscribe_propertyat line 206.🛡️ Proposed defensive fix
+ if (change_param_size > ARRAY_SIZE(payload) - 1) { + dev_err(core->dev, "change_param_size exceeds payload capacity\n"); + return -EINVAL; + } + for (i = 0; i < change_param_size; i++) payload[i + 1] = change_param[i];patch/kernel/archive/sm8550-6.13/0019_media--iris--allocate,-initialize-and-queue-intern.patch-855-864 (1)
855-864:⚠️ Potential issue | 🟠 MajorLogic bug:
HFI_PORT_NONErejected forPERSISTbuffers.The first condition allows
HFI_PORT_NONEwhenbuffer_type == HFI_BUFFER_PERSIST, but the second condition then rejectsHFI_PORT_NONEbecause it's neitherHFI_PORT_BITSTREAMnorHFI_PORT_RAW. PERSIST buffers with PORT_NONE will incorrectly returnfalse.🐛 Proposed fix
static bool iris_hfi_gen2_is_valid_hfi_port(u32 port, u32 buffer_type) { + if (port == HFI_PORT_NONE && buffer_type == HFI_BUFFER_PERSIST) + return true; + if (port == HFI_PORT_NONE && buffer_type != HFI_BUFFER_PERSIST) return false; if (port != HFI_PORT_BITSTREAM && port != HFI_PORT_RAW) return false; return true; }patch/kernel/archive/sm8550-6.13/0024_media--iris--add-check-whether-the-video-session-i.patch-56-141 (1)
56-141:⚠️ Potential issue | 🟠 MajorProtect
core->instancestraversal withcore->lock.
iris_check_session_supported()walkscore->instanceswithout holding the lock, which can race with instance add/remove and lead to UAF or false negatives. Please wrap the iteration with the same lock used elsewhere in this file.🔒 Proposed fix
static int iris_check_session_supported(struct iris_inst *inst) { struct iris_core *core = inst->core; struct iris_inst *instance = NULL; bool found = false; int ret; - list_for_each_entry(instance, &core->instances, list) { - if (instance == inst) - found = true; - } + mutex_lock(&core->lock); + list_for_each_entry(instance, &core->instances, list) { + if (instance == inst) { + found = true; + break; + } + } + mutex_unlock(&core->lock);patch/kernel/archive/sm8550-6.13/0015_media--iris--implement-query_cap-ioctl.patch-29-35 (1)
29-35:⚠️ Potential issue | 🟠 MajorSet
device_capsandcapabilitiesiniris_querycap.Iris is a V4L2 mem2mem decoder and must advertise its capabilities. Without
device_capsandcapabilities, userspace cannot detect that the device supports M2M/streaming operations. Set these using the standard pattern for M2M decoders:Suggested fix
static int iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) { strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver)); strscpy(cap->card, "Iris Decoder", sizeof(cap->card)); + cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING; + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; return 0; }patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch-1462-1477 (1)
1462-1477:⚠️ Potential issue | 🟠 MajorSilent failure when max session count exceeded.
iris_add_sessionsilently fails to add the instance to the list when the maximum session count is reached. The caller iniris_open(line 1537) has no way to detect this failure, causing the open to appear successful while the session is not actually registered.This will cause
iris_get_instanceto return NULL for HFI responses targeting this session, leading to dropped responses or potential crashes.🐛 Proposed fix
-static void iris_add_session(struct iris_inst *inst) +static int iris_add_session(struct iris_inst *inst) { struct iris_core *core = inst->core; struct iris_inst *iter; u32 count = 0; mutex_lock(&core->lock); list_for_each_entry(iter, &core->instances, list) count++; - if (count < core->iris_platform_data->max_session_count) + if (count < core->iris_platform_data->max_session_count) { list_add_tail(&inst->list, &core->instances); + mutex_unlock(&core->lock); + return 0; + } mutex_unlock(&core->lock); + return -EBUSY; }And update the caller:
- iris_add_session(inst); + ret = iris_add_session(inst); + if (ret) { + iris_vdec_inst_deinit(inst); + goto fail_m2m_ctx_release; + }patch/kernel/archive/sm8550-6.13/0014_media--iris--implement-iris-v4l2_ctrl_ops.patch-446-474 (1)
446-474:⚠️ Potential issue | 🟠 MajorHandle cleanup when iris_ctrls_init() fails to avoid leaks.
On failure,fmt_src/fmt_dstallocations from earlier iniris_vdec_inst_init()are leaked because the error path iniris_open()doesn’t free them.🧹 Proposed fix
int iris_vdec_inst_init(struct iris_inst *inst) { struct iris_core *core = inst->core; struct v4l2_format *f; + int ret; /* ... existing init ... */ memcpy(&inst->fw_caps[0], &core->inst_fw_caps[0], INST_FW_CAP_MAX * sizeof(struct platform_inst_fw_cap)); - return iris_ctrls_init(inst); + ret = iris_ctrls_init(inst); + if (ret) { + kfree(inst->fmt_src); + inst->fmt_src = NULL; + kfree(inst->fmt_dst); + inst->fmt_dst = NULL; + } + return ret; }patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch-268-287 (1)
268-287:⚠️ Potential issue | 🟠 MajorProp-set failures are ignored; surface them to streamon.
Line 284-286 discards the return value from
cap->set, so firmware rejects can be silently ignored and streamon continues partially configured.Proposed fix (propagate errors)
- if (cap->cap_id && cap->set) - cap->set(inst, i); + if (cap->cap_id && cap->set) { + ret = cap->set(inst, i); + if (ret) + return ret; + }patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch-425-445 (1)
425-445:⚠️ Potential issue | 🟠 MajorAvoid unbalanced runtime‑PM reference count on resume failure.
The code calls
pm_runtime_put_sync()at the exit label unconditionally, butpm_runtime_resume_and_get()fails without incrementing the usage counter. This causes a refcount underflow when the resume fails. Return early on failed resume without calling anypm_runtime_put*(), and only put after a successful get.Proposed fix
- ret = pm_runtime_resume_and_get(core->dev); - if (ret < 0) - goto exit; + ret = pm_runtime_resume_and_get(core->dev); + if (ret < 0) + return ret; mutex_lock(&core->lock); ret = iris_hfi_queue_cmd_write_locked(core, pkt, pkt_size); - if (ret) { - mutex_unlock(&core->lock); - goto exit; - } mutex_unlock(&core->lock); + if (ret) { + pm_runtime_put_sync(core->dev); + return ret; + } pm_runtime_mark_last_busy(core->dev); pm_runtime_put_autosuspend(core->dev); return 0; - -exit: - pm_runtime_put_sync(core->dev); - - return ret;patch/kernel/archive/sm8550-6.13/0021_media--iris--add-support-for-dynamic-resolution-ch.patch-44-66 (1)
44-66:⚠️ Potential issue | 🟠 MajorFix DCI‑P3 mapping in
iris_hfi_get_v4l2_color_primaries.The switch statement receives an HFI primaries enum value but line 61 checks against
V4L2_COLORSPACE_DCI_P3(a V4L2 enum), so this case never matches. When the firmware sendsHFI_PRIMARIES_SMPTE_RP431_2, it falls through to the default case and incorrectly returnsV4L2_COLORSPACE_DEFAULT. Swap the constants so the HFI value maps to the correct V4L2 colorspace:Fix
- case V4L2_COLORSPACE_DCI_P3: - return HFI_PRIMARIES_SMPTE_RP431_2; + case HFI_PRIMARIES_SMPTE_RP431_2: + return V4L2_COLORSPACE_DCI_P3;patch/kernel/archive/sm8550-6.13/0040_phy--qcom--qmp-combo--register-a-typec-mux-to-chan.patch-51-136 (1)
51-136:⚠️ Potential issue | 🟠 MajorAddress the mux state comparison and USB3 detection logic.
TYPEC_MODE_USB3is the correct constant (defined ininclude/linux/usb/typec_altmode.h); there is noTYPEC_STATE_USB3.However, the logic has two issues:
Pre-mutex comparison (race condition): The early return
if (state->mode == qmp->mode)executes beforemutex_lock(), allowing concurrent modification ofqmp->modebetween the check and lock.Stale state used for USB3 detection: The USB3 check uses
qmp->mux_mode(the previously stored state before the current update on line 108) rather thanstate->mode(the incoming request). When a USB3 event arrives, this causes detection to use the previous mux state instead of the current request, delaying or skipping USB3-only mode activation until the next mode change.Additionally, when bailing out due to
new_mode == qmp->init_mode(line 96), onlyqmp->modeis updated, notqmp->mux_mode, leaving the stored state inconsistent.🔧 Suggested fixes
- if (state->mode == qmp->mode) - return 0; - - mutex_lock(&qmp->phy_mutex); + mutex_lock(&qmp->phy_mutex); + if (state->mode == qmp->mux_mode) + goto out; @@ - if (qmp->mux_mode == TYPEC_MODE_USB3) + if (state->mode == TYPEC_MODE_USB3) new_mode = QPHY_MODE_USB_ONLY; @@ if (new_mode == qmp->init_mode) { dev_dbg(qmp->dev, "typec_mux_set: same phy mode, bail out\n"); qmp->mode = state->mode; + qmp->mux_mode = state->mode; goto out; }patch/kernel/archive/sm8550-6.13/0007_media--iris--introduce-host-firmware-interface-wit.patch-78-110 (1)
78-110:⚠️ Potential issue | 🟠 MajorReinitialize
core_init_donebefore waiting for the system response.When
iris_core_init()is called again (e.g., during error recovery viairis_sys_error_handler()), thecore_init_donecompletion may already be signaled from the previous call. On re-entry,wait_for_completion_timeout()would return immediately even before the firmware responds to the new HFI commands, since the internal completion counter is already > 0.🔧 Proposed fix
@@ - ret = iris_hfi_core_init(core); + reinit_completion(&core->core_init_done); + ret = iris_hfi_core_init(core);config/boards/ayn-odin2-portal-grub.csc-55-61 (1)
55-61:⚠️ Potential issue | 🟠 MajorFile mode
655prevents owner from executing scripts.Mode
655(rw-r-xr-x) denies execute permission to the owner while granting it to group/others. For executable scripts, use755(rwxr-xr-x).Proposed fix
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ - install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ + install -Dm755 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ + install -Dm755 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ - install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget - install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget - install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ - install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ + install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget + install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget + install -Dm755 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ + install -Dm755 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/
🟡 Minor comments (17)
extensions/odin2-preset-firstrun.sh-54-55 (1)
54-55:⚠️ Potential issue | 🟡 MinorIncomplete implementation: "clone starter scripts" comment with no code.
The comment suggests functionality that was intended but not implemented. Either remove the placeholder comment or implement the missing functionality.
patch/kernel/archive/sm8550-6.13/0018_media--iris--subscribe-parameters-and-properties-t.patch-99-103 (1)
99-103:⚠️ Potential issue | 🟡 MinorInconsistent error handling: logs error but returns success.
When the properties are already set for the given plane,
dev_err()is called but the function returns0(success). This is confusing - either it's an expected condition (remove the error log) or an actual error (return-EINVALor-EALREADY).💡 Suggested fix - treat as no-op without error log
if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) || (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) { - dev_err(core->dev, "invalid plane\n"); return 0; }patch/kernel/archive/sm8550-6.13/0003_media--iris--implement-iris-v4l2-file-ops.patch-198-207 (1)
198-207:⚠️ Potential issue | 🟡 MinorTypo in comment: "seralise" should be "serialize".
Minor typo in the struct documentation comment.
- * `@lock`: lock to seralise forward and reverse threads + * `@lock`: lock to serialize forward and reverse threadspatch/kernel/archive/sm8550-6.13/0029_arm64--dts--qcom--sm8550--Add-iris-video-codec-nod.patch-94-94 (1)
94-94:⚠️ Potential issue | 🟡 MinorMinor: Trailing whitespace on this line.
There appears to be trailing whitespace before the
videoccnode. While this won't affect functionality, it's a minor code hygiene issue.patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch-168-211 (1)
168-211:⚠️ Potential issue | 🟡 MinorError code from first HFI command is overwritten.
In the stop/release sequence (lines 183-191), if
HFI_CMD_SESSION_STOPfails butHFI_CMD_SESSION_RELEASE_RESOURCESsucceeds, the original error is lost. Similarly, the secondretassignment unconditionally overwrites the first.Consider preserving the first error:
Proposed fix
reinit_completion(&inst->completion); iris_hfi_gen1_packet_session_cmd(inst, &pkt, HFI_CMD_SESSION_STOP); ret = iris_hfi_queue_cmd_write(core, &pkt, pkt.shdr.hdr.size); if (!ret) ret = iris_wait_for_session_response(inst, false); reinit_completion(&inst->completion); iris_hfi_gen1_packet_session_cmd(inst, &pkt, HFI_CMD_SESSION_RELEASE_RESOURCES); - ret = iris_hfi_queue_cmd_write(core, &pkt, pkt.shdr.hdr.size); - if (!ret) - ret = iris_wait_for_session_response(inst, false); + if (!ret) { + ret = iris_hfi_queue_cmd_write(core, &pkt, pkt.shdr.hdr.size); + if (!ret) + ret = iris_wait_for_session_response(inst, false); + }patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch-649-654 (1)
649-654:⚠️ Potential issue | 🟡 MinorLogging bug: prints same value for old and new state.
The
dev_dbglogsinst->stateafter it's already been assignedrequest_state, so both values in the log message will be identical.Proposed fix
+ enum iris_inst_state old_state = inst->state; + change_state: inst->state = request_state; dev_dbg(inst->core->dev, "state changed from %x to %x\n", - inst->state, request_state); + old_state, request_state); return 0;patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch-1275-1281 (1)
1275-1281:⚠️ Potential issue | 🟡 MinorDefault return for invalid buffer types may mask bugs.
iris_v4l2_type_to_driverreturnsBUF_OUTPUTfor any type that isn'tV4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, including invalid types. This could silently hide bugs where an unexpected type is passed.🛡️ Suggested defensive check
static inline enum iris_buffer_type iris_v4l2_type_to_driver(u32 type) { if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) return BUF_INPUT; - else + else if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) return BUF_OUTPUT; + else + return BUF_TYPE_MAX; /* Invalid type indicator */ }Callers should then check for
BUF_TYPE_MAXas an error.patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch-1552-1566 (1)
1552-1566:⚠️ Potential issue | 🟡 MinorSession close called unconditionally regardless of whether session was opened.
iris_session_closeis always called iniris_close, but if the VB2 queue was never set up (e.g., open succeeded but no reqbufs was issued),session_openwas never called (once_per_session_setremains false).For gen2, this is handled by the NULL check on
inst_hfi_gen2->packet(line 754), but for gen1, a close command is sent to firmware for a session that was never opened.🛡️ Proposed guard
static void iris_session_close(struct iris_inst *inst) { const struct iris_hfi_command_ops *hfi_ops = inst->core->hfi_ops; bool wait_for_response = true; int ret; + if (!inst->once_per_session_set) + return; + reinit_completion(&inst->completion); ret = hfi_ops->session_close(inst);patch/kernel/archive/sm8550-6.13/0010_media--iris--implement-s_fmt,-g_fmt-and-try_fmt-io.patch-50-91 (1)
50-91:⚠️ Potential issue | 🟡 Minor
TRY_FMTmust return aligned dimensions consistent withS_FMT.
V4L2 spec requiresVIDIOC_TRY_FMTto return the exact same adjusted format thatVIDIOC_S_FMTwould accept. Currently,try_fmt()returns unaligned width/height, whiles_fmt()aligns to codec/stride constraints (OUTPUT: 16-byte alignment; CAPTURE: 128×32 with 128-byte stride). This mismatch misleads userspace on buffer sizing. Add the proposed alignment totry_fmt()to matchs_fmt().🔧 Proposed fix
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: if (f->fmt.pix_mp.pixelformat != V4L2_PIX_FMT_H264) { f_inst = inst->fmt_src; f->fmt.pix_mp.width = f_inst->fmt.pix_mp.width; f->fmt.pix_mp.height = f_inst->fmt.pix_mp.height; f->fmt.pix_mp.pixelformat = f_inst->fmt.pix_mp.pixelformat; } + f->fmt.pix_mp.width = ALIGN(f->fmt.pix_mp.width, DEFAULT_CODEC_ALIGNMENT); + f->fmt.pix_mp.height = ALIGN(f->fmt.pix_mp.height, DEFAULT_CODEC_ALIGNMENT); break; case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: if (f->fmt.pix_mp.pixelformat != V4L2_PIX_FMT_NV12) { f_inst = inst->fmt_dst; f->fmt.pix_mp.pixelformat = f_inst->fmt.pix_mp.pixelformat; f->fmt.pix_mp.width = f_inst->fmt.pix_mp.width; f->fmt.pix_mp.height = f_inst->fmt.pix_mp.height; } @@ if (vb2_is_streaming(src_q)) { f_inst = inst->fmt_src; f->fmt.pix_mp.height = f_inst->fmt.pix_mp.height; f->fmt.pix_mp.width = f_inst->fmt.pix_mp.width; } + f->fmt.pix_mp.width = ALIGN(f->fmt.pix_mp.width, 128); + f->fmt.pix_mp.height = ALIGN(f->fmt.pix_mp.height, 32); + f->fmt.pix_mp.plane_fmt[0].bytesperline = ALIGN(f->fmt.pix_mp.width, 128); break;patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch-634-661 (1)
634-661:⚠️ Potential issue | 🟡 MinorFix unsigned integer underflow in bandwidth delta check.
The expression
abs(bw_kbps - bw_prev)operates onunsigned longvalues. Whenbw_kbps < bw_prev, the subtraction underflows to a large positive value due to unsigned arithmetic, causing the threshold check to behave incorrectly. Additionally,abs()expects a signedintargument. Compute the delta explicitly by choosing the correct subtraction direction.🛠️ Proposed fix
- if (abs(bw_kbps - bw_prev) < BW_THRESHOLD && bw_prev) - return ret; + if (bw_prev) { + unsigned long diff = (bw_kbps > bw_prev) ? + (bw_kbps - bw_prev) : + (bw_prev - bw_kbps); + if (diff < BW_THRESHOLD) + return ret; + }patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch-677-689 (1)
677-689:⚠️ Potential issue | 🟡 MinorBalance
pm_runtime_get_sync()on failure.
pm_runtime_get_sync()increments the device's usage counter before attempting the resume; if the resume fails, the usage counter is not automatically decremented, causing a leak. Addpm_runtime_put_noidle()on error to balance the count.🛠️ Proposed fix
ret = pm_runtime_get_sync(pd_dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(pd_dev); return ret; + }patch/kernel/archive/sm8550-6.13/0005_media--iris--implement-video-firmware-load-unload.patch-181-185 (1)
181-185:⚠️ Potential issue | 🟡 MinorError code overwritten with
-ENOMEM.Line 185 always returns
-ENOMEMregardless of the actual error returned byiris_load_fw_to_memory(). This loses the original error information (could be-EINVAL,-EIO, etc.).🐛 Proposed fix
ret = iris_load_fw_to_memory(core, fwpath); if (ret) { dev_err(core->dev, "firmware download failed\n"); - return -ENOMEM; + return ret; }patch/kernel/archive/sm8550-6.13/0005_media--iris--implement-video-firmware-load-unload.patch-147-149 (1)
147-149:⚠️ Potential issue | 🟡 MinorMissing return value assignment on
memremapfailure.When
memremap()returns NULL at line 148, the code jumps toerr_release_fwbutretis not set, leaving it with its previous value (0 from successfulrequest_firmware). This causes the function to return success (0) despite the failure.🐛 Proposed fix
mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC); - if (!mem_virt) + if (!mem_virt) { + ret = -ENOMEM; goto err_release_fw; + }patch/kernel/archive/sm8550-6.13/0007_media--iris--introduce-host-firmware-interface-wit.patch-1286-1461 (1)
1286-1461:⚠️ Potential issue | 🟡 MinorPropagate queue write errors instead of masking them.
Any non-zero return from
iris_hfi_queue_write()is treated as “queue full” and mapped to-ENODATA, hiding-EINVALand other failures.🔧 Proposed fix
@@ int iris_hfi_queue_cmd_write_locked(struct iris_core *core, void *pkt, u32 pkt_size) { struct iris_iface_q_info *q_info = &core->command_queue; + int ret; if (core->state == IRIS_CORE_ERROR) return -EINVAL; - if (!iris_hfi_queue_write(q_info, pkt, pkt_size)) { - iris_vpu_raise_interrupt(core); - } else { - dev_err(core->dev, "queue full\n"); - return -ENODATA; - } - - return 0; + ret = iris_hfi_queue_write(q_info, pkt, pkt_size); + if (ret) { + dev_err(core->dev, "queue write failed: %d\n", ret); + return ret; + } + iris_vpu_raise_interrupt(core); + return 0; }patch/kernel/archive/sm8550-6.13/0004_media--iris--introduce-iris-core-state-management-.patch-519-530 (1)
519-530:⚠️ Potential issue | 🟡 MinorMutex not destroyed on probe error paths.
mutex_init(&core->lock)is called early in probe (line 545), but if any subsequent step fails (e.g.,dma_set_mask_and_coherent), the error path jumps to labels that don't callmutex_destroy. The mutex is only destroyed iniris_remove.This is a minor resource leak since
mutex_destroyis mostly a debug aid on Linux, but it's worth noting for completeness.config/boards/ayn-odin2-portal-grub.csc-32-46 (1)
32-46:⚠️ Potential issue | 🟡 MinorSubshell exit does not propagate failure to the caller.
If
cdfails (line 39),exit 6only exits the subshell, and the outer function continues silently. Consider using|| return 6outside the subshell or checking the subshell's exit status.Additionally, the download lacks integrity verification (e.g., checksum). Consider pinning to a specific commit SHA and verifying the download.
Proposed fix to propagate subshell failure
function pre_customize_image__ayn-odin2_alsa_ucm_conf() { if ! ayn-odin2portal_is_userspace_supported; then return 0 fi display_alert "Add alsa-ucm-conf for ${BOARD}" "${RELEASE}" "warn" ( cd "${SDCARD}/usr/share/alsa" || exit 6 curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" unzip -o temp.zip unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1) cp -rf "${unzip_dir}/"* . rm -rf "$unzip_dir" temp.zip - ) + ) || return $? }config/boards/ayn-odin2-portal-grub.csc-74-81 (1)
74-81:⚠️ Potential issue | 🟡 MinorMissing second argument to
display_alerton lines 75 and 79.Other calls use three arguments (message, detail, level). These calls only have message and level, which may cause incorrect output.
Proposed fix
- display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "warn" + display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "${RELEASE}" "warn" do_with_retries 3 chroot_sdcard add-apt-repository ppa:kisak/kisak-mesa --yes do_with_retries 3 chroot_sdcard_apt_get_update - display_alert "Installing Mesa Vulkan Drivers" + display_alert "Installing Mesa Vulkan Drivers" "${RELEASE}" "info"
| + core->sfr_vaddr = dma_alloc_attrs(core->dev, SFR_SIZE, | ||
| + &core->sfr_daddr, | ||
| + GFP_KERNEL, DMA_ATTR_WRITE_COMBINE); | ||
| + if (!core->sfr_vaddr) { | ||
| + dev_err(core->dev, "sfr alloc and map failed\n"); | ||
| + dma_free_attrs(core->dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr, | ||
| + core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE); | ||
| + return -ENOMEM; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*0004_media*iris*" -type fRepository: armbian/build
Length of output: 250
🏁 Script executed:
sed -n '240,265p' ./patch/kernel/archive/sm8550-6.13/0004_media--iris--introduce-iris-core-state-management-.patchRepository: armbian/build
Length of output: 1058
Memory leak in error cleanup: dma_free_attrs uses wrong size.
The first dma_alloc_attrs() allocates queue_size bytes (line 245), but the error cleanup when the second allocation fails (line 257) frees with sizeof(*q_tbl_hdr), which is a small fraction of the allocated size. This causes a significant DMA memory leak in the error path.
Fix
if (!core->sfr_vaddr) {
dev_err(core->dev, "sfr alloc and map failed\n");
- dma_free_attrs(core->dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr,
+ dma_free_attrs(core->dev, queue_size, core->iface_q_table_vaddr,
core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
return -ENOMEM;
}📝 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.
| + core->sfr_vaddr = dma_alloc_attrs(core->dev, SFR_SIZE, | |
| + &core->sfr_daddr, | |
| + GFP_KERNEL, DMA_ATTR_WRITE_COMBINE); | |
| + if (!core->sfr_vaddr) { | |
| + dev_err(core->dev, "sfr alloc and map failed\n"); | |
| + dma_free_attrs(core->dev, sizeof(*q_tbl_hdr), core->iface_q_table_vaddr, | |
| + core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE); | |
| + return -ENOMEM; | |
| + } | |
| core->sfr_vaddr = dma_alloc_attrs(core->dev, SFR_SIZE, | |
| &core->sfr_daddr, | |
| GFP_KERNEL, DMA_ATTR_WRITE_COMBINE); | |
| if (!core->sfr_vaddr) { | |
| dev_err(core->dev, "sfr alloc and map failed\n"); | |
| dma_free_attrs(core->dev, queue_size, core->iface_q_table_vaddr, | |
| core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE); | |
| return -ENOMEM; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0004_media--iris--introduce-iris-core-state-management-.patch`
around lines 252 - 260, The error cleanup after allocating core->sfr_vaddr
incorrectly calls dma_free_attrs(core->dev, sizeof(*q_tbl_hdr),
core->iface_q_table_vaddr, core->iface_q_table_daddr, DMA_ATTR_WRITE_COMBINE);
replace the size argument with the original allocation size (queue_size) used
for core->iface_q_table_vaddr so the full DMA region is freed; ensure the
dma_free_attrs call still uses the matching DMA_ATTR_WRITE_COMBINE flag and the
same buffer pointers core->iface_q_table_vaddr/core->iface_q_table_daddr.
| @@ -0,0 +1,215 @@ | ||
| +// SPDX-License-Identifier: GPL-2.0-only | ||
| +/* | ||
| + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. | ||
| + */ | ||
| + | ||
| +#include "iris_hfi_gen2.h" | ||
| +#include "iris_hfi_gen2_defines.h" | ||
| +#include "iris_hfi_gen2_packet.h" | ||
| +#include "iris_vpu_common.h" | ||
| + | ||
| +struct iris_hfi_gen2_core_hfi_range { | ||
| + u32 begin; | ||
| + u32 end; | ||
| + int (*handle)(struct iris_core *core, struct iris_hfi_packet *pkt); | ||
| +}; | ||
| + | ||
| +static int iris_hfi_gen2_validate_packet(u8 *response_pkt, u8 *core_resp_pkt) | ||
| +{ | ||
| + u8 *response_limit = core_resp_pkt + IFACEQ_CORE_PKT_SIZE; | ||
| + u32 response_pkt_size = *(u32 *)response_pkt; | ||
| + | ||
| + if (!response_pkt_size) | ||
| + return -EINVAL; | ||
| + | ||
| + if (response_pkt_size < sizeof(struct iris_hfi_packet)) | ||
| + return -EINVAL; | ||
| + | ||
| + if (response_pkt + response_pkt_size > response_limit) | ||
| + return -EINVAL; | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_validate_hdr_packet(struct iris_core *core, struct iris_hfi_header *hdr) | ||
| +{ | ||
| + struct iris_hfi_packet *packet; | ||
| + int ret; | ||
| + u8 *pkt; | ||
| + u32 i; | ||
| + | ||
| + if (hdr->size < sizeof(*hdr) + sizeof(*packet)) | ||
| + return -EINVAL; | ||
| + | ||
| + pkt = (u8 *)((u8 *)hdr + sizeof(*hdr)); | ||
| + | ||
| + for (i = 0; i < hdr->num_packets; i++) { | ||
| + packet = (struct iris_hfi_packet *)pkt; | ||
| + ret = iris_hfi_gen2_validate_packet(pkt, core->response_packet); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + pkt += packet->size; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_system_error(struct iris_core *core, | ||
| + struct iris_hfi_packet *pkt) | ||
| +{ | ||
| + dev_err(core->dev, "received system error of type %#x\n", pkt->type); | ||
| + | ||
| + core->state = IRIS_CORE_ERROR; | ||
| + schedule_delayed_work(&core->sys_error_handler, msecs_to_jiffies(10)); | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_system_init(struct iris_core *core, | ||
| + struct iris_hfi_packet *pkt) | ||
| +{ | ||
| + if (!(pkt->flags & HFI_FW_FLAGS_SUCCESS)) { | ||
| + core->state = IRIS_CORE_ERROR; | ||
| + return 0; | ||
| + } | ||
| + | ||
| + complete(&core->core_init_done); | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_image_version_property(struct iris_core *core, | ||
| + struct iris_hfi_packet *pkt) | ||
| +{ | ||
| + u8 *str_image_version = (u8 *)pkt + sizeof(*pkt); | ||
| + u32 req_bytes = pkt->size - sizeof(*pkt); | ||
| + char fw_version[IRIS_FW_VERSION_LENGTH]; | ||
| + u32 i; | ||
| + | ||
| + if (req_bytes < IRIS_FW_VERSION_LENGTH - 1) | ||
| + return -EINVAL; | ||
| + | ||
| + for (i = 0; i < IRIS_FW_VERSION_LENGTH - 1; i++) { | ||
| + if (str_image_version[i] != '\0') | ||
| + fw_version[i] = str_image_version[i]; | ||
| + else | ||
| + fw_version[i] = ' '; | ||
| + } | ||
| + fw_version[i] = '\0'; | ||
| + dev_dbg(core->dev, "firmware version: %s\n", fw_version); | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_system_property(struct iris_core *core, | ||
| + struct iris_hfi_packet *pkt) | ||
| +{ | ||
| + switch (pkt->type) { | ||
| + case HFI_PROP_IMAGE_VERSION: | ||
| + return iris_hfi_gen2_handle_image_version_property(core, pkt); | ||
| + default: | ||
| + return 0; | ||
| + } | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_system_response(struct iris_core *core, | ||
| + struct iris_hfi_header *hdr) | ||
| +{ | ||
| + u8 *start_pkt = (u8 *)((u8 *)hdr + sizeof(*hdr)); | ||
| + struct iris_hfi_packet *packet; | ||
| + u32 i, j; | ||
| + u8 *pkt; | ||
| + int ret; | ||
| + static const struct iris_hfi_gen2_core_hfi_range range[] = { | ||
| + {HFI_SYSTEM_ERROR_BEGIN, HFI_SYSTEM_ERROR_END, iris_hfi_gen2_handle_system_error }, | ||
| + {HFI_PROP_BEGIN, HFI_PROP_END, iris_hfi_gen2_handle_system_property }, | ||
| + {HFI_CMD_BEGIN, HFI_CMD_END, iris_hfi_gen2_handle_system_init }, | ||
| + }; | ||
| + | ||
| + for (i = 0; i < ARRAY_SIZE(range); i++) { | ||
| + pkt = start_pkt; | ||
| + for (j = 0; j < hdr->num_packets; j++) { | ||
| + packet = (struct iris_hfi_packet *)pkt; | ||
| + if (packet->flags & HFI_FW_FLAGS_SYSTEM_ERROR) { | ||
| + ret = iris_hfi_gen2_handle_system_error(core, packet); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + if (packet->type > range[i].begin && packet->type < range[i].end) { | ||
| + ret = range[i].handle(core, packet); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + if (packet->type > HFI_SYSTEM_ERROR_BEGIN && | ||
| + packet->type < HFI_SYSTEM_ERROR_END) | ||
| + return 0; | ||
| + } | ||
| + pkt += packet->size; | ||
| + } | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_handle_response(struct iris_core *core, void *response) | ||
| +{ | ||
| + struct iris_hfi_header *hdr = (struct iris_hfi_header *)response; | ||
| + int ret; | ||
| + | ||
| + ret = iris_hfi_gen2_validate_hdr_packet(core, hdr); | ||
| + if (ret) | ||
| + return iris_hfi_gen2_handle_system_error(core, NULL); | ||
| + | ||
| + return iris_hfi_gen2_handle_system_response(core, hdr); | ||
| +} | ||
| + | ||
| +static void iris_hfi_gen2_flush_debug_queue(struct iris_core *core, u8 *packet) | ||
| +{ | ||
| + struct hfi_debug_header *pkt; | ||
| + u8 *log; | ||
| + | ||
| + while (!iris_hfi_queue_dbg_read(core, packet)) { | ||
| + pkt = (struct hfi_debug_header *)packet; | ||
| + | ||
| + if (pkt->size < sizeof(*pkt)) | ||
| + continue; | ||
| + | ||
| + if (pkt->size >= IFACEQ_CORE_PKT_SIZE) | ||
| + continue; | ||
| + | ||
| + packet[pkt->size] = '\0'; | ||
| + log = (u8 *)packet + sizeof(*pkt) + 1; | ||
| + dev_dbg(core->dev, "%s", log); | ||
| + } | ||
| +} | ||
| + | ||
| +static void iris_hfi_gen2_response_handler(struct iris_core *core) | ||
| +{ | ||
| + if (iris_vpu_watchdog(core, core->intr_status)) { | ||
| + struct iris_hfi_packet pkt = {.type = HFI_SYS_ERROR_WD_TIMEOUT}; | ||
| + | ||
| + dev_err(core->dev, "cpu watchdog error received\n"); | ||
| + core->state = IRIS_CORE_ERROR; | ||
| + iris_hfi_gen2_handle_system_error(core, &pkt); | ||
| + | ||
| + return; | ||
| + } | ||
| + | ||
| + memset(core->response_packet, 0, sizeof(struct iris_hfi_header)); | ||
| + while (!iris_hfi_queue_msg_read(core, core->response_packet)) { | ||
| + iris_hfi_gen2_handle_response(core, core->response_packet); | ||
| + memset(core->response_packet, 0, sizeof(struct iris_hfi_header)); | ||
| + } | ||
| + | ||
| + iris_hfi_gen2_flush_debug_queue(core, core->response_packet); | ||
| +} | ||
| + | ||
| +static const struct iris_hfi_response_ops iris_hfi_gen2_response_ops = { | ||
| + .hfi_response_handler = iris_hfi_gen2_response_handler, | ||
| +}; | ||
| + | ||
| +void iris_hfi_gen2_response_ops_init(struct iris_core *core) | ||
| +{ | ||
| + core->hfi_response_ops = &iris_hfi_gen2_response_ops; | ||
| +} |
There was a problem hiding this comment.
Guard against NULL in system-error handling on validation failures.
iris_hfi_gen2_handle_response() calls iris_hfi_gen2_handle_system_error(core, NULL) when validation fails, but iris_hfi_gen2_handle_system_error() unconditionally dereferences pkt->type.
🐛 Proposed fix
@@
static int iris_hfi_gen2_handle_system_error(struct iris_core *core,
struct iris_hfi_packet *pkt)
{
- dev_err(core->dev, "received system error of type %#x\n", pkt->type);
+ u32 type = pkt ? pkt->type : 0;
+ dev_err(core->dev, "received system error of type %#x\n", type);📝 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.
| @@ -0,0 +1,215 @@ | |
| +// SPDX-License-Identifier: GPL-2.0-only | |
| +/* | |
| + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. | |
| + */ | |
| + | |
| +#include "iris_hfi_gen2.h" | |
| +#include "iris_hfi_gen2_defines.h" | |
| +#include "iris_hfi_gen2_packet.h" | |
| +#include "iris_vpu_common.h" | |
| + | |
| +struct iris_hfi_gen2_core_hfi_range { | |
| + u32 begin; | |
| + u32 end; | |
| + int (*handle)(struct iris_core *core, struct iris_hfi_packet *pkt); | |
| +}; | |
| + | |
| +static int iris_hfi_gen2_validate_packet(u8 *response_pkt, u8 *core_resp_pkt) | |
| +{ | |
| + u8 *response_limit = core_resp_pkt + IFACEQ_CORE_PKT_SIZE; | |
| + u32 response_pkt_size = *(u32 *)response_pkt; | |
| + | |
| + if (!response_pkt_size) | |
| + return -EINVAL; | |
| + | |
| + if (response_pkt_size < sizeof(struct iris_hfi_packet)) | |
| + return -EINVAL; | |
| + | |
| + if (response_pkt + response_pkt_size > response_limit) | |
| + return -EINVAL; | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_validate_hdr_packet(struct iris_core *core, struct iris_hfi_header *hdr) | |
| +{ | |
| + struct iris_hfi_packet *packet; | |
| + int ret; | |
| + u8 *pkt; | |
| + u32 i; | |
| + | |
| + if (hdr->size < sizeof(*hdr) + sizeof(*packet)) | |
| + return -EINVAL; | |
| + | |
| + pkt = (u8 *)((u8 *)hdr + sizeof(*hdr)); | |
| + | |
| + for (i = 0; i < hdr->num_packets; i++) { | |
| + packet = (struct iris_hfi_packet *)pkt; | |
| + ret = iris_hfi_gen2_validate_packet(pkt, core->response_packet); | |
| + if (ret) | |
| + return ret; | |
| + | |
| + pkt += packet->size; | |
| + } | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_system_error(struct iris_core *core, | |
| + struct iris_hfi_packet *pkt) | |
| +{ | |
| + dev_err(core->dev, "received system error of type %#x\n", pkt->type); | |
| + | |
| + core->state = IRIS_CORE_ERROR; | |
| + schedule_delayed_work(&core->sys_error_handler, msecs_to_jiffies(10)); | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_system_init(struct iris_core *core, | |
| + struct iris_hfi_packet *pkt) | |
| +{ | |
| + if (!(pkt->flags & HFI_FW_FLAGS_SUCCESS)) { | |
| + core->state = IRIS_CORE_ERROR; | |
| + return 0; | |
| + } | |
| + | |
| + complete(&core->core_init_done); | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_image_version_property(struct iris_core *core, | |
| + struct iris_hfi_packet *pkt) | |
| +{ | |
| + u8 *str_image_version = (u8 *)pkt + sizeof(*pkt); | |
| + u32 req_bytes = pkt->size - sizeof(*pkt); | |
| + char fw_version[IRIS_FW_VERSION_LENGTH]; | |
| + u32 i; | |
| + | |
| + if (req_bytes < IRIS_FW_VERSION_LENGTH - 1) | |
| + return -EINVAL; | |
| + | |
| + for (i = 0; i < IRIS_FW_VERSION_LENGTH - 1; i++) { | |
| + if (str_image_version[i] != '\0') | |
| + fw_version[i] = str_image_version[i]; | |
| + else | |
| + fw_version[i] = ' '; | |
| + } | |
| + fw_version[i] = '\0'; | |
| + dev_dbg(core->dev, "firmware version: %s\n", fw_version); | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_system_property(struct iris_core *core, | |
| + struct iris_hfi_packet *pkt) | |
| +{ | |
| + switch (pkt->type) { | |
| + case HFI_PROP_IMAGE_VERSION: | |
| + return iris_hfi_gen2_handle_image_version_property(core, pkt); | |
| + default: | |
| + return 0; | |
| + } | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_system_response(struct iris_core *core, | |
| + struct iris_hfi_header *hdr) | |
| +{ | |
| + u8 *start_pkt = (u8 *)((u8 *)hdr + sizeof(*hdr)); | |
| + struct iris_hfi_packet *packet; | |
| + u32 i, j; | |
| + u8 *pkt; | |
| + int ret; | |
| + static const struct iris_hfi_gen2_core_hfi_range range[] = { | |
| + {HFI_SYSTEM_ERROR_BEGIN, HFI_SYSTEM_ERROR_END, iris_hfi_gen2_handle_system_error }, | |
| + {HFI_PROP_BEGIN, HFI_PROP_END, iris_hfi_gen2_handle_system_property }, | |
| + {HFI_CMD_BEGIN, HFI_CMD_END, iris_hfi_gen2_handle_system_init }, | |
| + }; | |
| + | |
| + for (i = 0; i < ARRAY_SIZE(range); i++) { | |
| + pkt = start_pkt; | |
| + for (j = 0; j < hdr->num_packets; j++) { | |
| + packet = (struct iris_hfi_packet *)pkt; | |
| + if (packet->flags & HFI_FW_FLAGS_SYSTEM_ERROR) { | |
| + ret = iris_hfi_gen2_handle_system_error(core, packet); | |
| + return ret; | |
| + } | |
| + | |
| + if (packet->type > range[i].begin && packet->type < range[i].end) { | |
| + ret = range[i].handle(core, packet); | |
| + if (ret) | |
| + return ret; | |
| + | |
| + if (packet->type > HFI_SYSTEM_ERROR_BEGIN && | |
| + packet->type < HFI_SYSTEM_ERROR_END) | |
| + return 0; | |
| + } | |
| + pkt += packet->size; | |
| + } | |
| + } | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_handle_response(struct iris_core *core, void *response) | |
| +{ | |
| + struct iris_hfi_header *hdr = (struct iris_hfi_header *)response; | |
| + int ret; | |
| + | |
| + ret = iris_hfi_gen2_validate_hdr_packet(core, hdr); | |
| + if (ret) | |
| + return iris_hfi_gen2_handle_system_error(core, NULL); | |
| + | |
| + return iris_hfi_gen2_handle_system_response(core, hdr); | |
| +} | |
| + | |
| +static void iris_hfi_gen2_flush_debug_queue(struct iris_core *core, u8 *packet) | |
| +{ | |
| + struct hfi_debug_header *pkt; | |
| + u8 *log; | |
| + | |
| + while (!iris_hfi_queue_dbg_read(core, packet)) { | |
| + pkt = (struct hfi_debug_header *)packet; | |
| + | |
| + if (pkt->size < sizeof(*pkt)) | |
| + continue; | |
| + | |
| + if (pkt->size >= IFACEQ_CORE_PKT_SIZE) | |
| + continue; | |
| + | |
| + packet[pkt->size] = '\0'; | |
| + log = (u8 *)packet + sizeof(*pkt) + 1; | |
| + dev_dbg(core->dev, "%s", log); | |
| + } | |
| +} | |
| + | |
| +static void iris_hfi_gen2_response_handler(struct iris_core *core) | |
| +{ | |
| + if (iris_vpu_watchdog(core, core->intr_status)) { | |
| + struct iris_hfi_packet pkt = {.type = HFI_SYS_ERROR_WD_TIMEOUT}; | |
| + | |
| + dev_err(core->dev, "cpu watchdog error received\n"); | |
| + core->state = IRIS_CORE_ERROR; | |
| + iris_hfi_gen2_handle_system_error(core, &pkt); | |
| + | |
| + return; | |
| + } | |
| + | |
| + memset(core->response_packet, 0, sizeof(struct iris_hfi_header)); | |
| + while (!iris_hfi_queue_msg_read(core, core->response_packet)) { | |
| + iris_hfi_gen2_handle_response(core, core->response_packet); | |
| + memset(core->response_packet, 0, sizeof(struct iris_hfi_header)); | |
| + } | |
| + | |
| + iris_hfi_gen2_flush_debug_queue(core, core->response_packet); | |
| +} | |
| + | |
| +static const struct iris_hfi_response_ops iris_hfi_gen2_response_ops = { | |
| + .hfi_response_handler = iris_hfi_gen2_response_handler, | |
| +}; | |
| + | |
| +void iris_hfi_gen2_response_ops_init(struct iris_core *core) | |
| +{ | |
| + core->hfi_response_ops = &iris_hfi_gen2_response_ops; | |
| +} | |
| static int iris_hfi_gen2_handle_system_error(struct iris_core *core, | |
| struct iris_hfi_packet *pkt) | |
| { | |
| u32 type = pkt ? pkt->type : 0; | |
| dev_err(core->dev, "received system error of type %#x\n", type); | |
| core->state = IRIS_CORE_ERROR; | |
| schedule_delayed_work(&core->sys_error_handler, msecs_to_jiffies(10)); | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0007_media--iris--introduce-host-firmware-interface-wit.patch`
around lines 1066 - 1281, The response-path can call
iris_hfi_gen2_handle_system_error(core, NULL) on validation failure but
iris_hfi_gen2_handle_system_error dereferences pkt->type; modify
iris_hfi_gen2_handle_system_error to first check if pkt is NULL and handle that
case (log a generic system error message, set core->state = IRIS_CORE_ERROR and
schedule_delayed_work(&core->sys_error_handler, ...)) instead of dereferencing
pkt, leaving existing behavior for non-NULL pkt; this avoids changing callers
like iris_hfi_gen2_handle_response while ensuring safe NULL handling.
| + struct v4l2_format *f; | ||
| + | ||
| + inst->fmt_src = kzalloc(sizeof(*inst->fmt_src), GFP_KERNEL); | ||
| + inst->fmt_dst = kzalloc(sizeof(*inst->fmt_dst), GFP_KERNEL); | ||
| + | ||
| + inst->fw_min_count = MIN_BUFFERS; | ||
| + | ||
| + f = inst->fmt_src; | ||
| + f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; | ||
| + f->fmt.pix_mp.width = DEFAULT_WIDTH; | ||
| + f->fmt.pix_mp.height = DEFAULT_HEIGHT; | ||
| + f->fmt.pix_mp.pixelformat = V4L2_PIX_FMT_H264; | ||
| + f->fmt.pix_mp.num_planes = 1; | ||
| + f->fmt.pix_mp.plane_fmt[0].bytesperline = 0; | ||
| + f->fmt.pix_mp.plane_fmt[0].sizeimage = iris_get_buffer_size(inst, BUF_INPUT); | ||
| + f->fmt.pix_mp.field = V4L2_FIELD_NONE; | ||
| + inst->buffers[BUF_INPUT].min_count = iris_vpu_buf_count(inst, BUF_INPUT); | ||
| + inst->buffers[BUF_INPUT].size = f->fmt.pix_mp.plane_fmt[0].sizeimage; | ||
| + | ||
| + f = inst->fmt_dst; | ||
| + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; | ||
| + f->fmt.pix_mp.pixelformat = V4L2_PIX_FMT_NV12; | ||
| + f->fmt.pix_mp.width = ALIGN(DEFAULT_WIDTH, 128); | ||
| + f->fmt.pix_mp.height = ALIGN(DEFAULT_HEIGHT, 32); | ||
| + f->fmt.pix_mp.num_planes = 1; | ||
| + f->fmt.pix_mp.plane_fmt[0].bytesperline = ALIGN(DEFAULT_WIDTH, 128); | ||
| + f->fmt.pix_mp.plane_fmt[0].sizeimage = iris_get_buffer_size(inst, BUF_OUTPUT); | ||
| + f->fmt.pix_mp.field = V4L2_FIELD_NONE; | ||
| + f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_DEFAULT; | ||
| + f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT; | ||
| + f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; | ||
| + f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT; | ||
| + inst->buffers[BUF_OUTPUT].min_count = iris_vpu_buf_count(inst, BUF_OUTPUT); | ||
| + inst->buffers[BUF_OUTPUT].size = f->fmt.pix_mp.plane_fmt[0].sizeimage; | ||
| +} |
There was a problem hiding this comment.
Missing NULL checks for kzalloc allocations will cause kernel panic.
The kzalloc calls for inst->fmt_src and inst->fmt_dst at lines 1385-1386 do not check for allocation failure. If either allocation fails, the subsequent dereferences will cause a NULL pointer dereference and kernel crash.
The function should return an error code and the caller should handle the failure.
🐛 Proposed fix
-void iris_vdec_inst_init(struct iris_inst *inst)
+int iris_vdec_inst_init(struct iris_inst *inst)
{
struct v4l2_format *f;
inst->fmt_src = kzalloc(sizeof(*inst->fmt_src), GFP_KERNEL);
+ if (!inst->fmt_src)
+ return -ENOMEM;
+
inst->fmt_dst = kzalloc(sizeof(*inst->fmt_dst), GFP_KERNEL);
+ if (!inst->fmt_dst) {
+ kfree(inst->fmt_src);
+ return -ENOMEM;
+ }
inst->fw_min_count = MIN_BUFFERS;
/* ... rest of initialization ... */
+
+ return 0;
}The caller in iris_open should also check the return value:
- iris_vdec_inst_init(inst);
+ ret = iris_vdec_inst_init(inst);
+ if (ret)
+ goto fail_m2m_ctx_release;🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch`
around lines 1383 - 1417, The kzallocs for inst->fmt_src and inst->fmt_dst in
the initialization block must be checked for NULL to avoid kernel panics; after
each kzalloc (allocating inst->fmt_src and inst->fmt_dst) verify the pointer and
on failure free any prior allocation, set a proper error return (e.g. -ENOMEM)
and abort initialization so the caller (iris_open) can handle it; update the
function that contains these symbols (references: inst->fmt_src, inst->fmt_dst,
iris_get_buffer_size, iris_vpu_buf_count) to perform the NULL checks, free the
other fmt pointer if needed, and return an error code instead of continuing to
dereference.
| +void iris_session_init_caps(struct iris_core *core) | ||
| +{ | ||
| + struct platform_inst_fw_cap *caps; | ||
| + u32 i, num_cap, cap_id; | ||
| + | ||
| + caps = core->iris_platform_data->inst_fw_caps; | ||
| + num_cap = core->iris_platform_data->inst_fw_caps_size; | ||
| + | ||
| + for (i = 0; i < num_cap; i++) { | ||
| + cap_id = caps[i].cap_id; | ||
| + if (!iris_valid_cap_id(cap_id)) | ||
| + continue; | ||
| + | ||
| + core->inst_fw_caps[cap_id].cap_id = caps[i].cap_id; | ||
| + core->inst_fw_caps[cap_id].min = caps[i].min; | ||
| + core->inst_fw_caps[cap_id].max = caps[i].max; | ||
| + core->inst_fw_caps[cap_id].step_or_mask = caps[i].step_or_mask; | ||
| + core->inst_fw_caps[cap_id].value = caps[i].value; | ||
| + core->inst_fw_caps[cap_id].flags = caps[i].flags; | ||
| + core->inst_fw_caps[cap_id].hfi_id = caps[i].hfi_id; | ||
| + } | ||
| +} |
There was a problem hiding this comment.
Guard against NULL/empty platform caps to avoid probe-time NULL deref.
iris_session_init_caps() unconditionally dereferences inst_fw_caps; platforms without caps would crash at probe.
🛡️ Proposed fix
void iris_session_init_caps(struct iris_core *core)
{
struct platform_inst_fw_cap *caps;
u32 i, num_cap, cap_id;
caps = core->iris_platform_data->inst_fw_caps;
num_cap = core->iris_platform_data->inst_fw_caps_size;
+ if (!caps || !num_cap)
+ return;🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0014_media--iris--implement-iris-v4l2_ctrl_ops.patch`
around lines 219 - 240, The function iris_session_init_caps currently
dereferences platform caps without checks; update it to first validate
core->iris_platform_data, core->iris_platform_data->inst_fw_caps and
core->iris_platform_data->inst_fw_caps_size (and optionally core->inst_fw_caps)
and return early if any are NULL or size is 0 to avoid probe-time NULL derefs.
Locate iris_session_init_caps and add these guards before using caps, then
proceed with the existing loop that uses iris_valid_cap_id and fills
core->inst_fw_caps only when inputs are present.
| +static int iris_create_internal_buffer(struct iris_inst *inst, | ||
| + enum iris_buffer_type buffer_type, u32 index) | ||
| +{ | ||
| + struct iris_buffers *buffers = &inst->buffers[buffer_type]; | ||
| + struct iris_core *core = inst->core; | ||
| + struct iris_buffer *buffer; | ||
| + | ||
| + if (!buffers->size) | ||
| + return 0; | ||
| + | ||
| + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); | ||
| + if (!buffer) | ||
| + return -ENOMEM; | ||
| + | ||
| + INIT_LIST_HEAD(&buffer->list); | ||
| + buffer->type = buffer_type; | ||
| + buffer->index = index; | ||
| + buffer->buffer_size = buffers->size; | ||
| + buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING; | ||
| + list_add_tail(&buffer->list, &buffers->list); | ||
| + | ||
| + buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size, | ||
| + &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs); | ||
| + if (!buffer->kvaddr) | ||
| + return -ENOMEM; | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
Memory leak on DMA allocation failure.
If dma_alloc_attrs() fails at line 100-102, the buffer struct has already been added to the list at line 98. The function returns -ENOMEM without removing the buffer from the list or freeing it, causing a memory leak.
🐛 Proposed fix to clean up on allocation failure
buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING;
- list_add_tail(&buffer->list, &buffers->list);
buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size,
&buffer->device_addr, GFP_KERNEL, buffer->dma_attrs);
- if (!buffer->kvaddr)
+ if (!buffer->kvaddr) {
+ kfree(buffer);
return -ENOMEM;
+ }
+
+ list_add_tail(&buffer->list, &buffers->list);
return 0;
}📝 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.
| +static int iris_create_internal_buffer(struct iris_inst *inst, | |
| + enum iris_buffer_type buffer_type, u32 index) | |
| +{ | |
| + struct iris_buffers *buffers = &inst->buffers[buffer_type]; | |
| + struct iris_core *core = inst->core; | |
| + struct iris_buffer *buffer; | |
| + | |
| + if (!buffers->size) | |
| + return 0; | |
| + | |
| + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); | |
| + if (!buffer) | |
| + return -ENOMEM; | |
| + | |
| + INIT_LIST_HEAD(&buffer->list); | |
| + buffer->type = buffer_type; | |
| + buffer->index = index; | |
| + buffer->buffer_size = buffers->size; | |
| + buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING; | |
| + list_add_tail(&buffer->list, &buffers->list); | |
| + | |
| + buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size, | |
| + &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs); | |
| + if (!buffer->kvaddr) | |
| + return -ENOMEM; | |
| + | |
| + return 0; | |
| +} | |
| static int iris_create_internal_buffer(struct iris_inst *inst, | |
| enum iris_buffer_type buffer_type, u32 index) | |
| { | |
| struct iris_buffers *buffers = &inst->buffers[buffer_type]; | |
| struct iris_core *core = inst->core; | |
| struct iris_buffer *buffer; | |
| if (!buffers->size) | |
| return 0; | |
| buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); | |
| if (!buffer) | |
| return -ENOMEM; | |
| INIT_LIST_HEAD(&buffer->list); | |
| buffer->type = buffer_type; | |
| buffer->index = index; | |
| buffer->buffer_size = buffers->size; | |
| buffer->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_NO_KERNEL_MAPPING; | |
| buffer->kvaddr = dma_alloc_attrs(core->dev, buffer->buffer_size, | |
| &buffer->device_addr, GFP_KERNEL, buffer->dma_attrs); | |
| if (!buffer->kvaddr) { | |
| kfree(buffer); | |
| return -ENOMEM; | |
| } | |
| list_add_tail(&buffer->list, &buffers->list); | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0019_media--iris--allocate`,-initialize-and-queue-intern.patch
around lines 79 - 106, The function iris_create_internal_buffer adds the newly
allocated struct iris_buffer (variable buffer) into buffers->list before calling
dma_alloc_attrs, and if dma_alloc_attrs fails the code returns -ENOMEM leaving
buffer in the list and leaking memory; fix iris_create_internal_buffer by
undoing the prior list_add_tail on allocation failure: call
list_del(&buffer->list) and kfree(buffer) (no dma_free needed since kvaddr is
NULL) before returning the error from the dma_alloc_attrs call.
| + } else if (cmd == V4L2_DEC_CMD_STOP) { | ||
| + if (vb2_is_streaming(src_q)) | ||
| + if (inst->sub_state != IRIS_INST_SUB_DRAIN) | ||
| + return true; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure
find . -type f -name "*iris*" | head -20Repository: armbian/build
Length of output: 1934
🏁 Script executed:
#!/bin/bash
# Find the actual iris driver source code (not just patches)
find . -type f -path "*iris*" -name "*.c" -o -path "*iris*" -name "*.h" | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for the patch file mentioned in the review
find . -name "0023_media--iris--add-support-for-drain-sequence.patch" 2>/dev/nullRepository: armbian/build
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Read the patch file to understand the context around lines 392-396
cat -n ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch | sed -n '380,410p'Repository: armbian/build
Length of output: 1494
🏁 Script executed:
#!/bin/bash
# Search for IRIS_INST_SUB_DRAIN definition in the patch file
grep -n "IRIS_INST_SUB" ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch | head -30Repository: armbian/build
Length of output: 1662
🏁 Script executed:
#!/bin/bash
# Look for enum or define statements in the entire patch that show state definitions
grep -n -E "(enum|define|IRIS_INST_SUB_)" ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patchRepository: armbian/build
Length of output: 5630
🏁 Script executed:
#!/bin/bash
# Verify the pattern described in the review by checking how sub_state is manipulated
# Look at iris_inst_change_sub_state to understand if multiple bits can be set
grep -A 5 "iris_inst_change_sub_state" ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch | head -20Repository: armbian/build
Length of output: 585
🏁 Script executed:
#!/bin/bash
# Check other similar state checks in the patch for comparison
grep -n "sub_state.*==" ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patchRepository: armbian/build
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Final confirmation: Show the exact context of line 394 and surrounding similar checks
sed -n '370,400p' ./patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patchRepository: armbian/build
Length of output: 1018
Fix incorrect state check: use bitwise AND instead of inequality operator.
The condition inst->sub_state != IRIS_INST_SUB_DRAIN at line 394 checks for exact equality, which is inconsistent with how state bits are used throughout the patch. Since sub_state combines multiple single-bit flags (defined as BIT(3), BIT(4), etc.), it can have multiple bits set simultaneously—for example, IRIS_INST_SUB_DRAIN | IRIS_INST_SUB_INPUT_PAUSE. The inequality check returns true when sub_state has other bits set alongside IRIS_INST_SUB_DRAIN, allowing STOP to proceed while drain is still in progress. Every other state check in the patch uses bitwise AND (&), making this the only place using equality comparison. The fix should use !(inst->sub_state & IRIS_INST_SUB_DRAIN) to correctly block STOP when the drain bit is set.
Proposed fix
} else if (cmd == V4L2_DEC_CMD_STOP) {
if (vb2_is_streaming(src_q))
- if (inst->sub_state != IRIS_INST_SUB_DRAIN)
+ if (!(inst->sub_state & IRIS_INST_SUB_DRAIN))
return true;
}📝 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.
| + } else if (cmd == V4L2_DEC_CMD_STOP) { | |
| + if (vb2_is_streaming(src_q)) | |
| + if (inst->sub_state != IRIS_INST_SUB_DRAIN) | |
| + return true; | |
| + } | |
| } else if (cmd == V4L2_DEC_CMD_STOP) { | |
| if (vb2_is_streaming(src_q)) | |
| if (!(inst->sub_state & IRIS_INST_SUB_DRAIN)) | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch`
around lines 392 - 396, The condition guarding STOP uses an equality test
against inst->sub_state (inst->sub_state != IRIS_INST_SUB_DRAIN) which is wrong
for bitflags; change the check to test the drain bit with a bitwise AND and
negate it (i.e. use !(inst->sub_state & IRIS_INST_SUB_DRAIN)) so STOP is blocked
while the IRIS_INST_SUB_DRAIN bit is set; update the branch that handles
V4L2_DEC_CMD_STOP and streaming (vb2_is_streaming(src_q)) to use this bitwise
test against inst->sub_state.
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/boards/ayn-odin2-grub.csc`:
- Around line 67-110: The post_family_tweaks__ayn-odin2_enable_services function
currently unconditionally moves armbian.sources.disabled which can fail under
errexit and enables qbootctl.service twice; change the mv operations to be
guarded (e.g. test existence: [[ -f
"${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]] && mv ... ) and
similarly guard the restore move, and remove the duplicate chroot_sdcard
systemctl enable qbootctl.service so qbootctl.service is enabled only once;
update references to the exact filenames
("${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" and
"armbian.sources") and the command chroot_sdcard systemctl enable
qbootctl.service in the function.
🧹 Nitpick comments (1)
config/boards/ayn-odin2-grub.csc (1)
112-142: Avoid glob expansion errors in the initramfs hook when directories are missing.
Usenullglobso missing firmware directories don’t pass literal globs intoadd_firmware.🧩 Suggested hardening
. /usr/share/initramfs-tools/hook-functions + shopt -s nullglob for f in /lib/firmware/qcom/sm8550/* ; do add_firmware "${f#/lib/firmware/}" done @@ for f in /lib/firmware/qca/* ; do add_firmware "${f#/lib/firmware/}" done + shopt -u nullglob
| function post_family_tweaks__ayn-odin2_enable_services() { | ||
| if ! ayn-odin2_is_userspace_supported; then | ||
| if [[ "${RELEASE}" != "" ]]; then | ||
| display_alert "Missing userspace for ${BOARD}" "${RELEASE} does not have the userspace necessary to support the ${BOARD}" "warn" | ||
| fi | ||
| return 0 | ||
| fi | ||
|
|
||
| if [[ "${RELEASE}" == "jammy" ]] || [[ "${RELEASE}" == "noble" ]] || [[ "${RELEASE}" == "plucky" ]]; then | ||
| display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "warn" | ||
| do_with_retries 3 chroot_sdcard add-apt-repository ppa:kisak/kisak-mesa --yes | ||
|
|
||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| display_alert "Installing Mesa Vulkan Drivers" | ||
| do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools | ||
| fi | ||
|
|
||
| if [[ "${RELEASE}" == "trixie" ]]; then | ||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| display_alert "Installing Mesa Vulkan Drivers" | ||
| do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools | ||
| fi | ||
|
|
||
| # We need unudhcpd from armbian repo, so enable it | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources | ||
|
|
||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| display_alert "Installing ${BOARD} tweaks" "warn" | ||
| do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg | ||
| # disable armbian repo back | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled | ||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| chroot_sdcard systemctl enable qbootctl.service | ||
|
|
||
| # Add Gamepad udev rule | ||
| echo 'SUBSYSTEM=="input", ATTRS{name}=="AYN Odin2 Gamepad", MODE="0666", ENV{ID_INPUT_JOYSTICK}="1"' > "${SDCARD}"/etc/udev/rules.d/99-ignore-gamepad.rules | ||
| # Not Any driver support suspend mode | ||
| chroot_sdcard systemctl mask suspend.target | ||
|
|
||
| chroot_sdcard systemctl enable qbootctl.service | ||
| chroot_sdcard systemctl enable usbgadget-rndis.service | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
Guard Armbian sources toggling and drop duplicate qbootctl enablement.
If the armbian.sources.disabled file is absent or renamed, mv can fail and abort the build under errexit. Also, qbootctl.service is enabled twice.
🛠️ Suggested fix
# We need unudhcpd from armbian repo, so enable it
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources
+ local armbian_sources_was_disabled=""
+ if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" "${SDCARD}/etc/apt/sources.list.d/armbian.sources"
+ armbian_sources_was_disabled="yes"
+ fi
@@
# disable armbian repo back
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled
+ if [[ "${armbian_sources_was_disabled}" == "yes" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources" "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled"
+ fi
@@
- chroot_sdcard systemctl enable qbootctl.service
@@
chroot_sdcard systemctl enable qbootctl.service🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-grub.csc` around lines 67 - 110, The
post_family_tweaks__ayn-odin2_enable_services function currently unconditionally
moves armbian.sources.disabled which can fail under errexit and enables
qbootctl.service twice; change the mv operations to be guarded (e.g. test
existence: [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]
&& mv ... ) and similarly guard the restore move, and remove the duplicate
chroot_sdcard systemctl enable qbootctl.service so qbootctl.service is enabled
only once; update references to the exact filenames
("${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" and
"armbian.sources") and the command chroot_sdcard systemctl enable
qbootctl.service in the function.
a4c5f01 to
03c0034
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/boards/ayn-odin2.csc (1)
84-92:⚠️ Potential issue | 🟡 MinorGuard Armbian sources toggling to prevent build failures.
If
armbian.sources.disableddoesn't exist, themvcommand will fail undererrexit. This is the same issue flagged in the GRUB variant.🛠️ Suggested fix
# We need unudhcpd from armbian repo, so enable it - mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources + if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]; then + mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" "${SDCARD}/etc/apt/sources.list.d/armbian.sources" + fi do_with_retries 3 chroot_sdcard_apt_get_update display_alert "Installing ${BOARD} tweaks" "warn" do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg # disable armbian repo back - mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled + if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources" ]]; then + mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources" "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" + fi
🤖 Fix all issues with AI agents
In `@config/boards/ayn-odin2-portal-grub.csc`:
- Around line 89-97: The two bare mv operations that toggle armbian.sources can
fail under errexit if the files are missing; wrap both moves with existence
checks (e.g., test -e before mv) or use safe mv semantics and ensure the
disable-back step always runs (place it in a cleanup/finally path or trap) so
package installation (do_with_retries / chroot_sdcard_apt_get_install) cannot
cause the script to exit with the armbian repo left enabled; update the block
that references "${SDCARD}"/etc/apt/sources.list.d/armbian.sources and
armbian.sources.disabled accordingly.
🧹 Nitpick comments (2)
config/boards/ayn-odin2-grub.csc (1)
32-46: Consider error handling for network operations.The
curlandunzipcommands could fail silently within the subshell. Consider addingset -eor explicit error checks.♻️ Optional improvement
( + set -e cd "${SDCARD}/usr/share/alsa" || exit 6 curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" + [[ -f temp.zip ]] || { echo "Failed to download alsa-ucm-conf"; exit 1; } unzip -o temp.zipconfig/boards/ayn-odin2-portal-grub.csc (1)
32-46: Consider error handling for network operations.Same as the non-Portal variant:
curlandunzipcommands could fail silently.♻️ Optional improvement
( + set -e cd "${SDCARD}/usr/share/alsa" || exit 6 curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" + [[ -f temp.zip ]] || { echo "Failed to download alsa-ucm-conf"; exit 1; } unzip -o temp.zip
| # We need unudhcpd from armbian repo, so enable it | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources | ||
|
|
||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| display_alert "Installing ${BOARD} tweaks" "warn" | ||
| do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg | ||
| # disable armbian repo back | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled | ||
| do_with_retries 3 chroot_sdcard_apt_get_update |
There was a problem hiding this comment.
Guard Armbian sources toggling to prevent build failures.
The mv commands can fail under errexit if the source file doesn't exist.
🛠️ Suggested fix
# We need unudhcpd from armbian repo, so enable it
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources
+ if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" "${SDCARD}/etc/apt/sources.list.d/armbian.sources"
+ fi
do_with_retries 3 chroot_sdcard_apt_get_update
display_alert "Installing ${BOARD} tweaks" "warn"
do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg
# disable armbian repo back
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled
+ if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources" "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled"
+ fi📝 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.
| # We need unudhcpd from armbian repo, so enable it | |
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources | |
| do_with_retries 3 chroot_sdcard_apt_get_update | |
| display_alert "Installing ${BOARD} tweaks" "warn" | |
| do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg | |
| # disable armbian repo back | |
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled | |
| do_with_retries 3 chroot_sdcard_apt_get_update | |
| # We need unudhcpd from armbian repo, so enable it | |
| if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]; then | |
| mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" "${SDCARD}/etc/apt/sources.list.d/armbian.sources" | |
| fi | |
| do_with_retries 3 chroot_sdcard_apt_get_update | |
| display_alert "Installing ${BOARD} tweaks" "warn" | |
| do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg | |
| # disable armbian repo back | |
| if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources" ]]; then | |
| mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources" "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" | |
| fi | |
| do_with_retries 3 chroot_sdcard_apt_get_update |
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal-grub.csc` around lines 89 - 97, The two bare
mv operations that toggle armbian.sources can fail under errexit if the files
are missing; wrap both moves with existence checks (e.g., test -e before mv) or
use safe mv semantics and ensure the disable-back step always runs (place it in
a cleanup/finally path or trap) so package installation (do_with_retries /
chroot_sdcard_apt_get_install) cannot cause the script to exit with the armbian
repo left enabled; update the block that references
"${SDCARD}"/etc/apt/sources.list.d/armbian.sources and armbian.sources.disabled
accordingly.
|
hmm im wondering if maybe theres a better way for us to manage the grub images? any thoughts? |
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/boards/ayn-odin2-portal.csc`:
- Around line 50-56: The install commands in the diff use mode 655 for
executable scripts (e.g. setup-usbgadget-network.sh,
remove-usbgadget-network.sh, usb-gadget-initramfs-hook,
usb-gadget-initramfs-premount, dropbear, kill-dropbear) which prevents the owner
from executing them; change those install -Dm655 invocations to install -Dm755
so owner/group/others have the expected executable bit set for these scripts and
initramfs hooks.
- Around line 33-40: The download/unpack block around cd
"${SDCARD}/usr/share/alsa", curl -L -o temp.zip, unzip -o temp.zip and the
unzip_dir logic must propagate failures to the main script and handle bad
downloads: remove or change the subshell so a failing cd actually aborts the
script (or explicitly check cd and call exit), add curl -f (and check its exit
status) when fetching temp.zip, verify unzip succeeded (check its exit code and
that unzip_dir is non-empty) before copying, and ensure cleanup and a non-zero
exit from the main script on any of these failures so errors aren’t swallowed.
🧹 Nitpick comments (2)
config/boards/ayn-odin2-portal.csc (2)
69-82: Inconsistentdisplay_alertcalls and opportunity to consolidate Mesa installation.
Lines 70, 74, 80, 88 have inconsistent
display_alertarguments—some are missing the level parameter (third argument).The Mesa driver installation is duplicated between Ubuntu (lines 73-75) and Trixie (lines 79-81) blocks.
Proposed consolidation
- if [[ "${RELEASE}" == "jammy" ]] || [[ "${RELEASE}" == "noble" ]] || [[ "${RELEASE}" == "plucky" ]]; then - display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "warn" + if [[ "${RELEASE}" == "jammy" || "${RELEASE}" == "noble" || "${RELEASE}" == "plucky" ]]; then + display_alert "Adding Mesa PPA for ${BOARD}" "${RELEASE}" "info" do_with_retries 3 chroot_sdcard add-apt-repository ppa:kisak/kisak-mesa --yes - - do_with_retries 3 chroot_sdcard_apt_get_update - display_alert "Installing Mesa Vulkan Drivers" - do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools fi - if [[ "${RELEASE}" == "trixie" ]]; then - do_with_retries 3 chroot_sdcard_apt_get_update - display_alert "Installing Mesa Vulkan Drivers" - do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools - fi + do_with_retries 3 chroot_sdcard_apt_get_update + display_alert "Installing Mesa Vulkan Drivers" "${RELEASE}" "info" + do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools
85-91: Add existence check before renaming Armbian sources file.If
armbian.sources.disableddoesn't exist, themvcommand will fail. Consider adding a guard:Proposed fix
# We need unudhcpd from armbian repo, so enable it - mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources + [[ -f "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled ]] && \ + mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources do_with_retries 3 chroot_sdcard_apt_get_update display_alert "Installing ${BOARD} tweaks" "warn" do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg # disable armbian repo back - mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled + [[ -f "${SDCARD}"/etc/apt/sources.list.d/armbian.sources ]] && \ + mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled
| ( | ||
| cd "${SDCARD}/usr/share/alsa" || exit 6 | ||
| curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" | ||
| unzip -o temp.zip | ||
| unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1) | ||
| cp -rf "${unzip_dir}/"* . | ||
| rm -rf "$unzip_dir" temp.zip | ||
| ) |
There was a problem hiding this comment.
Add error handling and consider download reliability.
Several potential issues in this download block:
exit 6only exits the subshell—the main script continues even ifcdfailscurlwithout-fflag won't fail on HTTP 404 errors- No error handling for
unzipfailures
Consider adding -f to curl and propagating errors:
Proposed improvement
(
- cd "${SDCARD}/usr/share/alsa" || exit 6
- curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip"
- unzip -o temp.zip
+ cd "${SDCARD}/usr/share/alsa" || exit_with_error "Failed to cd to alsa directory"
+ curl -fL -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" || exit_with_error "Failed to download alsa-ucm-conf"
+ unzip -o temp.zip || exit_with_error "Failed to unzip alsa-ucm-conf"
unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1)
cp -rf "${unzip_dir}/"* .
rm -rf "$unzip_dir" temp.zip
- )
+ ) || true # or handle error appropriately🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal.csc` around lines 33 - 40, The download/unpack
block around cd "${SDCARD}/usr/share/alsa", curl -L -o temp.zip, unzip -o
temp.zip and the unzip_dir logic must propagate failures to the main script and
handle bad downloads: remove or change the subshell so a failing cd actually
aborts the script (or explicitly check cd and call exit), add curl -f (and check
its exit status) when fetching temp.zip, verify unzip succeeded (check its exit
code and that unzip_dir is non-empty) before copying, and ensure cleanup and a
non-zero exit from the main script on any of these failures so errors aren’t
swallowed.
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ |
There was a problem hiding this comment.
Incorrect file permissions: 655 should likely be 755.
The permission mode 655 (rw-r-xr-x) is unusual and likely a typo for 755. With 655, the owner cannot execute the file while group/others can—this is almost never the intended behavior for scripts.
Proposed fix
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/
install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/📝 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.
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ |
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal.csc` around lines 50 - 56, The install
commands in the diff use mode 655 for executable scripts (e.g.
setup-usbgadget-network.sh, remove-usbgadget-network.sh,
usb-gadget-initramfs-hook, usb-gadget-initramfs-premount, dropbear,
kill-dropbear) which prevents the owner from executing them; change those
install -Dm655 invocations to install -Dm755 so owner/group/others have the
expected executable bit set for these scripts and initramfs hooks.
…A dependency on existing ayn-odin2.csc Co-authored-by: Squishy123 <christian.gnaw@gmail.com>
…s current and 6.12 as old
…o download helper script folder from github
https://github.com/ROCKNIX/distribution From ROCKNIX/devices/SM8550/linux/linux.aarch64.conf From ROCKNIX/devices/SM8550/patches/linux/ - 0001-msm-adreno-enable-A32.patch - 0002-qcom-dispcc-sm8550-Fix-disp_cc_mdss_mdp_clk_src.patch - 0003_drm-msm-dpu-Set-vsync-source-irrespective-of-mdp-.patch - 0030-input-rmi4-add-reset-gpio.patch - 0031_input--Add-driver-for-RSInput-Gamepad.patch - 0033_leds--Add-driver-for-HEROIC-HTR3212.patch - 0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch - 0042_mmc--sdhci-msm--Toggle-the-FIFO-write-clock-after-.patch - 0047_ASoC--codecs--aw88166--AYN-Odin2-Specific-modifica.patch - 0050_pmk8550-pwm.patch - 0051-gpu-panel-add-Pocket-ACE-panel-driver.patch - 0052-gpu-panel-add-Pocket-DMG-panel-driver.patch - 0053-add-hynitron-touchscreen.patch - 0053-edt-ft5x06-add-no_regmap_bulk_read-option.patch - 0053-gpu-panel-add-Pocket-DS-lower-panel-driver.patch - 0054_sn3112-pwm-driver.patch - 0055_Synaptics-TD4328-LCD-panel.patch - 0056_Xm-Plus-XM91080G-panel.patch - 0057_Chipone-ICNA35XX-panel.patch - 0057_DDIC-CH13726A-panel.patch - 0058_AYN-Odin2-Mini--backlight.patch - 0059_AYN-Odin2-Mini--hynitron--cstxxx.patch - 0060-Add-Silergy-SY7758-backlight-driver.patch - 0061-regulator-add-sgm3804-i2c-regulator-for-panel-power-.patch - 0062_rsinput--regulator.patch - 0070-drm-msm-remove-DRIVER_SYNCOBJ_TIMELINE.patch - 0071-HACK-fix-usb-boot-hang.patch - 0100-SM8550-Fix-L2-cache-for-CPU2-and-add-cache-sizes.patch - 0101-SM8550-Add-DDR-LLCC-L3-CPU-bandwidth-scaling.patch - 0102-20240424_wuxilin123_ayn_odin_2_support.patch - 0103_arm64--dts--qcom--sm8550--add-UART15.patch - 0104-drm-panel-Add-Retroid-Pocket-6-panel.patch - 0120-20250728_konradybcio_gpu_cc_power_requirements_reality_check.patch - 0122-interconnect__qcom__sm8550__Enable_QoS_configuration.patch - 0154-dts-qcom-sm8550-add-opp-acd-level.patch - 0200_arm64--dts--qcom--Add-AYN-QCS8550-Common.patch - 0201_arm64--dts--qcom--Add-AYN-Odin2.patch - 0202_arm64--dts--qcom--Add-AYN-Odin2-Mini.patch - 0203_arm64--dts--qcom--Add-AYN-Odin2-Portal.patch - 0204_arm64--dts--qcom--Add-AYN-Thor.patch - 0210_arm64--dts--qcom--Add-AYANEO-Pocket-Common.patch - 0211_arm64--dts--qcom--Add-AYANEO-Pocket-ACE.patch - 0212_arm64--dts--qcom--Add-AYANEO-Pocket-DMG.patch - 0213_arm64--dts--qcom--Add-AYANEO-Pocket-EVO.patch - 0214_arm64--dts--qcom--Add-AYANEO-Pocket-DS.patch - 0500-ROCKNIX-set-boot-fanspeed.patch - 0501-ROCKNIX-fix-wifi-and-bt-mac.patch - 0503-ROCKNIX-battery-name.patch - v5_20251120_quic_utiwari_crypto_qce_add_runtime_pm_and_interconnect_bandwidth_scaling_support.patch
- Battery name patch - SDHCI Driver patch Added patch to allow aarch64 to do 32bit syscalls (this is helpful for emulation inputs)
…tal-grub, ayn-odin2 configs
Squishy123
force-pushed
the
added-config-improvements-and-fixes-to-ayn-odin2
branch
from
e860cbd to
5168111
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
patch/kernel/archive/sm8550-6.13/0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch (1)
35-55:⚠️ Potential issue | 🟠 MajorHandle clock and DAI-format failures in startup.
The return codes from
clk_prepare_enable()(line 46), bothsnd_soc_dai_set_fmt()calls (lines 47–48), andqcom_snd_sdw_startup()are ignored. This creates multiple failure paths with no error recovery:
- Clock enable can fail (provider not ready, -EPROBE_DEFER, etc.) but is never checked; if a subsequent operation fails, the clock is left enabled (resource leak).
- Format configuration can fail with -EINVAL or other errors, silently continuing with misconfigured DAI.
qcom_snd_sdw_startup()failure leaves the clock enabled with no cleanup path.Propagate all errors and unwind on failure, including cleanup of the enabled clock on error paths.
Suggested fix
static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) { unsigned int fmt = SND_SOC_DAIFMT_BP_FP; unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; struct snd_soc_pcm_runtime *rtd = substream->private_data; struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); + int ret; + bool i2s_clk_enabled = false; switch (cpu_dai->id) { case PRIMARY_MI2S_RX: - clk_prepare_enable(pdata->i2s_clk); - snd_soc_dai_set_fmt(cpu_dai, fmt); - snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); + ret = clk_prepare_enable(pdata->i2s_clk); + if (ret) + return ret; + i2s_clk_enabled = true; + + ret = snd_soc_dai_set_fmt(cpu_dai, fmt); + if (ret) + goto err_clk; + + ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); + if (ret) + goto err_clk; break; default: break; } - return qcom_snd_sdw_startup(substream); + ret = qcom_snd_sdw_startup(substream); + if (ret) + goto err_clk; + return 0; + +err_clk: + if (i2s_clk_enabled) + clk_disable_unprepare(pdata->i2s_clk); + return ret; }patch/kernel/archive/sm8550-6.13/0032_drm-panel--Add-panel-driver-for-Chipone-ICNA3512-b.patch (3)
111-131:⚠️ Potential issue | 🟠 MajorGuard against NULL connector before dereference.
pinfo->connectorcan be unset beforeget_modes()runs, which would cause a NULL deref here.🐛 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 || + !connector->state->crtc->state) return 0; crtc_state = connector->state->crtc->state;
263-300:⚠️ Potential issue | 🟠 MajorDisable regulators on all prepare failure paths.
If PPS/compression setup fails, regulators stay enabled, leaving the panel powered despite a failed prepare.🔧 Proposed fix
ret = pinfo->desc->init_sequence(pinfo); if (ret < 0) { - regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); dev_err(panel->dev, "failed to initialize panel: %d\n", ret); - return ret; + goto err_disable; } 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; + goto err_disable; } /* 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; + goto err_disable; } return 0; + +err_disable: + regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); + return ret;
392-424:⚠️ Potential issue | 🟡 MinorRestore LPM mode_flags on error paths.
If the DCS call fails,mode_flagsstays in non‑LPM, which can affect power behavior.🧯 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 mode_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 = mode_flags; + return ret; } static int icna3512_bl_get_brightness(struct backlight_device *bl) { struct mipi_dsi_device *dsi = bl_get_data(bl); u16 brightness; + unsigned long mode_flags = dsi->mode_flags; int ret; dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness); - if (ret < 0) - return ret; - - dsi->mode_flags |= MIPI_DSI_MODE_LPM; + dsi->mode_flags = mode_flags; + if (ret < 0) + return ret; return brightness; }patch/kernel/archive/sm8550-6.13/0031_input--Add-driver-for-RSInput-Gamepad.patch (5)
22-24:⚠️ Potential issue | 🟡 MinorPolish Kconfig prompt grammar.
Line 23’s prompt string is awkward; please rephrase for clarity.
✏️ Suggested wording
config JOYSTICK_RSINPUT - tristate "UART Based gamepad driver that found in AYN and Retroid Pocket products" + tristate "UART-based gamepad driver found in AYN and Retroid Pocket products" depends on SERIAL_DEV_BUS
101-109:⚠️ Potential issue | 🟠 MajorMake
prev_statesper-device (avoid static cross-talk).Line 211 uses a
staticbitmask shared across all instances; multiple gamepads will interfere with each other’s key-change detection. Store it instruct rsinput_driverinstead.✅ Per-device state
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_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; int i; - 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
333-369:⚠️ Potential issue | 🔴 CriticalKernel crash: ERR_PTR not cleared before passing to gpiod_set_value_cansleep().
Lines 333–352 log errors from
devm_gpiod_get_optional()withdev_warn()but do not clear the ERR_PTR. Lines 354–369 then checkif (drv->boot_gpio)and similar conditions, which evaluate true for ERR_PTR (non-zero pointer), causing a crash when passed togpiod_set_value_cansleep(). This function is not ERR_PTR-safe and will dereference the invalid pointer.Fix by returning an error via
dev_err_probe()(which properly handles-EPROBE_DEFER) or by nulling the pointer in the error path before later use.🛡️ Safer error handling
- 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->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)) { - error = PTR_ERR(drv->enable_gpio); - dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); - } + 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)) { - error = PTR_ERR(drv->reset_gpio); - dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); - } + 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");
281-311:⚠️ Potential issue | 🟠 MajorHandle partial frames by buffering across
receive_bufcallbacks.The
rsinput_rxfunction overwritesdrv->rx_bufon each callback withmemcpy(drv->rx_buf, data, count)and does not accumulate incomplete frames. When serdev delivers data in chunks smaller than a complete frame (MCU_PKT_SIZE_MIN = 9 bytes), partial frames are lost:
- Callback receives incomplete frame →
rsinput_process_datadetects incomplete frame (line 246) and returns- Next callback overwrites buffer → previous partial frame data is lost
- Trailing bytes from
rsinput_process_data(line 277) are warned about but discardedAdd an
rx_lenfield to track buffered data and preserve trailing bytes across callbacks. Buffer incoming data and only parse when a complete frame is available.
370-438:⚠️ Potential issue | 🟠 MajorUnwind serdev device on probe errors.
Lines 370–427: After
serdev_device_open()succeeds, failures at lines 376 (baudrate), 409 (input allocation), and 413 (input registration) return without closing the UART device. Redirect these paths to a common error label that closes serdev before returning.Note: The input device is allocated with
devm_input_allocate_device(), so devres automatically handles unregistration and cleanup on probe failure. Theinput_unregister_device()call inrsinput_remove()is redundant and can be removed.🔧 Suggested error-unwind pattern
error = serdev_device_set_baudrate(serdev, 115200); - if (error < 0) - return dev_err_probe(&serdev->dev, error, - "Failed to set up host baud rate\n"); + if (error < 0) { + error = dev_err_probe(&serdev->dev, error, + "Failed to set up host baud rate\n"); + goto err_close; + } serdev_device_set_flow_control(serdev, false); drv->input = devm_input_allocate_device(&serdev->dev); - if (!drv->input) - return -ENOMEM; + if (!drv->input) { + error = -ENOMEM; + goto err_close; + } @@ - error = input_register_device(drv->input); - if (error) - return error; + error = input_register_device(drv->input); + if (error) + goto err_close; @@ error = rsinput_init_commands(drv); - if (error < 0) { - serdev_device_close(serdev); - return error; - } + if (error < 0) + goto err_close; return 0; +err_close: + serdev_device_close(serdev); + return error; @@ static void rsinput_remove(struct serdev_device *serdev) { struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); serdev_device_close(serdev); - input_unregister_device(drv->input); if (drv->enable_gpio) gpiod_set_value_cansleep(drv->enable_gpio, 0);
🤖 Fix all issues with AI agents
In `@config/boards/ayn-odin2-grub.csc`:
- Around line 55-61: The install lines set executable scripts with mode 655
(owner missing execute) which is incorrect; update the permission mode from 655
to 755 for all executable installs (the lines installing
setup-usbgadget-network.sh, remove-usbgadget-network.sh,
usb-gadget-initramfs-hook, usb-gadget-initramfs-premount, dropbear, and
kill-dropbear) while leaving the usbgadget-rndis.service install at 644; ensure
each install invocation for those script filenames uses 755 so the owner can
execute them.
In `@config/boards/ayn-odin2-portal.csc`:
- Around line 84-92: The mv calls that toggle armbian.sources (using
"${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled and armbian.sources)
can fail under errexit if the source/target file doesn't exist; update the block
around the chroot_sdcard_apt_get_update/install calls to guard the moves by
checking existence (e.g., test -e on
"${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled before the first mv
and test -e on armbian.sources before the second mv) or make the moves no-op on
missing files (e.g., conditional mv or append || true) so
chroot_sdcard_apt_get_update and do_with_retries calls are not aborted.
In `@extensions/odin2-preset-firstrun.sh`:
- Around line 60-74: The script currently clones the repo twice (chroot_sdcard
git clone ... to /usr/local/odin2-scripts and the runtime installer written to
${launcher_file} which clones to ~/sys/odin2-scripts) causing redundancy; decide
one of two fixes: (A) If scripts must be preinstalled, remove the git clone
inside the installer and change the installer contents to reference
/usr/local/odin2-scripts (update references to ${launcher_dir}/${launcher_file}
or install-odin2-scripts accordingly), or (B) If scripts must be installed on
first run, remove the build-time chroot_sdcard git clone line and keep the
installer as the sole cloner; also remove or change the final cd
~/sys/odin2-scripts in the installer since it doesn’t affect the calling
shell—either document that the installer must be sourced to change the caller’s
directory or replace it with an explicit command that runs the intended setup
actions from that directory.
- Around line 41-48: The script currently writes weak static passwords into
.not_logged_in_yet (PRESET_ROOT_PASSWORD, PRESET_USER_PASSWORD,
PRESET_USER_NAME); update the preset logic so passwords are not hardcoded to
"1234" — either write empty values (e.g. PRESET_ROOT_PASSWORD= and
PRESET_USER_PASSWORD=) to force creation at first login, or generate and store
strong random passwords if the first-run flow supports it; ensure the entries
still use the same keys (PRESET_ROOT_PASSWORD, PRESET_USER_PASSWORD,
PRESET_USER_NAME) so downstream first-run code can read them.
In `@patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml`:
- Line 1: The file-level comment at the top incorrectly references sm8250-6.7;
update the comment string in the header (the line starting with "config: # This
is file") to reference the correct path "sm8550-6.13/0000.patching_config.yaml"
so the inline comment matches the actual file location.
In
`@patch/kernel/archive/sm8550-6.13/0003_media--iris--implement-iris-v4l2-file-ops.patch`:
- Around line 331-354: The vb2 queues in iris_m2m_queue_init are missing
required fields before calling vb2_queue_init: set src_vq->ops and dst_vq->ops
to the instance's vb2 ops (inst->core->iris_vb2_ops), set src_vq->mem_ops and
dst_vq->mem_ops to &vb2_dma_contig_memops, and set src_vq->buf_struct_size and
dst_vq->buf_struct_size to sizeof(struct iris_buffer); ensure these assignments
occur for both src_vq and dst_vq (alongside the existing
type/io_modes/timestamp_flags/drv_priv/dev/lock) before calling vb2_queue_init
on each queue.
In
`@patch/kernel/archive/sm8550-6.13/0005_media--iris--implement-video-firmware-load-unload.patch`:
- Around line 147-149: The memremap failure path in iris_load_fw_to_memory does
a goto err_release_fw without setting ret, so a previous success leaves ret==0
and a failure is reported as success; update the memremap error handling (the
block that calls memremap(mem_phys, res_size, MEMREMAP_WC) and tests mem_virt)
to set ret to an appropriate negative error code (e.g., -ENOMEM or -EIO) before
jumping to err_release_fw, and ensure iris_load_fw_to_memory returns that error
instead of 0.
In
`@patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch`:
- Around line 677-690: The function iris_enable_power_domains incorrectly
returns the positive usage count from pm_runtime_get_sync (in
iris_enable_power_domains), causing callers to treat success as error; change
the logic so after calling pm_runtime_get_sync(pd_dev) you check for ret < 0 and
return that error, but on non-negative success return 0 (i.e., normalize any
non-error pm_runtime_get_sync result to 0) while preserving the existing
dev_pm_opp_set_rate(core->dev, ULONG_MAX) error handling.
In
`@patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch`:
- Around line 1462-1477: The helper iris_add_session currently silently skips
adding when core->iris_platform_data->max_session_count is reached; change its
signature to return an int (e.g., 0 on success, -EUSERS or -ENOSPC when full),
keep the mutex_lock/mutex_unlock and count logic but return an error instead of
dropping silently, and update the caller iris_open to check the return value and
propagate the error to the caller (cleaning up any partially initialized state
if needed) so the caller knows the session wasn't tracked; reference
functions/fields: iris_add_session, iris_open,
core->iris_platform_data->max_session_count, list_add_tail, core->instances,
mutex_lock/mutex_unlock.
In
`@patch/kernel/archive/sm8550-6.13/0015_media--iris--implement-query_cap-ioctl.patch`:
- Around line 29-35: The current iris_querycap implementation only fills driver
and card, leaving cap->capabilities and cap->device_caps zero; update
iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) to set
cap->capabilities to include V4L2_CAP_DEVICE_CAPS (and any other global caps you
support), populate cap->device_caps with the appropriate device-level flags for
a video M2M decoder (e.g., V4L2_CAP_VIDEO_M2M_MPLANE and V4L2_CAP_STREAMING as
applicable), and fill cap->bus_info with a unique bus identifier (for example
using strscpy from the underlying device name or pci/dev identifier). Ensure you
reference IRIS_DRV_NAME and v4l2_capability fields when making these changes so
userspace can discover the device capabilities.
In
`@patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch`:
- Around line 268-286: The loop in iris_set_properties currently ignores the
return value of each per-capability setter, so failures from cap->set() are
swallowed; update iris_set_properties to capture the int ret = cap->set(inst, i)
for each valid capability (after checking iris_valid_cap_id and cap->cap_id) and
if ret is non-zero immediately return that error (propagating failures), while
keeping the existing early return for hfi_ops->session_set_config_params;
reference functions/fields: iris_set_properties,
hfi_ops->session_set_config_params, iris_valid_cap_id, inst->fw_caps, and
cap->set.
In
`@patch/kernel/archive/sm8550-6.13/0018_media--iris--subscribe-parameters-and-properties-t.patch`:
- Around line 87-227: The functions iris_hfi_gen2_subscribe_change_param and
iris_hfi_gen2_subscribe_property write into a fixed payload[32] without bounds
checks and continue on unsupported properties; add explicit checks that
change_param_size + 1 and subscribe_prop_size + 1 do not exceed
ARRAY_SIZE(payload) and return -EINVAL immediately if they do, and when the
default case in the change_param switch is hit return the error immediately (do
not continue the loop or set
inst_hfi_gen2->opsc_properties_set/ipsc_properties_set), and ensure the payload
pointer passed to
iris_hfi_gen2_session_set_property/iris_hfi_gen2_session_subscribe_mode uses the
payload buffer correctly (e.g., &payload[0]) after the bounds check.
In
`@patch/kernel/archive/sm8550-6.13/0021_media--iris--add-support-for-dynamic-resolution-ch.patch`:
- Around line 61-62: The switch in iris_hfi_get_v4l2_color_primaries has the
DCI-P3 branch reversed (it currently matches V4L2_COLORSPACE_DCI_P3 and returns
HFI_PRIMARIES_SMPTE_RP431_2); change that branch to match the HFI constant and
return the V4L2 constant instead—i.e., use case HFI_PRIMARIES_SMPTE_RP431_2:
return V4L2_COLORSPACE_DCI_P3; so the function consistently maps HFI primaries
-> V4L2 values.
In
`@patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch`:
- Around line 103-134: The stack-local flush_pkt in
iris_hfi_gen1_session_ftb_done is only partially populated and may contain
uninitialized garbage; zero-initialize flush_pkt before setting its
header/session/flush_type (e.g., use memset or an aggregate initializer) so all
fields are deterministic, then keep the existing assignments to
flush_pkt.shdr.hdr.size, pkt_type, shdr.session_id, and flush_type and call
iris_hfi_queue_cmd_write, reinit_completion(&inst->flush_completion) and
iris_inst_sub_state_change_drain_last as present.
🧹 Nitpick comments (16)
patch/kernel/archive/sm8550-6.13/0020_media--iris--implement-vb2-ops-for-buf_queue-and-f.patch (2)
489-507: Minor: Dead code check onbufferspointer.The check
if (!buffers)at line 491 (patch context) is dead code sincebuffersis assigned from&inst->buffers[BUF_DPB], which is a fixed array member ofstruct iris_instand can never be NULL.However, this is upstream code that has been reviewed and merged, and the check is defensive/harmless. No action required unless you want to submit a cleanup patch upstream.
728-757: Minor: Unusedretvariable iniris_hfi_gen2_handle_session_info.The variable
retis initialized to 0 and returned unchanged. This could be simplified, but since this is upstream code, it may be intentional for future extension or consistency with other handlers.int ret = 0; // ... switch statement that doesn't modify ret ... return ret;No action needed - upstream reviewers accepted this pattern.
patch/kernel/archive/sm8550-6.13/0039_phy--qcom--qmp-combo--introduce-QPHY_MODE.patch (1)
50-75: Add a defensive default for unexpected init_mode values.If
init_modeever ends up with an invalid value, the switch will skip all resets and mode programming silently. A fallback to COMBO (optionally with a warn) keeps behavior safe.Proposed tweak
- switch (qmp->init_mode) { - case QPHY_MODE_COMBO: + switch (qmp->init_mode) { + case QPHY_MODE_COMBO: + default: + if (qmp->init_mode != QPHY_MODE_COMBO) + WARN_ONCE(1, "qmp-combo: unknown init_mode %d, defaulting to COMBO\n", + qmp->init_mode); writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL); /* bring both QMP USB and QMP DP PHYs PCS block out of reset */ qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL, SW_DPPHY_RESET_MUX | SW_DPPHY_RESET | SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); break;patch/kernel/archive/sm8550-6.13/0021_media--iris--add-support-for-dynamic-resolution-ch.patch (2)
304-369: Consider validating data pointer bounds during property parsing.The loop iterates based on
num_properties_changedfrom the firmware packet, advancingdata_ptrthrough the packet data. If the firmware sends malformed data wherenum_properties_changedexceeds the actual properties present, or if individual property sizes are incorrect, this could potentially read beyond the packet buffer.Given this is upstream kernel code that has been reviewed and the firmware is trusted, this may be acceptable. However, adding bounds checking (e.g., tracking remaining bytes and validating before each read) would make this more robust against firmware bugs.
622-627: Potential unsigned underflow in crop dimension calculation.The crop width/height calculations subtract offsets from dimensions without validation. If firmware provides crop offsets larger than the image dimensions (e.g., due to a firmware bug), the subtraction would underflow since these are unsigned values, resulting in very large crop dimensions.
Since this is upstream kernel code with trusted firmware interaction, this may be acceptable. The code at lines 633-638 does check for unsupported content and transitions to error state, which provides some protection.
patch/kernel/archive/sm8550-6.13/0024_media--iris--add-check-whether-the-video-session-i.patch (1)
108-142: Locking inconsistency in instance list iteration.The function iterates
core->instanceswithout holdingcore->lock(line 115-118), whileiris_check_core_mbpf()acquires the lock before iterating the same list. This creates a potential race window if instances are added/removed concurrently.For consistency and safety, consider holding
core->lockduring the instance verification check iniris_check_session_supported(), matching the locking discipline used iniris_check_core_mbpf().Note: This is upstream kernel code that has passed multiple rounds of review and been tested on multiple hardware platforms. The inconsistency may be intentional if callers are expected to hold the lock, but harmonizing the locking would improve safety.
extensions/odin2-preset-firstrun.sh (1)
57-65: Variablelauncher_diris shadowed with a different value.
launcher_diris assigned on line 57 (${SDCARD}/usr/local) and then reassigned on line 63 (${SDCARD}/usr/local/bin). While this works, it reduces readability. Use distinct variable names for clarity.Proposed fix
- local launcher_dir="${SDCARD}/usr/local" - run_host_command_logged mkdir -pv "${launcher_dir}" + local scripts_dir="${SDCARD}/usr/local" + run_host_command_logged mkdir -pv "${scripts_dir}" - chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git "$launcher_dir/odin2-scripts" + chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git "${scripts_dir}/odin2-scripts" local launcher_dir="${SDCARD}/usr/local/bin"patch/kernel/archive/sm8550-6.13/0025_media--iris--implement-power-scaling-for-vpu2-and-.patch (1)
258-270: Hardcoded DEFAULT_FPS (30) for bandwidth voting.The function uses
DEFAULT_FPSinstead of the actual frame rate, which may not accurately reflect real bandwidth requirements for 60fps or variable frame rate content. This appears to be an intentional simplification in the initial implementation.config/desktop/trixie/environments/kde-plasma-mobile/config_base/packages (1)
12-20: For full PipeWire audio on KDE Plasma Mobile, addpipewire-pulseandwireplumber.Adding
pipewirealone is incomplete for Debian Trixie. KDE Plasma's audio controls use the PulseAudio API, so you needpipewire-pulse(the PulseAudio-compatible server) not justpipewire, pluswireplumberas the session manager. Thepulseaudiopackages won't cause hard conflicts withpipewirein Trixie, but only one sound daemon should be active per session—ensurepipewire-pulseis configured to start instead ofpulseaudio. Also verify thatplasma-paremains installed so the audio applet doesn't disappear.If this is only for camera/screencast features, then
pipewirealone is acceptable; please confirm the intended audio stack.packages/bsp/ayn-odin2portal/zz-update-abl-kernel (1)
4-11: Glob patterns may match multiple files unexpectedly.The glob patterns (
vmlinuz-*-sm8550-arm64,linux-image-*-sm8550-arm64,initrd.img-*-sm8550-arm64) could match multiple files if more than one kernel version is installed, leading to incorrect boot image generation:
- Line 4:
gzip -c /boot/vmlinuz-*would concatenate multiple kernels- Line 11:
--ramdisk /boot/initrd.img-*would fail or use wrong fileAlso, Line 8 sources
/boot/armbianEnv.txtbut no variables from it appear to be used in this script.Consider using more specific matching or validating single matches:
♻️ Suggested improvement
#!/bin/bash set -ex new_rootfs_image_uuid=$(sed -e 's/^.*root=UUID=//' -e 's/ .*$//' < /proc/cmdline) -gzip -c /boot/vmlinuz-*-sm8550-arm64 > /tmp/Image.gz + +# Find the latest installed kernel +kernel_img=$(ls -1t /boot/vmlinuz-*-sm8550-arm64 2>/dev/null | head -1) +[[ -z "$kernel_img" ]] && { echo "No kernel found"; exit 1; } +kernel_version="${kernel_img#/boot/vmlinuz-}" + +gzip -c "$kernel_img" > /tmp/Image.gz -cat /tmp/Image.gz /usr/lib/linux-image-*-sm8550-arm64/qcom/qcs8550-ayn-odin2-hypdtbo.dtb > /tmp/Image.gz-dtb +cat /tmp/Image.gz "/usr/lib/linux-image-${kernel_version}/qcom/qcs8550-ayn-odin2-hypdtbo.dtb" > /tmp/Image.gz-dtb -source /boot/armbianEnv.txt /usr/bin/mkbootimg \ --kernel /tmp/Image.gz-dtb \ - --ramdisk /boot/initrd.img-*-sm8550-arm64 \ + --ramdisk "/boot/initrd.img-${kernel_version}" \config/boards/ayn-odin2-portal-grub.csc (1)
1-1: Align the header comment with the standard single-line hardware spec format.Consider updating the header to the usual one-line hardware summary (SoC, RAM options, key features) to match other board configs.
Based on learnings: In Armbian board configuration files, the standard pattern is to have only one line as a comment describing the board hardware specifications.
patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml (1)
3-8: Remove unused metadata fields.These metadata fields (
name,kind,type,branch,last-known-good-tag) are not used by the patching scripts and should be removed entirely. Based on learnings from this repository, these fields serve no functional purpose.Proposed fix
config: # This is file 'patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml' - # Just some info stuff; not used by the patching scripts - name: sm8550-6.13 - kind: kernel - type: mainline # or: vendor - branch: linux-6.13.y - last-known-good-tag: v6.13.11 - # .dts files in these directories will be copied as-is to the build tree; later ones overwrite earlier ones.patch/kernel/archive/sm8550-6.13/0005_media--iris--implement-video-firmware-load-unload.patch (1)
182-185: Error code override loses specific failure reason.When
iris_load_fw_to_memoryfails, the specific error code is discarded and replaced with-ENOMEM. Consider propagating the actual error.Proposed fix
ret = iris_load_fw_to_memory(core, fwpath); if (ret) { dev_err(core->dev, "firmware download failed\n"); - return -ENOMEM; + return ret; }patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch (1)
634-653: State-change log prints the new state twice.
dev_dbg()logsinst->stateafter assignment, so “from” and “to” are identical. Capture the previous state before overwriting.🧭 Suggested fix
-change_state: - inst->state = request_state; - dev_dbg(inst->core->dev, "state changed from %x to %x\n", - inst->state, request_state); +change_state: + { + enum iris_inst_state prev_state = inst->state; + inst->state = request_state; + dev_dbg(inst->core->dev, "state changed from %x to %x\n", + prev_state, request_state); + }patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch (1)
503-507: Consider adding a named constant for the magic register offset.The register offset
0xB0088is a magic number. A named constant would improve readability and maintainability.patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch (1)
713-715: Consider using a named constant for packet allocation size.The magic number
4096for packet allocation could be replaced with a named constant for clarity.
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ |
There was a problem hiding this comment.
Incorrect file permissions: 655 should likely be 755.
The permission mode 655 (rw-r-xr-x) is unusual and likely a typo for 755. With 655, the owner cannot execute the file while group/others can.
🛠️ Proposed fix
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/
install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/
+ install -Dm755 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/📝 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.
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | |
| install -Dm755 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ |
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-grub.csc` around lines 55 - 61, The install lines set
executable scripts with mode 655 (owner missing execute) which is incorrect;
update the permission mode from 655 to 755 for all executable installs (the
lines installing setup-usbgadget-network.sh, remove-usbgadget-network.sh,
usb-gadget-initramfs-hook, usb-gadget-initramfs-premount, dropbear, and
kill-dropbear) while leaving the usbgadget-rndis.service install at 644; ensure
each install invocation for those script filenames uses 755 so the owner can
execute them.
| # We need unudhcpd from armbian repo, so enable it | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources | ||
|
|
||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| display_alert "Installing ${BOARD} tweaks" "warn" | ||
| do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg | ||
| # disable armbian repo back | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled | ||
| do_with_retries 3 chroot_sdcard_apt_get_update |
There was a problem hiding this comment.
Guard Armbian sources toggling to prevent failure.
Same issue as other board configs - the unconditional mv commands can fail under errexit if armbian.sources.disabled doesn't exist. Consider adding existence checks.
🛠️ Suggested fix
# We need unudhcpd from armbian repo, so enable it
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources
+ local armbian_sources_was_disabled=""
+ if [[ -f "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled" "${SDCARD}/etc/apt/sources.list.d/armbian.sources"
+ armbian_sources_was_disabled="yes"
+ fi
do_with_retries 3 chroot_sdcard_apt_get_update
display_alert "Installing ${BOARD} tweaks" "warn"
do_with_retries 3 chroot_sdcard_apt_get_install alsa-ucm-conf qbootctl qrtr-tools unudhcpd mkbootimg
# disable armbian repo back
- mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled
+ if [[ "${armbian_sources_was_disabled}" == "yes" ]]; then
+ mv "${SDCARD}/etc/apt/sources.list.d/armbian.sources" "${SDCARD}/etc/apt/sources.list.d/armbian.sources.disabled"
+ fi🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal.csc` around lines 84 - 92, The mv calls that
toggle armbian.sources (using
"${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled and armbian.sources)
can fail under errexit if the source/target file doesn't exist; update the block
around the chroot_sdcard_apt_get_update/install calls to guard the moves by
checking existence (e.g., test -e on
"${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled before the first mv
and test -e on armbian.sources before the second mv) or make the moves no-op on
missing files (e.g., conditional mv or append || true) so
chroot_sdcard_apt_get_update and do_with_retries calls are not aborted.
| # Preset root password | ||
| echo "PRESET_ROOT_PASSWORD=1234" >> "${SDCARD}"/root/.not_logged_in_yet | ||
|
|
||
| # Preset username | ||
| echo "PRESET_USER_NAME=odin2" >> "${SDCARD}"/root/.not_logged_in_yet | ||
|
|
||
| # Preset user password | ||
| echo "PRESET_USER_PASSWORD=1234" >> "${SDCARD}"/root/.not_logged_in_yet |
There was a problem hiding this comment.
Weak default credentials pose a security risk.
Using 1234 for both root and user passwords, even as defaults meant to be changed at first login, is a security posture gap. Users may skip changing them, leaving devices exposed with known credentials.
Consider using PRESET_ROOT_PASSWORD= and PRESET_USER_PASSWORD= (empty values) to force password creation at first login, or use stronger random defaults if the first-run flow supports it.
🤖 Prompt for AI Agents
In `@extensions/odin2-preset-firstrun.sh` around lines 41 - 48, The script
currently writes weak static passwords into .not_logged_in_yet
(PRESET_ROOT_PASSWORD, PRESET_USER_PASSWORD, PRESET_USER_NAME); update the
preset logic so passwords are not hardcoded to "1234" — either write empty
values (e.g. PRESET_ROOT_PASSWORD= and PRESET_USER_PASSWORD=) to force creation
at first login, or generate and store strong random passwords if the first-run
flow supports it; ensure the entries still use the same keys
(PRESET_ROOT_PASSWORD, PRESET_USER_PASSWORD, PRESET_USER_NAME) so downstream
first-run code can read them.
| chroot_sdcard git clone https://github.com/Squishy123/odin2-scripts.git "$launcher_dir/odin2-scripts" | ||
|
|
||
|
|
||
| local launcher_dir="${SDCARD}/usr/local/bin" | ||
| local launcher_file="${launcher_dir}/install-odin2-scripts" | ||
| run_host_command_logged mkdir -pv "${launcher_dir}" | ||
|
|
||
| cat <<- 'INSTALL_ODIN_2_SCRIPT' > "${launcher_file}" | ||
| #!/usr/bin/env bash | ||
| if [[ ! -d ~/sys/odin2-scripts ]]; then | ||
| mkdir -p ~/sys | ||
| git clone https://github.com/Squishy123/odin2-scripts.git ~/sys/odin2-scripts | ||
| fi | ||
| cd ~/sys/odin2-scripts | ||
| INSTALL_ODIN_2_SCRIPT |
There was a problem hiding this comment.
Scripts are cloned twice to different locations.
Line 60 clones the repository to /usr/local/odin2-scripts during image build, while the installer script (lines 69-72) clones again to ~/sys/odin2-scripts at runtime. This appears redundant—clarify the intended behavior:
- If scripts should be pre-installed: keep only line 60 and update the installer to reference
/usr/local/odin2-scripts. - If scripts should be user-installed on first run: remove line 60 and keep only the installer.
Additionally, line 73 (cd ~/sys/odin2-scripts) has no effect since the script exits immediately after—the calling shell won't change directory. If this is meant to be sourced rather than executed, consider documenting that usage.
🤖 Prompt for AI Agents
In `@extensions/odin2-preset-firstrun.sh` around lines 60 - 74, The script
currently clones the repo twice (chroot_sdcard git clone ... to
/usr/local/odin2-scripts and the runtime installer written to ${launcher_file}
which clones to ~/sys/odin2-scripts) causing redundancy; decide one of two
fixes: (A) If scripts must be preinstalled, remove the git clone inside the
installer and change the installer contents to reference
/usr/local/odin2-scripts (update references to ${launcher_dir}/${launcher_file}
or install-odin2-scripts accordingly), or (B) If scripts must be installed on
first run, remove the build-time chroot_sdcard git clone line and keep the
installer as the sole cloner; also remove or change the final cd
~/sys/odin2-scripts in the installer since it doesn’t affect the calling
shell—either document that the installer must be sourced to change the caller’s
directory or replace it with an explicit command that runs the intended setup
actions from that directory.
| @@ -0,0 +1,27 @@ | |||
| config: # This is file 'patch/kernel/archive/sm8250-6.7/0000.patching_config.yaml' | |||
There was a problem hiding this comment.
Incorrect file path in comment.
The comment references sm8250-6.7 but this file is located in sm8550-6.13. Update to match the actual file path.
-config: # This is file 'patch/kernel/archive/sm8250-6.7/0000.patching_config.yaml'
+config: # This is file 'patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config: # This is file 'patch/kernel/archive/sm8250-6.7/0000.patching_config.yaml' | |
| config: # This is file 'patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml' |
🤖 Prompt for AI Agents
In `@patch/kernel/archive/sm8550-6.13/0000.patching_config.yaml` at line 1, The
file-level comment at the top incorrectly references sm8250-6.7; update the
comment string in the header (the line starting with "config: # This is file")
to reference the correct path "sm8550-6.13/0000.patching_config.yaml" so the
inline comment matches the actual file location.
| +static int iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) | ||
| +{ | ||
| + strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver)); | ||
| + strscpy(cap->card, "Iris Decoder", sizeof(cap->card)); | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
Set device_caps and capabilities in querycap to report proper device capabilities.
The implementation only populates driver and card, leaving capabilities and device_caps zeroed. This prevents userspace from discovering what the device can do (e.g., video encoding/decoding, streaming support). For a video M2M decoder, populate device_caps with the supported modes and set the V4L2_CAP_DEVICE_CAPS flag in capabilities. The bus_info field should also be set for complete device identification.
Suggested fix
static int iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap)
{
strscpy(cap->driver, IRIS_DRV_NAME, sizeof(cap->driver));
strscpy(cap->card, "Iris Decoder", sizeof(cap->card));
+ cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
+ cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}Also applies to: 40-45
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0015_media--iris--implement-query_cap-ioctl.patch`
around lines 29 - 35, The current iris_querycap implementation only fills driver
and card, leaving cap->capabilities and cap->device_caps zero; update
iris_querycap(struct file *filp, void *fh, struct v4l2_capability *cap) to set
cap->capabilities to include V4L2_CAP_DEVICE_CAPS (and any other global caps you
support), populate cap->device_caps with the appropriate device-level flags for
a video M2M decoder (e.g., V4L2_CAP_VIDEO_M2M_MPLANE and V4L2_CAP_STREAMING as
applicable), and fill cap->bus_info with a unique bus identifier (for example
using strscpy from the underlying device name or pci/dev identifier). Ensure you
reference IRIS_DRV_NAME and v4l2_capability fields when making these changes so
userspace can discover the device capabilities.
| +int iris_set_properties(struct iris_inst *inst, u32 plane) | ||
| +{ | ||
| + const struct iris_hfi_command_ops *hfi_ops = inst->core->hfi_ops; | ||
| + struct platform_inst_fw_cap *cap; | ||
| + int ret; | ||
| + u32 i; | ||
| + | ||
| + ret = hfi_ops->session_set_config_params(inst, plane); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + for (i = 1; i < INST_FW_CAP_MAX; i++) { | ||
| + cap = &inst->fw_caps[i]; | ||
| + if (!iris_valid_cap_id(cap->cap_id)) | ||
| + continue; | ||
| + | ||
| + if (cap->cap_id && cap->set) | ||
| + cap->set(inst, i); | ||
| + } |
There was a problem hiding this comment.
Propagate errors from per-capability setters.
iris_set_properties() calls cap->set() but ignores its return value. If a property fails to program into firmware, streamon still proceeds silently. Please propagate failures.
🛠️ Suggested fix
for (i = 1; i < INST_FW_CAP_MAX; i++) {
cap = &inst->fw_caps[i];
if (!iris_valid_cap_id(cap->cap_id))
continue;
- if (cap->cap_id && cap->set)
- cap->set(inst, i);
+ if (cap->cap_id && cap->set) {
+ ret = cap->set(inst, i);
+ if (ret)
+ return ret;
+ }
}🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch`
around lines 268 - 286, The loop in iris_set_properties currently ignores the
return value of each per-capability setter, so failures from cap->set() are
swallowed; update iris_set_properties to capture the int ret = cap->set(inst, i)
for each valid capability (after checking iris_valid_cap_id and cap->cap_id) and
if ret is non-zero immediately return that error (propagating failures), while
keeping the existing early return for hfi_ops->session_set_config_params;
reference functions/fields: iris_set_properties,
hfi_ops->session_set_config_params, iris_valid_cap_id, inst->fw_caps, and
cap->set.
| +static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane) | ||
| +{ | ||
| + struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst); | ||
| + struct hfi_subscription_params subsc_params; | ||
| + u32 prop_type, payload_size, payload_type; | ||
| + struct iris_core *core = inst->core; | ||
| + const u32 *change_param; | ||
| + u32 change_param_size; | ||
| + u32 payload[32] = {0}; | ||
| + u32 hfi_port = 0, i; | ||
| + int ret; | ||
| + | ||
| + if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) || | ||
| + (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) { | ||
| + dev_err(core->dev, "invalid plane\n"); | ||
| + return 0; | ||
| + } | ||
| + | ||
| + change_param = core->iris_platform_data->input_config_params; | ||
| + change_param_size = core->iris_platform_data->input_config_params_size; | ||
| + | ||
| + payload[0] = HFI_MODE_PORT_SETTINGS_CHANGE; | ||
| + | ||
| + for (i = 0; i < change_param_size; i++) | ||
| + payload[i + 1] = change_param[i]; | ||
| + | ||
| + ret = iris_hfi_gen2_session_subscribe_mode(inst, | ||
| + HFI_CMD_SUBSCRIBE_MODE, | ||
| + plane, | ||
| + HFI_PAYLOAD_U32_ARRAY, | ||
| + &payload[0], | ||
| + ((change_param_size + 1) * sizeof(u32))); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | ||
| + inst_hfi_gen2->ipsc_properties_set = true; | ||
| + } else { | ||
| + hfi_port = iris_hfi_gen2_get_port(V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); | ||
| + memcpy(&inst_hfi_gen2->dst_subcr_params, | ||
| + &inst_hfi_gen2->src_subcr_params, | ||
| + sizeof(inst_hfi_gen2->src_subcr_params)); | ||
| + subsc_params = inst_hfi_gen2->dst_subcr_params; | ||
| + for (i = 0; i < change_param_size; i++) { | ||
| + payload[0] = 0; | ||
| + payload[1] = 0; | ||
| + payload_size = 0; | ||
| + payload_type = 0; | ||
| + prop_type = change_param[i]; | ||
| + switch (prop_type) { | ||
| + case HFI_PROP_BITSTREAM_RESOLUTION: | ||
| + payload[0] = subsc_params.bitstream_resolution; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_CROP_OFFSETS: | ||
| + payload[0] = subsc_params.crop_offsets[0]; | ||
| + payload[1] = subsc_params.crop_offsets[1]; | ||
| + payload_size = sizeof(u64); | ||
| + payload_type = HFI_PAYLOAD_64_PACKED; | ||
| + break; | ||
| + case HFI_PROP_CODED_FRAMES: | ||
| + payload[0] = subsc_params.coded_frames; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT: | ||
| + payload[0] = subsc_params.fw_min_count; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_PIC_ORDER_CNT_TYPE: | ||
| + payload[0] = subsc_params.pic_order_cnt; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_SIGNAL_COLOR_INFO: | ||
| + payload[0] = subsc_params.color_info; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_PROFILE: | ||
| + payload[0] = subsc_params.profile; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + case HFI_PROP_LEVEL: | ||
| + payload[0] = subsc_params.level; | ||
| + payload_size = sizeof(u32); | ||
| + payload_type = HFI_PAYLOAD_U32; | ||
| + break; | ||
| + default: | ||
| + prop_type = 0; | ||
| + ret = -EINVAL; | ||
| + break; | ||
| + } | ||
| + if (prop_type) { | ||
| + ret = iris_hfi_gen2_session_set_property(inst, | ||
| + prop_type, | ||
| + HFI_HOST_FLAGS_NONE, | ||
| + hfi_port, | ||
| + payload_type, | ||
| + &payload, | ||
| + payload_size); | ||
| + if (ret) | ||
| + return ret; | ||
| + } | ||
| + } | ||
| + inst_hfi_gen2->opsc_properties_set = true; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_subscribe_property(struct iris_inst *inst, u32 plane) | ||
| +{ | ||
| + struct iris_core *core = inst->core; | ||
| + u32 subscribe_prop_size, i; | ||
| + const u32 *subcribe_prop; | ||
| + u32 payload[32] = {0}; | ||
| + | ||
| + payload[0] = HFI_MODE_PROPERTY; | ||
| + | ||
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | ||
| + subscribe_prop_size = core->iris_platform_data->dec_input_prop_size; | ||
| + subcribe_prop = core->iris_platform_data->dec_input_prop; | ||
| + } else { | ||
| + subscribe_prop_size = core->iris_platform_data->dec_output_prop_size; | ||
| + subcribe_prop = core->iris_platform_data->dec_output_prop; | ||
| + } | ||
| + | ||
| + for (i = 0; i < subscribe_prop_size; i++) | ||
| + payload[i + 1] = subcribe_prop[i]; | ||
| + | ||
| + return iris_hfi_gen2_session_subscribe_mode(inst, | ||
| + HFI_CMD_SUBSCRIBE_MODE, | ||
| + plane, | ||
| + HFI_PAYLOAD_U32_ARRAY, | ||
| + &payload[0], | ||
| + (subscribe_prop_size + 1) * sizeof(u32)); | ||
| +} |
There was a problem hiding this comment.
Guard payload array bounds and fail fast on unsupported change params.
payload[32] is filled with change_param_size + 1 / subscribe_prop_size + 1 elements without bounds checks, and the default branch sets ret = -EINVAL but still continues and marks opsc_properties_set. This can mask unsupported properties and risks stack overwrite if platform data grows.
🔧 Proposed fix
static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane)
{
struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
struct hfi_subscription_params subsc_params;
u32 prop_type, payload_size, payload_type;
struct iris_core *core = inst->core;
const u32 *change_param;
u32 change_param_size;
u32 payload[32] = {0};
u32 hfi_port = 0, i;
int ret;
@@
change_param = core->iris_platform_data->input_config_params;
change_param_size = core->iris_platform_data->input_config_params_size;
+ if (change_param_size > ARRAY_SIZE(payload) - 1)
+ return -EINVAL;
@@
case HFI_PROP_LEVEL:
payload[0] = subsc_params.level;
payload_size = sizeof(u32);
payload_type = HFI_PAYLOAD_U32;
break;
default:
- prop_type = 0;
- ret = -EINVAL;
- break;
+ dev_err(core->dev, "unsupported change param: %u\n", prop_type);
+ return -EINVAL;
}
if (prop_type) {
ret = iris_hfi_gen2_session_set_property(inst,
prop_type,
HFI_HOST_FLAGS_NONE,
hfi_port,
payload_type,
&payload,
payload_size);
if (ret)
return ret;
}
}
@@
static int iris_hfi_gen2_subscribe_property(struct iris_inst *inst, u32 plane)
{
struct iris_core *core = inst->core;
u32 subscribe_prop_size, i;
const u32 *subcribe_prop;
u32 payload[32] = {0};
@@
if (V4L2_TYPE_IS_OUTPUT(plane)) {
subscribe_prop_size = core->iris_platform_data->dec_input_prop_size;
subcribe_prop = core->iris_platform_data->dec_input_prop;
} else {
subscribe_prop_size = core->iris_platform_data->dec_output_prop_size;
subcribe_prop = core->iris_platform_data->dec_output_prop;
}
+ if (subscribe_prop_size > ARRAY_SIZE(payload) - 1)
+ return -EINVAL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane) | |
| +{ | |
| + struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst); | |
| + struct hfi_subscription_params subsc_params; | |
| + u32 prop_type, payload_size, payload_type; | |
| + struct iris_core *core = inst->core; | |
| + const u32 *change_param; | |
| + u32 change_param_size; | |
| + u32 payload[32] = {0}; | |
| + u32 hfi_port = 0, i; | |
| + int ret; | |
| + | |
| + if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) || | |
| + (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) { | |
| + dev_err(core->dev, "invalid plane\n"); | |
| + return 0; | |
| + } | |
| + | |
| + change_param = core->iris_platform_data->input_config_params; | |
| + change_param_size = core->iris_platform_data->input_config_params_size; | |
| + | |
| + payload[0] = HFI_MODE_PORT_SETTINGS_CHANGE; | |
| + | |
| + for (i = 0; i < change_param_size; i++) | |
| + payload[i + 1] = change_param[i]; | |
| + | |
| + ret = iris_hfi_gen2_session_subscribe_mode(inst, | |
| + HFI_CMD_SUBSCRIBE_MODE, | |
| + plane, | |
| + HFI_PAYLOAD_U32_ARRAY, | |
| + &payload[0], | |
| + ((change_param_size + 1) * sizeof(u32))); | |
| + if (ret) | |
| + return ret; | |
| + | |
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| + inst_hfi_gen2->ipsc_properties_set = true; | |
| + } else { | |
| + hfi_port = iris_hfi_gen2_get_port(V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); | |
| + memcpy(&inst_hfi_gen2->dst_subcr_params, | |
| + &inst_hfi_gen2->src_subcr_params, | |
| + sizeof(inst_hfi_gen2->src_subcr_params)); | |
| + subsc_params = inst_hfi_gen2->dst_subcr_params; | |
| + for (i = 0; i < change_param_size; i++) { | |
| + payload[0] = 0; | |
| + payload[1] = 0; | |
| + payload_size = 0; | |
| + payload_type = 0; | |
| + prop_type = change_param[i]; | |
| + switch (prop_type) { | |
| + case HFI_PROP_BITSTREAM_RESOLUTION: | |
| + payload[0] = subsc_params.bitstream_resolution; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_CROP_OFFSETS: | |
| + payload[0] = subsc_params.crop_offsets[0]; | |
| + payload[1] = subsc_params.crop_offsets[1]; | |
| + payload_size = sizeof(u64); | |
| + payload_type = HFI_PAYLOAD_64_PACKED; | |
| + break; | |
| + case HFI_PROP_CODED_FRAMES: | |
| + payload[0] = subsc_params.coded_frames; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT: | |
| + payload[0] = subsc_params.fw_min_count; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_PIC_ORDER_CNT_TYPE: | |
| + payload[0] = subsc_params.pic_order_cnt; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_SIGNAL_COLOR_INFO: | |
| + payload[0] = subsc_params.color_info; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_PROFILE: | |
| + payload[0] = subsc_params.profile; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + case HFI_PROP_LEVEL: | |
| + payload[0] = subsc_params.level; | |
| + payload_size = sizeof(u32); | |
| + payload_type = HFI_PAYLOAD_U32; | |
| + break; | |
| + default: | |
| + prop_type = 0; | |
| + ret = -EINVAL; | |
| + break; | |
| + } | |
| + if (prop_type) { | |
| + ret = iris_hfi_gen2_session_set_property(inst, | |
| + prop_type, | |
| + HFI_HOST_FLAGS_NONE, | |
| + hfi_port, | |
| + payload_type, | |
| + &payload, | |
| + payload_size); | |
| + if (ret) | |
| + return ret; | |
| + } | |
| + } | |
| + inst_hfi_gen2->opsc_properties_set = true; | |
| + } | |
| + | |
| + return 0; | |
| +} | |
| + | |
| +static int iris_hfi_gen2_subscribe_property(struct iris_inst *inst, u32 plane) | |
| +{ | |
| + struct iris_core *core = inst->core; | |
| + u32 subscribe_prop_size, i; | |
| + const u32 *subcribe_prop; | |
| + u32 payload[32] = {0}; | |
| + | |
| + payload[0] = HFI_MODE_PROPERTY; | |
| + | |
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| + subscribe_prop_size = core->iris_platform_data->dec_input_prop_size; | |
| + subcribe_prop = core->iris_platform_data->dec_input_prop; | |
| + } else { | |
| + subscribe_prop_size = core->iris_platform_data->dec_output_prop_size; | |
| + subcribe_prop = core->iris_platform_data->dec_output_prop; | |
| + } | |
| + | |
| + for (i = 0; i < subscribe_prop_size; i++) | |
| + payload[i + 1] = subcribe_prop[i]; | |
| + | |
| + return iris_hfi_gen2_session_subscribe_mode(inst, | |
| + HFI_CMD_SUBSCRIBE_MODE, | |
| + plane, | |
| + HFI_PAYLOAD_U32_ARRAY, | |
| + &payload[0], | |
| + (subscribe_prop_size + 1) * sizeof(u32)); | |
| +} | |
| static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane) | |
| { | |
| struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst); | |
| struct hfi_subscription_params subsc_params; | |
| u32 prop_type, payload_size, payload_type; | |
| struct iris_core *core = inst->core; | |
| const u32 *change_param; | |
| u32 change_param_size; | |
| u32 payload[32] = {0}; | |
| u32 hfi_port = 0, i; | |
| int ret; | |
| if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) || | |
| (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) { | |
| dev_err(core->dev, "invalid plane\n"); | |
| return 0; | |
| } | |
| change_param = core->iris_platform_data->input_config_params; | |
| change_param_size = core->iris_platform_data->input_config_params_size; | |
| if (change_param_size > ARRAY_SIZE(payload) - 1) | |
| return -EINVAL; | |
| payload[0] = HFI_MODE_PORT_SETTINGS_CHANGE; | |
| for (i = 0; i < change_param_size; i++) | |
| payload[i + 1] = change_param[i]; | |
| ret = iris_hfi_gen2_session_subscribe_mode(inst, | |
| HFI_CMD_SUBSCRIBE_MODE, | |
| plane, | |
| HFI_PAYLOAD_U32_ARRAY, | |
| &payload[0], | |
| ((change_param_size + 1) * sizeof(u32))); | |
| if (ret) | |
| return ret; | |
| if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| inst_hfi_gen2->ipsc_properties_set = true; | |
| } else { | |
| hfi_port = iris_hfi_gen2_get_port(V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); | |
| memcpy(&inst_hfi_gen2->dst_subcr_params, | |
| &inst_hfi_gen2->src_subcr_params, | |
| sizeof(inst_hfi_gen2->src_subcr_params)); | |
| subsc_params = inst_hfi_gen2->dst_subcr_params; | |
| for (i = 0; i < change_param_size; i++) { | |
| payload[0] = 0; | |
| payload[1] = 0; | |
| payload_size = 0; | |
| payload_type = 0; | |
| prop_type = change_param[i]; | |
| switch (prop_type) { | |
| case HFI_PROP_BITSTREAM_RESOLUTION: | |
| payload[0] = subsc_params.bitstream_resolution; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_CROP_OFFSETS: | |
| payload[0] = subsc_params.crop_offsets[0]; | |
| payload[1] = subsc_params.crop_offsets[1]; | |
| payload_size = sizeof(u64); | |
| payload_type = HFI_PAYLOAD_64_PACKED; | |
| break; | |
| case HFI_PROP_CODED_FRAMES: | |
| payload[0] = subsc_params.coded_frames; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_BUFFER_FW_MIN_OUTPUT_COUNT: | |
| payload[0] = subsc_params.fw_min_count; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_PIC_ORDER_CNT_TYPE: | |
| payload[0] = subsc_params.pic_order_cnt; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_SIGNAL_COLOR_INFO: | |
| payload[0] = subsc_params.color_info; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_PROFILE: | |
| payload[0] = subsc_params.profile; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| case HFI_PROP_LEVEL: | |
| payload[0] = subsc_params.level; | |
| payload_size = sizeof(u32); | |
| payload_type = HFI_PAYLOAD_U32; | |
| break; | |
| default: | |
| dev_err(core->dev, "unsupported change param: %u\n", prop_type); | |
| return -EINVAL; | |
| } | |
| if (prop_type) { | |
| ret = iris_hfi_gen2_session_set_property(inst, | |
| prop_type, | |
| HFI_HOST_FLAGS_NONE, | |
| hfi_port, | |
| payload_type, | |
| &payload, | |
| payload_size); | |
| if (ret) | |
| return ret; | |
| } | |
| } | |
| inst_hfi_gen2->opsc_properties_set = true; | |
| } | |
| return 0; | |
| } | |
| static int iris_hfi_gen2_subscribe_property(struct iris_inst *inst, u32 plane) | |
| { | |
| struct iris_core *core = inst->core; | |
| u32 subscribe_prop_size, i; | |
| const u32 *subcribe_prop; | |
| u32 payload[32] = {0}; | |
| payload[0] = HFI_MODE_PROPERTY; | |
| if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| subscribe_prop_size = core->iris_platform_data->dec_input_prop_size; | |
| subcribe_prop = core->iris_platform_data->dec_input_prop; | |
| } else { | |
| subscribe_prop_size = core->iris_platform_data->dec_output_prop_size; | |
| subcribe_prop = core->iris_platform_data->dec_output_prop; | |
| } | |
| if (subscribe_prop_size > ARRAY_SIZE(payload) - 1) | |
| return -EINVAL; | |
| for (i = 0; i < subscribe_prop_size; i++) | |
| payload[i + 1] = subcribe_prop[i]; | |
| return iris_hfi_gen2_session_subscribe_mode(inst, | |
| HFI_CMD_SUBSCRIBE_MODE, | |
| plane, | |
| HFI_PAYLOAD_U32_ARRAY, | |
| &payload[0], | |
| (subscribe_prop_size + 1) * sizeof(u32)); | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0018_media--iris--subscribe-parameters-and-properties-t.patch`
around lines 87 - 227, The functions iris_hfi_gen2_subscribe_change_param and
iris_hfi_gen2_subscribe_property write into a fixed payload[32] without bounds
checks and continue on unsupported properties; add explicit checks that
change_param_size + 1 and subscribe_prop_size + 1 do not exceed
ARRAY_SIZE(payload) and return -EINVAL immediately if they do, and when the
default case in the change_param switch is hit return the error immediately (do
not continue the loop or set
inst_hfi_gen2->opsc_properties_set/ipsc_properties_set), and ensure the payload
pointer passed to
iris_hfi_gen2_session_set_property/iris_hfi_gen2_session_subscribe_mode uses the
payload buffer correctly (e.g., &payload[0]) after the bounds check.
| + case V4L2_COLORSPACE_DCI_P3: | ||
| + return HFI_PRIMARIES_SMPTE_RP431_2; |
There was a problem hiding this comment.
Bug: Case and return values are swapped for DCI-P3 color primaries.
In the iris_hfi_get_v4l2_color_primaries function, the case statement incorrectly checks for a V4L2 value (V4L2_COLORSPACE_DCI_P3) and returns an HFI value (HFI_PRIMARIES_SMPTE_RP431_2). This is reversed compared to all other cases in the switch, which check HFI values and return V4L2 values.
This will cause DCI-P3 color primaries to never be correctly converted, and could cause unexpected behavior if V4L2_COLORSPACE_DCI_P3 happens to match an HFI constant numerically.
Proposed fix
- case V4L2_COLORSPACE_DCI_P3:
- return HFI_PRIMARIES_SMPTE_RP431_2;
+ case HFI_PRIMARIES_SMPTE_RP431_2:
+ return V4L2_COLORSPACE_DCI_P3;📝 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.
| + case V4L2_COLORSPACE_DCI_P3: | |
| + return HFI_PRIMARIES_SMPTE_RP431_2; | |
| case HFI_PRIMARIES_SMPTE_RP431_2: | |
| return V4L2_COLORSPACE_DCI_P3; |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0021_media--iris--add-support-for-dynamic-resolution-ch.patch`
around lines 61 - 62, The switch in iris_hfi_get_v4l2_color_primaries has the
DCI-P3 branch reversed (it currently matches V4L2_COLORSPACE_DCI_P3 and returns
HFI_PRIMARIES_SMPTE_RP431_2); change that branch to match the HFI constant and
return the V4L2 constant instead—i.e., use case HFI_PRIMARIES_SMPTE_RP431_2:
return V4L2_COLORSPACE_DCI_P3; so the function consistently maps HFI primaries
-> V4L2 values.
patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch
Show resolved
Hide resolved
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
config/desktop/trixie/environments/kde-plasma-mobile/config_base/packages (1)
12-20:⚠️ Potential issue | 🟠 MajorPotential conflict:
pipewireadded alongsidepulseaudio.Installing both
pipewireandpulseaudio(lines 12, 18) can cause audio device contention at runtime. In Debian Trixie, the typical approach is one of:
- PipeWire as full replacement: Drop
pulseaudioandpulseaudio-module-bluetooth, addpipewire-pulse,pipewire-audio, andwireplumber. The PulseAudio client tools (pavucontrol, etc.) will continue to work via the compatibility layer.- PipeWire for video/portal only: If
pipewireis here solely for screen-sharing (xdg-desktop-portal), this is fine, but it would be worth adding a comment to clarify intent and avoid future confusion.Could you clarify the intended audio stack? If this device should use PipeWire for audio (which is the modern default for Plasma Mobile), the PulseAudio packages should be swapped out.
patch/kernel/archive/sm8550-6.13/0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch (1)
35-55:⚠️ Potential issue | 🟡 MinorUnchecked return value from
clk_prepare_enable.
clk_prepare_enable()can fail, and its return value should be checked and propagated. Currently the startup function ignores the error and continues.Proposed fix
+static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) +{ + ... + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); + ... + switch (cpu_dai->id) { + case PRIMARY_MI2S_RX: - clk_prepare_enable(pdata->i2s_clk); + ret = clk_prepare_enable(pdata->i2s_clk); + if (ret) + return ret; snd_soc_dai_set_fmt(cpu_dai, fmt); snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); break;patch/kernel/archive/sm8550-6.13/0032_drm-panel--Add-panel-driver-for-Chipone-ICNA3512-b.patch (2)
263-300:⚠️ Potential issue | 🟠 MajorResource leak on error paths in
icna3512_prepare.After
init_sequencesucceeds, ifmipi_dsi_picture_parameter_set(line 286) ormipi_dsi_compression_mode(line 293) fails, the function returns without disabling regulators or de-asserting the reset GPIO. The cleanup at line 279 only covers theinit_sequencefailure path.Proposed fix — add cleanup to remaining error paths
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_disable; } /* 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; + goto err_disable; } return 0; + +err_disable: + regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); + return ret; }
431-442:⚠️ Potential issue | 🟡 MinorBacklight default brightness equals max brightness.
Both
brightnessandmax_brightnessare set to 4096. This means the panel starts at maximum brightness. Typicallybrightnessis set lower (e.g., 2/3 of max) to avoid blinding the user on first power-on and to leave headroom for brightness adjustments.patch/kernel/archive/sm8550-6.13/0033_leds--Add-driver-for-HEROIC-HTR3212.patch (2)
292-311:⚠️ Potential issue | 🟠 MajorRegulator enabled but never disabled on probe error paths.
regulator_enable(priv->vdd)at line 292 is not matched byregulator_disableifhtr3212_init_regs(line 303) orhtr3212_parse_dt(line 307) fails. The regulator was obtained viadevm_regulator_getwhich only manages theget/putlifecycle, not enable/disable. This leaks a regulator enable reference.Proposed fix — add cleanup on error
ret = htr3212_init_regs(priv); - if (ret) - return ret; + if (ret) { + regulator_disable(priv->vdd); + return ret; + } ret = htr3212_parse_dt(dev, priv); - if (ret) - return ret; + if (ret) { + regulator_disable(priv->vdd); + return ret; + }
348-348:⚠️ Potential issue | 🟡 MinorMissing closing
>inMODULE_AUTHOR.The email address is missing the closing angle bracket.
Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");patch/kernel/archive/sm8550-6.13/0031_input--Add-driver-for-RSInput-Gamepad.patch (6)
54-54:⚠️ Potential issue | 🟠 Major
#define DEBUGleft in — enables alldev_dbgcalls unconditionally.This should be removed before merging. It was likely left in during development and will produce excessive kernel log output in production.
Proposed fix
-#define DEBUG - `#include` <linux/errno.h>
281-314:⚠️ Potential issue | 🔴 CriticalSleeping in atomic/callback context —
rsinput_init_commandscalled fromrsinput_rx.
rsinput_rxis the serdev.receive_bufcallback, invoked from the serial core, potentially in interrupt or softirq context. On checksum mismatch (line 307), it callsrsinput_init_commandswhich containsmsleep(100)— sleeping in a non-sleepable context will trigger a BUG.This re-initialization on bad checksum should be deferred to a workqueue.
333-352:⚠️ Potential issue | 🔴 CriticalGPIO error pointers stored and later dereferenced.
devm_gpiod_get_optionalreturnsERR_PTRon real failures. The code warns but does not return or clear the error pointer. Later,if (drv->boot_gpio)/if (drv->reset_gpio)/if (drv->enable_gpio)are truthy forERR_PTRvalues, sogpiod_set_value_cansleepwill be called on an invalid pointer (lines 354-367).Proposed fix — propagate errors
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");
370-428:⚠️ Potential issue | 🟠 Major
serdev_device_opennot closed on multiple error paths.After
serdev_device_opensucceeds at line 370, the following error paths return without callingserdev_device_close: baudrate failure (line 381), input allocation failure (line 387), and input registration failure (line 420). Only theinit_commandsfailure path (line 426) properly closes.Proposed fix — close serdev on all error paths
error = serdev_device_set_baudrate(serdev, 115200); if (error < 0) - return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); + goto err_close; serdev_device_set_flow_control(serdev, false); drv->input = devm_input_allocate_device(&serdev->dev); - if (!drv->input) - return -ENOMEM; + if (!drv->input) { + error = -ENOMEM; + goto err_close; + } ... error = input_register_device(drv->input); if (error) - return error; + goto err_close; ... + return 0; + +err_close: + serdev_device_close(serdev); + return error;
433-443:⚠️ Potential issue | 🔴 CriticalDouble-unregister of devm-managed input device.
drv->inputis allocated viadevm_input_allocate_device(line 385), which automatically handles unregistration on device removal. Callinginput_unregister_devicemanually inrsinput_remove(line 437) will cause a double-free/use-after-free.Proposed fix
static void rsinput_remove(struct serdev_device *serdev) { struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); serdev_device_close(serdev); - input_unregister_device(drv->input); if (drv->enable_gpio) gpiod_set_value_cansleep(drv->enable_gpio, 0);
209-241:⚠️ Potential issue | 🟠 Major
staticlocalprev_statesis shared across all device instances.
prev_statesat line 211 is a function-localstatic, meaning it is shared if multiple RSInput devices are probed. It should be a field instruct rsinput_driver.Proposed fix
Add
unsigned long prev_states;tostruct rsinput_driverand referencedrv->prev_statesinstead:struct rsinput_driver { ... + unsigned long prev_states; };static void handle_cmd_status(...) { 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; + unsigned long current_states = keys; + unsigned long changes; ... - prev_states = keys; + drv->prev_states = keys; }
🤖 Fix all issues with AI agents
In `@config/boards/ayn-odin2-grub.csc`:
- Around line 83-87: The trixie Mesa driver block is commented out causing GRUB
builds to skip installing Mesa Vulkan drivers; either restore the block or
document why it's intentionally disabled: if no GRUB+trixie issue exists,
uncomment the RELEASE-check block that calls do_with_retries with
chroot_sdcard_apt_get_update, display_alert "Installing Mesa Vulkan Drivers",
and do_with_retries chroot_sdcard_apt_get_install for libgl1-mesa-dri
mesa-vulkan-drivers vulkan-tools; if it is intentional, add a brief comment
above the RELEASE check explaining the GRUB+trixie incompatibility and reference
this behavior so maintainers know it’s deliberate.
In `@config/boards/ayn-odin2-portal-grub.csc`:
- Around line 38-45: The curl invocation that writes "temp.zip" should include
--fail (e.g. curl -L --fail -o temp.zip) and check its exit status before
running unzip so HTTP errors (404/etc.) don't produce invalid zip files; after
the curl command, if it fails, exit with a non-zero code and avoid calling unzip
on "temp.zip" (also remove any partial temp.zip). Additionally, replace the
branch zip URL with a pinned commit hash or tag instead of the branch path
("ayn/v1.2.13.zip") to ensure reproducible builds; update the code that
references temp.zip and unzip_dir accordingly.
- Around line 52-61: Fix the incorrect file mode and unquoted variables: change
all install mode arguments from 655 to 755 for the script/binary installs (the
lines installing setup-usbgadget-network.sh, remove-usbgadget-network.sh,
usb-gadget-initramfs-hook, usb-gadget-initramfs-premount, dropbear,
kill-dropbear and any other script installs) so owner execute bit is set, and
quote $destination and $SRC in the mkdir and install commands (e.g., use
"$destination" and "$SRC") to prevent word-splitting/expansion issues; keep the
same target paths and modes otherwise.
In `@config/kernel/old-linux-sm8550-edge.config`:
- Around line 934-936: The config enables AppArmor via
CONFIG_SECURITY_APPARMOR=y but does not include "apparmor" in the CONFIG_LSM
string; update CONFIG_LSM to include apparmor (e.g., add apparmor into the
comma-separated list used by CONFIG_LSM) so the LSM list contains "apparmor" and
keep CONFIG_SECURITY_APPARMOR=y in sync; ensure you don't duplicate entries and
preserve the existing order/commas in the CONFIG_LSM value.
- Line 1: The config file named old-linux-sm8550-edge.config does not follow the
Armbian naming convention and will be ignored; either delete
old-linux-sm8550-edge.config if it's obsolete, or rename it to
linux-sm8550-edge.config so the build system recognizes it, and ensure there are
no duplicate linux-sm8550-edge.config files (update any references to the old
name if present).
In
`@patch/kernel/archive/sm8550-6.13/0004_media--iris--introduce-iris-core-state-management-.patch`:
- Around line 519-530: The probe path allocates and calls mutex_init but on
failure returns via the err_vdev_unreg and err_v4l2_unreg labels without calling
mutex_destroy; update the probe failure cleanup to mirror iris_remove by calling
mutex_destroy(&core->lock) before those error labels return. Specifically, in
the probe function (after mutex_init) add mutex_destroy(&core->lock) in the
err_vdev_unreg and err_v4l2_unreg cleanup paths so the mutex is always destroyed
on probe failure; ensure you keep existing calls to iris_core_deinit,
video_unregister_device, and v4l2_device_unregister in their respective error
paths intact.
In
`@patch/kernel/archive/sm8550-6.13/0005_media--iris--implement-video-firmware-load-unload.patch`:
- Around line 182-184: The code discards the actual error from
iris_load_fw_to_memory by logging a generic message and returning -ENOMEM;
change the handler in the caller (where ret holds iris_load_fw_to_memory's
result) to log the real error and propagate it: use dev_err(core->dev, "firmware
download failed: %d\n", ret) (or similar to include error text) and return ret
instead of returning -ENOMEM so callers receive the original errno from
iris_load_fw_to_memory.
- Around line 156-167: The success path currently returns immediately after
calling qcom_scm_pas_auth_and_reset, leaking mem_virt and firmware; fix by
performing the same cleanup as the error paths before returning on success:
after qcom_scm_pas_auth_and_reset succeeds, call memunmap(mem_virt) and
release_firmware(firmware) (or reorder labels so the normal return jumps to a
cleanup label that calls memunmap and release_firmware) then return ret; keep
the existing err_mem_unmap/err_release_fw labels for error flows and ensure
mem_virt and firmware are only released once.
In
`@patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch`:
- Around line 421-446: The early-return path in iris_hfi_queue_cmd_write wrongly
calls pm_runtime_put_sync even when pm_runtime_resume_and_get failed, causing a
usage-count underflow; update the control flow so that after calling
pm_runtime_resume_and_get (in iris_hfi_queue_cmd_write) you only call
pm_runtime_put_sync if pm_runtime_resume_and_get succeeded (i.e., when you later
need to unwind after a failure from iris_hfi_queue_cmd_write_locked), while
retaining the current mutex_lock/ mutex_unlock and the pm_runtime_mark_last_busy
+ pm_runtime_put_autosuspend sequence on the success path; in practice, split
the exit handling into two paths (one for resume failure that skips
pm_runtime_put_sync, and one for post-resume errors that calls
pm_runtime_put_sync before returning).
- Around line 1174-1177: The call to dev_pm_opp_set_rate(core->dev, freq) must
have its return value checked and handled; replace the discarded-call with code
that captures the return (e.g., ret = dev_pm_opp_set_rate(...)) and if ret < 0
jumps to an err_power_off_hw label that tears down the hardware power domain
(mirroring the cleanup done in iris_enable_power_domains failure paths) and then
falls through to the existing err_power_off_ctrl label to finish cleanup; add
the err_power_off_hw label if missing and ensure it undoes the HW power domain
enable before proceeding to err_power_off_ctrl.
In
`@patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch`:
- Around line 1009-1047: The handler loop in
iris_hfi_gen2_handle_session_response currently overwrites ret for each packet
so earlier errors are lost; after calling range[i].handle(inst, packet) store
its return into a temporary (or into ret) and if it is non-zero immediately
break out of the loops (or return) so the first error is propagated; ensure you
still unlock inst->lock before returning (i.e., check the handler result, break
both inner/outer loops or goto out to run mutex_unlock(&inst->lock) and then
return ret); reference the variables/funcs:
iris_hfi_gen2_handle_session_response, ret, range[i].handle, packet, inst->lock
and hdr->num_packets when making the change.
- Around line 1233-1247: iris_get_instance currently returns a raw pointer after
dropping core->lock which exposes a TOCTOU/use-after-free race (e.g., between
iris_get_instance and iris_close or callers like
iris_hfi_gen1_handle_response/iris_hfi_gen2_handle_session_response). Fix by
ensuring the instance is held before releasing core->lock: either (A) acquire
inst->lock while still holding core->lock (grab core->lock, find inst,
mutex_lock(&inst->lock), then mutex_unlock(&core->lock) and return inst) or (B)
add kref-based refcounting to struct iris_inst and call kref_get(&inst->ref)
while still holding core->lock and drop core->lock before returning, then
require callers to call kref_put when done; update iris_close to use kref_put
and to remove/free only when refcount reaches zero. Ensure all callers follow
the new refcount/lock semantics.
- Around line 1031-1041: The packet-level HFI_FW_FLAGS_SESSION_ERROR check
currently coexists with the range dispatch and causes
iris_hfi_gen2_handle_session_error to be invoked twice for packets whose type
falls within the HFI_SESSION_ERROR range; update the logic so that when
(packet->flags & HFI_FW_FLAGS_SESSION_ERROR) is true you call
iris_hfi_gen2_handle_session_error(inst, packet) and then skip the range handler
(e.g., continue to next packet) instead of letting the subsequent
range[i].handle call run, or alternatively move the flag check outside the for
(i ...) range loop and only run it once per packet before any range dispatches.
- Around line 57-81: The iris_vb2_ops struct is missing wait callbacks which
causes ctx_q_lock (assigned to vb2_queue.lock) to be held while VB2 blocks,
leading to deadlocks; update the iris_vb2_ops definition to add .wait_prepare =
vb2_ops_wait_prepare and .wait_finish = vb2_ops_wait_finish so the queue lock
(vb2_queue.lock / ctx_q_lock) is released during blocking operations like DQBUF
and properly re-acquired afterwards.
In
`@patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch`:
- Around line 648-654: The debug log prints the same value because inst->state
is assigned before logging; in the change_state block capture the previous state
into a temporary (e.g., old_state) before assigning inst->state = request_state,
then call dev_dbg(inst->core->dev, "state changed from %x to %x\n", old_state,
request_state); reference the change_state label and the
inst->state/request_state variables and update the dev_dbg argument order to use
the saved old_state.
In
`@patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch`:
- Line 1055: Rename the misspelled function iris_hfi_gen2_set_bit_dpeth to
iris_hfi_gen2_set_bit_depth and update all references (including the entry in
the handler table that currently points to iris_hfi_gen2_set_bit_dpeth) to use
the corrected symbol; ensure the function prototype, definition, and any forward
declarations match the new name so linkage and the handler table resolve
correctly.
- Around line 798-823: The code always loads
core->iris_platform_data->input_config_params into config_params and
config_params_size so the V4L2_TYPE_IS_CAPTURE branch compares capture plane
property IDs against prop_type_handle_out_arr using the input list (silently
skipping capture properties); fix by selecting the correct platform array based
on plane before the loops—i.e., set config_params/config_params_size =
core->iris_platform_data->input_config_params/input_config_params_size when
V4L2_TYPE_IS_OUTPUT(plane) and set them =
core->iris_platform_data->output_config_params/output_config_params_size when
V4L2_TYPE_IS_CAPTURE(plane), then keep the existing loops that match
config_params[i] against prop_type_handle_inp_arr or prop_type_handle_out_arr
and call their .handle(inst).
In
`@patch/kernel/archive/sm8550-6.13/0020_media--iris--implement-vb2-ops-for-buf_queue-and-f.patch`:
- Around line 488-506: The null-check on buffers is dead because buffers is
assigned from &inst->buffers[BUF_DPB], an embedded array element that cannot be
NULL; remove the if (!buffers) goto error; branch and any associated error label
reliance, and simply iterate/list_for_each_entry over
inst->buffers[BUF_DPB].list after assigning buffers = &inst->buffers[BUF_DPB];
(keep the surrounding logic: iris_split_mode_enabled(inst), pkt->stream_id == 0,
found detection comparing iter->index to output_tag and iter->data_offset to
offset, and setting buf = iter when found).
In
`@patch/kernel/archive/sm8550-6.13/0033_leds--Add-driver-for-HEROIC-HTR3212.patch`:
- Around line 282-284: The devm_gpiod_get call for priv->sdb must be checked
with IS_ERR before calling PTR_ERR; change the logic in the probe path where
devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) is assigned to first test if
(IS_ERR(priv->sdb)) then get the error via PTR_ERR(priv->sdb) and if that equals
-EPROBE_DEFER return -EPROBE_DEFER else return that error (or propagate
appropriately) so you never store an error pointer in priv->sdb that later gets
dereferenced by gpiod_set_value_cansleep.
🧹 Nitpick comments (17)
patch/kernel/archive/sm8550-6.13/0008_media--iris--implement-power-management.patch (1)
594-598:__maybe_unusedannotations should be dropped in favor ofpm_sleep_ptr()/DEFINE_RUNTIME_DEV_PM_OPS.The
SET_SYSTEM_SLEEP_PM_OPS/SET_RUNTIME_PM_OPSmacros together with__maybe_unusedare the legacy pattern. Modern kernels (≥ 6.4) preferDEFINE_RUNTIME_DEV_PM_OPSorpm_sleep_ptr()which let the compiler drop dead code without the__maybe_unusedworkaround. Since this targets 6.13, the newer macros are available.patch/kernel/archive/sm8550-6.13/0007_media--iris--introduce-host-firmware-interface-wit.patch (2)
625-638: Variable shadowing: innerpktshadows outerpktiniris_hfi_gen1_flush_debug_queue.Line 633 declares
struct hfi_msg_sys_debug_pkt *pktinside theifblock, shadowing the outerstruct hfi_msg_sys_coverage_pkt *pktat line 627. This is valid C and works correctly since the inner scope uses the debug packet type while the outer uses coverage, but it can confuse readers and some compilers will warn with-Wshadow. Since this is upstream Qualcomm code carried as a patch, this is a minor observation.
1569-1576:iris_sys_error_handlerre-inits without cancelling itself or checking state.If
iris_core_initat line 1575 fails (e.g., firmware load timeout), it may trigger another system error, which schedulessys_error_handleragain, creating an unbounded retry loop. There's no backoff, retry limit, or cancellation of the delayed work before re-init. Since this is upstream code, this is a minor observation — the 10ms delay provides some throttling, and theIRIS_CORE_ERRORstate gate iniris_wait_for_system_responsewould return-EIObefore re-triggering HFI init.patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch (1)
535-561: Silently swallowing-EOPNOTSUPPbut the callee never returns it.
hfi_gen1_set_propertyconverts-EOPNOTSUPPto success, butiris_hfi_gen1_packet_session_set_propertyreturns-EINVALfor unknown property types, never-EOPNOTSUPP. The special-case is dead code. Consider aligning the error codes or removing the-EOPNOTSUPPhandling.config/boards/ayn-odin2.csc (2)
69-82: Mesa driver handling restructured appropriately.The split between Ubuntu releases (using kisak-mesa PPA) and Debian trixie (using stock repos) is correct. However, note that the trixie block could be simplified by combining with the Ubuntu block's install lines since only the PPA addition differs.
114-119: Firmware glob onsm8550/*will match subdirectories too.The glob
/lib/firmware/qcom/sm8550/*will match both files and theayn/subdirectory itself. Whileadd_firmwarelikely handles directory entries gracefully, the loop at Line 117–119 forsm8550/ayn/*is partially redundant since files directly insm8550/ayn/would have their parent dir entry already matched. This is a minor nit — the behavior is correct sinceadd_firmwareshould skip non-files.config/boards/ayn-odin2-grub.csc (1)
1-141: Significant code duplication across board configs.This file is nearly identical to
ayn-odin2.csc(minus ABL-specific settings, plus GRUB settings). The same pattern repeats acrossayn-odin2-portal.cscand presumablyayn-odin2-portal-grub.csc. Functions likeis_userspace_supported,alsa_ucm_conf,firmware,enable_services, andbsp_firmware_in_initrdare copy-pasted with minimal differences.Consider extracting shared logic into a common include file (e.g.,
config/boards/include/ayn-odin2-common.inc) to reduce maintenance burden. Any bug fix (like the 655→755 permission fix) currently needs to be applied in 4 places.config/boards/ayn-odin2-portal-grub.csc (1)
74-87: Mesa driver installation logic is duplicated across release branches.The
apt_get_update+apt_get_installfor Mesa Vulkan drivers is identical for both the Ubuntu and Trixie paths. Consider extracting the common install block and only conditionally adding the PPA for Ubuntu releases.♻️ Suggested refactor
- if [[ "${RELEASE}" == "jammy" ]] || [[ "${RELEASE}" == "noble" ]] || [[ "${RELEASE}" == "plucky" ]]; then - display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "warn" - do_with_retries 3 chroot_sdcard add-apt-repository ppa:kisak/kisak-mesa --yes - - do_with_retries 3 chroot_sdcard_apt_get_update - display_alert "Installing Mesa Vulkan Drivers" - do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools - fi - - if [[ "${RELEASE}" == "trixie" ]]; then - do_with_retries 3 chroot_sdcard_apt_get_update - display_alert "Installing Mesa Vulkan Drivers" - do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools - fi + if [[ "${RELEASE}" == "jammy" ]] || [[ "${RELEASE}" == "noble" ]] || [[ "${RELEASE}" == "plucky" ]]; then + display_alert "Adding Mesa PPA For Ubuntu ${BOARD}" "warn" + do_with_retries 3 chroot_sdcard add-apt-repository ppa:kisak/kisak-mesa --yes + fi + + do_with_retries 3 chroot_sdcard_apt_get_update + display_alert "Installing Mesa Vulkan Drivers" + do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-toolspatch/kernel/archive/sm8550-6.13/0032_drm-panel--Add-panel-driver-for-Chipone-ICNA3512-b.patch (1)
133-201: Inconsistent indentation — mixed tabs and spaces throughouticna3512_init_sequence.The function uses tabs for some
mipi_dsi_dcs_write_seqcalls and spaces for others (e.g., lines 151, 154, 157-160 vs. lines 145, 152). The} else {at line 162 also violates kernel coding style (should be on the same line as}). This pattern recurs throughout the file (lines 177-181, 189-191, 269, 279, 327, 483, 490-499).patch/kernel/archive/sm8550-6.13/0027_media--iris--enable-video-driver-probe-of-SM8250-S.patch (1)
212-217: Inconsistent indentation iniris_dt_matchtable entry.The SM8250 entry uses double-indented braces compared to the SM8550 entry above it. This should match the surrounding style for consistency.
Proposed fix
`#if` (!IS_ENABLED(CONFIG_VIDEO_QCOM_VENUS)) - { - .compatible = "qcom,sm8250-venus", - .data = &sm8250_data, - }, + { + .compatible = "qcom,sm8250-venus", + .data = &sm8250_data, + }, `#endif`patch/kernel/archive/sm8550-6.13/0024_media--iris--add-check-whether-the-video-session-i.patch (1)
108-142: Unprotectedcore->instanceslist traversal iniris_check_session_supported.The loop at the "found" check (lines 115-118 in the patch diff) iterates
core->instanceswithout holdingcore->lock, whileiris_check_core_mbpf(called immediately after) acquires the same lock for its own traversal. A concurrentiris_open/iris_closecould modify the list during the unprotected iteration.That said, this is an upstream-accepted patch (reviewed/tested by multiple Linaro engineers), and the caller likely already holds
inst->lockwhich may provide sufficient serialization in practice. Flagging for awareness rather than as a blocker.patch/kernel/archive/sm8550-6.13/0021_media--iris--add-support-for-dynamic-resolution-ch.patch (1)
304-369:defaultcase in property-parsing loop does not advancedata_ptr.In
iris_hfi_gen1_read_changed_params, when theswitch(ptype)hits thedefaultbranch,data_ptris not advanced. The loop will re-read the same unknownptypevalue on every remaining iteration (untilnum_properties_changeddecrements to zero), silently skipping all subsequent properties.This is upstream-accepted code, but worth noting: if the firmware ever sends an unrecognized property, all properties after it in the notification will be lost.
patch/kernel/archive/sm8550-6.13/0018_media--iris--subscribe-parameters-and-properties-t.patch (1)
99-103: Misleading error log and inconsistent return value when properties are already set.When
ipsc_properties_setoropsc_properties_setis alreadytrue, the code logs"invalid plane"(which is inaccurate—the plane is valid, the properties were just already submitted) and returns0(success) despite logging at error level. Consider either making this a no-op silently, or returning an error if it's truly unexpected.patch/kernel/archive/sm8550-6.13/0025_media--iris--implement-power-scaling-for-vpu2-and-.patch (1)
316-334:iris_scale_powerresume-and-put pattern has a subtle TOCTOU window.Between
pm_runtime_put_autosuspend(line 326) and the subsequentiris_scale_clocks→dev_pm_opp_set_ratecall, the device could autosuspend again. In practicedev_pm_opp_set_rateinternally handles runtime PM, so this is unlikely to cause issues. Just noting for awareness.patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch (1)
851-865: State checked without holdinginst->lock.Lines 858–859 read
inst->statebeforeinst->lockis acquired on line 861. The same pattern appears iniris_vb2_stop_streamingat lines 897–898. While VB2 typically serializes streaming ops per queue, a concurrent error-path or system-error handler (which does iterate instances and setIRIS_INST_ERRORundercore->lockbut notinst->lock) could race with this check.Consider moving the early-return check inside the locked section, or document why external serialization makes this safe.
patch/kernel/archive/sm8550-6.13/0019_media--iris--allocate,-initialize-and-queue-intern.patch (1)
307-323:iris_hfi_gen1_buf_type_from_driverreturns-EINVALfrom au32function.The function signature is
static u32, but the default case returns-EINVAL(a negativeint). The callers (lines 353–354, 386–388) compare theu32result against-EINVAL, which works due to implicit unsigned conversion but is a type-safety code smell. Since this is upstream code, noting for awareness only.patch/kernel/archive/sm8550-6.13/0009_media--iris--implement-reqbuf-ioctl-with-vb2_queue.patch (1)
694-694: Minor:u32 default_header = false;uses a boolean literal for au32.While functionally equivalent to
0, usingfalsefor au32variable is misleading. Consideru32 default_header = 0;.
config/boards/ayn-odin2-grub.csc
Outdated
| #if [[ "${RELEASE}" == "trixie" ]]; then | ||
| # do_with_retries 3 chroot_sdcard_apt_get_update | ||
| # display_alert "Installing Mesa Vulkan Drivers" | ||
| # do_with_retries 3 chroot_sdcard_apt_get_install libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools | ||
| #fi |
There was a problem hiding this comment.
Commented-out trixie Mesa block creates a feature gap.
The trixie Mesa driver installation is active in ayn-odin2.csc and ayn-odin2-portal.csc but commented out here. Trixie users building with the GRUB variant won't get Mesa Vulkan drivers installed. If this is intentional (e.g., known issue with GRUB + trixie), add a brief comment explaining why. Otherwise, uncomment it.
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-grub.csc` around lines 83 - 87, The trixie Mesa
driver block is commented out causing GRUB builds to skip installing Mesa Vulkan
drivers; either restore the block or document why it's intentionally disabled:
if no GRUB+trixie issue exists, uncomment the RELEASE-check block that calls
do_with_retries with chroot_sdcard_apt_get_update, display_alert "Installing
Mesa Vulkan Drivers", and do_with_retries chroot_sdcard_apt_get_install for
libgl1-mesa-dri mesa-vulkan-drivers vulkan-tools; if it is intentional, add a
brief comment above the RELEASE check explaining the GRUB+trixie incompatibility
and reference this behavior so maintainers know it’s deliberate.
| ( | ||
| cd "${SDCARD}/usr/share/alsa" || exit 6 | ||
| curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" | ||
| unzip -o temp.zip | ||
| unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1) | ||
| cp -rf "${unzip_dir}/"* . | ||
| rm -rf "$unzip_dir" temp.zip | ||
| ) |
There was a problem hiding this comment.
Add --fail to curl to detect HTTP errors.
Without --fail, a 404 or other HTTP error silently saves the error page as temp.zip, and the subsequent unzip would fail with a confusing error. Also consider pinning to a commit hash for reproducible builds.
🛠️ Suggested fix
- curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip"
+ curl -fL -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip"📝 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.
| ( | |
| cd "${SDCARD}/usr/share/alsa" || exit 6 | |
| curl -L -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" | |
| unzip -o temp.zip | |
| unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1) | |
| cp -rf "${unzip_dir}/"* . | |
| rm -rf "$unzip_dir" temp.zip | |
| ) | |
| ( | |
| cd "${SDCARD}/usr/share/alsa" || exit 6 | |
| curl -fL -o temp.zip "https://github.com/AYNTechnologies/alsa-ucm-conf/archive/refs/heads/ayn/v1.2.13.zip" | |
| unzip -o temp.zip | |
| unzip_dir=$(unzip -Z1 temp.zip | head -n1 | cut -d/ -f1) | |
| cp -rf "${unzip_dir}/"* . | |
| rm -rf "$unzip_dir" temp.zip | |
| ) |
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal-grub.csc` around lines 38 - 45, The curl
invocation that writes "temp.zip" should include --fail (e.g. curl -L --fail -o
temp.zip) and check its exit status before running unzip so HTTP errors
(404/etc.) don't produce invalid zip files; after the curl command, if it fails,
exit with a non-zero code and avoid calling unzip on "temp.zip" (also remove any
partial temp.zip). Additionally, replace the branch zip URL with a pinned commit
hash or tag instead of the branch path ("ayn/v1.2.13.zip") to ensure
reproducible builds; update the code that references temp.zip and unzip_dir
accordingly.
| mkdir -p $destination/usr/local/bin/ | ||
| mkdir -p $destination/usr/lib/systemd/system/ | ||
| mkdir -p $destination/etc/initramfs-tools/scripts/init-bottom/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ |
There was a problem hiding this comment.
Permission mode 655 is almost certainly a typo for 755.
Mode 655 (rw-r-xr-x) removes the owner execute bit, which is unconventional for scripts. While root can still execute files with any execute bit set, this is clearly unintentional and would break execution for non-root owners. Also, $destination and $SRC should be quoted.
🛠️ Suggested fix
- mkdir -p $destination/usr/local/bin/
- mkdir -p $destination/usr/lib/systemd/system/
- mkdir -p $destination/etc/initramfs-tools/scripts/init-bottom/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/
- install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/
+ mkdir -p "${destination}/usr/local/bin/"
+ mkdir -p "${destination}/usr/lib/systemd/system/"
+ mkdir -p "${destination}/etc/initramfs-tools/scripts/init-bottom/"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh" "${destination}/usr/local/bin/"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh" "${destination}/usr/local/bin/"
+ install -Dm644 "${SRC}/packages/bsp/usb-gadget-network/usbgadget-rndis.service" "${destination}/usr/lib/systemd/system/"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook" "${destination}/etc/initramfs-tools/hooks/usb-gadget"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount" "${destination}/etc/initramfs-tools/scripts/init-premount/usb-gadget"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/dropbear" "${destination}/etc/initramfs-tools/scripts/init-premount/"
+ install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/kill-dropbear" "${destination}/etc/initramfs-tools/scripts/init-bottom/"📝 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.
| mkdir -p $destination/usr/local/bin/ | |
| mkdir -p $destination/usr/lib/systemd/system/ | |
| mkdir -p $destination/etc/initramfs-tools/scripts/init-bottom/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | |
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | |
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ | |
| mkdir -p "${destination}/usr/local/bin/" | |
| mkdir -p "${destination}/usr/lib/systemd/system/" | |
| mkdir -p "${destination}/etc/initramfs-tools/scripts/init-bottom/" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/setup-usbgadget-network.sh" "${destination}/usr/local/bin/" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh" "${destination}/usr/local/bin/" | |
| install -Dm644 "${SRC}/packages/bsp/usb-gadget-network/usbgadget-rndis.service" "${destination}/usr/lib/systemd/system/" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook" "${destination}/etc/initramfs-tools/hooks/usb-gadget" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount" "${destination}/etc/initramfs-tools/scripts/init-premount/usb-gadget" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/dropbear" "${destination}/etc/initramfs-tools/scripts/init-premount/" | |
| install -Dm755 "${SRC}/packages/bsp/usb-gadget-network/kill-dropbear" "${destination}/etc/initramfs-tools/scripts/init-bottom/" |
🤖 Prompt for AI Agents
In `@config/boards/ayn-odin2-portal-grub.csc` around lines 52 - 61, Fix the
incorrect file mode and unquoted variables: change all install mode arguments
from 655 to 755 for the script/binary installs (the lines installing
setup-usbgadget-network.sh, remove-usbgadget-network.sh,
usb-gadget-initramfs-hook, usb-gadget-initramfs-premount, dropbear,
kill-dropbear and any other script installs) so owner execute bit is set, and
quote $destination and $SRC in the mkdir and install commands (e.g., use
"$destination" and "$SRC") to prevent word-splitting/expansion issues; keep the
same target paths and modes otherwise.
| @@ -0,0 +1,975 @@ | |||
| # Armbian defconfig generated with 6.18 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check existing kernel config naming patterns
echo "=== Kernel config files in config/kernel/ ==="
fd -t f '\.config$' config/kernel/ | sort
echo ""
echo "=== Search for references to 'old-linux-' pattern ==="
rg 'old-linux-' config/ lib/ --type=sh 2>/dev/null | head -20
echo ""
echo "=== Check build system logic for kernel config lookup ==="
rg -A 5 'KERNEL_TARGET\|kernel.*config' lib/ --type=sh | grep -A 5 'old-linux\|linux-.*edge' | head -30Repository: armbian/build
Length of output: 4024
Remove or rename old-linux-sm8550-edge.config to match standard naming convention.
The file deviates from Armbian's kernel config naming pattern linux-{LINUXFAMILY}-{BRANCH}.config. With the old- prefix, the build system will not recognize or use this file—the standard linux-sm8550-edge.config is the expected file. This appears to be orphaned legacy code that should either be deleted or renamed to linux-sm8550-edge.config.
🤖 Prompt for AI Agents
In `@config/kernel/old-linux-sm8550-edge.config` at line 1, The config file named
old-linux-sm8550-edge.config does not follow the Armbian naming convention and
will be ignored; either delete old-linux-sm8550-edge.config if it's obsolete, or
rename it to linux-sm8550-edge.config so the build system recognizes it, and
ensure there are no duplicate linux-sm8550-edge.config files (update any
references to the old name if present).
| CONFIG_SECURITY=y | ||
| CONFIG_SECURITY_APPARMOR=y | ||
| CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,bpf" |
There was a problem hiding this comment.
AppArmor enabled but missing from LSM list.
CONFIG_SECURITY_APPARMOR=y is set on Line 935, but apparmor is not included in the CONFIG_LSM string on Line 936. This means AppArmor won't be active by default unless explicitly added via boot parameter. If AppArmor support is intended, add it to the LSM list.
Suggested fix
-CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,bpf"
+CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,apparmor,bpf"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CONFIG_SECURITY=y | |
| CONFIG_SECURITY_APPARMOR=y | |
| CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,bpf" | |
| CONFIG_SECURITY=y | |
| CONFIG_SECURITY_APPARMOR=y | |
| CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,apparmor,bpf" |
🤖 Prompt for AI Agents
In `@config/kernel/old-linux-sm8550-edge.config` around lines 934 - 936, The
config enables AppArmor via CONFIG_SECURITY_APPARMOR=y but does not include
"apparmor" in the CONFIG_LSM string; update CONFIG_LSM to include apparmor
(e.g., add apparmor into the comma-separated list used by CONFIG_LSM) so the LSM
list contains "apparmor" and keep CONFIG_SECURITY_APPARMOR=y in sync; ensure you
don't duplicate entries and preserve the existing order/commas in the CONFIG_LSM
value.
| + | ||
| +change_state: | ||
| + inst->state = request_state; | ||
| + dev_dbg(inst->core->dev, "state changed from %x to %x\n", | ||
| + inst->state, request_state); | ||
| + | ||
| + return 0; |
There was a problem hiding this comment.
Debug log prints the same value for old and new state.
inst->state is overwritten on line 650 before the dev_dbg on lines 651–652 reads it, so both %x placeholders will print request_state. The original state should be captured before the assignment.
🐛 Proposed fix
+ enum iris_inst_state old_state = inst->state;
+
change_state:
inst->state = request_state;
- dev_dbg(inst->core->dev, "state changed from %x to %x\n",
- inst->state, request_state);
+ dev_dbg(inst->core->dev, "state changed from %x to %x\n",
+ old_state, request_state);📝 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.
| + | |
| +change_state: | |
| + inst->state = request_state; | |
| + dev_dbg(inst->core->dev, "state changed from %x to %x\n", | |
| + inst->state, request_state); | |
| + | |
| + return 0; | |
| enum iris_inst_state old_state = inst->state; | |
| change_state: | |
| inst->state = request_state; | |
| dev_dbg(inst->core->dev, "state changed from %x to %x\n", | |
| old_state, request_state); | |
| return 0; |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0016_media--iris--implement-vb2-streaming-ops.patch`
around lines 648 - 654, The debug log prints the same value because inst->state
is assigned before logging; in the change_state block capture the previous state
into a temporary (e.g., old_state) before assigning inst->state = request_state,
then call dev_dbg(inst->core->dev, "state changed from %x to %x\n", old_state,
request_state); reference the change_state label and the
inst->state/request_state variables and update the dev_dbg argument order to use
the saved old_state.
| + config_params = core->iris_platform_data->input_config_params; | ||
| + config_params_size = core->iris_platform_data->input_config_params_size; | ||
| + | ||
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | ||
| + for (i = 0; i < config_params_size; i++) { | ||
| + for (j = 0; j < ARRAY_SIZE(prop_type_handle_inp_arr); j++) { | ||
| + if (prop_type_handle_inp_arr[j].type == config_params[i]) { | ||
| + ret = prop_type_handle_inp_arr[j].handle(inst); | ||
| + if (ret) | ||
| + return ret; | ||
| + break; | ||
| + } | ||
| + } | ||
| + } | ||
| + } else if (V4L2_TYPE_IS_CAPTURE(plane)) { | ||
| + for (i = 0; i < config_params_size; i++) { | ||
| + for (j = 0; j < ARRAY_SIZE(prop_type_handle_out_arr); j++) { | ||
| + if (prop_type_handle_out_arr[j].type == config_params[i]) { | ||
| + ret = prop_type_handle_out_arr[j].handle(inst); | ||
| + if (ret) | ||
| + return ret; | ||
| + break; | ||
| + } | ||
| + } | ||
| + } | ||
| + } |
There was a problem hiding this comment.
Gen1 always uses input_config_params for both planes — capture properties are silently skipped.
Lines 798–799 unconditionally load input_config_params / input_config_params_size, so the V4L2_TYPE_IS_CAPTURE branch iterates over input property IDs against the output handler array. Since they won't match, capture-side properties are never programmed on Gen1.
Compare with the Gen2 implementation (lines 1265–1271) which correctly selects between input_config_params and output_config_params based on plane.
🐛 Proposed fix
static int iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 plane)
{
struct iris_core *core = inst->core;
u32 config_params_size, i, j;
const u32 *config_params;
int ret;
/* ... handler arrays ... */
- config_params = core->iris_platform_data->input_config_params;
- config_params_size = core->iris_platform_data->input_config_params_size;
-
if (V4L2_TYPE_IS_OUTPUT(plane)) {
+ config_params = core->iris_platform_data->input_config_params;
+ config_params_size = core->iris_platform_data->input_config_params_size;
for (i = 0; i < config_params_size; i++) {
for (j = 0; j < ARRAY_SIZE(prop_type_handle_inp_arr); j++) {
if (prop_type_handle_inp_arr[j].type == config_params[i]) {
ret = prop_type_handle_inp_arr[j].handle(inst);
if (ret)
return ret;
break;
}
}
}
} else if (V4L2_TYPE_IS_CAPTURE(plane)) {
+ config_params = core->iris_platform_data->output_config_params;
+ config_params_size = core->iris_platform_data->output_config_params_size;
for (i = 0; i < config_params_size; i++) {
for (j = 0; j < ARRAY_SIZE(prop_type_handle_out_arr); j++) {
if (prop_type_handle_out_arr[j].type == config_params[i]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + config_params = core->iris_platform_data->input_config_params; | |
| + config_params_size = core->iris_platform_data->input_config_params_size; | |
| + | |
| + if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| + for (i = 0; i < config_params_size; i++) { | |
| + for (j = 0; j < ARRAY_SIZE(prop_type_handle_inp_arr); j++) { | |
| + if (prop_type_handle_inp_arr[j].type == config_params[i]) { | |
| + ret = prop_type_handle_inp_arr[j].handle(inst); | |
| + if (ret) | |
| + return ret; | |
| + break; | |
| + } | |
| + } | |
| + } | |
| + } else if (V4L2_TYPE_IS_CAPTURE(plane)) { | |
| + for (i = 0; i < config_params_size; i++) { | |
| + for (j = 0; j < ARRAY_SIZE(prop_type_handle_out_arr); j++) { | |
| + if (prop_type_handle_out_arr[j].type == config_params[i]) { | |
| + ret = prop_type_handle_out_arr[j].handle(inst); | |
| + if (ret) | |
| + return ret; | |
| + break; | |
| + } | |
| + } | |
| + } | |
| + } | |
| static int iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 plane) | |
| { | |
| struct iris_core *core = inst->core; | |
| u32 config_params_size, i, j; | |
| const u32 *config_params; | |
| int ret; | |
| /* ... handler arrays ... */ | |
| if (V4L2_TYPE_IS_OUTPUT(plane)) { | |
| config_params = core->iris_platform_data->input_config_params; | |
| config_params_size = core->iris_platform_data->input_config_params_size; | |
| for (i = 0; i < config_params_size; i++) { | |
| for (j = 0; j < ARRAY_SIZE(prop_type_handle_inp_arr); j++) { | |
| if (prop_type_handle_inp_arr[j].type == config_params[i]) { | |
| ret = prop_type_handle_inp_arr[j].handle(inst); | |
| if (ret) | |
| return ret; | |
| break; | |
| } | |
| } | |
| } | |
| } else if (V4L2_TYPE_IS_CAPTURE(plane)) { | |
| config_params = core->iris_platform_data->output_config_params; | |
| config_params_size = core->iris_platform_data->output_config_params_size; | |
| for (i = 0; i < config_params_size; i++) { | |
| for (j = 0; j < ARRAY_SIZE(prop_type_handle_out_arr); j++) { | |
| if (prop_type_handle_out_arr[j].type == config_params[i]) { | |
| ret = prop_type_handle_out_arr[j].handle(inst); | |
| if (ret) | |
| return ret; | |
| break; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch`
around lines 798 - 823, The code always loads
core->iris_platform_data->input_config_params into config_params and
config_params_size so the V4L2_TYPE_IS_CAPTURE branch compares capture plane
property IDs against prop_type_handle_out_arr using the input list (silently
skipping capture properties); fix by selecting the correct platform array based
on plane before the loops—i.e., set config_params/config_params_size =
core->iris_platform_data->input_config_params/input_config_params_size when
V4L2_TYPE_IS_OUTPUT(plane) and set them =
core->iris_platform_data->output_config_params/output_config_params_size when
V4L2_TYPE_IS_CAPTURE(plane), then keep the existing loops that match
config_params[i] against prop_type_handle_inp_arr or prop_type_handle_out_arr
and call their .handle(inst).
| + sizeof(u64)); | ||
| +} | ||
| + | ||
| +static int iris_hfi_gen2_set_bit_dpeth(struct iris_inst *inst) |
There was a problem hiding this comment.
Typo in function name: iris_hfi_gen2_set_bit_dpeth → iris_hfi_gen2_set_bit_depth.
This also propagates to the handler table reference at line 1255.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0017_media--iris--implement-set-properties-to-firmware-.patch`
at line 1055, Rename the misspelled function iris_hfi_gen2_set_bit_dpeth to
iris_hfi_gen2_set_bit_depth and update all references (including the entry in
the handler table that currently points to iris_hfi_gen2_set_bit_dpeth) to use
the corrected symbol; ensure the function prototype, definition, and any forward
declarations match the new name so linkage and the handler table resolve
correctly.
| + | ||
| + if (iris_split_mode_enabled(inst) && pkt->stream_id == 0) { | ||
| + buffers = &inst->buffers[BUF_DPB]; | ||
| + if (!buffers) | ||
| + goto error; | ||
| + | ||
| + found = false; | ||
| + list_for_each_entry(iter, &buffers->list, list) { | ||
| + if (!(iter->attr & BUF_ATTR_QUEUED)) | ||
| + continue; | ||
| + | ||
| + found = (iter->index == output_tag && | ||
| + iter->data_offset == offset); | ||
| + | ||
| + if (found) { | ||
| + buf = iter; | ||
| + break; | ||
| + } | ||
| + } |
There was a problem hiding this comment.
Dead null-check: buffers is an embedded array element and can never be NULL.
&inst->buffers[BUF_DPB] points into a struct-embedded array; it's never NULL. The check on line 491 is dead code.
Proposed fix
if (iris_split_mode_enabled(inst) && pkt->stream_id == 0) {
buffers = &inst->buffers[BUF_DPB];
- if (!buffers)
- goto error;
-
found = false;🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0020_media--iris--implement-vb2-ops-for-buf_queue-and-f.patch`
around lines 488 - 506, The null-check on buffers is dead because buffers is
assigned from &inst->buffers[BUF_DPB], an embedded array element that cannot be
NULL; remove the if (!buffers) goto error; branch and any associated error label
reliance, and simply iterate/list_for_each_entry over
inst->buffers[BUF_DPB].list after assigning buffers = &inst->buffers[BUF_DPB];
(keep the surrounding logic: iris_split_mode_enabled(inst), pkt->stream_id == 0,
found detection comparing iter->index to output_tag and iter->data_offset to
offset, and setting buf = iter when found).
| + priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(priv->sdb)) | ||
| + return PTR_ERR(priv->sdb); | ||
| + if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) | ||
| + return -EPROBE_DEFER; |
There was a problem hiding this comment.
Broken GPIO error handling — missing IS_ERR check, silent error swallowing.
devm_gpiod_get can return any error, but this code only checks for -EPROBE_DEFER without first verifying IS_ERR(priv->sdb). If the call returns a different error (e.g., -EINVAL, -ENOENT), the invalid error pointer is stored and later dereferenced via gpiod_set_value_cansleep at line 297, causing a kernel crash. If it succeeds, PTR_ERR on a valid pointer is undefined.
Proposed fix
priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH);
- if (PTR_ERR(priv->sdb) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
+ if (IS_ERR(priv->sdb))
+ return dev_err_probe(dev, PTR_ERR(priv->sdb),
+ "Failed to get sdb gpio\n");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | |
| + if (IS_ERR(priv->sdb)) | |
| + return PTR_ERR(priv->sdb); | |
| + if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) | |
| + return -EPROBE_DEFER; | |
| priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | |
| if (IS_ERR(priv->sdb)) | |
| return dev_err_probe(dev, PTR_ERR(priv->sdb), | |
| "Failed to get sdb gpio\n"); |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0033_leds--Add-driver-for-HEROIC-HTR3212.patch`
around lines 282 - 284, The devm_gpiod_get call for priv->sdb must be checked
with IS_ERR before calling PTR_ERR; change the logic in the probe path where
devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) is assigned to first test if
(IS_ERR(priv->sdb)) then get the error via PTR_ERR(priv->sdb) and if that equals
-EPROBE_DEFER return -EPROBE_DEFER else return that error (or propagate
appropriately) so you never store an error pointer in priv->sdb that later gets
dereferenced by gpiod_set_value_cansleep.
5a3c1fe to
5168111
Compare
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
4 participants
Description
General Improvements to Support Ayn Odin2 Device
Changes:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
Used the following config:
Used the following config:
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
New Features
Documentation
Chores