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

[Oreoluwa Babatunde <oreoluwa.babatunde@oss.qualcomm.com>]

On 9/30/2026 8:04 AM, sashiko-bot@kernel.org 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 <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.

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