On Thu, Aug 06, 2026 at 12:55:01PM -0700, Saravanakrishnan Krishnamoorthy wrote:
> These are the single-block cipher API used for software-fallback paths:
> CCM empty-input tag computation (2 ECB encryptions + XOR) and XCBC(SM4)
> empty-message workaround (3 ECB encryptions + XOR). No public wrapper
> exists; this is the same pattern used by in-tree crypto/ccm.c,
> crypto/cmac.c, and crypto/xcbc.c.
>
> Co-developed-by: Saravanakrishnan Krishnamoorthy <[email protected]>
> Signed-off-by: Saravanakrishnan Krishnamoorthy <[email protected]>
> Signed-off-by: Alex Ousherovitch <[email protected]>
> Reviewed-by: Joel Wittenauer <[email protected]>
> Reviewed-by: Thi Nguyen <[email protected]>
These two reviews did not happen, drop the tags. The code has trivial
mistakes from downstream code. Internal review would point these trivial
issues, so I do not believe you received valid, meaningful internal
review which would justify the tags.
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#include "cmh.h"
> +#include "cmh_dma.h"
> +#include "cmh_mqi.h"
> +#include "cmh_txn.h"
> +#include "cmh_rh.h"
> +#include "cmh_registers.h"
> +#include "cmh_debugfs.h"
> +#include "cmh_sysfs.h"
> +
> +#include <linux/iopoll.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alex Ousherovitch <[email protected]>");
> +MODULE_AUTHOR("Saravanakrishnan Krishnamoorthy
> <[email protected]>");
> +MODULE_AUTHOR("Joel Wittenauer <[email protected]>");
> +MODULE_DESCRIPTION("Rambus CryptoManager Hub (CMH) hardware crypto
> accelerator");
> +MODULE_ALIAS("platform:cmh");
Why?
> +MODULE_IMPORT_NS("CRYPTO_INTERNAL");
Why all this is in completely odd place, not next to the module init
code?
> +
> +#ifdef CONFIG_CRYPTO_DEV_CMH_DEBUG
> +static bool skip_fw_check;
> +module_param(skip_fw_check, bool, 0444);
> +MODULE_PARM_DESC(skip_fw_check,
> + "[debug] Skip eSW boot status check at probe (default:
> false)");
> +#else
> +#define skip_fw_check false
> +#endif
> +
> +/* Global device state (single-instance module) */
> +
> +static struct cmh_device *g_cmh_dev;
Eh, no. Don't implement singletons.
> +
> +/* SIC Sanity Check */
> +
> +static int cmh_check_sic(struct cmh_config *cfg)
> +{
> + const u32 ready = SIC_SW_BOOT_STATUS_MISSION |
> + SIC_SW_BOOT_STATUS_MISSION2;
> + u32 boot_status;
> + u32 hw_version;
> + u32 sw_boot;
> + int ret;
> +
> + boot_status = cmh_reg_read32(cfg->sic_mapped, R_SIC_BOOT_STATUS);
> + hw_version = cmh_reg_read32(cfg->sic_mapped, R_SIC_HW_VERSION0);
> +
> + dev_info(cmh_dev(), "SIC boot_status=0x%08x hw_version=0x%08x\n",
> + boot_status, hw_version);
> +
> + if ((boot_status & SIC_BOOT_STATUS_MASK) != SIC_BOOT_STATUS_PASS) {
> + dev_err(cmh_dev(), "SIC boot status check failed (0x%02x !=
> 0x%02x)\n",
> + boot_status & SIC_BOOT_STATUS_MASK,
> SIC_BOOT_STATUS_PASS);
> + return -EIO;
> + }
> +
> + /*
> + * Wait for eSW readiness: MISSION signals the primary VCQ engine,
> + * MISSION2 the sidecar engine (set asynchronously). The driver
> + * uses both, so require both bits.
> + */
> + ret = read_poll_timeout(ioread32, sw_boot,
> + (sw_boot & ready) == ready,
> + 1000,
> + (unsigned long)cfg->fw_ready_timeout_ms *
> 1000UL,
> + false,
> + cfg->sic_mapped + R_SIC_SW_BOOT_STATUS);
> + if (ret) {
> + sw_boot = cmh_reg_read32(cfg->sic_mapped, R_SIC_SW_BOOT_STATUS);
> + dev_err(cmh_dev(), "CMH eSW not ready (sw_boot_status=0x%08x,
> timeout=%ums)\n",
> + sw_boot, cfg->fw_ready_timeout_ms);
> + return -ETIMEDOUT;
> + }
> +
> + dev_info(cmh_dev(), "CMH eSW fully operational
> (sw_boot_status=0x%08x)\n",
> + sw_boot);
Drop
> +
> + return 0;
> +}
> +
> +/* Module Init -- platform driver probe */
> +
> +static int cmh_probe(struct platform_device *pdev)
> +{
> + struct cmh_device *dev;
> + struct cmh_config *cfg;
> + struct clk_bulk_data *clks;
> + struct gpio_desc *reset;
> + unsigned int i;
> + int ret;
> +
> + /* Single-instance guard: reject if already probed */
> + if (g_cmh_dev)
> + return -EBUSY;
NAK
> +
> + dev_info(&pdev->dev, "loading v%s\n", CMH_VERSION);
NAK and internal review should tell you that
> +
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
> +
> + dev->dev = &pdev->dev;
> + cfg = &dev->config;
> +
> + /* Declare DMA addressing capability */
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (ret) {
> + dev_err(&pdev->dev, "dma_set_mask_and_coherent failed
> (rc=%d)\n",
> + ret);
> + goto err_free_dev;
> + }
> +
> + /* Initialize DMA backend (standard API or FPGA pool) */
> + ret = cmh_dma_init(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "DMA init failed (rc=%d)\n", ret);
> + goto err_free_dev;
> + }
> +
> + /* Step 1: Parse and validate configuration (DT + module params) */
> + ret = cmh_config_init(cfg, pdev);
> + if (ret)
> + goto err_dma_init;
> +
> + dev_info(cmh_dev(), "sic_base=0x%llx size=0x%zx mbx_count=%u\n",
> + (unsigned long long)cfg->sic_base, cfg->sic_size,
> + cfg->mbx_count);
How many dev_info are you going to print?
> +
> + /*
> + * Enable functional clocks and release reset. Both are optional --
> + * integrations where a separate management/power controller owns the
> + * clock and reset lines describe neither, and these calls are
> + * no-ops. The hub gates its clocks internally, but
> + * clk_disable_unused() would otherwise gate an always-on input the
> + * driver never claimed, so the driver enables whatever clocks the
> + * device tree provides. Clocks come up before reset is released (a
> + * hard reset requires an active clock) and before any SIC register
> + * access. The reset line is acquired already deasserted; the driver
> + * does not drive a reset pulse -- the eSW boots independently and its
> + * mission-mode readiness is verified separately below, and no in-tree
> + * platform wires this line for a reset sequence to be exercised.
> + * devm unwinds both on remove or probe error.
> + */
> + ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
> + if (ret < 0) {
> + dev_err(cmh_dev(), "failed to enable clocks (rc=%d)\n", ret);
> + goto err_dma_init;
> + }
> + if (ret > 0)
> + dev_info(cmh_dev(), "enabled %d clock(s)\n", ret);
Again?
> +
> + reset = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(reset)) {
> + ret = PTR_ERR(reset);
> + dev_err(cmh_dev(), "failed to acquire reset GPIO (rc=%d)\n",
> + ret);
Syntax is ret = dev_err_probe and internal review should tell you that.
> + goto err_dma_init;
> + }
> +
> + /* Step 2: ioremap the SIC region */
> + cfg->sic_mapped = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(cfg->sic_mapped)) {
> + ret = PTR_ERR(cfg->sic_mapped);
> + cfg->sic_mapped = NULL;
> + dev_err(cmh_dev(), "ioremap failed for SIC region (rc=%d)\n",
> + ret);
> + goto err_dma_init;
> + }
> +
> + /* Step 3: Verify CMH is alive */
> + if (skip_fw_check) {
> + dev_info(cmh_dev(), "skipping eSW boot check
> (skip_fw_check=1)\n");
> + } else {
> + ret = cmh_check_sic(cfg);
> + if (ret)
> + goto err_dma_init;
> + }
> +
> + /* Step 3.5: Discover crypto cores from the SIC CORE_ENABLE register */
> + ret = cmh_config_discover_cores(cfg);
> + if (ret)
> + goto err_dma_init;
> +
> + /* Step 4: Compute per-instance register bases */
> + for (i = 0; i < cfg->mbx_count; i++) {
> + struct cmh_mbx_config *m = &cfg->mailboxes[i];
> +
> + m->reg_base = cmh_mbx_instance_base(cfg->sic_mapped,
> + m->instance);
> +
> + dev_dbg(cmh_dev(), "mbx[%u] instance=%u reg_base=%p\n",
> + i, m->instance, m->reg_base);
> + }
> +
> + (void)cmh_debugfs_init(cfg);
Why the cast?
> +
> + /* Initialise mailbox queue interface */
> + ret = cmh_mqi_init(cfg);
> + if (ret)
> + goto err_mqi_init;
> +
> + /* Initialise transaction manager */
> + ret = cmh_tm_init(cfg);
> + if (ret)
> + goto err_tm_init;
> +
> + /* Initialise response handler */
> + ret = cmh_rh_init(cfg);
> + if (ret)
> + goto err_rh_init;
> +
> + g_cmh_dev = dev;
Nope (and we do not call things "g" from globals)
> + platform_set_drvdata(pdev, dev);
> +
> + dev_info(cmh_dev(), "initialized successfully\n");
NAK
> + return 0;
> +
> +err_rh_init:
> + cmh_tm_cleanup();
> +err_tm_init:
> + cmh_mqi_cleanup(cfg);
> +err_mqi_init:
> + cmh_debugfs_cleanup();
> +err_dma_init:
> + cmh_dma_cleanup();
> +err_free_dev:
> + return ret;
> +}
> +
> +/* Module Exit -- platform driver remove */
> +
> +static void cmh_remove(struct platform_device *pdev)
> +{
> + struct cmh_device *dev = platform_get_drvdata(pdev);
> + struct cmh_config *cfg;
> +
> + if (!dev)
> + return;
> +
> + cfg = &dev->config;
> +
> + cmh_rh_cleanup(cfg);
> + cmh_tm_cleanup();
> + cmh_mqi_cleanup(cfg);
> + cmh_debugfs_cleanup();
> + cmh_dma_cleanup();
> +
> + dev_info(&pdev->dev, "unloaded successfully\n");
NAK
> +
> + g_cmh_dev = NULL;
> +}
> +
> +static const struct of_device_id cmh_of_match[] = {
> + { .compatible = "rambus,cmh-v1030" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, cmh_of_match);
> +
> +/*
> + * PM suspend/resume.
> + *
> + * Suspend: drain the TM first (while the RH is still active and can
> + * deliver completions for in-flight transactions), then quiesce the
> + * RH (cancel watchdog, mask HW interrupts). This ordering ensures
> + * the drain_timeout_ms wait in cmh_tm_quiesce() can actually succeed
> + * -- if we suspended RH first, no completions would be delivered and
> + * the drain would always hit the force-cancel path.
> + *
> + * IRQ handlers remain registered (standard PM pattern: the kernel
> + * disables the IRQ lines during suspend, no need to free/re-request).
> + *
> + * Resume: re-check the SIC/SW boot status, re-synchronise the RH
> + * with hardware (head positions, interrupt masks, watchdog), then
> + * restart the TM kthread.
> + */
> +
> +static int cmh_suspend(struct device *dev)
> +{
> + struct cmh_device *cmh = dev_get_drvdata(dev);
> +
> + if (!cmh)
> + return 0;
> +
> + dev_info(dev, "suspending\n");
Really, how many dev_info do you intend to have?
> + cmh_tm_quiesce();
> + cmh_rh_suspend(&cmh->config);
> + return 0;
> +}
> +
> +static int cmh_resume(struct device *dev)
> +{
> + struct cmh_device *cmh = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!cmh)
> + return 0;
> +
> + ret = cmh_check_sic(&cmh->config);
> + if (ret) {
> + dev_err(dev, "resume: CMH eSW health check failed (%d)\n",
> + ret);
> + return ret;
> + }
> +
> + /*
> + * cmh_rh_resume() is void: it only re-syncs MMIO head pointers,
> + * clears stale interrupt status bits (W1C), re-enables interrupt
> + * masks, and re-arms the watchdog timer -- none of which can fail
> + * after the SIC health check above has confirmed HW accessibility.
> + */
> + cmh_rh_resume(&cmh->config);
> +
> + ret = cmh_tm_resume();
> + if (ret) {
> + dev_err(dev, "resume: TM restart failed (%d)\n", ret);
> + return ret;
> + }
> + dev_info(dev, "resumed successfully\n");
NAK
> + return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(cmh_pm_ops,
> + cmh_suspend,
> + cmh_resume);
> +
> +/*
> + * Runtime PM is intentionally not implemented. The CMH hardware does
> + * not expose HLOS-accessible clock gates or power domains -- the eSW
> + * firmware manages HW power state independently. There is no mechanism
> + * for the kernel to idle, gate clocks, or power down the accelerator
> + * block from HLOS. If a future platform variant exposes power control
> + * to HLOS (e.g. via a SCMI power domain), runtime PM support can be
> + * added at that time using SET_RUNTIME_PM_OPS and pm_runtime_get/put
> + * around VCQ submission paths.
> + *
> + * System sleep (suspend/resume) is supported via DEFINE_SIMPLE_DEV_PM_OPS
> + * above: suspend quiesces the TM and masks IRQs; resume re-verifies
> + * eSW health (SIC status) and restarts the TM thread.
> + */
> +
> +static struct platform_driver cmh_driver = {
> + .probe = cmh_probe,
> + .remove = cmh_remove,
> + .driver = {
> + .name = CMH_DRV_NAME,
Use name directly.
> + .of_match_table = cmh_of_match,
> + .dev_groups = cmh_sysfs_groups,
> + .pm = pm_sleep_ptr(&cmh_pm_ops),
> + },
> +};
> +
> +static int __init cmh_init(void)
> +{
> + /*
> + * Register the platform driver and let the driver core drive
> + * probing. Probe failures are logged by probe() itself; a probe
> + * that returns -EPROBE_DEFER (e.g. its interrupt controller is not
> + * yet ready) is retried by the core. Do not gate module load on
> + * g_cmh_dev -- that would defeat deferred probe.
> + */
Why do you need this? Why your driver is done differently than all
others?
> + return platform_driver_register(&cmh_driver);
> +}
> +
> +static void __exit cmh_exit(void)
> +{
> + platform_driver_unregister(&cmh_driver);
> +}
> +
> +module_init(cmh_init);
> +module_exit(cmh_exit);
And this is not module_platform_driver because...?
> diff --git a/drivers/crypto/cmh/cmh_mqi.c b/drivers/crypto/cmh/cmh_mqi.c
> new file mode 100644
> index 000000000000..99ba44695d3c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mqi.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2026 Cryptography Research, Inc. (CRI).
> + * CMH LKM -- Mailbox Queue Initializer
> + *
> + * Responsibilities:
> + * - Allocate queue buffers for each configured mailbox
> + * - Execute the MBX lock/setup/enable register sequence
> + * - Readback-verify all critical register writes
> + * - Hold lock for MBX lifetime (CMH eSW requires it for host access)
> + * - Clean up (flush + unlock + free) on exit or error
> + *
> + * Register sequence per instance (per CMH MBX hardware specification):
> + * 1. Read R_MBX_LOCK -> non-zero = ownership token acquired
> + * 2. W1C stale R_MBX_INTERRUPT bits (avoids spurious error cascade)
> + * 3. Set R_MBX_INTERRUPT_MASK = MBX_IRQ_MASK
> + * 4. Write QUEUE_LO/HI, SLOTS, STRIDE (queue address + geometry)
> + * 5. Sync TAIL = HEAD (CMH eSW owns HEAD; avoids stale-queue parse)
> + * 6. Readback verify QUEUE_LO/HI/SLOTS/STRIDE
> + * 7. Write COMMAND = MBX_COMMAND_RUN
> + * 8. Lock stays held -- released only in teardown
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
...
> +/**
> + * cmh_mqi_init() - Initialize all mailbox queues
> + * @cfg: CMH configuration describing the mailboxes to set up
> + *
> + * Allocates DMA queue buffers for each configured mailbox, then executes
> + * the MBX lock/setup/enable register sequence. On failure, all
> + * successfully initialized mailboxes are torn down and buffers freed.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int cmh_mqi_init(struct cmh_config *cfg)
> +{
> + unsigned int i, j;
> + int ret;
> +
> + /* Allocate queue buffers */
> + for (i = 0; i < cfg->mbx_count; i++) {
> + struct cmh_mbx_config *m = &cfg->mailboxes[i];
> +
> + m->virt_addr = cmh_dma_alloc(m->queue_size, &m->dma_handle,
> + GFP_KERNEL);
> + if (!m->virt_addr) {
> + ret = -ENOMEM;
> + goto err_free_bufs;
> + }
> +
> + dev_dbg(cmh_dev(), "mqi[%u] alloc %zu bytes @ virt=%pK
> dma=%pad\n",
> + i, m->queue_size, m->virt_addr, &m->dma_handle);
> + }
> +
> + /* Lock/setup/enable each mailbox */
> + for (i = 0; i < cfg->mbx_count; i++) {
> + ret = cmh_mbx_setup_one(&cfg->mailboxes[i]);
> + if (ret) {
> + dev_err(cmh_dev(), "mqi[%u] setup failed (rc=%d)\n",
> + i, ret);
> + goto err_teardown;
> + }
> + }
> +
> + dev_info(cmh_dev(), "MQI init complete (%u mailboxes)\n",
> cfg->mbx_count);
NAK
> + return 0;
> +
> +err_teardown:
> + for (j = 0; j < i; j++)
> + cmh_mbx_teardown_one(&cfg->mailboxes[j]);
> +err_free_bufs:
> + for (j = 0; j < cfg->mbx_count; j++) {
> + if (cfg->mailboxes[j].virt_addr)
> + cmh_dma_free(cfg->mailboxes[j].queue_size,
> + cfg->mailboxes[j].virt_addr,
> + cfg->mailboxes[j].dma_handle);
> + cfg->mailboxes[j].virt_addr = NULL;
> + cfg->mailboxes[j].dma_handle = 0;
> + }
> + return ret;
> +}
> +
> +/**
> + * cmh_mqi_cleanup() - Clean up all mailbox queues
> + * @cfg: CMH configuration describing the mailboxes to tear down
> + *
> + * Tears down each mailbox (flush + unlock) and frees the DMA queue
> + * buffers allocated by cmh_mqi_init().
> + */
> +void cmh_mqi_cleanup(struct cmh_config *cfg)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < cfg->mbx_count; i++) {
> + struct cmh_mbx_config *m = &cfg->mailboxes[i];
> +
> + cmh_mbx_teardown_one(m);
> +
> + if (m->virt_addr)
> + cmh_dma_free(m->queue_size, m->virt_addr,
> + m->dma_handle);
> + m->virt_addr = NULL;
> + m->dma_handle = 0;
> + }
> +
> + dev_info(cmh_dev(), "MQI cleanup complete\n");
NAK
...
> +
> +/*
> + * Resolve per-MBX Linux virqs for the CMH interrupt lines.
> + *
> + * Each mailbox declares its own completion interrupt in its device-tree
> + * child node; cmh_config_init() resolves these to Linux virqs and stores
> + * them in cfg->mailboxes[i].irq (-1 when the mailbox has no interrupt).
> + * IRQ mode requires every configured mailbox to have an interrupt; if
> + * none do (or only some), the response handler uses watchdog polling.
> + *
> + * Populates rh.irqs[] and rh.nirqs. Returns 0 on success, or a
> + * negative errno if no IRQs could be resolved (polling-only mode).
> + */
> +static int cmh_rh_resolve_irqs(struct cmh_config *cfg)
> +{
> + u32 i, nwith = 0;
> +
> + rh.nirqs = 0;
> +
> + for (i = 0; i < cfg->mbx_count; i++)
> + if (cfg->mailboxes[i].irq >= 0)
> + nwith++;
> +
> + if (nwith == 0) {
> + dev_info(cmh_dev(), "rh: no mailbox IRQs -- polling mode\n");
So a warning or info?
> + return -ENODEV;
> + }
> +
> + if (nwith != cfg->mbx_count) {
> + dev_warn(cmh_dev(),
> + "rh: only %u/%u mailboxes have IRQs -- falling back to
> polling\n",
> + nwith, cfg->mbx_count);
> + return -ENODEV;
> + }
> +
> + for (i = 0; i < cfg->mbx_count; i++) {
> + rh.irqs[i] = cfg->mailboxes[i].irq;
> + dev_dbg(cmh_dev(), "rh: MBX%u -> IRQ %d\n", i, rh.irqs[i]);
> + }
> +
> + rh.nirqs = cfg->mbx_count;
> + return 0;
> +}
> +
> +/**
> + * cmh_rh_init() - Initialize the response handler
> + * @cfg: Device configuration (mailbox count, MMIO bases, IRQ info)
> + *
> + * Resolve per-mailbox IRQs from the device tree (or module parameter
> + * override), register threaded IRQ handlers (hardirq + kthread), and
> + * arm the missed-IRQ software watchdog timer. If no IRQs can be
> + * resolved, falls back to watchdog-only polling mode.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int cmh_rh_init(struct cmh_config *cfg)
> +{
> + int ret;
> + u32 i;
> +
> + rh.cfg = cfg;
> + rh.nirqs = 0;
> + rh.active = false;
> + atomic_set(&rh.irq_count, 0);
> +
> + /* Allocate per-MBX tracking */
> + rh.mbx = kcalloc(cfg->mbx_count, sizeof(*rh.mbx), GFP_KERNEL);
> + if (!rh.mbx)
> + return -ENOMEM;
> +
> + /* Resolve per-MBX IRQs */
> + if (cmh_rh_resolve_irqs(cfg) < 0) {
> + /*
> + * No IRQs available. The watchdog timer provides
> + * a polling fallback: it reads MBX head registers
> + * periodically and processes completions. This is
> + * slower than IRQ-driven completion but functional.
> + *
> + * Completion latency in polling-only mode is bounded
> + * by the watchdog interval (default 200 ms, tunable
> + * via debugfs config/watchdog_ms).
> + */
> + dev_warn(cmh_dev(),
> + "rh: no IRQs -- using watchdog polling (interval %u
> ms)\n",
> + watchdog_ms);
And here it is a warning?
> +
> + /* Seed last_head from HW before first watchdog tick */
> + for (i = 0; i < cfg->mbx_count; i++)
> + rh.mbx[i].last_head =
> + cmh_reg_read32(cfg->mailboxes[i].reg_base,
> + R_MBX_QUEUE_HEAD);
> +
> + rh.active = true;
> + timer_setup(&rh_watchdog, cmh_rh_watchdog_fn, 0);
> + mod_timer(&rh_watchdog, jiffies +
> + msecs_to_jiffies(max(watchdog_ms,
> + CMH_RH_WATCHDOG_MS_MIN)));
> + return 0;
> + }
> +
> + /* Initialize per-MBX state: read current head positions */
> + for (i = 0; i < cfg->mbx_count; i++)
> + rh.mbx[i].last_head =
> cmh_reg_read32(rh.cfg->mailboxes[i].reg_base,
> + R_MBX_QUEUE_HEAD);
> +
> + /*
> + * Register threaded IRQ handlers.
> + *
> + * DT per-MBX path: one distinct virq per MBX, nirqs == mbx_count.
> + * DT single-IRQ path: one shared IRQ, nirqs == 1. The handler
> + * scans all mailboxes unconditionally, so a single registration
> + * suffices.
> + *
> + * Use IRQF_SHARED only for the single-IRQ path where one line
> + * is shared across all MBXes. Dedicated per-MBX virqs need no
> + * sharing flag.
> + */
> + {
What is with this indentation?
> + unsigned long irqflags = (rh.nirqs == 1 && cfg->mbx_count > 1)
> + ? IRQF_SHARED : 0;
> +
> + for (i = 0; i < rh.nirqs; i++) {
> + ret = request_threaded_irq(rh.irqs[i],
> + cmh_rh_hardirq,
> + cmh_rh_thread,
> + irqflags,
> + "cmh", cfg);
> + if (ret) {
> + dev_err(cmh_dev(), "rh:
> request_threaded_irq(%d) for MBX%u failed (rc=%d)\n",
> + rh.irqs[i], i, ret);
> + /* Unwind previously registered IRQs */
> + while (i--)
> + free_irq(rh.irqs[i], cfg);
> + rh.nirqs = 0;
> + kfree(rh.mbx);
> + rh.mbx = NULL;
> + return ret;
> + }
> + }
> + }
> +
> + rh.active = true;
> +
> + /* Enable MBX completion interrupts (DONE + ERROR) */
> + for (i = 0; i < cfg->mbx_count; i++) {
> + u32 stale;
> +
> + /*
> + * W1C any interrupt bits that accumulated between
> + * MQI setup and now (e.g. CMH eSW processing stale
> + * commands) before enabling the mask.
> + */
> + stale = cmh_reg_read32(cfg->mailboxes[i].reg_base,
> + R_MBX_INTERRUPT);
> + if (stale)
> + cmh_reg_write32(stale, cfg->mailboxes[i].reg_base,
> + R_MBX_INTERRUPT);
> +
> + cmh_reg_write32(MBX_IRQ_MASK,
> + cfg->mailboxes[i].reg_base,
> + R_MBX_INTERRUPT_MASK);
> + }
> +
> + dev_info(cmh_dev(), "rh: initialized (%u IRQs, %u mailboxes, watchdog
> %u ms)\n",
> + rh.nirqs, cfg->mbx_count, watchdog_ms);
One more dev_info....
> +
> + /* Arm missed-IRQ watchdog timer */
> + timer_setup(&rh_watchdog, cmh_rh_watchdog_fn, 0);
> + mod_timer(&rh_watchdog, jiffies +
> + msecs_to_jiffies(max(watchdog_ms,
> + CMH_RH_WATCHDOG_MS_MIN)));
> +
> + return 0;
> +}
> +
> +/**
> + * cmh_rh_suspend() - Suspend the response handler
> + * @cfg: Device configuration
> + *
> + * Stop the watchdog timer and mask mailbox interrupts at the hardware
> + * level. The IRQ handlers remain registered so that resume can
> + * re-enable them without re-requesting.
> + */
> +void cmh_rh_suspend(struct cmh_config *cfg)
> +{
> + u32 i;
> +
> + if (!rh.active)
> + return;
> +
> + /* Stop the watchdog before masking HW interrupts */
> + timer_delete_sync(&rh_watchdog);
> +
> + /* Mask MBX interrupts at the hardware level */
> + for (i = 0; i < cfg->mbx_count; i++)
> + cmh_reg_write32(0, cfg->mailboxes[i].reg_base,
> + R_MBX_INTERRUPT_MASK);
> +
> + /*
> + * Ensure no threaded IRQ handler is still in-flight.
> + * After masking, a handler may already have been scheduled.
> + * synchronize_irq() waits for it to complete before we
> + * proceed with suspend (which tears down TM state).
> + */
> + for (i = 0; i < rh.nirqs; i++)
> + synchronize_irq(rh.irqs[i]);
> +
> + rh.active = false;
> + dev_dbg(cmh_dev(), "rh: suspended\n");
No, you cannot have even debug messages which re-implement and
duplicated standard tracing and standard debugging code.
...
> +static unsigned int cmq_max_depth = 256;
> +module_param(cmq_max_depth, uint, 0444);
> +MODULE_PARM_DESC(cmq_max_depth,
> + "Max pending commands in the Command Message Queue (default:
> 256)");
> +
> +static unsigned int backlog_max_depth = 1024;
> +module_param(backlog_max_depth, uint, 0444);
Drop all module params, not really right way to do things.
> +MODULE_PARM_DESC(backlog_max_depth,
> + "Max pending commands in the backlog queue (0 = disable
> backlog, default: 1024)");
> +
> +static unsigned int async_timeout_ms = 2000;
> +
> +#define CMH_TM_BACKOFF_MIN_US 100 /* queue-full backoff range (us) */
> +#define CMH_TM_BACKOFF_MAX_US 500
> +static unsigned int cmq_depth; /* current CMQ depth, protected by
> tm.cmq_lock */
...
> diff --git a/drivers/crypto/cmh/include/cmh.h
> b/drivers/crypto/cmh/include/cmh.h
> new file mode 100644
> index 000000000000..18150ba39129
> --- /dev/null
> +++ b/drivers/crypto/cmh/include/cmh.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2026 Cryptography Research, Inc. (CRI).
> + * CMH LKM -- Top-level Device Structure
> + */
> +
> +#ifndef CMH_H
> +#define CMH_H
> +
> +#include <linux/device.h>
> +
> +#include "cmh_config.h"
> +
> +#define CMH_DRV_NAME "cmh"
> +#define CMH_VERSION "1.0.0"
NAK, this is not allowed in Linux kernel code. Do you see drivers
(except complete junk from AMD) doing this?
Best regards,
Krzysztof
