Add cix-p1 support#9854
Conversation
Squashed commit from PR armbian#9395. Includes: - Board vendor configuration - Current (6.18) and edge (6.19) kernel support based on Sky1-Linux - Firmware and mesa backports for GPU support - Various fixes and improvements
I assume CONFIG_CXL_ACPI=m was thrown out due to unmet dependencies
this needs rework
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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 (1)
config/kernel/linux-cix-p1-vendor.config (1)
1-1209: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRegenerate this as a minimal kernel overlay, not a full defconfig dump.
config/kernel/*.configin this repo should be minimal overlays; this full config will be hard to maintain and will drift. Please regenerate with./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch>sosavedefconfigtrims it appropriately.Based on learnings: kernel config files under
config/kernel/must be minimal overlays regenerated viarewrite-kernel-configandsavedefconfig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/kernel/linux-cix-p1-vendor.config` around lines 1 - 1209, Summary: This file is a full defconfig dump and must be replaced with a minimal kernel overlay. Fix: remove the large full defconfig contents in config/kernel/linux-cix-p1-vendor.config and regenerate a trimmed overlay using the provided helper: run ./compile.sh rewrite-kernel-config BOARD=<board> BRANCH=<branch> (which uses savedefconfig) to produce a minimal config overlay; commit the regenerated minimal file in place of the current full dump. Reference: target file config/kernel/linux-cix-p1-vendor.config and the helper script ./compile.sh rewrite-kernel-config (savedefconfig).
🟠 Major comments (27)
config/sources/families/cix-p1.conf-123-127 (1)
123-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin and verify firmware downloads before adding them to the image.
These
curlcalls fetch binary firmware from mutable branch refs, which is non-reproducible and weakens supply-chain integrity. Please switch to pinned revisions and verify hashes/signatures before install.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/sources/families/cix-p1.conf` around lines 123 - 127, Replace the mutable-branch curl downloads for dsp_fw.bin and mali_csffw.bin with pinned-commit URLs (use the specific commit SHA in the GitHub raw URLs) and add verification steps: download to a temporary location, compute and check a known-good SHA256 (or verify a detached GPG signature if available) before moving to "${SDCARD}/lib/firmware/..." and only proceed after successful verification; update the install sequence around the existing curl lines (references: dsp_fw.bin, mali_csffw.bin, mkdir -p "${SDCARD}"/lib/firmware/arm/mali/arch12.8 and display_alert "$BOARD" "Installing Mali firmware") so the image includes only verified, pinned firmware artifacts.patch/kernel/archive/cix-6.18/0001-mailbox-add-acpi-support-to-cix-mailbox-driver.patch-1-9 (1)
1-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
Signed-off-bytrailer.The 7.0 copy includes the DCO/provenance trailer, but this 6.18 patch header does not. Keeping the archived patch metadata consistent matters for traceability and future rebases of the patch stack.
Based on learnings,
patch/kernel/archive/patches are normally treated as upstream artifacts only when the usual format-patch provenance markers are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0001-mailbox-add-acpi-support-to-cix-mailbox-driver.patch` around lines 1 - 9, The patch header for "mailbox: add acpi support to cix-mailbox driver" authored by Jianfeng Liu is missing the Signed-off-by trailer; update the patch header in the patch/kernel/archive/cix-6.18/0001... patch to append a proper "Signed-off-by: Your Name <email@domain>" (or the committer's correct DCO signature) line after the existing Subject/Date/From lines so the patch includes the standard provenance trailer used across archive patches; ensure the trailer format matches the other archived patches (Signed-off-by: Name <email>) and save the patch.patch/kernel/archive/cix-6.18/2004-acpi-add-backward-complibility-to-old-firmware-with-.patch-86-96 (1)
86-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the
_STAoverride to the broken namespace only.This currently forces
ACPI_STA_DEFAULTfor everyCIXHA008/CIXHA009device with UID0. The comment above says the bug is limited to old firmware children underCIXHA006, so this should also verify the parent and only override when the firmware-reported*statusis actually0; otherwise newer firmware/SKUs can get devices unintentionally enumerated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/2004-acpi-add-backward-complibility-to-old-firmware-with-.patch` around lines 86 - 96, The override in acpi_sta_override_firmware_quirk is too broad: narrow it so it only applies to the broken namespace and only when firmware actually reports zero status. Modify the function to additionally verify the device's parent matches the old-broken chassis ID (check the parent ACPI device with acpi_match_device_ids for the CIXHA006 parent), and only set *status = ACPI_STA_DEFAULT and return true if the incoming *status is 0; otherwise return false. Keep the existing checks (cix_scmi_proto_sta_ids and acpi_dev_uid_match) and add the parent and *status==0 checks before performing the override.patch/kernel/archive/cix-6.18/0023-syscon-add-device_syscon_regmap_lookup_by_property.patch-78-83 (1)
78-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStub function missing
static inlinequalifier.The stub function in the header file will cause multiple definition linker errors when included in multiple compilation units. It needs
static inline.Proposed fix
-struct regmap *device_syscon_regmap_lookup_by_property(struct device *dev, +static inline struct regmap *device_syscon_regmap_lookup_by_property(struct device *dev, const char *property) { return ERR_PTR(-ENOTSUPP); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0023-syscon-add-device_syscon_regmap_lookup_by_property.patch` around lines 78 - 83, The stub function device_syscon_regmap_lookup_by_property is missing the static inline qualifier and will cause multiple-definition linker errors when that header is included in multiple C files; change its declaration to "static inline struct regmap *device_syscon_regmap_lookup_by_property(struct device *dev, const char *property)" (keeping the current ERR_PTR(-ENOTSUPP) return) so the function is inlined per-translation-unit and no duplicate symbols are emitted.patch/kernel/archive/cix-6.18/0023-syscon-add-device_syscon_regmap_lookup_by_property.patch-28-48 (1)
28-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
fwnode_handle_put()causes reference leak.
fwnode_find_reference()increments the reference count on the returned fwnode handle. The function must callfwnode_handle_put(fwnode)after use to avoid leaking the reference. Additionally,fwnode->devmay be NULL if the device hasn't been bound yet.Proposed fix
struct regmap *device_syscon_regmap_lookup_by_property(struct device *dev, const char *property) { struct fwnode_handle *fwnode; struct platform_device *pdev; struct syscon *syscon; fwnode = fwnode_find_reference(dev_fwnode(dev), property, 0); if (IS_ERR_OR_NULL(fwnode)) return ERR_PTR(-ENODEV); + if (!fwnode->dev) { + fwnode_handle_put(fwnode); + return ERR_PTR(-EPROBE_DEFER); + } + pdev = to_platform_device(fwnode->dev); - if (!pdev->dev.driver_data) + fwnode_handle_put(fwnode); + + if (!pdev->dev.driver_data) return ERR_PTR(-ENOENT); syscon = platform_get_drvdata(pdev); if (!syscon) return ERR_PTR(-ENODEV); return syscon->regmap; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0023-syscon-add-device_syscon_regmap_lookup_by_property.patch` around lines 28 - 48, The function device_syscon_regmap_lookup_by_property leaks the fwnode reference returned by fwnode_find_reference and dereferences fwnode->dev unsafely; after calling fwnode_find_reference in device_syscon_regmap_lookup_by_property you must ensure fwnode_handle_put(fwnode) is called on all return paths (including error paths) and check that fwnode->dev is non-NULL before using to_platform_device(fwnode->dev); adjust the control flow so you call to_platform_device only when fwnode->dev is valid, store/inspect platform data, then put the fwnode via fwnode_handle_put before returning the regmap or an ERR_PTR.patch/kernel/archive/cix-6.18/0012-irqchip-add-cix-sky1-pdc-driver.patch-268-282 (1)
268-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncorrect error handling for
devm_ioremap_resource.
devm_ioremap_resource()returnsERR_PTR()on failure, notNULL. The current checkif (!cd->pdc_base)won't catch errors. Additionally, callingiounmap()on a devm-managed resource is incorrect—devm handles cleanup automatically.Proposed fix
raw_spin_lock_init(&cd->rlock); struct resource *res_dp = platform_get_resource(pdev, IORESOURCE_MEM, 0); cd->pdc_base = devm_ioremap_resource(&pdev->dev, res_dp); - if (!cd->pdc_base) { + if (IS_ERR(cd->pdc_base)) { pr_err("%pACPI: unable to map pdc registers\n", pdev); kfree(cd); - return -ENOMEM; + return PTR_ERR(cd->pdc_base); } domain = acpi_irq_create_hierarchy(0, PDC_MAX_IRQS, pdev->dev.fwnode, &pdcv1_irqchip_data_domain_ops, cd); if (!domain) { - iounmap(cd->pdc_base); + /* devm_ioremap_resource is device-managed, no manual unmap needed */ kfree(cd); return -ENOMEM; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0012-irqchip-add-cix-sky1-pdc-driver.patch` around lines 268 - 282, devm_ioremap_resource returns ERR_PTR on failure and the dev-managed mapping must not be iounmap'd manually; change the pdc register mapping check to use IS_ERR(cd->pdc_base), capture the error with PTR_ERR(cd->pdc_base) and return that (after logging with dev_err or pr_err) instead of checking for NULL, and when acpi_irq_create_hierarchy(...) (domain) fails do not call iounmap(cd->pdc_base) — just free cd (kfree(cd)) and return the appropriate error code; update handling around devm_ioremap_resource, cd->pdc_base, acpi_irq_create_hierarchy, PTR_ERR/IS_ERR, and remove the iounmap call.patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch-1276-1285 (1)
1276-1285:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemoving vring zeroing from generic remoteproc_virtio.c affects all remoteproc drivers.
This patch removes the
memset(addr, 0, size)call from therp_find_vq()function indrivers/remoteproc/remoteproc_virtio.c, which is the shared virtio discovery code used by ALL remoteproc drivers. The change is not conditional or CIX-specific—it applies globally.While the comment explains the rationale for rpmsg-lite firmware (which pre-initializes vrings before the kernel), this is a breaking change for any firmware that expects vrings to be zeroed on startup. Consider adding a driver option (via resource table, device tree, or driver ops) to allow specific drivers to opt out of vring zeroing rather than disabling it unconditionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch` around lines 1276 - 1285, The patch removed the unconditional vring zeroing in rp_find_vq() (removal of memset(addr, 0, size)), which changes behavior for all remoteproc drivers; restore zeroing by default and make skipping it opt-in: add a per-driver flag (e.g., bool vring_preinitialized) to the remoteproc/virtio driver ops or remoteproc resource parsing, check that flag in rp_find_vq() and only omit the memset(addr, 0, size) when vring_preinitialized is true (leave default behavior to call memset), update the CIX driver to set vring_preinitialized for rpmsg-lite firmware so only that driver skips zeroing.patch/kernel/archive/cix-6.18/0013-sound-hda-add-cix-ipbloq-hda-driver.patch-617-660 (1)
617-660:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAsync probe silently swallows errors without cleanup.
cix_ipbloq_hda_probe_workreturns toout_freeon errors but the label only doespm_runtime_put. The sound card allocated incix_ipbloq_hda_probeis never freed on async probe failure, leaving a zombie card. Consider callingsnd_card_freeon error paths or marking the device as failed.Suggested approach
Add error handling to free the card on failure:
out_free: pm_runtime_put(hda->dev); - return; /* no error return from async probe */ + if (err < 0) { + dev_err(hda->dev, "async probe failed: %d\n", err); + snd_card_free(chip->card); + } }Note: This requires tracking
errproperly through all failure paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0013-sound-hda-add-cix-ipbloq-hda-driver.patch` around lines 617 - 660, The async probe abandons the allocated sound card on error: in cix_ipbloq_hda_probe_work all error paths jump to out_free which only does pm_runtime_put, leaving chip->card allocated by the original probe; fix by freeing/unregistering the card on failure (check chip->card and call snd_card_free(chip->card) or snd_card_unregister where appropriate) before returning, clear to_hda_bus(bus)->bus_probing if set, and ensure chip->card is nulled after freeing so subsequent cleanup is safe; apply this cleanup for failures returned from cix_ipbloq_hda_init, azx_probe_codecs, azx_codec_configure and snd_card_register.patch/kernel/archive/cix-6.18/0020-firmware-add-cix-dsp-ipc-driver.patch-115-132 (1)
115-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential NULL pointer dereference in rx_callback.
If
chan->ipc->opsis NULL whencix_dsp_rx_callbackis invoked, dereferencingops->handle_replyorops->handle_requestwill crash the kernel. Theopspointer is assigned by external code (e.g.,sky1_probein the SOF driver) aftercix_dsp_request_mbox, so there's a window where callbacks could fire before ops is set.Proposed fix
static void cix_dsp_rx_callback(struct mbox_client *cl, void *msg) { struct cix_dsp_chan *chan = container_of(cl, struct cix_dsp_chan, cl); struct device *dev = cl->dev; + if (!chan->ipc->ops) { + dev_err(dev, "rx callback before ops registered\n"); + return; + } + switch (chan->idx) { case CIX_DSP_MBOX_REPLY: chan->ipc->ops->handle_reply(chan->ipc);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0020-firmware-add-cix-dsp-ipc-driver.patch` around lines 115 - 132, cix_dsp_rx_callback may dereference a NULL ops pointer; before calling chan->ipc->ops->handle_reply or ->handle_request in cix_dsp_rx_callback, validate chan, chan->ipc and chan->ipc->ops and verify the specific handler function pointer (handle_reply or handle_request) is non-NULL; if missing, log an error via dev_err with chan->idx and, for the CIX_DSP_MBOX_REPLY case, still call mbox_client_txdone(chan->ch, -EINVAL) (or an appropriate error code) to ack/fail the message to avoid a hang, then return without calling the handler. Ensure checks reference cix_dsp_rx_callback, struct cix_dsp_chan, chan->ipc, chan->ipc->ops, handle_reply, and handle_request so the guard logic is applied in the correct switch branches.patch/kernel/archive/cix-6.18/0014-kernel-dma-Export-dma_declare_coherent_memory-for-mo.patch-14-28 (1)
14-28:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe patch's comment misrepresents memremap's actual behavior; this needs clarification.
The patch changes
MEMREMAP_WCtoMEMREMAP_WC | MEMREMAP_WB, but the added comment claims it will "try write-combining first" and "fall back to write-back." In reality,memremapdoes not implement a fallback mechanism. When multiple flags are provided, the kernel evaluates them in a fixed priority order: WB is attempted first, then WT, then WC. If WB succeeds, WC is never tried—this is not a fallback, it's a priority hierarchy.This change therefore reverses the preference from WC-only to WB-primary. While the comment suggests this enables DMA on firmware-reserved System RAM, it actually makes write-back mapping the preferred type across all platforms. On architectures without hardware cache coherency, preferring WB for DMA-coherent memory could cause issues.
The patch's own comment should be corrected to accurately describe this behavior change, and the architectural implications should be validated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0014-kernel-dma-Export-dma_declare_coherent_memory-for-mo.patch` around lines 14 - 28, The comment in dma_init_coherent_memory misstates memremap behavior: memremap does not "try WC first and fall back to WB"—when you pass MEMREMAP_WC | MEMREMAP_WB the kernel picks mapping types by priority (WB then WT then WC), so WB becomes preferred; update the comment to accurately state this priority semantics for memremap and note the architectural implication that combining MEMREMAP_WB makes WB preferred on all platforms, which may be unsafe on architectures without hardware cache coherency; to actually implement the intended behavior, change the code to choose flags conditionally (e.g., inspect whether the region is System RAM or use an arch_has_cache_coherency/arch_dma_coherent check) instead of passing both MEMREMAP_WC and MEMREMAP_WB blindly, and update the comment to reference dma_init_coherent_memory and memremap and explain the chosen flag selection policy.patch/kernel/archive/cix-6.18/0003-firmware-arm_scmi-add-acpi-support-to-SCMI.patch-647-656 (1)
647-656:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame incorrect error check pattern for
fwnode_find_reference.The
smc_chan_availablefunction checksif (!np)butfwnode_find_referencereturnsERR_PTRon error.Proposed fix
static bool smc_chan_available(struct fwnode_handle *fwnode, int idx) { struct fwnode_handle *np __free(fwnode_handle) = fwnode_find_reference(fwnode, "shmem", 0); - if (!np) + if (IS_ERR_OR_NULL(np)) return false; return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0003-firmware-arm_scmi-add-acpi-support-to-SCMI.patch` around lines 647 - 656, The current check in smc_chan_available still treats the result of fwnode_find_reference as a NULL pointer; fwnode_find_reference returns ERR_PTR on error, so replace the conditional "if (!np)" with an error-pointer check like "if (IS_ERR(np)) return false;" (and keep a separate "if (!np) return false;" only if you want to treat missing phandle differently); update the check that references the local fwnode_handle *np returned by fwnode_find_reference to use IS_ERR(np) and ensure proper cleanup via the existing __free(fwnode_handle) attribute on np.patch/kernel/archive/cix-6.18/0003-firmware-arm_scmi-add-acpi-support-to-SCMI.patch-569-583 (1)
569-583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
IS_ERR_OR_NULL()to properly checkfwnode_find_reference()errors.
fwnode_find_reference()returnsERR_PTR()on failure, notNULL. The check!np_tx || !np_rxwill never catch errors sinceERR_PTR(-ENOENT)is non-NULL. This silently allows error pointers to be used as valid references.Proposed fix
if (num_sh > 1) { struct fwnode_handle *np_tx __free(fwnode_handle) = fwnode_find_reference(fwnode, "shmem", 0); struct fwnode_handle *np_rx __free(fwnode_handle) = fwnode_find_reference(fwnode, "shmem", 1); - if (!np_tx || !np_rx || np_tx == np_rx) { + if (IS_ERR_OR_NULL(np_tx) || IS_ERR_OR_NULL(np_rx) || np_tx == np_rx) { dev_warn(cdev, "Invalid shmem descriptor for '%s'\n", fwnode_get_name(fwnode)); ret = -EINVAL; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0003-firmware-arm_scmi-add-acpi-support-to-SCMI.patch` around lines 569 - 583, Replace the current NULL checks on np_tx/np_rx with IS_ERR_OR_NULL(np_tx) and IS_ERR_OR_NULL(np_rx) after the fwnode_find_reference calls in the block handling num_sh > 1; if either is an error pointer use PTR_ERR(np_tx) / PTR_ERR(np_rx) to set ret (e.g., ret = PTR_ERR(...) or -EINVAL for equality), and update the dev_warn to log the fwnode name via fwnode_get_name(fwnode) but include the specific error code when an ERR_PTR was returned; references: fwnode_find_reference, IS_ERR_OR_NULL, PTR_ERR, fwnode_get_name, dev_warn.patch/kernel/archive/cix-7.0/0005-clk-add-cix-clk-driver.patch-400-406 (1)
400-406:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffStatic save array breaks multi-instance support.
Same issue as the 6.18 version -
sky1_audss_reg_saveshould be moved to per-device private data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-7.0/0005-clk-add-cix-clk-driver.patch` around lines 400 - 406, The global static sky1_audss_reg_save array breaks multi-instance support; move it into the device private data (e.g., add a member like reg_save[][] in the driver private struct such as sky1_audss_priv / cix_clk_priv), allocate/initialize it in probe using ARRAY_SIZE of the original list (or store the register list elsewhere as a const table and copy into priv->reg_save), update all uses (save/restore functions and any references to INFO_HIFI0, INFO_CLK_GATE, INFO_CLK_DIV, INFO_CLK_MUX, INFO_MCLK) to access priv->reg_save instead of the static, and remove the static definition to ensure each device instance has its own save array.patch/kernel/archive/cix-6.18/0006-reset-add-cix-reset-driver.patch-205-227 (1)
205-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatic lookup array modified at runtime is not safe.
Same issue as the 7.0 version - the
lookupsarray should be dynamically allocated to avoid data corruption on driver rebind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0006-reset-add-cix-reset-driver.patch` around lines 205 - 227, The static lookups[] is unsafe because it’s modified at runtime; replace the static array with a dynamically allocated array (e.g., kcalloc or devm_kcalloc) sized ARRAY_SIZE(lookups_template) inside the has_acpi_companion(dev) block, copy or initialize the template entries (index/dev_id/con_id), set each .provider = dev_name(dev), then call reset_controller_add_lookup() with the allocated array; store the pointer (e.g., in driver data via dev_set_drvdata) and on driver remove call reset_controller_remove_lookup() and kfree (or rely on devm_ cleanup) to avoid leaking and to prevent corruption on rebind.patch/kernel/archive/cix-7.0/0005-clk-add-cix-clk-driver.patch-319-326 (1)
319-326:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHardcoded physical address bypasses device description.
Same issue as the 6.18 version - RCSU address should come from ACPI/DT.
Also applies to: 1083-1095
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-7.0/0005-clk-add-cix-clk-driver.patch` around lines 319 - 326, The patch currently hardcodes the RCSU physical address and related length/offset/timeouts via SKY1_AUDSS_RCSU_ADDR, SKY1_AUDSS_RCSU_LEN, SKY1_AUDSS_RCSU_REMAP, SKY1_AUDSS_RCSU_REMAP_VAL, SKY1_AUDSS_RCSU_TIMEOUT, SKY1_AUDSS_RCSU_TIMEOUT_EN and SKY1_AUDSS_RCSU_TIMEOUT_VAL; instead, remove the fixed SKY1_AUDSS_RCSU_ADDR/SKY1_AUDSS_RCSU_LEN defines and obtain the RCSU register range from the firmware resource at probe time (use the platform/device resource helpers for ACPI/DT, e.g. platform_get_resource()/devm_ioremap_resource() or of_address_to_resource()/of_iomap() as appropriate), then use the mapped base to compute the REMAP and TIMEOUT offsets/values rather than relying on a hardcoded physical address; ensure any callers that referenced SKY1_AUDSS_RCSU_ADDR are updated to use the mapped pointer/offsets.patch/kernel/archive/cix-6.18/0005-clk-add-cix-clk-driver.patch-318-325 (1)
318-325:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHardcoded physical address bypasses device description.
The RCSU base address
0x07000000is hardcoded. This should come from the ACPI device description or DT for proper hardware abstraction and portability. Usingioremap()with hardcoded addresses is fragile and may conflict with memory management.Also applies to: 1082-1094
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0005-clk-add-cix-clk-driver.patch` around lines 318 - 325, The RCSU physical base (SKY1_AUDSS_RCSU_ADDR/SKY1_AUDSS_RCSU_LEN) must not be hardcoded; instead obtain the memory resource from the hardware description in the driver's probe (e.g., cix_clk_probe) using platform_get_resource()/of_address_to_resource()/acpi equivalents and map it with devm_ioremap_resource() (or devm_of_iomap()), then use resource_size()/resource->start where you need length/address values; remove the fixed 0x07000000/0x10000 constants and replace uses of SKY1_AUDSS_RCSU_ADDR/LEN with the mapped resource or its size/addr (keep SKY1_AUDSS_RCSU_REMAP, _REMAP_VAL, _TIMEOUT symbols but reference the dynamic mapping instead of hardcoded base).patch/kernel/archive/cix-6.18/0007-soc-add-cix-acpi-resource-lookup-driver.patch-348-362 (1)
348-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDevice references not released and hardcoded device name.
bus_find_device_by_acpi_dev()andbus_find_device_by_name()return devices with incremented refcounts that are never released.- The hardcoded device name
"CIXHA010:00"is fragile; the:00suffix depends on enumeration order.🔧 Suggested fix
consumer = bus_find_device_by_acpi_dev(&platform_bus_type, adev); if (!consumer) return 0; supplier = bus_find_device_by_name(&platform_bus_type, NULL, "CIXHA010:00"); - if (!supplier || supplier == consumer) + if (!supplier || supplier == consumer) { + put_device(consumer); + if (supplier) + put_device(supplier); return 0; + } device_link_add(consumer, supplier, DL_FLAG_AUTOREMOVE_CONSUMER); dev_dbg(data, "clock link [%s]->[CIXHA010:00]\n", dev_name(consumer)); + put_device(consumer); + put_device(supplier); + return 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0007-soc-add-cix-acpi-resource-lookup-driver.patch` around lines 348 - 362, The code leaks refcounts from bus_find_device_by_acpi_dev() and bus_find_device_by_name() and uses a fragile hardcoded "CIXHA010:00" exact name; change to (1) find the supplier by iterating platform devices and matching the name prefix "CIXHA010" (e.g. use bus_for_each_dev(&platform_bus_type, NULL, NULL, match callback that checks strncmp(dev_name(dev), "CIXHA010", strlen("CIXHA010"))==0) or equivalent) instead of bus_find_device_by_name(..., "CIXHA010:00"), (2) after using consumer and supplier call device_put(consumer) and device_put(supplier) to drop the refs (do this even after device_link_add), and (3) preserve the existing device_link_add(consumer, supplier, DL_FLAG_AUTOREMOVE_CONSUMER) and dev_dbg(...) but make sure dev_dbg prints the canonical dev_name(consumer) and that both device_put calls are executed on all code paths to avoid leaks.patch/kernel/archive/cix-7.0/0006-reset-add-cix-reset-driver.patch-335-357 (1)
335-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatic lookup array modified at runtime is not safe.
The
lookupsarray isstaticbut itsproviderfield is modified in each probe call. If the driver is rebound or multiple instances exist, this causes data corruption. Either make it non-static (allocated per-probe) or initializeproviderstatically if it's always the same device name pattern.🔧 Suggested fix using dynamic allocation
if (has_acpi_companion(dev)) { - static struct reset_control_lookup lookups[] = { - { .index = 15, .dev_id = "CIXH1006:00", - .con_id = "dma_reset" }, - { .index = 14, .dev_id = "CIXH6020:00", - .con_id = "hda" }, - }; + struct reset_control_lookup *lookups; int i; - for (i = 0; i < ARRAY_SIZE(lookups); i++) + lookups = devm_kcalloc(dev, 2, sizeof(*lookups), GFP_KERNEL); + if (!lookups) + return -ENOMEM; + + lookups[0] = (struct reset_control_lookup){ + .index = 15, .dev_id = "CIXH1006:00", .con_id = "dma_reset" + }; + lookups[1] = (struct reset_control_lookup){ + .index = 14, .dev_id = "CIXH6020:00", .con_id = "hda" + }; + + for (i = 0; i < 2; i++) { + INIT_LIST_HEAD(&lookups[i].list); lookups[i].provider = dev_name(dev); - reset_controller_add_lookup(lookups, ARRAY_SIZE(lookups)); + } + reset_controller_add_lookup(lookups, 2); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-7.0/0006-reset-add-cix-reset-driver.patch` around lines 335 - 357, The static reset_control_lookup array named lookups is modified at runtime (provider = dev_name(dev)) which is unsafe across re-probes or multiple instances; change it to a per-probe (non-static) allocation or stack array and populate provider for each entry before calling reset_controller_add_lookup, or alternatively initialize provider statically if it never changes; specifically update the code around lookups, the loop that sets lookups[i].provider, and the call to reset_controller_add_lookup so that lookups is not a static global and is allocated/initialized inside the probe path that calls has_acpi_companion(dev) and dev_name(dev).patch/kernel/archive/cix-6.18/0005-clk-add-cix-clk-driver.patch-399-405 (1)
399-405:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffMove
sky1_audss_reg_savefrom static global to per-device storage.The static array at lines 399-405 is not safe for multiple driver instances. When multiple AUDSS devices are present (supported by the platform_driver model for device tree), concurrent suspend/resume operations will corrupt saved register state. Move this array into the
sky1_audss_privstructure to make state isolation per-device.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0005-clk-add-cix-clk-driver.patch` around lines 399 - 405, The static array sky1_audss_reg_save must be moved into per-device storage: add a reg_save field (e.g., u32 reg_save[5][2] or a pointer) to the sky1_audss_priv struct, allocate or initialize it in probe (use devm_kzalloc if pointer or just zero the embedded array) and replace all references to the global sky1_audss_reg_save with priv->reg_save; ensure suspend/resume/save/restore helpers use the per-device priv instance (sky1_audss_priv) rather than the global, and free any allocation in remove if you used non-devm allocation. Use ARRAY_SIZE or the known 5-entry size to keep the dimensions consistent.patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch-1333-1356 (1)
1333-1356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't free
cdns_sky1_pdataafter publishing it to children.The ACPI path assigns
cdev->platform_data = cdns_sky1_pdataas it walks children, then jumps toerr:on a later mismatch andkfree()s the same object. Any child updated before the failure keeps a danglingplatform_datapointer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch` around lines 1333 - 1356, The loop in device_for_each_child_node assigns the shared pointer cdns_sky1_pdata to cdev->platform_data for some children but on a later mismatch jumps to err and kfree(cdns_sky1_pdata), leaving earlier children with dangling platform_data; fix by not freeing cdns_sky1_pdata on the err path (remove kfree(cdns_sky1_pdata) in the err: block) or change ownership semantics so you either transfer unique copies to children or clear cdev->platform_data for all previously-updated children before freeing; locate the device_for_each_child_node loop, the assignment cdev->platform_data = cdns_sky1_pdata, and the err: block with kfree(cdns_sky1_pdata) and update accordingly.patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch-689-691 (1)
689-691:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't convert every port-init failure into
-EPROBE_DEFER.
rts5453h_init_ports()also returns permanent errors such as missing child nodes, bad properties, or port-registration failures. Rewriting all of them to-EPROBE_DEFERwill trap broken firmware in an endless reprobe loop.Suggested fix
ret = rts5453h_init_ports(rts); if (ret) - return -EPROBE_DEFER; + return ret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch` around lines 689 - 691, rts5453h_init_ports() can return permanent errors, so don't collapse every non-zero return into -EPROBE_DEFER; instead propagate the original error code. Replace the current block that does "ret = rts5453h_init_ports(rts); if (ret) return -EPROBE_DEFER;" with logic that returns ret (e.g., "if (ret) return ret;") so callers receive the actual failure from rts5453h_init_ports.patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch-1366-1370 (1)
1366-1370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the resets before shutting the block down.
cdnsp_sky1_remove()currently deasserts both resets and then disables clocks.cdnsp_sky1_shutdown()already does the safer thing by asserting reset first, and the remove path should quiesce hardware the same way.Suggested fix
sky1_put_addr(data->xhci_base); sky1_put_addr(data->device_base); - reset_control_deassert(data->reset); - reset_control_deassert(data->preset); + reset_control_assert(data->reset); + reset_control_assert(data->preset); sky1_usb_clk_disable_all(dev); platform_set_drvdata(pdev, NULL); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch` around lines 1366 - 1370, cdnsp_sky1_remove() currently deasserts resets then disables clocks; change it to quiesce hardware like cdnsp_sky1_shutdown() by asserting the resets first. In cdnsp_sky1_remove, replace the reset_control_deassert calls on data->reset and data->preset with reset_control_assert (assert preset then assert reset), do those asserts before calling sky1_usb_clk_disable_all(dev), and only after clocks are disabled call sky1_put_addr(data->xhci_base) and sky1_put_addr(data->device_base).patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch-319-330 (1)
319-330:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
partnerwhen registration fails.If
typec_register_partner()returns anERR_PTR, that error pointer stays intypec->partner. The next disconnect path callstypec_unregister_partner(typec->partner)and will trip over the stored error value.Suggested fix
if (NULL == typec->partner) { dev_info(typec->dev, "typec port(%d): register partner\n", typec->id); typec->partner = typec_register_partner(typec->port, &typec->desc); } if (IS_ERR(typec->partner)) { dev_info(typec->dev, "typec port(%d): register partner error \n", typec->id); + typec->partner = NULL; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch` around lines 319 - 330, The code leaves an ERR_PTR in typec->partner when typec_register_partner() fails, causing typec_unregister_partner(typec->partner) to be called with an error pointer later; change the failure handling in the block that calls typec_register_partner (the branch that assigns typec->partner) to detect IS_ERR(typec->partner), log the error, and then clear typec->partner (set to NULL) so the stored pointer isn't an ERR_PTR; ensure this uses the same symbols typec_register_partner, IS_ERR, typec_unregister_partner, and the typec->partner field.patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch-1273-1279 (1)
1273-1279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall the PHY dependency gate before
drd_init.
cdnsp_sky1_check_phy_deps()is defined for the exact ACPI probe-order race described above, butcdnsp_sky1_probe()never invokes it beforecdnsp_sky1_drd_init(). That leaves the wrapper free to deassert reset before the USB3 PHY provider has probed, so the late PHY reset can still drop the link and break SuperSpeed enumeration.Suggested fix
+ if (ACPI_COMPANION(dev)) { + ret = cdnsp_sky1_check_phy_deps(dev); + if (ret) + return ret; + } + mutex_lock(&cdnsp_sky1_init_lock); ret = cdnsp_sky1_drd_init(data); mutex_unlock(&cdnsp_sky1_init_lock);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch` around lines 1273 - 1279, Call the PHY dependency gate before performing DRD init: in cdnsp_sky1_probe invoke cdnsp_sky1_check_phy_deps() (protected by the existing cdnsp_sky1_init_lock if needed) and if it returns -EPROBE_DEFER return -EPROBE_DEFER; only after that proceed to cdnsp_sky1_drd_init() (and keep the current mutex lock/unlock and error handling for cdnsp_sky1_drd_init). This ensures the PHY provider is ready before cdnsp_sky1_drd_init() runs.patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch-441-448 (1)
441-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail probe when the Type-C handles are missing.
This only warns when
role_sw,mux, orori_swis absent, butrts5453h_typec_port_update()andrts5453h_unregister_ports()dereference all three unconditionally. Returning success here turns bad firmware wiring into a later NULL-deref.Suggested fix
typec->role_sw = fwnode_usb_role_switch_get(fwnode); if (IS_ERR(typec->role_sw)) return dev_err_probe(dev, PTR_ERR(typec->role_sw), "USB role switch handle is error\n"); - if (!typec->role_sw || !typec->mux || !typec->ori_sw) - dev_warn(dev, "mux or switch or role switch has not found\n"); + if (!typec->role_sw || !typec->mux || !typec->ori_sw) + return dev_err_probe(dev, -EPROBE_DEFER, + "mux/switch/role-switch not ready\n"); return 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch` around lines 441 - 448, The probe must fail if any Type-C handles are missing because rts5453h_typec_port_update() and rts5453h_unregister_ports() use typec->mux, typec->ori_sw and typec->role_sw unconditionally; replace the current dev_warn check with a failing probe return (use dev_err_probe) when any of typec->role_sw, typec->mux or typec->ori_sw is NULL, and ensure the earlier IS_ERR(typec->role_sw) check remains; reference the symbols typec->mux, typec->ori_sw, typec->role_sw, rts5453h_typec_port_update and rts5453h_unregister_ports so the probe returns a suitable error (e.g. -ENODEV via dev_err_probe) instead of continuing on missing handles.patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch-518-526 (1)
518-526:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
disable_irq_wake()call in resume to balance suspend'senable_irq_wake().Suspend enables wake on
cdns->wakeup_irq, but resume never disables it. This leaves IRQ wake accounting unbalanced across suspend/resume cycles and triggers kernel warnings about wake-state mismatches.Suggested fix
static int cdnsp_plat_resume(struct device *dev) { + struct cdns *cdns = dev_get_drvdata(dev); + + if (device_may_wakeup(dev) && cdns->wakeup_irq) + disable_irq_wake(cdns->wakeup_irq); + return cdnsp_controller_resume(dev, PMSG_RESUME); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0025-usb-add-usb-cdns3-driver-for-cix-soc.patch` around lines 518 - 526, The resume path (function cdnsp_plat_resume) must undo the wake IRQ enable done in suspend: when device_may_wakeup(dev) and cdns->wakeup_irq are used with enable_irq_wake(cdns->wakeup_irq) in the suspend flow, add the complementary disable_irq_wake(cdns->wakeup_irq) in cdnsp_plat_resume before returning (locate cdnsp_plat_resume and cdnsp_controller_resume to insert the disable call), ensuring wake IRQ accounting is balanced and then call/return cdnsp_controller_resume as before.patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch-708-710 (1)
708-710:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd delayed work cancellation and track IRQ ownership in remove() to prevent use-after-free.
The delayed work
rts_request_irq_work_fn()can still be queued whenremove()executes, causing it to access freed driver state. Additionally,free_irq()is called unconditionally even for instances that only encountered-EBUSYfromrequest_threaded_irq()and never actually acquired the IRQ, leading to callingfree_irq()with an unregistereddev_id.Suggested fix
struct rts5453h { ... struct delayed_work irq_work; + bool irq_requested; }; ... } else { /* succeed! remove from polling list and handle separately. */ mutex_lock(&rts_lock); list_del_init(&rts->list); mutex_unlock(&rts_lock); + rts->irq_requested = true; } } ... static void rts5453h_remove(struct i2c_client *client) { struct rts5453h *rts = i2c_get_clientdata(client); + cancel_delayed_work_sync(&rts->irq_work); device_remove_file(&client->dev, &dev_attr_disable_pd_interrupt); ... - if (client->irq > 0) + if (rts->irq_requested) free_irq(client->irq, rts); ... }Also applies to: 722-746
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch` around lines 708 - 710, The delayed work rts->irq_work may run after remove(), and free_irq() is called even when request_threaded_irq() returned -EBUSY, so in remove() cancel the delayed work with cancel_delayed_work_sync(&rts->irq_work) to ensure rts_request_irq_work_fn() has finished and cannot touch freed state, and add an ownership flag (e.g., rts->irq_owner or similar) that you set only after request_threaded_irq() returns success; only call free_irq(..., rts) when that flag is true (and clear it before freeing), and only schedule_delayed_work(&rts->irq_work, ...) when the IRQ has actually been acquired (or set the owner atomically before scheduling). Ensure all references use the rts->irq_work, rts_request_irq_work_fn, request_threaded_irq(), free_irq(), and remove() symbols so the logic is applied in the correct places.
🟡 Minor comments (7)
patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch-460-478 (1)
460-478:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded MMIO addresses are fragile and non-portable.
cix_dsp_mbox_dump_regs()uses hardcoded physical addresses (0x070f0000) and offsets. This is fragile—if the SoC revision changes or the driver is used on a variant, it will produce incorrect results or crash. Consider obtaining this address from device tree/ACPI or removing the debug dump entirely for production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch` around lines 460 - 478, The function cix_dsp_mbox_dump_regs currently ioremaps a hardcoded physical address (RCSU_MBOX_BASE) and should instead obtain the MMIO region from firmware-provided resources or device tree/ACPI: replace the hardcoded ioremap with a lookup (e.g., use platform_get_resource()/devm_ioremap_resource() or of_iomap/of_address_to_resource on rproc->dev->of_node) to retrieve the mailbox resource and size, then read registers from that mapped base; alternatively remove this debug-only dump or gate it behind a debug/boot option if you cannot obtain a resource, but do not keep raw hardcoded addresses in cix_dsp_mbox_dump_regs.patch/kernel/archive/cix-6.18/0016-dma-arm-dma350-add-acpi-support-for-cix-soc.patch-583-605 (1)
583-605:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStatic variable
last_irqinside IRQ handler causes incorrect behavior with multiple channels/instances.The
static ktime_t last_irqvariable is shared across all DMA channels and instances. This means the timing debug output will be incorrect when multiple cyclic transfers are active simultaneously, as it tracks the last IRQ from any channel rather than the specific channel being serviced.Suggested fix: move to per-channel state or remove debug code
If this debug timing is needed, consider moving
last_irqintostruct d350_chan. Otherwise, remove the debug code for production:} else if (intr_flags & CH_STAT_INTR_DONE) { if (dch->desc->cyclic) { - static ktime_t last_irq; - ktime_t now = ktime_get(); - s64 delta_us = ktime_to_us(ktime_sub(now, last_irq)); - - if (delta_us > 1000) /* Only log if > 1ms gap */ - pr_debug("dma350: period %zu, delta %lld us\n", - dch->period_idx, delta_us); - last_irq = now; - /* * Cyclic DMA: notify each period completion. * The DMA continues to run automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0016-dma-arm-dma350-add-acpi-support-for-cix-soc.patch` around lines 583 - 605, The static ktime_t last_irq inside the IRQ/callback block is shared across all channels and causes cross-instance timing corruption; move timing state into each channel by adding a ktime_t last_irq member to struct d350_chan and replace the static/last_irq usage with dch->last_irq in the IRQ handler (the block that updates period_idx, residue and calls vchan_cyclic_callback), ensuring the field is initialized when the channel is allocated; alternatively, if this debug timing is not required in production, remove the entire timing/debug block (the ktime_get/ktime_sub/ktime_to_us/pr_debug lines) instead of keeping a static variable.patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch-528-538 (1)
528-538:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExcessive info-level logging in hot path.
dev_info()calls incix_dsp_rproc_kick()will spam the kernel log on every vring kick. These should bedev_dbg()for production.Suggested fix
msg[0] = MBOX_MSG_LEN * sizeof(u32); msg[MBOX_MSG_OFFSET] = vqid; - dev_info(rproc_priv->dev, "kick: sending msg[0]=0x%x msg[1]=0x%x (vqid=%d)\n", + dev_dbg(rproc_priv->dev, "kick: sending msg[0]=0x%x msg[1]=0x%x (vqid=%d)\n", msg[0], msg[1], vqid); err = mbox_send_message(rproc_priv->tx_ch, (void *)msg); if (err < 0) { cix_dsp_mbox_dump_regs(rproc); dev_err(rproc_priv->dev, "%s: failed (%d, err:%d)\n", __func__, vqid, err); } else { - dev_info(rproc_priv->dev, "kick: mbox_send_message returned %d\n", err); + dev_dbg(rproc_priv->dev, "kick: mbox_send_message returned %d\n", err); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0009-remoteproc-add-cix-dsp-remoteproc-driver.patch` around lines 528 - 538, The two dev_info() calls in cix_dsp_rproc_kick() (the one logging "kick: sending msg..." before mbox_send_message and the one logging the returned err on success) are too verbose for a hot path; change them to dev_dbg() so vring kicks don't spam the kernel log. Keep the existing mbox_send_message call, error handling that calls cix_dsp_mbox_dump_regs() and dev_err(), and only replace the dev_info symbols with dev_dbg in cix_dsp_rproc_kick().patch/kernel/archive/cix-6.18/0013-sound-hda-add-cix-ipbloq-hda-driver.patch-1014-1047 (1)
1014-1047:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing clock disable on reset failure in runtime_resume.
If
reset_control_bulk_assertorreset_control_bulk_deassertfails, the function returns an error but leaves clocks enabled. This leaks clock references and may cause power issues.Proposed fix
static int __maybe_unused cix_ipbloq_hda_runtime_resume(struct device *dev) { ... rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks); if (rc) { dev_err(dev, "failed to enable clk bulk, rc: %d\n", rc); return rc; } rc = reset_control_bulk_assert(hda->nresets, hda->resets); if (rc) { dev_err(dev, "failed to assert reset bulk, rc: %d\n", rc); - return rc; + goto err_clk_disable; } usleep_range(10, 20); rc = reset_control_bulk_deassert(hda->nresets, hda->resets); if (rc) { dev_err(dev, "failed to deassert reset bulk, rc: %d\n", rc); - return rc; + goto err_clk_disable; } if (chip && chip->running) azx_init_chip(chip, 1); return 0; + +err_clk_disable: + clk_bulk_disable_unprepare(hda->nclocks, hda->clocks); + return rc; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0013-sound-hda-add-cix-ipbloq-hda-driver.patch` around lines 1014 - 1047, cix_ipbloq_hda_runtime_resume currently enables clocks with clk_bulk_prepare_enable(hda->nclocks, hda->clocks) but returns on reset_control_bulk_assert or reset_control_bulk_deassert failures without disabling them; update the error paths so that on any failure after successful clk_bulk_prepare_enable you call clk_bulk_disable_unprepare(hda->nclocks, hda->clocks) before returning (do this for both the assert and deassert error returns), ensuring clocks are always cleaned up if runtime_resume fails.patch/kernel/archive/cix-6.18/0021-sound-soc-add-cix-sof-driver.patch-579-588 (1)
579-588:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
of_reserved_mem_device_releasein remove path.
sky1_probecallsof_reserved_mem_device_init(&pdev->dev)(line 520) butsky1_removedoes not callof_reserved_mem_device_release. This can leak reserved memory state.Proposed fix
static void sky1_remove(struct snd_sof_dev *sdev) { struct sky1_priv *priv = sdev->pdata->hw_pdata; + struct platform_device *pdev = + container_of(sdev->dev, struct platform_device, dev); sky1_free_mbox(priv); cix_disable_clocks(sdev, priv->clks); + of_reserved_mem_device_release(&pdev->dev); + platform_device_unregister(priv->ipc_dev); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0021-sound-soc-add-cix-sof-driver.patch` around lines 579 - 588, sky1_remove is missing a call to release the reserved memory initialized in sky1_probe; add a call to of_reserved_mem_device_release(&priv->pdev->dev) (using the same platform device stored in priv) in sky1_remove before returning (e.g., before platform_device_unregister(priv->ipc_dev) or right after sky1_free_mbox) so the of_reserved_mem_device_init done in sky1_probe is properly released.patch/kernel/archive/cix-6.18/0006-reset-add-cix-reset-driver.patch-542-559 (1)
542-559:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing error handling in reset function.
Same issue as the 7.0 version -
sky1_reset()should check and propagate errors fromsky1_reset_set().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-6.18/0006-reset-add-cix-reset-driver.patch` around lines 542 - 559, sky1_reset currently ignores return values from sky1_reset_set; update it to check and propagate errors from both sky1_reset_set(rcdev, id, true) and sky1_reset_set(rcdev, id, false). Capture the int return (e.g., ret = sky1_reset_set(...)); if ret != 0 return ret immediately (do not continue to sleep or call the next operation), and only call the following usleep_range and the second sky1_reset_set when the prior call succeeded; finally return 0 on success.patch/kernel/archive/cix-7.0/0006-reset-add-cix-reset-driver.patch-672-689 (1)
672-689:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing error handling in reset function.
sky1_reset()ignores return values fromsky1_reset_set(). If assert fails, deassert is still attempted. Check and propagate errors.🛡️ Suggested fix
static int sky1_reset(struct reset_controller_dev *rcdev, unsigned long id) { - sky1_reset_set(rcdev, id, true); + int ret; + + ret = sky1_reset_set(rcdev, id, true); + if (ret) + return ret; + usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US); - sky1_reset_set(rcdev, id, false); + ret = sky1_reset_set(rcdev, id, false); + if (ret) + return ret; /* * Ensure component is taken out reset state by sleeping also after🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/cix-7.0/0006-reset-add-cix-reset-driver.patch` around lines 672 - 689, sky1_reset currently ignores return values from sky1_reset_set and proceeds to deassert and sleep even if the assert failed; update sky1_reset to check the return of sky1_reset_set when called to assert (and again when called to deassert), return early with the error code if either call fails, and only perform the usleep_range/SKY1_RESET_SLEEP_* waits after successful operations so errors are propagated to the caller.
| +static struct device *acpi_obj_to_platform_device(const union acpi_object *obj) | ||
| +{ | ||
| + struct acpi_device *adev; | ||
| + | ||
| + if (!obj) | ||
| + return NULL; | ||
| + | ||
| + if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) { | ||
| + adev = acpi_fetch_acpi_dev(obj->reference.handle); | ||
| + } else if (obj->type == ACPI_TYPE_STRING) { | ||
| + acpi_handle handle; | ||
| + acpi_status status; | ||
| + | ||
| + status = acpi_get_handle(NULL, obj->string.pointer, &handle); | ||
| + if (ACPI_FAILURE(status)) | ||
| + return NULL; | ||
| + adev = acpi_fetch_acpi_dev(handle); | ||
| + } else { | ||
| + return NULL; | ||
| + } | ||
| + | ||
| + if (!adev) | ||
| + return NULL; | ||
| + | ||
| + return bus_find_device_by_acpi_dev(&platform_bus_type, adev); | ||
| +} | ||
| + | ||
| +static const char *acpi_obj_to_devname(const union acpi_object *obj) | ||
| +{ | ||
| + struct device *dev; | ||
| + | ||
| + dev = acpi_obj_to_platform_device(obj); | ||
| + if (!dev) | ||
| + return NULL; | ||
| + | ||
| + return dev_name(dev); | ||
| +} |
There was a problem hiding this comment.
Device reference leak in helper functions.
bus_find_device_by_acpi_dev() returns a device with incremented reference count that must be released with put_device(). acpi_obj_to_platform_device() and acpi_obj_to_devname() leak these references. Every call site that uses these helpers will leak device references.
Consider either:
- Having callers responsible for
put_device(), or - Refactoring to avoid holding device references (e.g., copy the name string)
🐛 Suggested fix for acpi_obj_to_devname
-static const char *acpi_obj_to_devname(const union acpi_object *obj)
+static const char *acpi_obj_to_devname(struct device *lookup_dev,
+ const union acpi_object *obj)
{
struct device *dev;
+ const char *name;
dev = acpi_obj_to_platform_device(obj);
if (!dev)
return NULL;
- return dev_name(dev);
+ name = devm_kstrdup(lookup_dev, dev_name(dev), GFP_KERNEL);
+ put_device(dev);
+ return name;
}📝 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 struct device *acpi_obj_to_platform_device(const union acpi_object *obj) | |
| +{ | |
| + struct acpi_device *adev; | |
| + | |
| + if (!obj) | |
| + return NULL; | |
| + | |
| + if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) { | |
| + adev = acpi_fetch_acpi_dev(obj->reference.handle); | |
| + } else if (obj->type == ACPI_TYPE_STRING) { | |
| + acpi_handle handle; | |
| + acpi_status status; | |
| + | |
| + status = acpi_get_handle(NULL, obj->string.pointer, &handle); | |
| + if (ACPI_FAILURE(status)) | |
| + return NULL; | |
| + adev = acpi_fetch_acpi_dev(handle); | |
| + } else { | |
| + return NULL; | |
| + } | |
| + | |
| + if (!adev) | |
| + return NULL; | |
| + | |
| + return bus_find_device_by_acpi_dev(&platform_bus_type, adev); | |
| +} | |
| + | |
| +static const char *acpi_obj_to_devname(const union acpi_object *obj) | |
| +{ | |
| + struct device *dev; | |
| + | |
| + dev = acpi_obj_to_platform_device(obj); | |
| + if (!dev) | |
| + return NULL; | |
| + | |
| + return dev_name(dev); | |
| +} | |
| static struct device *acpi_obj_to_platform_device(const union acpi_object *obj) | |
| { | |
| struct acpi_device *adev; | |
| if (!obj) | |
| return NULL; | |
| if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) { | |
| adev = acpi_fetch_acpi_dev(obj->reference.handle); | |
| } else if (obj->type == ACPI_TYPE_STRING) { | |
| acpi_handle handle; | |
| acpi_status status; | |
| status = acpi_get_handle(NULL, obj->string.pointer, &handle); | |
| if (ACPI_FAILURE(status)) | |
| return NULL; | |
| adev = acpi_fetch_acpi_dev(handle); | |
| } else { | |
| return NULL; | |
| } | |
| if (!adev) | |
| return NULL; | |
| return bus_find_device_by_acpi_dev(&platform_bus_type, adev); | |
| } | |
| static const char *acpi_obj_to_devname(struct device *lookup_dev, | |
| const union acpi_object *obj) | |
| { | |
| struct device *dev; | |
| const char *name; | |
| dev = acpi_obj_to_platform_device(obj); | |
| if (!dev) | |
| return NULL; | |
| name = devm_kstrdup(lookup_dev, dev_name(dev), GFP_KERNEL); | |
| put_device(dev); | |
| return name; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@patch/kernel/archive/cix-6.18/0007-soc-add-cix-acpi-resource-lookup-driver.patch`
around lines 103 - 139, acpi_obj_to_platform_device() calls
bus_find_device_by_acpi_dev() which returns a device with an incremented
refcount but never calls put_device(), and acpi_obj_to_devname() returns
dev_name() from that leaked device; update the helpers so they do not leak
references: either document that callers must call put_device() and return the
device as-is from acpi_obj_to_platform_device(), or (preferred) have
acpi_obj_to_devname() immediately copy the device name into a caller-owned
buffer or a newly allocated string and call put_device() before returning, and
ensure any call sites that use acpi_obj_to_platform_device() either call
put_device() when done or are refactored to use the new devname-copying helper;
reference functions: acpi_obj_to_platform_device, acpi_obj_to_devname,
bus_find_device_by_acpi_dev, and put_device.
| + rdev = acpi_obj_to_platform_device(&el[0]); | ||
| + type = el[1].integer.value; | ||
| + index = el[2].integer.value; | ||
| + name = devm_kstrdup(dev, el[3].string.pointer, GFP_KERNEL); | ||
| + if (!rdev || !name) | ||
| + continue; | ||
| + | ||
| + res = platform_get_resource(to_platform_device(rdev), | ||
| + type, index); | ||
| + if (!res) | ||
| + continue; | ||
| + res->name = name; | ||
| + | ||
| + if ((res->flags & IORESOURCE_IRQ) && | ||
| + (res->flags & IORESOURCE_DISABLED)) | ||
| + acpi_irq_get(ACPI_HANDLE(rdev), index, res); | ||
| + | ||
| + dev_dbg(&adev->dev, "resource name [%s][0x%x][%d]->[%s]\n", | ||
| + dev_name(rdev), type, index, name); | ||
| + | ||
| + count++; | ||
| + } |
There was a problem hiding this comment.
Device references not released.
Calls to acpi_obj_to_platform_device() at lines 243 and 306-307 obtain device references that are never released. Add put_device() calls after use.
🐛 Suggested fix for resource_name_lookup_handle
rdev = acpi_obj_to_platform_device(&el[0]);
type = el[1].integer.value;
index = el[2].integer.value;
name = devm_kstrdup(dev, el[3].string.pointer, GFP_KERNEL);
- if (!rdev || !name)
+ if (!rdev || !name) {
+ if (rdev)
+ put_device(rdev);
continue;
+ }
res = platform_get_resource(to_platform_device(rdev),
type, index);
- if (!res)
+ if (!res) {
+ put_device(rdev);
continue;
+ }
res->name = name;
if ((res->flags & IORESOURCE_IRQ) &&
(res->flags & IORESOURCE_DISABLED))
acpi_irq_get(ACPI_HANDLE(rdev), index, res);
dev_dbg(&adev->dev, "resource name [%s][0x%x][%d]->[%s]\n",
dev_name(rdev), type, index, name);
+ put_device(rdev);
count++;Also applies to: 306-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@patch/kernel/archive/cix-6.18/0007-soc-add-cix-acpi-resource-lookup-driver.patch`
around lines 243 - 264, The ACPI-to-platform device conversion via
acpi_obj_to_platform_device (rdev) in resource_name_lookup_handle is leaking
references; after you finish using rdev (after
platform_get_resource/to_platform_device lookup, any acpi_irq_get call, and the
dev_dbg logging where rdev is referenced) call put_device(rdev) to release the
reference; apply the same fix to the other identical block around the later
occurrence (the second acpi_obj_to_platform_device usage at the 306-313 region)
so every successful acpi_obj_to_platform_device() has a matching put_device().
| +static ssize_t disable_pd_interrupt_store(struct device *dev, | ||
| + struct device_attribute *attr, | ||
| + const char *buf, size_t count) | ||
| +{ | ||
| + struct i2c_client *client = to_i2c_client(dev); | ||
| + struct rts5453h *rts = i2c_get_clientdata(client); | ||
| + bool value; | ||
| + int ret; | ||
| + | ||
| + ret = kstrtobool(buf, &value); | ||
| + if (!ret) { | ||
| + rts->pd_int_disabled = value; | ||
| + | ||
| + if (value) | ||
| + disable_irq(client->irq); | ||
| + else | ||
| + enable_irq(client->irq); | ||
| + | ||
| + dev_alert(dev, "Only for USB Compliance test, PD int is %s\n", | ||
| + value ? "disabled" : "enabled"); | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && wc -l 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 97
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && head -100 0026-typec-add-rts5453-driver.patch | tail -50Repository: armbian/build
Length of output: 1454
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '1,50p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 1916
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '560,590p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 703
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '760,830p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 1662
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '200,350p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 5378
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '100,500p' 0026-typec-add-rts5453-driver.patch | grep -n "probe\|request_irq\|devm_request"Repository: armbian/build
Length of output: 153
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '430,550p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 3113
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && grep -n "probe\|request_irq\|devm_request" 0026-typec-add-rts5453-driver.patch | head -30Repository: armbian/build
Length of output: 375
🏁 Script executed:
cd patch/kernel/archive/cix-6.18 && sed -n '600,750p' 0026-typec-add-rts5453-driver.patchRepository: armbian/build
Length of output: 3961
Guard the IRQ paths when firmware doesn't provide an IRQ.
The probe function accepts devices where client->irq == 0 (confirmed at line 710-711 and the code supports polling on IRQ-less systems), but disable_pd_interrupt_store() and the PM callbacks (suspend, resume, restore) call enable_irq()/disable_irq() unconditionally without checking for valid IRQ numbers. On IRQ-less systems, toggling disable_pd_interrupt or suspending/resuming will operate on IRQ 0, which is invalid.
Note: rts5453h_shutdown() and rts5453h_remove() already guard with if (client->irq), but disable_pd_interrupt_store(), rts5453h_suspend(), rts5453h_resume(), and rts5453h_restore() do not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/cix-6.18/0026-typec-add-rts5453-driver.patch` around
lines 565 - 585, The interrupt enable/disable paths call
enable_irq()/disable_irq() unconditionally, which is unsafe when client->irq ==
0; update disable_pd_interrupt_store (and the PM callbacks rts5453h_suspend,
rts5453h_resume, rts5453h_restore) to first obtain the i2c_client/rts5453h as
you already do and guard any calls to enable_irq()/disable_irq() with a check
like if (client->irq) so the IRQ toggles are skipped on IRQ-less devices; keep
the existing dev_alert/dev_dbg behavior but only call IRQ APIs when client->irq
is non-zero.
|
Testing rn kernel edge 7.0.x |
root@cix-acpi:~# uname -r
7.0.9-edge-cix-p1
root@cix-acpi:~#
|
2 participants
Description
This is in messy state but fair enough to collect feedback.
A few things that bug me and need enhancement, not sure though how to approach:
I worked around this by utilizing a similar approach like the out-of-tree wifi drivers and package this into the kernel before building. This however turned out to be quite ugly and I doubt this is future-proof.
I think the best approach would be to have the debs pre-built (via Github Action) in (our) apt repo to simply pull them.
Atm it is pulled from sky1 because there curl doesn't hit any Anubis wall.
custom_kernel_configfor some reason do not affect the cache, therefore rebuilding needsARTIFACT_IGNORE_CACHEin order to have effect. Ricardo mentioned thatkernel_config_modifying_hashesneed to be set consistently, not sure though what that means (don't hit me too hard, I'm just an amateur 😁 )Other things
Summary by CodeRabbit
Release Notes