Re: [RFC PATCH v2 02/15] misc/arm-cla: Add launch operation helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] misc/arm-cla: 1-second busy loop in atomic poll function stalls CPU
- [High] misc/arm-cla: 32-bit truncation of max_regidx breaks register access unconditionally
--

commit 3a9a1e5f334e803060b4eaff6708691a049dc197
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Add launch operation helpers

This commit introduces helpers for CLA commands that execute sequences on the 
local CPU, wait for hardware responses, and translate response codes into Linux 
errors. It also implements accelerator reset and register read/write operations 
built upon these helpers.

> diff --git a/drivers/misc/arm-cla/cla-ops.c b/drivers/misc/arm-cla/cla-ops.c
> new file mode 100644
> index 0000000000000..46026304a057a
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-ops.c

[ ... ]

> +/**
> + * cla_op_wait_lresp - Wait for any LAUNCH op to complete.
> + * @dev: CLA device.
> + * @lresp: last LRESP value read.
> + *
> + * Return: 0 on success, -ETIMEDOUT in case of timeout.
> + */
> +int cla_op_wait_lresp(struct cla_dev *dev, u64 *lresp)
> +{
> +	return readq_relaxed_poll_timeout_atomic(dev->regs + CLA_REG_LRESP,
> +			*lresp, FIELD_GET(CLA_LRESP_PENDING, *lresp) == 0,
> +			CLA_LRESP_DELAY_US, CLA_LRESP_TIMEOUT_US);
> +}

[Severity: High]
Does this atomic polling loop stall the CPU for up to 1 second?

The timeout here is CLA_LRESP_TIMEOUT_US (1,000,000 microseconds). Using the
_atomic variant of the poll macro will busy-wait without yielding the CPU.

Since the surrounding code in cla_wait_status_dmb() uses the sleepable
poll_timeout_us(), does this context permit sleeping? If so, using the
non-atomic readq_relaxed_poll_timeout() macro might prevent long scheduler
delays.

[ ... ]

> +/* regidx is unsigned long here to avoid overflow during calculations */
> +static int cla_op_access_reg(struct cla_dev *dev, u8 op,
> +			     enum cla_launch_data_mode data_mode,
> +			     unsigned int accid, unsigned long regidx,
> +			     size_t nregs, u64 *regs)
> +{
> +	int ret = -EINVAL;
> +	unsigned long max_regidx;
> +	struct cla_launch launch = {
> +		.op = op,
> +		.accid = accid,
> +		.data_mode = data_mode,
> +	};
> +
> +	switch (op) {
> +	case CLA_LAUNCH_OP_REGREAD:
> +	case CLA_LAUNCH_OP_REGWRITE:
> +		max_regidx = 0x100000000;

[Severity: High]
Will this unconditionally fail register accesses on 32-bit architectures?

The constant 0x100000000 requires 33 bits. When assigned to an unsigned long
on 32-bit platforms, it is implicitly truncated to 0.

> +		break;
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(regidx + nregs > max_regidx))

Since max_regidx becomes 0 on 32-bit platforms, this bounds check evaluates
to regidx + nregs > 0, which is always true when nregs is greater than zero.

Should max_regidx and regidx both be explicitly typed as u64 to prevent this
truncation and correctly handle overflow calculations?

> +		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015142552.4128028-1-ryan.roberts@arm.com?part=2