Added config improvements and fixes to ayn odin2#9316
Added config improvements and fixes to ayn odin2#9316Squishy123 wants to merge 10 commits intoarmbian:mainfrom
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds two Ayn Odin2 GRUB board configs and presets; BSP audio/loader files; SM8550 kernel defconfig and a large sm8550-6.13 patch series implementing the Qualcomm Iris video accelerator (drivers, HFI, V4L2/VB2, firmware, power, buffers, session/state management); DT updates and several ancillary drivers, configs, 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.
| @@ -386,6 +386,7 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | ||
| struct hfi_msg_session_fbd_uncompressed_plane0_pkt *pkt = packet; | ||
| struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; | ||
| struct v4l2_m2m_buffer *m2m_buffer, *n; | ||
| + struct hfi_session_flush_pkt flush_pkt; | ||
| u32 timestamp_hi = pkt->time_stamp_hi; | ||
| u32 timestamp_lo = pkt->time_stamp_lo; | ||
| struct iris_core *core = inst->core; | ||
| @@ -394,11 +395,25 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | ||
| u32 output_tag = pkt->output_tag; | ||
| struct iris_buffer *buf, *iter; | ||
| struct iris_buffers *buffers; | ||
| + u32 hfi_flags = pkt->flags; | ||
| u32 offset = pkt->offset; | ||
| u64 timestamp_us = 0; | ||
| bool found = false; | ||
| u32 flags = 0; | ||
|
|
||
| + if ((hfi_flags & HFI_BUFFERFLAG_EOS) && !filled_len) { | ||
| + reinit_completion(&inst->flush_completion); | ||
| + | ||
| + flush_pkt.shdr.hdr.size = sizeof(struct hfi_session_flush_pkt); | ||
| + flush_pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_FLUSH; | ||
| + flush_pkt.shdr.session_id = inst->session_id; | ||
| + flush_pkt.flush_type = HFI_FLUSH_OUTPUT; | ||
| + iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size); | ||
| + iris_inst_sub_state_change_drain_last(inst); | ||
| + | ||
| + return; | ||
| + } | ||
| + | ||
| if (iris_split_mode_enabled(inst) && pkt->stream_id == 0) { |
There was a problem hiding this comment.
Initialize flush_pkt to avoid leaking garbage fields.
flush_pkt is stack-allocated and only partially assigned, so unspecified fields may carry garbage into the firmware command. Zero-init it like other packets.
🔧 Proposed fix
- struct hfi_session_flush_pkt flush_pkt;
+ struct hfi_session_flush_pkt flush_pkt = {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.
| @@ -386,6 +386,7 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | |
| struct hfi_msg_session_fbd_uncompressed_plane0_pkt *pkt = packet; | |
| struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; | |
| struct v4l2_m2m_buffer *m2m_buffer, *n; | |
| + struct hfi_session_flush_pkt flush_pkt; | |
| u32 timestamp_hi = pkt->time_stamp_hi; | |
| u32 timestamp_lo = pkt->time_stamp_lo; | |
| struct iris_core *core = inst->core; | |
| @@ -394,11 +395,25 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | |
| u32 output_tag = pkt->output_tag; | |
| struct iris_buffer *buf, *iter; | |
| struct iris_buffers *buffers; | |
| + u32 hfi_flags = pkt->flags; | |
| u32 offset = pkt->offset; | |
| u64 timestamp_us = 0; | |
| bool found = false; | |
| u32 flags = 0; | |
| + if ((hfi_flags & HFI_BUFFERFLAG_EOS) && !filled_len) { | |
| + reinit_completion(&inst->flush_completion); | |
| + | |
| + flush_pkt.shdr.hdr.size = sizeof(struct hfi_session_flush_pkt); | |
| + flush_pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_FLUSH; | |
| + flush_pkt.shdr.session_id = inst->session_id; | |
| + flush_pkt.flush_type = HFI_FLUSH_OUTPUT; | |
| + iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size); | |
| + iris_inst_sub_state_change_drain_last(inst); | |
| + | |
| + return; | |
| + } | |
| + | |
| if (iris_split_mode_enabled(inst) && pkt->stream_id == 0) { | |
| @@ -386,6 +386,7 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | |
| struct hfi_msg_session_fbd_uncompressed_plane0_pkt *pkt = packet; | |
| struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx; | |
| struct v4l2_m2m_buffer *m2m_buffer, *n; | |
| struct hfi_session_flush_pkt flush_pkt = {0}; | |
| u32 timestamp_hi = pkt->time_stamp_hi; | |
| u32 timestamp_lo = pkt->time_stamp_lo; | |
| struct iris_core *core = inst->core; | |
| @@ -394,11 +395,25 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet) | |
| u32 output_tag = pkt->output_tag; | |
| struct iris_buffer *buf, *iter; | |
| struct iris_buffers *buffers; | |
| u32 hfi_flags = pkt->flags; | |
| u32 offset = pkt->offset; | |
| u64 timestamp_us = 0; | |
| bool found = false; | |
| u32 flags = 0; | |
| if ((hfi_flags & HFI_BUFFERFLAG_EOS) && !filled_len) { | |
| reinit_completion(&inst->flush_completion); | |
| flush_pkt.shdr.hdr.size = sizeof(struct hfi_session_flush_pkt); | |
| flush_pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_FLUSH; | |
| flush_pkt.shdr.session_id = inst->session_id; | |
| flush_pkt.flush_type = HFI_FLUSH_OUTPUT; | |
| iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size); | |
| iris_inst_sub_state_change_drain_last(inst); | |
| return; | |
| } | |
| if (iris_split_mode_enabled(inst) && pkt->stream_id == 0) { |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.13/0023_media--iris--add-support-for-drain-sequence.patch`
around lines 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.
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