Re: [PATCH 2/6] soc: qcom: qpace: Add Qualcomm Page Compression Engine driver

[sashiko-bot@kernel.org]

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 <georgi.djakov@oss.qualcomm.com>

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.

> +
> +	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.

> +
> +	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.

> +	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.

> +
> +	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.

> +	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.

> +	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.

> +	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.

[ ... ]
> +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.

[ ... ]
> +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.

> +
> +out_unlock:
> +	if (ret)
> +		dev_err(dev, "failed to resume QPaCE: %d\n", ret);
> +	mutex_unlock(&qpace_ref_lock);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930145215.3380977-1-georgi.djakov@oss.qualcomm.com?part=2