On 9/30/2026 8:04 AM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 8 potential issue(s)
to consider:
- [High] soc: qcom: qpace: concurrent MMIO access from CPU aliasing on >8 cores
- [High] soc: qcom: qpace: UAF of global qpace_priv after driver unbind
- [High] soc: qcom: qpace: 100ms busy wait with preemption disabled causes
latency spikes
- [High] soc: qcom: qpace: interrupt storm due to uncleared level-triggered IRQ
- [High] soc: qcom: qpace: stray DMA writes on urgent command timeout
- [High] soc: qcom: qpace: hardware stuck in STOP mode after suspend/resume
- [Medium] soc: qcom: qpace: silent truncation of unaligned output_addr causes
DMA corruption
- [Medium] soc: qcom: qpace: inline stp assembly breaks big-endian architectures
--
commit 5edbc123f3ebd977977e7d874e4ecebc5ed95e6a
Author: Georgi Djakov <[email protected]>
soc: qcom: qpace: Add Qualcomm Page Compression Engine driver
This commit adds a platform driver for the Qualcomm Page Compression Engine
(QPaCE). It provides the urgent command path for synchronous single-page
compression and decompression, supporting low-latency operations for
compressed-memory users like zram.
diff --git a/drivers/soc/qcom/qpace.c b/drivers/soc/qcom/qpace.c
index 0000000000000..69aa14b7c4772
--- /dev/null
+++ b/drivers/soc/qcom/qpace.c
[ ... ]
+static inline int qpace_urgent_command_trigger(dma_addr_t input_addr,
+ dma_addr_t output_addr,
+ int urg_reg_num,
+ enum urg_reg_cxts command)
+{
+ void __iomem *td_dst_src_reg = qpace_priv->urg_regs +
+ (urg_reg_num * QPACE_REG_PAGE_SIZE) +
+
QPACE_URG_CMD_0_TD_DST_ADDR_L_CFG_CNTXT_OFFSET;
+ u64 urg_addr_field_lower, urg_addr_field_upper;
+ u32 stat_reg;
+ unsigned long ret;
+
+ urg_addr_field_lower =
FIELD_PREP(URG_CMD_0_TD_DST_ADDR_L__CMD_CFG_CNTXT,
+ command);
+ urg_addr_field_lower |= GENMASK(63, 8) & output_addr;
[Severity: Medium]
Will this silently truncate the output_addr if the caller passes an address
that is not 256-byte aligned? In qpace_urgent_command_trigger(), this drops
the lowest 8 bits, meaning the hardware will write to the incorrectly
truncated address and overwrite up to 255 bytes of memory before the
intended buffer.
Current use case for qpace is zram (although it is not limited to only
zram). ZRAM operates on a page granularity which means that all
addresses submitted to qpace will be 4KB aligned which is also 256 byte
aligned.
+
+ urg_addr_field_upper = input_addr;
+
+ /* Clear a stale request-on-active error before ringing the doorbell. */
+ qpace_write_urg_cmd(qpace_priv, urg_reg_num,
QPACE_URG_CMD_0_STAT_CLR_OFFSET,
+ URG_CMD_0_STAT_CLR_REQ_ON_ACTIVE_ERR);
+
+ /* Ensure that preceding stores that QPaCE will depend on are done
executing */
+ dma_wmb();
+
+ /*
+ * Ring the doorbell with a single 128-bit atomic store. QPaCE
+ * triggers processing when it observes the (dst_addr, src_addr)
+ * pair land together.
+ */
+ asm volatile("stp %0, %1, [%2]\n" :
+ : "r" (urg_addr_field_lower), "r" (urg_addr_field_upper),
"r" (td_dst_src_reg)
+ : "memory");
[Severity: Medium]
Does this inline assembly break big-endian architectures? By bypassing the
standard writeq() accessors, this stp instruction skips implicit cpu_to_le64
conversions, causing the hardware to receive byte-swapped, garbage memory
addresses and context configurations if the kernel is compiled with
CONFIG_CPU_BIG_ENDIAN.
ACK. Qpace is used in only little endian arch so far. We can look at a
handling for big endian configurations as well.
+
+ ret = readl_poll_timeout_atomic(qpace_priv->urg_regs +
+ urg_reg_num * QPACE_REG_PAGE_SIZE +
+ QPACE_URG_CMD_0_ED_STAT_OFFSET,
+ stat_reg,
+ FIELD_GET(URG_CMD_0_ED_STAT_COMP_CODE,
+ stat_reg) != OP_URG_ONGOING,
+ 1, 100 * USEC_PER_MSEC);
[Severity: High]
Does this cause severe scheduling latency spikes or RCU stalls? The caller,
such as qpace_urgent_compress(), disables preemption via get_cpu() before
initiating this hardware poll. Executing a 100ms busy-wait loop in
qpace_urgent_command_trigger() while preemption is completely disabled
can freeze CPU cores for unacceptable durations under load.
Under normal operating conditions qpace will finish processing a request
much sooner than 100ms.We have not seen any RCU stalls as a result of
this 100ms timeout loop.
+ if (ret) {
+ dev_err(qpace_priv->dev, "QPaCE urgent command timed out\n");
+ return -ETIMEDOUT;
+ }
[Severity: High]
Are we returning a timeout error here without stopping the hardware from
continuing the DMA operation? If the hardware takes longer than 100ms in
qpace_urgent_command_trigger(), this returns -ETIMEDOUT while the device
continues. If the caller (like zram) frees the output page, the hardware
could later finish and cause delayed asynchronous memory corruption.
If we ever run into the timeout path, then that means a hang has
happened and qpace all toegther is not making forward progress on the
request. This means it will not be doing any more DMA accesses.
But regardless I think adding some error handling to stop Qpace from SW
is a good idea and can be implemented. Thanks.
+
+ return stat_reg;
+}
+
+int qpace_urgent_compress(dma_addr_t input_addr,
+ dma_addr_t output_addr,
+ struct qpace_algorithm *algo)
+{
+ int urg_reg_num;
+ int stat_reg;
+ u32 stat_reg_val;
+ int ret;
+
+ ret = qpace_get();
+ if (ret)
+ return ret;
+
+ urg_reg_num = get_cpu() % NUM_TRS_ERS_URG_CMD_REGS;
+ stat_reg = qpace_urgent_command_trigger(input_addr, output_addr,
urg_reg_num,
+ algo->urg_comp_cntxt);
[Severity: High]
Can this assign identical hardware registers to different CPUs running
concurrently when NR_CPUS is greater than 8 without cross-CPU mutual
exclusion? In qpace_urgent_compress(), get_cpu() disables preemption but
provides no mutual exclusion across different cores. If two threads initiate
compression on CPU 0 and CPU 8 simultaneously, they will both be assigned
urg_reg_num = 0 and race on the same MMIO block, leading to data corruption
and hardware timeouts.
Current qpace HW configuraion is to work on soc with 8 cores so that
these urg_cmd registers match without overlap. Alternatively, we can add
a check at probe time for cpu topology and abort device initialization
if num cpus > 8.
+ put_cpu();
+
+ qpace_put();
[ ... ]
+int qpace_urgent_decompress(dma_addr_t input_addr,
+ dma_addr_t output_addr,
+ size_t input_size,
+ struct qpace_algorithm *algo)
+{
+ int urg_reg_num;
+ int stat_reg;
+ u32 stat_reg_val;
+ int ret;
+
+ ret = qpace_get();
+ if (ret)
+ goto out;
+
+ urg_reg_num = get_cpu() % NUM_TRS_ERS_URG_CMD_REGS;
+ qpace_write_urg_cmd_ctx(qpace_priv,
QPACE_URG_CMD_0_CFG_CNTXT_SIZE_n_OFFSET,
+ urg_reg_num, algo->urg_decomp_cntxt,
+ FIELD_PREP(URG_CMD_0_CFG_CNTXT_SIZE_SIZE,
input_size));
[Severity: High]
Does this dynamically overwrite the shared hardware context size register
concurrently across aliasing CPUs? In qpace_urgent_decompress(), with
more than 8 cores, aliasing CPUs will race on this MMIO block leading to
data corruption.
ACK. Same response as above comment about > 8 cpus.
+ stat_reg = qpace_urgent_command_trigger(input_addr, output_addr,
urg_reg_num,
+ algo->urg_decomp_cntxt);
+ put_cpu();
[ ... ]
+int qpace_get(void)
+{
+ int ret = 0;
+
+ mutex_lock(&qpace_ref_lock);
+ if (qpace_priv->suspended || READ_ONCE(qpace_priv->broken))
+ ret = -EBUSY;
[Severity: High]
Can this exported API blindly dereference the global pointer after the driver
has been unbound? If the device is unbound via sysfs, qpace_priv is freed,
but the global pointer remains set. A subsequent call from a client like zram
will call qpace_get() which dereferences qpace_priv->suspended, resulting in
a use-after-free.
ACK. Possibly we can make use of qpace_drv_probed in exported functions
to avoid using qpace if it has been removed or not probed.
+ else
+ _get_qpace();
+ mutex_unlock(&qpace_ref_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qpace_get);
[ ... ]
+static irqreturn_t urgent_interrupt_handler(int irq, void *unused)
+{
+ pr_debug("Urgent interrupt handled\n");
+ return IRQ_HANDLED;
+}
[Severity: High]
Will this cause a catastrophic interrupt storm? The
urgent_interrupt_handler() fails to acknowledge or clear the interrupt at
the device level. Because the GIC interrupt is level-triggered, returning
IRQ_HANDLED without clearing it on the hardware will cause it to
immediately re-fire forever, resulting in a hard CPU lockup if the
hardware ever asserts this line.
Currently as seen above, SW is noified of a completion event by polling
the HW register and not using interrupts. When other usecases are added
which use interrupt, then you are right that the HW registers will be
reset/cleared.
[ ... ]
+static void qpace_remove(struct platform_device *pdev)
+{
+ mutex_lock(&qpace_ref_lock);
+ qpace_priv->suspended = true;
+ mutex_unlock(&qpace_ref_lock);
+
+ static_branch_disable(&qpace_drv_probed);
+
+ wait_for_completion(&qpace_priv->no_active_refs);
+
+ /* No callers remain; tear down the hardware. */
+ cancel_work_sync(&qpace_priv->disable_work);
+ qpace_power_off(&pdev->dev);
+}
[Severity: High]
Does this leave the global qpace_priv pointer set? In qpace_remove(), the
device is torn down and devres will free the memory backing qpace_priv, but
the global pointer is never cleared, enabling the use-after-free mentioned
above.
ACK. The pointer can be set to NULL to be safe and qpace_drv_probed can
be checked by exported functions to avoid using qpace if it has been
removed or not probed.
[ ... ]
+static int qpace_resume(struct device *dev)
+{
+ int ret;
+
+ mutex_lock(&qpace_ref_lock);
+
+ if (qpace_priv->active_rings)
+ dev_err(dev, "active_rings not 0, unexpected case\n");
+
+ ret = icc_set_bw(qpace_priv->interconnect, 0, 1);
+ if (ret)
+ goto out_unlock;
+
+ program_urg_command_contexts_v2();
+ program_decomp_core_cfg();
+
+ qpace_priv->suspended = false;
[Severity: High]
Will the hardware be stuck in STOP mode after suspend/resume? In
qpace_resume(), we restore contexts but fail to wake the hardware engine
up to RUN mode (omitting the QPACE_RUN write) and fail to reprogram
essential bulk mode and cache index settings from qpace_hw_init(). This
means all hardware operations will indefinitely time out after the system
resumes.
In suspend case the HW is in power collapse mode, but once woken up will
retain it's run status.
+
+out_unlock:
+ if (ret)
+ dev_err(dev, "failed to resume QPaCE: %d\n", ret);
+ mutex_unlock(&qpace_ref_lock);
+ return ret;
+}
Thanks,
Oreoluwa
